| Message ID | 1445502750-22672-2-git-send-email-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote: > GPIO chips have been around for years, but were never real devices, > instead they were piggy-backing on a parent device (such as a > platform_device or amba_device) but this was always optional. > GPIO chips could also exist without any device at all, with its > struct device *dev pointer being set to null. > > When sysfs was in use, a mock device would be created, with the > optional parent assigned, or just floating orphaned with NULL > as parent. > > For a proper userspace ABI we need gpiochips to *always have a > populated struct device, so add this in the gpio_chip struct. > The name "dev" is unfortunately already take so we use "device" > to name it. > > If sysfs is active, it will use this device as parent, and the > former parent device "dev" will be set as parent of the new > "device" struct member. > > From this point on, gpiochips are devices. > > Cc: Johan Hovold <johan@kernel.org> > Cc: Michael Welling <mwelling@ieee.org> > Cc: Markus Pargmann <mpa@pengutronix.de> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpio/gpiolib-sysfs.c | 2 +- > drivers/gpio/gpiolib.c | 44 +++++++++++++++++++++++++++++++++++++++----- > include/linux/gpio/driver.h | 9 +++++++-- > 3 files changed, 47 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c > index b57ed8e55ab5..7e5bc5736e47 100644 > --- a/drivers/gpio/gpiolib-sysfs.c > +++ b/drivers/gpio/gpiolib-sysfs.c > @@ -605,7 +605,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change) > if (chip->names && chip->names[offset]) > ioname = chip->names[offset]; > > - dev = device_create_with_groups(&gpio_class, chip->dev, > + dev = device_create_with_groups(&gpio_class, &chip->device, > MKDEV(0, 0), data, gpio_groups, > ioname ? ioname : "gpio%u", > desc_to_gpio(desc)); > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 6798355c61c6..0ab4f75b7f8e 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -16,6 +16,7 @@ > #include <linux/gpio/driver.h> > #include <linux/gpio/machine.h> > #include <linux/pinctrl/consumer.h> > +#include <linux/idr.h> > > #include "gpiolib.h" > > @@ -42,6 +43,9 @@ > #define extra_checks 0 > #endif > > +/* Device and char device-related information */ > +static DEFINE_IDA(gpio_ida); > + > /* gpio_lock prevents conflicts during gpio_desc[] table updates. > * While any GPIO is requested, its gpio_chip is not removable; > * each GPIO's "requested" flag serves as a lock and refcount. > @@ -52,7 +56,6 @@ static DEFINE_MUTEX(gpio_lookup_lock); > static LIST_HEAD(gpio_lookup_list); > LIST_HEAD(gpio_chips); > > - > static void gpiochip_free_hogs(struct gpio_chip *chip); > static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip); > > @@ -300,7 +303,7 @@ int gpiochip_add(struct gpio_chip *chip) > { > unsigned long flags; > int status = 0; > - unsigned id; > + unsigned i; > int base = chip->base; > struct gpio_desc *descs; > > @@ -308,6 +311,28 @@ int gpiochip_add(struct gpio_chip *chip) > if (!descs) > return -ENOMEM; > > + /* > + * The "dev" member of gpiochip is the parent, and the actual > + * device is named "device" for historical reasons. > + * > + * We memset the struct to zero to avoid reentrance issues. > + */ > + memset(&chip->device, 0, sizeof(chip->device)); > + if (chip->dev) { > + chip->device.parent = chip->dev; > + chip->device.of_node = chip->dev->of_node; > + } else { > +#ifdef CONFIG_OF_GPIO > + /* If the gpiochip has an assigned OF node this takes precedence */ > + if (chip->of_node) > + chip->device.of_node = chip->of_node; This is in the else case. I think this should be indented an additional level. Best Regards, Markus > +#endif > + } > + chip->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL); > + dev_set_name(&chip->device, "gpiochip%d", chip->id); > + device_initialize(&chip->device); > + dev_set_drvdata(&chip->device, chip); > + > spin_lock_irqsave(&gpio_lock, flags); > > if (base < 0) { > @@ -326,8 +351,8 @@ int gpiochip_add(struct gpio_chip *chip) > goto err_free_descs; > } > > - for (id = 0; id < chip->ngpio; id++) { > - struct gpio_desc *desc = &descs[id]; > + for (i = 0; i < chip->ngpio; i++) { > + struct gpio_desc *desc = &descs[i]; > > desc->chip = chip; > > @@ -361,16 +386,22 @@ int gpiochip_add(struct gpio_chip *chip) > > acpi_gpiochip_add(chip); > > - status = gpiochip_sysfs_register(chip); > + status = device_add(&chip->device); > if (status) > goto err_remove_chip; > > + status = gpiochip_sysfs_register(chip); > + if (status) > + goto err_remove_device; > + > pr_debug("%s: registered GPIOs %d to %d on device: %s\n", __func__, > chip->base, chip->base + chip->ngpio - 1, > chip->label ? : "generic"); > > return 0; > > +err_remove_device: > + device_del(&chip->device); > err_remove_chip: > acpi_gpiochip_remove(chip); > gpiochip_free_hogs(chip); > @@ -381,6 +412,7 @@ err_remove_from_list: > spin_unlock_irqrestore(&gpio_lock, flags); > chip->desc = NULL; > err_free_descs: > + ida_simple_remove(&gpio_ida, chip->id); > kfree(descs); > > /* failures here can mean systems won't boot... */ > @@ -426,6 +458,8 @@ void gpiochip_remove(struct gpio_chip *chip) > if (requested) > dev_crit(chip->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n"); > > + device_del(&chip->device); > + ida_simple_remove(&gpio_ida, chip->id); > kfree(chip->desc); > chip->desc = NULL; > } > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index d1baebf350d8..cf3028eccc4b 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -1,6 +1,7 @@ > #ifndef __LINUX_GPIO_DRIVER_H > #define __LINUX_GPIO_DRIVER_H > > +#include <linux/device.h> > #include <linux/types.h> > #include <linux/module.h> > #include <linux/irq.h> > @@ -8,8 +9,8 @@ > #include <linux/irqdomain.h> > #include <linux/lockdep.h> > #include <linux/pinctrl/pinctrl.h> > +#include <linux/cdev.h> > > -struct device; > struct gpio_desc; > struct of_phandle_args; > struct device_node; > @@ -20,7 +21,9 @@ struct seq_file; > /** > * struct gpio_chip - abstract a GPIO controller > * @label: for diagnostics > - * @dev: optional device providing the GPIOs > + * @id: numerical ID number for the GPIO chip > + * @device: the GPIO device struct. > + * @dev: optional parent device (hardware) providing the GPIOs > * @cdev: class device used by sysfs interface (may be NULL) > * @owner: helps prevent removal of modules exporting active GPIOs > * @list: links gpio_chips together for traversal > @@ -89,6 +92,8 @@ struct seq_file; > */ > struct gpio_chip { > const char *label; > + int id; > + struct device device; > struct device *dev; > struct device *cdev; > struct module *owner; > -- > 2.4.3 > > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Thu, Oct 22, 2015 at 5:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > GPIO chips have been around for years, but were never real devices, > instead they were piggy-backing on a parent device (such as a > platform_device or amba_device) but this was always optional. > GPIO chips could also exist without any device at all, with its > struct device *dev pointer being set to null. > > When sysfs was in use, a mock device would be created, with the > optional parent assigned, or just floating orphaned with NULL > as parent. > > For a proper userspace ABI we need gpiochips to *always have a > populated struct device, so add this in the gpio_chip struct. > The name "dev" is unfortunately already take so we use "device" > to name it. > > If sysfs is active, it will use this device as parent, and the > former parent device "dev" will be set as parent of the new > "device" struct member. Why not rename "dev" to "parent" so "dev" becomes what we expect it to be? The two members being of the same type, keeping it that way seems error-prone to me. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 25, 2015 at 04:06:26PM +0900, Alexandre Courbot wrote: > On Thu, Oct 22, 2015 at 5:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > > If sysfs is active, it will use this device as parent, and the > > former parent device "dev" will be set as parent of the new > > "device" struct member. > Why not rename "dev" to "parent" so "dev" becomes what we expect it to > be? The two members being of the same type, keeping it that way seems > error-prone to me. Indeed, that's going to be hard to follow with nothing in the names that says which is which. Another option that some subsystems use is to add the subsystem specific device with a name like gpio_dev that indicates it's the subsystem version of the device.
On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote: > GPIO chips have been around for years, but were never real devices, > instead they were piggy-backing on a parent device (such as a > platform_device or amba_device) but this was always optional. > GPIO chips could also exist without any device at all, with its > struct device *dev pointer being set to null. > > When sysfs was in use, a mock device would be created, with the > optional parent assigned, or just floating orphaned with NULL > as parent. > > For a proper userspace ABI we need gpiochips to *always have a > populated struct device, so add this in the gpio_chip struct. > The name "dev" is unfortunately already take so we use "device" > to name it. > > If sysfs is active, it will use this device as parent, and the > former parent device "dev" will be set as parent of the new > "device" struct member. > > From this point on, gpiochips are devices. > > Cc: Johan Hovold <johan@kernel.org> > Cc: Michael Welling <mwelling@ieee.org> > Cc: Markus Pargmann <mpa@pengutronix.de> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpio/gpiolib-sysfs.c | 2 +- > drivers/gpio/gpiolib.c | 44 +++++++++++++++++++++++++++++++++++++++----- > include/linux/gpio/driver.h | 9 +++++++-- > 3 files changed, 47 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c > index b57ed8e55ab5..7e5bc5736e47 100644 > --- a/drivers/gpio/gpiolib-sysfs.c > +++ b/drivers/gpio/gpiolib-sysfs.c > @@ -605,7 +605,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change) > if (chip->names && chip->names[offset]) > ioname = chip->names[offset]; > > - dev = device_create_with_groups(&gpio_class, chip->dev, > + dev = device_create_with_groups(&gpio_class, &chip->device, > MKDEV(0, 0), data, gpio_groups, > ioname ? ioname : "gpio%u", > desc_to_gpio(desc)); > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 6798355c61c6..0ab4f75b7f8e 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -16,6 +16,7 @@ > #include <linux/gpio/driver.h> > #include <linux/gpio/machine.h> > #include <linux/pinctrl/consumer.h> > +#include <linux/idr.h> > > #include "gpiolib.h" > > @@ -42,6 +43,9 @@ > #define extra_checks 0 > #endif > > +/* Device and char device-related information */ > +static DEFINE_IDA(gpio_ida); > + > /* gpio_lock prevents conflicts during gpio_desc[] table updates. > * While any GPIO is requested, its gpio_chip is not removable; > * each GPIO's "requested" flag serves as a lock and refcount. > @@ -52,7 +56,6 @@ static DEFINE_MUTEX(gpio_lookup_lock); > static LIST_HEAD(gpio_lookup_list); > LIST_HEAD(gpio_chips); > > - > static void gpiochip_free_hogs(struct gpio_chip *chip); > static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip); > > @@ -300,7 +303,7 @@ int gpiochip_add(struct gpio_chip *chip) > { > unsigned long flags; > int status = 0; > - unsigned id; > + unsigned i; > int base = chip->base; > struct gpio_desc *descs; > > @@ -308,6 +311,28 @@ int gpiochip_add(struct gpio_chip *chip) > if (!descs) > return -ENOMEM; > > + /* > + * The "dev" member of gpiochip is the parent, and the actual > + * device is named "device" for historical reasons. > + * > + * We memset the struct to zero to avoid reentrance issues. > + */ > + memset(&chip->device, 0, sizeof(chip->device)); This is an indication of a larger problem. First of all, you must never register the same device structure twice. And the larger problem is: With the current interface where a struct gpio_chip is passed and registered, how would you prevent the device from going away while in use? You grab a reference to the chip->device when opening the node (in a later patch), but it is not used to manage the life time of struct gpio_chip. > + if (chip->dev) { > + chip->device.parent = chip->dev; > + chip->device.of_node = chip->dev->of_node; > + } else { > +#ifdef CONFIG_OF_GPIO > + /* If the gpiochip has an assigned OF node this takes precedence */ > + if (chip->of_node) > + chip->device.of_node = chip->of_node; > +#endif > + } > + chip->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL); Missing error handling. > + dev_set_name(&chip->device, "gpiochip%d", chip->id); > + device_initialize(&chip->device); > + dev_set_drvdata(&chip->device, chip); > + > spin_lock_irqsave(&gpio_lock, flags); > > if (base < 0) { Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 02, 2015 at 11:31:16AM +0100, Johan Hovold wrote: > On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote: > > + /* > > + * The "dev" member of gpiochip is the parent, and the actual > > + * device is named "device" for historical reasons. > > + * > > + * We memset the struct to zero to avoid reentrance issues. > > + */ > > + memset(&chip->device, 0, sizeof(chip->device)); > This is an indication of a larger problem. > First of all, you must never register the same device structure twice. Well, you can unregister and reregister (and it is reasonable practice to make sure that the struct isn't full of noise) - we usually allocate things out of kzalloc(). > And the larger problem is: With the current interface where a struct > gpio_chip is passed and registered, how would you prevent the device > from going away while in use? Hrm, indeed. Why aren't there complaints about a missing release function there? > You grab a reference to the chip->device when opening the node (in a > later patch), but it is not used to manage the life time of struct > gpio_chip. That's a slightly separate thing and even with a different implementation of the file we still have to assume that the driver core might hold a reference to the device for longer (for example as a result of sysfs interactions).
On Mon, Nov 02, 2015 at 12:25:14PM +0000, Mark Brown wrote: > On Mon, Nov 02, 2015 at 11:31:16AM +0100, Johan Hovold wrote: > > On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote: > > > > + /* > > > + * The "dev" member of gpiochip is the parent, and the actual > > > + * device is named "device" for historical reasons. > > > + * > > > + * We memset the struct to zero to avoid reentrance issues. > > > + */ > > > + memset(&chip->device, 0, sizeof(chip->device)); > > > This is an indication of a larger problem. > > > First of all, you must never register the same device structure twice. > > Well, you can unregister and reregister (and it is reasonable practice > to make sure that the struct isn't full of noise) - we usually allocate > things out of kzalloc(). Actually, no. It's an explicitly forbidden practise to reregister the same struct device. > > And the larger problem is: With the current interface where a struct > > gpio_chip is passed and registered, how would you prevent the device > > from going away while in use? > > Hrm, indeed. Why aren't there complaints about a missing release > function there? > > > You grab a reference to the chip->device when opening the node (in a > > later patch), but it is not used to manage the life time of struct > > gpio_chip. > > That's a slightly separate thing and even with a different > implementation of the file we still have to assume that the driver core > might hold a reference to the device for longer (for example as a result > of sysfs interactions). Yes, there may be other references, but sysfs should be ok due to the kernfs active protection. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 02, 2015 at 01:43:23PM +0100, Johan Hovold wrote: > On Mon, Nov 02, 2015 at 12:25:14PM +0000, Mark Brown wrote: > > > First of all, you must never register the same device structure twice. > > Well, you can unregister and reregister (and it is reasonable practice > > to make sure that the struct isn't full of noise) - we usually allocate > > things out of kzalloc(). > Actually, no. It's an explicitly forbidden practise to reregister the > same struct device. A memset() should be enough, if not then we have problems with any dynamically allocated struct device.
On Mon, Nov 02, 2015 at 12:47:37PM +0000, Mark Brown wrote: > On Mon, Nov 02, 2015 at 01:43:23PM +0100, Johan Hovold wrote: > > On Mon, Nov 02, 2015 at 12:25:14PM +0000, Mark Brown wrote: > > > > > First of all, you must never register the same device structure twice. > > > > Well, you can unregister and reregister (and it is reasonable practice > > > to make sure that the struct isn't full of noise) - we usually allocate > > > things out of kzalloc(). > > > Actually, no. It's an explicitly forbidden practise to reregister the > > same struct device. > > A memset() should be enough, if not then we have problems with any > dynamically allocated struct device. And how would you know that it is safe to memset that struct device? There can still be references to it. And driver core explicitly forbids this (see device_add() for example). Dynamically allocated struct device are not the problem as then you're not *reusing* the same device structure. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 02, 2015 at 01:53:47PM +0100, Johan Hovold wrote: > On Mon, Nov 02, 2015 at 12:47:37PM +0000, Mark Brown wrote: > > A memset() should be enough, if not then we have problems with any > > dynamically allocated struct device. > And how would you know that it is safe to memset that struct device? > There can still be references to it. And driver core explicitly forbids > this (see device_add() for example). My point here is that the memset() of something that was passed in externally isn't really a problem, it's the suspicious lack of integration with a release function (hence my question about why the driver core isn't complaining when the device gets registered). The comment is more of a concern than the memset() itself. > Dynamically allocated struct device are not the problem as then you're > not *reusing* the same device structure. You may end up doing exactly that depending on what you get back from the allocator of course. The point is to make sure that that the driver model called release(), dynamically allocating and freeing in the release function is the easiest way to do this but it's not magic.
On Mon, Nov 02, 2015 at 01:06:33PM +0000, Mark Brown wrote: > On Mon, Nov 02, 2015 at 01:53:47PM +0100, Johan Hovold wrote: > > On Mon, Nov 02, 2015 at 12:47:37PM +0000, Mark Brown wrote: > > > > A memset() should be enough, if not then we have problems with any > > > dynamically allocated struct device. > > > And how would you know that it is safe to memset that struct device? > > There can still be references to it. And driver core explicitly forbids > > this (see device_add() for example). > > My point here is that the memset() of something that was passed in > externally isn't really a problem, it's the suspicious lack of > integration with a release function (hence my question about why the > driver core isn't complaining when the device gets registered). The > comment is more of a concern than the memset() itself. Precisely, the memset itself is not the problem, but rather why was done in the first place: * We memset the struct to zero to avoid reentrance issues. */ And to that point, I mentioned that it is illegal to register the same struct device twice (and that this was indicative of a larger problem). > > Dynamically allocated struct device are not the problem as then you're > > not *reusing* the same device structure. > > You may end up doing exactly that depending on what you get back from > the allocator of course. But then the memory has already been released. You're not deregistering and reregistering the same device as in your example. > The point is to make sure that that the driver > model called release(), dynamically allocating and freeing in the > release function is the easiest way to do this but it's not magic. Agreed. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 02, 2015 at 02:14:56PM +0100, Johan Hovold wrote: > On Mon, Nov 02, 2015 at 01:06:33PM +0000, Mark Brown wrote: > > > Dynamically allocated struct device are not the problem as then you're > > > not *reusing* the same device structure. > > You may end up doing exactly that depending on what you get back from > > the allocator of course. > But then the memory has already been released. You're not deregistering > and reregistering the same device as in your example. Depends on your definition of "same" :) You might get the same address back which has been known to confuse unhelpfully designed things.
On Mon, Nov 02, 2015 at 01:17:01PM +0000, Mark Brown wrote: > On Mon, Nov 02, 2015 at 02:14:56PM +0100, Johan Hovold wrote: > > On Mon, Nov 02, 2015 at 01:06:33PM +0000, Mark Brown wrote: > > > > > Dynamically allocated struct device are not the problem as then you're > > > > not *reusing* the same device structure. > > > > You may end up doing exactly that depending on what you get back from > > > the allocator of course. > > > But then the memory has already been released. You're not deregistering > > and reregistering the same device as in your example. > > Depends on your definition of "same" :) You might get the same address > back which has been known to confuse unhelpfully designed things. Yeah, I got your point, but again my claim was that device_unregister(dev); memset(dev); device_initialise(dev); ... device_add(dev); is not legal (and the current patch does not prevent this). Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 25, 2015 at 8:06 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Thu, Oct 22, 2015 at 5:32 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> GPIO chips have been around for years, but were never real devices, >> instead they were piggy-backing on a parent device (such as a >> platform_device or amba_device) but this was always optional. >> GPIO chips could also exist without any device at all, with its >> struct device *dev pointer being set to null. >> >> When sysfs was in use, a mock device would be created, with the >> optional parent assigned, or just floating orphaned with NULL >> as parent. >> >> For a proper userspace ABI we need gpiochips to *always have a >> populated struct device, so add this in the gpio_chip struct. >> The name "dev" is unfortunately already take so we use "device" >> to name it. >> >> If sysfs is active, it will use this device as parent, and the >> former parent device "dev" will be set as parent of the new >> "device" struct member. > > Why not rename "dev" to "parent" so "dev" becomes what we expect it to > be? The two members being of the same type, keeping it that way seems > error-prone to me. It's because that is set all over the universe. But I'll cook some separate patch renaming it across the tree I guess... Could use Cocinelle for it maybe. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 2, 2015 at 11:31 AM, Johan Hovold <johan@kernel.org> wrote: > On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote: >> + * We memset the struct to zero to avoid reentrance issues. >> + */ >> + memset(&chip->device, 0, sizeof(chip->device)); > > This is an indication of a larger problem. > > First of all, you must never register the same device structure twice. > > And the larger problem is: With the current interface where a struct > gpio_chip is passed and registered, how would you prevent the device > from going away while in use? OK I see. What about the design pattern elsewhere to pass a gpiochip desc and have a new gpiochip_register() create the gpiochip and delete gpiochip_add()? That's the usual pattern in subsystems. > You grab a reference to the chip->device when opening the node (in a > later patch), but it is not used to manage the life time of struct > gpio_chip. OK I think the above approach should solve that? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 3, 2015 at 10:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Nov 2, 2015 at 11:31 AM, Johan Hovold <johan@kernel.org> wrote: >> On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote: > >>> + * We memset the struct to zero to avoid reentrance issues. >>> + */ >>> + memset(&chip->device, 0, sizeof(chip->device)); >> >> This is an indication of a larger problem. >> >> First of all, you must never register the same device structure twice. >> >> And the larger problem is: With the current interface where a struct >> gpio_chip is passed and registered, how would you prevent the device >> from going away while in use? > > OK I see. What about the design pattern elsewhere to pass a > gpiochip desc and have a new gpiochip_register() create the > gpiochip and delete gpiochip_add()? > > That's the usual pattern in subsystems. Thinking about it maybe it's simplest to just make ->dev a pointer and kzalloc() it at gpiochip_add(). That should solve this. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 04, 2015 at 11:48:47AM +0100, Linus Walleij wrote: > On Tue, Nov 3, 2015 at 10:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Nov 2, 2015 at 11:31 AM, Johan Hovold <johan@kernel.org> wrote: > >> On Thu, Oct 22, 2015 at 10:32:25AM +0200, Linus Walleij wrote: > > > >>> + * We memset the struct to zero to avoid reentrance issues. > >>> + */ > >>> + memset(&chip->device, 0, sizeof(chip->device)); > >> > >> This is an indication of a larger problem. > >> > >> First of all, you must never register the same device structure twice. > >> > >> And the larger problem is: With the current interface where a struct > >> gpio_chip is passed and registered, how would you prevent the device > >> from going away while in use? > > > > OK I see. What about the design pattern elsewhere to pass a > > gpiochip desc and have a new gpiochip_register() create the > > gpiochip and delete gpiochip_add()? > > > > That's the usual pattern in subsystems. > > Thinking about it maybe it's simplest to just make ->dev a pointer > and kzalloc() it at gpiochip_add(). > > That should solve this. You'd avoid ever reregistering the same struct device, but that would not solve the bigger life-time issue by itself. Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 05, 2015 at 10:44:11AM +0100, Johan Hovold wrote: > On Wed, Nov 04, 2015 at 11:48:47AM +0100, Linus Walleij wrote: > > Thinking about it maybe it's simplest to just make ->dev a pointer > > and kzalloc() it at gpiochip_add(). > > That should solve this. > You'd avoid ever reregistering the same struct device, but that would > not solve the bigger life-time issue by itself. You'd need to do something like have the file reference things via something attached to that device and then ensure that it's made safe (eg, by setting a pointer to NULL and then checking for that on use) when removing the device.
On Thu, Nov 5, 2015 at 10:44 AM, Johan Hovold <johan@kernel.org> wrote: > On Wed, Nov 04, 2015 at 11:48:47AM +0100, Linus Walleij wrote: >> Thinking about it maybe it's simplest to just make ->dev a pointer >> and kzalloc() it at gpiochip_add(). >> >> That should solve this. Turns out that it's a bad idea to have a struct device as pointer because it makes it impossible to use container_of() in e.g. .remove. > You'd avoid ever reregistering the same struct device, but that would > not solve the bigger life-time issue by itself. Since device_del() unconditionally kills off the devince from the system this is equivalent to saying device_add() and device_del() should really not be used by subsystems, am I right? Right now there is a cryptic comment in device_del() that says it should "only be used if device_add() was also used manually" I wonder who this man is that is using things manually inside the kernel, I guess it actually means they must be paired. If I don't use device_add()/device_del(), I see this must mean I have to use device_register()/device_unregister() as the latter only decrease the refcount and wait for the instance to die off. So am I reading it correctly if I understand that this is what we should be doing? I.e device_register()/device_unregister() and then wait for the .remove() call to do its deed, so the kobj/struct device is kept around if e.g. sysfs or the chardev has open files. (It makes sense. Harder to code up, but makes sense.) Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Nov 16, 2015 at 3:27 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > If I don't use device_add()/device_del(), I see this must mean I have to use > device_register()/device_unregister() as the latter only decrease the refcount > and wait for the instance to die off. > > So am I reading it correctly if I understand that this is what we should be > doing? I.e device_register()/device_unregister() and then wait for the > .remove() call to do its deed, so the kobj/struct device is kept around if > e.g. sysfs or the chardev has open files. Hard to get advice on design here I see. Anyways, the clean thing to do (after some weeks of thinking) seems to be this: 1. Introduce a void *data into gpio_chip and gpiochip_set_data() gpiochip_get_data() set/get it. 2. Replace all occurences of container_of() dereferencing in all GPIO drivers with gpiochip_[set|get]_data() so as to avoid having gpio_chip to be embedded into state containers. 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks and other static config called struct gpio_chip, and struct gpio_device that is returned as a pointer from gpiochip_add(). It will need to be free:ed by gpiodevice_remove() after that. 3-2. In the same patch set rename all functions to indicate that we are now operating on a gpio_device not a gpio_chip. 3-3. After a GPIO driver issues gpio_device_unregister() it may stay around if there are still references to the struct device. This is a very tiresome refactoring, but I'm growing confident that it is what needs to happen to manage gpio devices in the future. After step 3-3 we have a clean gpio_device containing a struct device, and from there we apply the initial chardev interface. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 3, 2015 at 3:04 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks > and other static config called struct gpio_chip, and > struct gpio_device that is returned as a pointer from > gpiochip_add(). It will need to be free:ed by gpiodevice_remove() > after that. This will look something like this uglyhack: https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=gpiochip-desc&id=610b19c1832acfdf6ff62523addfa08b76f77343 Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 03, 2015 at 03:06:48PM +0100, Linus Walleij wrote: > On Thu, Dec 3, 2015 at 3:04 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > > > 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks > > and other static config called struct gpio_chip, and > > struct gpio_device that is returned as a pointer from > > gpiochip_add(). It will need to be free:ed by gpiodevice_remove() > > after that. > > This will look something like this uglyhack: > https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=gpiochip-desc&id=610b19c1832acfdf6ff62523addfa08b76f77343 > Lots of red and green just in time for Christmas. Let me take a look at what it would take to recover from said theoretical stab. > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 03, 2015 at 03:06:48PM +0100, Linus Walleij wrote: > On Thu, Dec 3, 2015 at 3:04 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > > > 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks > > and other static config called struct gpio_chip, and > > struct gpio_device that is returned as a pointer from > > gpiochip_add(). It will need to be free:ed by gpiodevice_remove() > > after that. > > This will look something like this uglyhack: > https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=gpiochip-desc&id=610b19c1832acfdf6ff62523addfa08b76f77343 > Still quite a few place where a gpio_device is being access as if a gpio_chip: gpio_device_set_desc_names gpio_device_set_multiple gpiod_hog gpio_device_free_hogs gpiolib_dbg_show gpiolib_seq_show .. There is still some calls to gpiod_to_chip which was changed to gpiod_to_device. gpiod_hog .. The gpiochip_add function prototype was changed in the header but the definition did not change. Lots of work to do still. Let me know if and how I can help. > Yours, > Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 03, 2015 at 03:04:14PM +0100, Linus Walleij wrote: > On Mon, Nov 16, 2015 at 3:27 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > > > If I don't use device_add()/device_del(), I see this must mean I have to use > > device_register()/device_unregister() as the latter only decrease the refcount > > and wait for the instance to die off. > > > > So am I reading it correctly if I understand that this is what we should be > > doing? I.e device_register()/device_unregister() and then wait for the > > .remove() call to do its deed, so the kobj/struct device is kept around if > > e.g. sysfs or the chardev has open files. > > Hard to get advice on design here I see. Anyways, the clean thing to > do (after some weeks of thinking) seems to be this: > > 1. Introduce a void *data into gpio_chip and gpiochip_set_data() > gpiochip_get_data() set/get it. > > 2. Replace all occurences of container_of() dereferencing in all > GPIO drivers with gpiochip_[set|get]_data() so as to avoid having > gpio_chip to be embedded into state containers. > > 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks > and other static config called struct gpio_chip, and > struct gpio_device that is returned as a pointer from > gpiochip_add(). It will need to be free:ed by gpiodevice_remove() > after that. > > 3-2. In the same patch set rename all functions to indicate that > we are now operating on a gpio_device not a gpio_chip. > > 3-3. After a GPIO driver issues gpio_device_unregister() > it may stay around if there are still references to the > struct device. > > This is a very tiresome refactoring, but I'm growing confident that > it is what needs to happen to manage gpio devices in the future. > > After step 3-3 we have a clean gpio_device containing a > struct device, and from there we apply the initial chardev > interface. This sounds unnecessarily complicated. Why not use a more standard approach of gpiochip_create() that allocates the state container that the driver can then initialise further (e.g. callback pointers) and finally register with a call to: gpiochip_add() When the parent device is going away, it calls gpiochip_remove() which unregisters the gpiochip device (i.e. calls device_del), updates its state to "disconnected" and drops the gpiolib's reference. There may still be other devices (consumers) holding references to the gpiochip device, which is not deallocated until the final reference is dropped (i.e. at final gpiod_free). Johan -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 4, 2015 at 11:31 PM, Michael Welling <mwelling@ieee.org> wrote: > On Thu, Dec 03, 2015 at 03:06:48PM +0100, Linus Walleij wrote: >> On Thu, Dec 3, 2015 at 3:04 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> >> > 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks >> > and other static config called struct gpio_chip, and >> > struct gpio_device that is returned as a pointer from >> > gpiochip_add(). It will need to be free:ed by gpiodevice_remove() >> > after that. >> >> This will look something like this uglyhack: >> https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=gpiochip-desc&id=610b19c1832acfdf6ff62523addfa08b76f77343 >> > > Still quite a few place where a gpio_device is being access as if a gpio_chip: > gpio_device_set_desc_names > gpio_device_set_multiple > gpiod_hog > gpio_device_free_hogs > gpiolib_dbg_show > gpiolib_seq_show > .. > > There is still some calls to gpiod_to_chip which was changed to gpiod_to_device. > gpiod_hog > .. > > The gpiochip_add function prototype was changed in the header but the definition did not change. > > Lots of work to do still. Let me know if and how I can help. Yeah it doesn't even compile. It was more to illustrate. I am now figuring out how to proceed after merging 187 patches removing the use of container_of() from all GPIO drivers. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 8, 2015 at 10:29 AM, Johan Hovold <johan@kernel.org> wrote: > On Thu, Dec 03, 2015 at 03:04:14PM +0100, Linus Walleij wrote: >> On Mon, Nov 16, 2015 at 3:27 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> >> > If I don't use device_add()/device_del(), I see this must mean I have to use >> > device_register()/device_unregister() as the latter only decrease the refcount >> > and wait for the instance to die off. >> > >> > So am I reading it correctly if I understand that this is what we should be >> > doing? I.e device_register()/device_unregister() and then wait for the >> > .remove() call to do its deed, so the kobj/struct device is kept around if >> > e.g. sysfs or the chardev has open files. >> >> Hard to get advice on design here I see. Anyways, the clean thing to >> do (after some weeks of thinking) seems to be this: >> >> 1. Introduce a void *data into gpio_chip and gpiochip_set_data() >> gpiochip_get_data() set/get it. >> >> 2. Replace all occurences of container_of() dereferencing in all >> GPIO drivers with gpiochip_[set|get]_data() so as to avoid having >> gpio_chip to be embedded into state containers. >> >> 3-1. Split gpio_chip in a static descriptor with a vtable for callbacks >> and other static config called struct gpio_chip, and >> struct gpio_device that is returned as a pointer from >> gpiochip_add(). It will need to be free:ed by gpiodevice_remove() >> after that. >> >> 3-2. In the same patch set rename all functions to indicate that >> we are now operating on a gpio_device not a gpio_chip. >> >> 3-3. After a GPIO driver issues gpio_device_unregister() >> it may stay around if there are still references to the >> struct device. >> >> This is a very tiresome refactoring, but I'm growing confident that >> it is what needs to happen to manage gpio devices in the future. >> >> After step 3-3 we have a clean gpio_device containing a >> struct device, and from there we apply the initial chardev >> interface. > > This sounds unnecessarily complicated. > > Why not use a more standard approach of > > gpiochip_create() > > that allocates the state container that the driver can then initialise > further (e.g. callback pointers) and finally register with a call to: > > gpiochip_add() > > When the parent device is going away, it calls > > gpiochip_remove() > > which unregisters the gpiochip device (i.e. calls device_del), updates > its state to "disconnected" and drops the gpiolib's reference. > > There may still be other devices (consumers) holding references to the > gpiochip device, which is not deallocated until the final reference is > dropped (i.e. at final gpiod_free). Yeah after discussing with Russell it seems something like this is better. I really like what netdev is doing with alloc_netdev(), so I imagine: struct gpio_device *alloc_gpiodev(sizeof_priv, name, setup) Where sizeof_priv is the size of the state container for the GPIO driver instance, setup() is a callback to fill in vtable etc and name is what we now call label. Then ther will be gpiodev_priv(dev) to get the private data as a void * pointer. The whole thing is memory managed by the gpiolib core. gpiodev_remove() to get rid of the driver reference. And this would come with a proper struct device and all. Right now I'm thinking about how to put in this interface so that we can convert drivers one by one and need not convert all in a gigantice 20.000 line patch. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c index b57ed8e55ab5..7e5bc5736e47 100644 --- a/drivers/gpio/gpiolib-sysfs.c +++ b/drivers/gpio/gpiolib-sysfs.c @@ -605,7 +605,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change) if (chip->names && chip->names[offset]) ioname = chip->names[offset]; - dev = device_create_with_groups(&gpio_class, chip->dev, + dev = device_create_with_groups(&gpio_class, &chip->device, MKDEV(0, 0), data, gpio_groups, ioname ? ioname : "gpio%u", desc_to_gpio(desc)); diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 6798355c61c6..0ab4f75b7f8e 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -16,6 +16,7 @@ #include <linux/gpio/driver.h> #include <linux/gpio/machine.h> #include <linux/pinctrl/consumer.h> +#include <linux/idr.h> #include "gpiolib.h" @@ -42,6 +43,9 @@ #define extra_checks 0 #endif +/* Device and char device-related information */ +static DEFINE_IDA(gpio_ida); + /* gpio_lock prevents conflicts during gpio_desc[] table updates. * While any GPIO is requested, its gpio_chip is not removable; * each GPIO's "requested" flag serves as a lock and refcount. @@ -52,7 +56,6 @@ static DEFINE_MUTEX(gpio_lookup_lock); static LIST_HEAD(gpio_lookup_list); LIST_HEAD(gpio_chips); - static void gpiochip_free_hogs(struct gpio_chip *chip); static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip); @@ -300,7 +303,7 @@ int gpiochip_add(struct gpio_chip *chip) { unsigned long flags; int status = 0; - unsigned id; + unsigned i; int base = chip->base; struct gpio_desc *descs; @@ -308,6 +311,28 @@ int gpiochip_add(struct gpio_chip *chip) if (!descs) return -ENOMEM; + /* + * The "dev" member of gpiochip is the parent, and the actual + * device is named "device" for historical reasons. + * + * We memset the struct to zero to avoid reentrance issues. + */ + memset(&chip->device, 0, sizeof(chip->device)); + if (chip->dev) { + chip->device.parent = chip->dev; + chip->device.of_node = chip->dev->of_node; + } else { +#ifdef CONFIG_OF_GPIO + /* If the gpiochip has an assigned OF node this takes precedence */ + if (chip->of_node) + chip->device.of_node = chip->of_node; +#endif + } + chip->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL); + dev_set_name(&chip->device, "gpiochip%d", chip->id); + device_initialize(&chip->device); + dev_set_drvdata(&chip->device, chip); + spin_lock_irqsave(&gpio_lock, flags); if (base < 0) { @@ -326,8 +351,8 @@ int gpiochip_add(struct gpio_chip *chip) goto err_free_descs; } - for (id = 0; id < chip->ngpio; id++) { - struct gpio_desc *desc = &descs[id]; + for (i = 0; i < chip->ngpio; i++) { + struct gpio_desc *desc = &descs[i]; desc->chip = chip; @@ -361,16 +386,22 @@ int gpiochip_add(struct gpio_chip *chip) acpi_gpiochip_add(chip); - status = gpiochip_sysfs_register(chip); + status = device_add(&chip->device); if (status) goto err_remove_chip; + status = gpiochip_sysfs_register(chip); + if (status) + goto err_remove_device; + pr_debug("%s: registered GPIOs %d to %d on device: %s\n", __func__, chip->base, chip->base + chip->ngpio - 1, chip->label ? : "generic"); return 0; +err_remove_device: + device_del(&chip->device); err_remove_chip: acpi_gpiochip_remove(chip); gpiochip_free_hogs(chip); @@ -381,6 +412,7 @@ err_remove_from_list: spin_unlock_irqrestore(&gpio_lock, flags); chip->desc = NULL; err_free_descs: + ida_simple_remove(&gpio_ida, chip->id); kfree(descs); /* failures here can mean systems won't boot... */ @@ -426,6 +458,8 @@ void gpiochip_remove(struct gpio_chip *chip) if (requested) dev_crit(chip->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n"); + device_del(&chip->device); + ida_simple_remove(&gpio_ida, chip->id); kfree(chip->desc); chip->desc = NULL; } diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index d1baebf350d8..cf3028eccc4b 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -1,6 +1,7 @@ #ifndef __LINUX_GPIO_DRIVER_H #define __LINUX_GPIO_DRIVER_H +#include <linux/device.h> #include <linux/types.h> #include <linux/module.h> #include <linux/irq.h> @@ -8,8 +9,8 @@ #include <linux/irqdomain.h> #include <linux/lockdep.h> #include <linux/pinctrl/pinctrl.h> +#include <linux/cdev.h> -struct device; struct gpio_desc; struct of_phandle_args; struct device_node; @@ -20,7 +21,9 @@ struct seq_file; /** * struct gpio_chip - abstract a GPIO controller * @label: for diagnostics - * @dev: optional device providing the GPIOs + * @id: numerical ID number for the GPIO chip + * @device: the GPIO device struct. + * @dev: optional parent device (hardware) providing the GPIOs * @cdev: class device used by sysfs interface (may be NULL) * @owner: helps prevent removal of modules exporting active GPIOs * @list: links gpio_chips together for traversal @@ -89,6 +92,8 @@ struct seq_file; */ struct gpio_chip { const char *label; + int id; + struct device device; struct device *dev; struct device *cdev; struct module *owner;
GPIO chips have been around for years, but were never real devices, instead they were piggy-backing on a parent device (such as a platform_device or amba_device) but this was always optional. GPIO chips could also exist without any device at all, with its struct device *dev pointer being set to null. When sysfs was in use, a mock device would be created, with the optional parent assigned, or just floating orphaned with NULL as parent. For a proper userspace ABI we need gpiochips to *always have a populated struct device, so add this in the gpio_chip struct. The name "dev" is unfortunately already take so we use "device" to name it. If sysfs is active, it will use this device as parent, and the former parent device "dev" will be set as parent of the new "device" struct member. From this point on, gpiochips are devices. Cc: Johan Hovold <johan@kernel.org> Cc: Michael Welling <mwelling@ieee.org> Cc: Markus Pargmann <mpa@pengutronix.de> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpio/gpiolib-sysfs.c | 2 +- drivers/gpio/gpiolib.c | 44 +++++++++++++++++++++++++++++++++++++++----- include/linux/gpio/driver.h | 9 +++++++-- 3 files changed, 47 insertions(+), 8 deletions(-)