| Message ID | 20201209161821.92931-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: sysfs: Try numbered exports if symbolic names fail | expand |
On Wed, Dec 09, 2020 at 05:18:21PM +0100, Linus Walleij wrote: > If a GPIO line cannot be exported using a symbolic name from > the .names array in the gpiochip, fall back to using the > "gpioN" naming system instead of just failing. > > Cc: Manivannan Sadhasivam <mani@kernel.org> > Cc: Johan Hovold <johan@kernel.org> > Suggested-by: Johan Hovold <johan@kernel.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpio/gpiolib-sysfs.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c > index 728f6c687182..a5a0e9238217 100644 > --- a/drivers/gpio/gpiolib-sysfs.c > +++ b/drivers/gpio/gpiolib-sysfs.c > @@ -627,10 +627,24 @@ 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, &gdev->dev, > - MKDEV(0, 0), data, gpio_groups, > - ioname ? ioname : "gpio%u", > - desc_to_gpio(desc)); > + /* > + * If we have a symbolic name for the GPIO we try to use that > + * for the exported sysfs device/file, as legacy scripts depend > + * on it. If we don't have a symbolic name or if there is a > + * namespace collision, we stick with the "gpioN" name. > + */ > + dev = NULL; > + if (ioname) > + dev = device_create_with_groups(&gpio_class, &gdev->dev, > + MKDEV(0, 0), data, gpio_groups, > + ioname, > + desc_to_gpio(desc)); > + if (IS_ERR_OR_NULL(dev)) > + dev = device_create_with_groups(&gpio_class, &gdev->dev, > + MKDEV(0, 0), data, gpio_groups, > + "gpio%u", > + desc_to_gpio(desc)); I suggested having the driver set a flag which determines whether to use the line names in sysfs or not. The above will trigger a bunch of nasty warnings and backtraces in the sysfs code (for every gpio line!), which is not something we want for normal operation. Having the sysfs interface for the same USB device depend on probe order is not very nice either. Since the USB GPIO controller do not register any names today (as gpiolib currently require a flat name space), there's no need to worry about legacy scripts depending on those either (or rather, the argument goes the other way since adding names now could break a functioning script). Just add a flag to suppress the renaming and we can safely start adding names to hotpluggable controllers (if the rest of gpiolib can handle non-unique names). Johan
On Thu, Dec 10, 2020 at 9:33 AM Johan Hovold <johan@kernel.org> wrote: > I suggested having the driver set a flag which determines whether to use > the line names in sysfs or not. Aha I get it. I need to think about if I can fix that in some good way. > The above will trigger a bunch of nasty warnings and backtraces in the > sysfs code (for every gpio line!), which is not something we want for > normal operation. At this point I feel any use of sysfs kind of deserves that but OK it's a bit nasty. > Having the sysfs interface for the same USB device > depend on probe order is not very nice either. The sysfs for a USB device is already very dependent on probe order. Since all dynamic gpio_chips pass -1 as base they will be allocated some global GPIO numbers at random (well, semi-random) depending on probe order. The user will not have any idea whatsoever what to echo into the sysfs export file without inspecting other things such as debugfs. That's how unstable this interface is, and one of the reasons we are trying to get rid of the global GPIO numberspace to begin with... Maybe that is actually an argument for any multi-instance GPIO devices to depends on !GPIO_SYSFS It's a sad excuse for an ABI, the form it has was maybe acceptable in debugfs. > Since the USB GPIO controller do not register any names today (as > gpiolib currently require a flat name space), there's no need to worry > about legacy scripts depending on those either (or rather, the argument > goes the other way since adding names now could break a functioning > script). OK I get how you think this should work. > Just add a flag to suppress the renaming and we can safely start adding > names to hotpluggable controllers (if the rest of gpiolib can handle > non-unique names). I'll see if I can think of something clean enough. I don't really want to add fields into the struct gpio_chip pertaining to a legacy ABI. Yours, Linus Walleij
On Sat, Dec 12, 2020 at 12:41:50AM +0100, Linus Walleij wrote: > On Thu, Dec 10, 2020 at 9:33 AM Johan Hovold <johan@kernel.org> wrote: > > > I suggested having the driver set a flag which determines whether to use > > the line names in sysfs or not. > > Aha I get it. > > I need to think about if I can fix that in some good way. > > > The above will trigger a bunch of nasty warnings and backtraces in the > > sysfs code (for every gpio line!), which is not something we want for > > normal operation. > > At this point I feel any use of sysfs kind of deserves that but OK > it's a bit nasty. It would be a bug in gpiolib as the warnings are due to collisions when trying to register two sysfs files using the same path. > > Having the sysfs interface for the same USB device > > depend on probe order is not very nice either. > > The sysfs for a USB device is already very dependent on probe order. > Since all dynamic gpio_chips pass -1 as base they will be allocated > some global GPIO numbers at random (well, semi-random) > depending on probe order. > > The user will not have any idea whatsoever what to echo into the sysfs > export file without inspecting other things such as debugfs. > That's how unstable this interface is, and one of the reasons we > are trying to get rid of the global GPIO numberspace to begin with... That's not true. The gpiochip has a "base" sysfs attribute from which any user can construct the gpio number by simply adding an offset. Having the lines sometimes show up using their names and sometimes using this number, is what would be a problem. > Maybe that is actually an argument for any multi-instance GPIO > devices to > depends on !GPIO_SYSFS No, not at all. Just suppress the rename of the sysfs directories based on line names, which was a bad idea from the start (the names should have been exported through a separate name attribute). Johan
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c index 728f6c687182..a5a0e9238217 100644 --- a/drivers/gpio/gpiolib-sysfs.c +++ b/drivers/gpio/gpiolib-sysfs.c @@ -627,10 +627,24 @@ 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, &gdev->dev, - MKDEV(0, 0), data, gpio_groups, - ioname ? ioname : "gpio%u", - desc_to_gpio(desc)); + /* + * If we have a symbolic name for the GPIO we try to use that + * for the exported sysfs device/file, as legacy scripts depend + * on it. If we don't have a symbolic name or if there is a + * namespace collision, we stick with the "gpioN" name. + */ + dev = NULL; + if (ioname) + dev = device_create_with_groups(&gpio_class, &gdev->dev, + MKDEV(0, 0), data, gpio_groups, + ioname, + desc_to_gpio(desc)); + if (IS_ERR_OR_NULL(dev)) + dev = device_create_with_groups(&gpio_class, &gdev->dev, + MKDEV(0, 0), data, gpio_groups, + "gpio%u", + desc_to_gpio(desc)); + if (IS_ERR(dev)) { status = PTR_ERR(dev); goto err_free_data;
If a GPIO line cannot be exported using a symbolic name from the .names array in the gpiochip, fall back to using the "gpioN" naming system instead of just failing. Cc: Manivannan Sadhasivam <mani@kernel.org> Cc: Johan Hovold <johan@kernel.org> Suggested-by: Johan Hovold <johan@kernel.org> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpio/gpiolib-sysfs.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)