| Message ID | 20210105082758.77762-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] gpiolib: Disallow identical line names in the same chip | expand |
On Tue, Jan 5, 2021 at 10:31 AM Linus Walleij <linus.walleij@linaro.org> wrote: > We need to make this namespace hierarchical: at least do not > allow two lines on the same chip to have the same name, this > is just too much flexibility. If we name a line on a chip, > name it uniquely on that chip. > > This does not affect device tree and other gpiochips that > get named from device properties: the uniqueness > per-chip however affect all hotplugged devices such as > GPIO expanders on USB. ... > [Dropped warning for globally unique] > + * - Allow names to not be globally unique but warn about it. Is the second part of this sentence still ture? Maybe I missed a warning we are talking about here? -- With Best Regards, Andy Shevchenko
On Tue, Jan 5, 2021 at 6:28 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Jan 5, 2021 at 10:31 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > We need to make this namespace hierarchical: at least do not > > allow two lines on the same chip to have the same name, this > > is just too much flexibility. If we name a line on a chip, > > name it uniquely on that chip. > > > > This does not affect device tree and other gpiochips that > > get named from device properties: the uniqueness > > per-chip however affect all hotplugged devices such as > > GPIO expanders on USB. > > ... > > > [Dropped warning for globally unique] > > > + * - Allow names to not be globally unique but warn about it. > > Is the second part of this sentence still ture? > Maybe I missed a warning we are talking about here? Oops old text, Bartosz if this looks OK otherwise can you fix this when applying? (Just delete that line.) Yours, Linus Walleij
On Wed, Jan 6, 2021 at 12:24 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jan 5, 2021 at 6:28 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Tue, Jan 5, 2021 at 10:31 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > We need to make this namespace hierarchical: at least do not > > > allow two lines on the same chip to have the same name, this > > > is just too much flexibility. If we name a line on a chip, > > > name it uniquely on that chip. > > > > > > This does not affect device tree and other gpiochips that > > > get named from device properties: the uniqueness > > > per-chip however affect all hotplugged devices such as > > > GPIO expanders on USB. > > > > ... > > > > > [Dropped warning for globally unique] > > > > > + * - Allow names to not be globally unique but warn about it. > > > > Is the second part of this sentence still ture? > > Maybe I missed a warning we are talking about here? > > Oops old text, Bartosz if this looks OK otherwise can you fix > this when applying? (Just delete that line.) > > Yours, > Linus Walleij I can do it alright. But in the context of user-space I think this doesn't really change anything. DT users still can use non-unique names and libgpiod still has to account for that if the API is to be considered correct. Is this change really useful? How does it affect ACPI users that already define non-unique names? Bartosz
On Wed, Jan 6, 2021 at 12:09 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > On Wed, Jan 6, 2021 at 12:24 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jan 5, 2021 at 6:28 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Tue, Jan 5, 2021 at 10:31 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > We need to make this namespace hierarchical: at least do not > > > > allow two lines on the same chip to have the same name, this > > > > is just too much flexibility. If we name a line on a chip, > > > > name it uniquely on that chip. > > > > > > > > This does not affect device tree and other gpiochips that > > > > get named from device properties: the uniqueness > > > > per-chip however affect all hotplugged devices such as > > > > GPIO expanders on USB. > > > > > > ... > > > > > > > [Dropped warning for globally unique] > > > > > > > + * - Allow names to not be globally unique but warn about it. > > > > > > Is the second part of this sentence still ture? > > > Maybe I missed a warning we are talking about here? > > > > Oops old text, Bartosz if this looks OK otherwise can you fix > > this when applying? (Just delete that line.) > I can do it alright. But in the context of user-space I think this > doesn't really change anything. DT users still can use non-unique > names and libgpiod still has to account for that if the API is to be > considered correct. Is this change really useful? IMHO it is useful and the earliest we do the better. > How does it affect > ACPI users that already define non-unique names? I suppose that in ACPI we don't have many users that do it on their own (for IoT Intel platforms GPIO expanders have unique names). Also see above. I prefer to have a bug report with a clear source of the issue (like a table that the user can't / won't change which predates the date of kernel release with a patch. +cc: to the user who lately was active in the area. Flavio, perhaps one more rule to the gpio-line-names property has to be added into documentation (Bart, same to DT docs?): - names inside one chip must be unique -- With Best Regards, Andy Shevchenko
On Wed, Jan 6, 2021 at 2:34 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Jan 6, 2021 at 12:09 PM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: > > On Wed, Jan 6, 2021 at 12:24 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > On Tue, Jan 5, 2021 at 6:28 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Jan 5, 2021 at 10:31 AM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > We need to make this namespace hierarchical: at least do not > > > > > allow two lines on the same chip to have the same name, this > > > > > is just too much flexibility. If we name a line on a chip, > > > > > name it uniquely on that chip. > > > > > > > > > > This does not affect device tree and other gpiochips that > > > > > get named from device properties: the uniqueness > > > > > per-chip however affect all hotplugged devices such as > > > > > GPIO expanders on USB. > > > > > > > > ... > > > > > > > > > [Dropped warning for globally unique] > > > > > > > > > + * - Allow names to not be globally unique but warn about it. > > > > > > > > Is the second part of this sentence still ture? > > > > Maybe I missed a warning we are talking about here? > > > > > > Oops old text, Bartosz if this looks OK otherwise can you fix > > > this when applying? (Just delete that line.) > > > I can do it alright. But in the context of user-space I think this > > doesn't really change anything. DT users still can use non-unique > > names and libgpiod still has to account for that if the API is to be > > considered correct. Is this change really useful? > > IMHO it is useful and the earliest we do the better. > I'm wondering if user-space should make this assumption too then. That a non-unique name is either an error or signifies some special value (N/A). > > How does it affect > > ACPI users that already define non-unique names? > > I suppose that in ACPI we don't have many users that do it on their > own (for IoT Intel platforms GPIO expanders have unique names). > Also see above. I prefer to have a bug report with a clear source of > the issue (like a table that the user can't / won't change which > predates the date of kernel release with a patch. > > +cc: to the user who lately was active in the area. > > Flavio, perhaps one more rule to the gpio-line-names property has to > be added into documentation (Bart, same to DT docs?): > - names inside one chip must be unique > Once we have a proper, core yaml binding for all GPIO devices, we'll be able to even enforce it if we agree on a set of exceptions. Bart
On Wed, Jan 6, 2021 at 4:25 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > On Wed, Jan 6, 2021 at 2:34 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Jan 6, 2021 at 12:09 PM Bartosz Golaszewski > > <bgolaszewski@baylibre.com> wrote: > > > On Wed, Jan 6, 2021 at 12:24 AM Linus Walleij <linus.walleij@linaro.org> wrote: ... > > > I can do it alright. But in the context of user-space I think this > > > doesn't really change anything. DT users still can use non-unique > > > names and libgpiod still has to account for that if the API is to be > > > considered correct. Is this change really useful? > > > > IMHO it is useful and the earliest we do the better. > > I'm wondering if user-space should make this assumption too then. That > a non-unique name is either an error or signifies some special value > (N/A). My understanding that names are basically aliases to the pin numbers inside a chip, so gpiochipX:Y should == gpiochipX:$NAME Obviously we can't guarantee that if there is no uniqueness assumption made. Otherwise the idea behind naming the lines sounds controversial to me. That said, if we allow non-unique names inside one chip, then the name field is basically *informative*, which means we may not take it anyhow as a parameter to any of the tools or anything. > > > How does it affect > > > ACPI users that already define non-unique names? > > > > I suppose that in ACPI we don't have many users that do it on their > > own (for IoT Intel platforms GPIO expanders have unique names). > > Also see above. I prefer to have a bug report with a clear source of > > the issue (like a table that the user can't / won't change which > > predates the date of kernel release with a patch. > > > > +cc: to the user who lately was active in the area. > > > > Flavio, perhaps one more rule to the gpio-line-names property has to > > be added into documentation (Bart, same to DT docs?): > > - names inside one chip must be unique > > > > Once we have a proper, core yaml binding for all GPIO devices, we'll > be able to even enforce it if we agree on a set of exceptions. -- With Best Regards, Andy Shevchenko
On Thu, Jan 7, 2021 at 12:03 PM Flavio Suligoi <f.suligoi@asem.it> wrote: > > Flavio, perhaps one more rule to the gpio-line-names property has to > > be added into documentation (Bart, same to DT docs?): > > - names inside one chip must be unique > > ok, I'll add this rule Thanks! Main point for calling you is that you may tell that what you also assumed when assigned line names in your case(s). -- With Best Regards, Andy Shevchenko
On Wed, Jan 6, 2021 at 11:09 AM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > I can do it alright. But in the context of user-space I think this > doesn't really change anything. DT users still can use non-unique > names and libgpiod still has to account for that if the API is to be > considered correct. Is this change really useful? How does it affect > ACPI users that already define non-unique names? For hardware description instances the problem remains: device tree line-names and device properties can be non-unique. What it solves is to enforce unique line names for gpio chips with the struct gpio_chip .names array set to some names, that each name in this array must be unique. This happens for example when two USB FTDI converters with the same GPIO lines are plugged in. Each chip can have a "TX" line but it can no longer have two "TX" lines. Yours, Linus Walleij
Hi Linus and Bartosz, > On Wed, Jan 6, 2021 at 11:09 AM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: > > > I can do it alright. But in the context of user-space I think this > > doesn't really change anything. DT users still can use non-unique > > names and libgpiod still has to account for that if the API is to be > > considered correct. Is this change really useful? How does it affect > > ACPI users that already define non-unique names? > > For hardware description instances the problem remains: device tree > line-names and device properties can be non-unique. > > What it solves is to enforce unique line names for gpio chips with > the struct gpio_chip .names array set to some names, that each > name in this array must be unique. > > This happens for example when two USB FTDI converters > with the same GPIO lines are plugged in. Each chip can have a > "TX" line but it can no longer have two "TX" lines. > > Yours, > Linus Walleij about the duplicate line names, what do you think about adding to the command "gpiofind" of libgpiod tools, the possibility to discover all the duplicate gpio lines? For example, something like the following: # gpiofind button_1 gpiochip0 20 gpiochip0 22 (duplicate) gpiochip2 12 (duplicate) # Best regards, Flavio Suligoi
Hi Flavio, On Thu, Jan 7, 2021 at 2:49 PM Flavio Suligoi <f.suligoi@asem.it> wrote: > > On Wed, Jan 6, 2021 at 11:09 AM Bartosz Golaszewski > > <bgolaszewski@baylibre.com> wrote: > > > I can do it alright. But in the context of user-space I think this > > > doesn't really change anything. DT users still can use non-unique > > > names and libgpiod still has to account for that if the API is to be > > > considered correct. Is this change really useful? How does it affect > > > ACPI users that already define non-unique names? > > > > For hardware description instances the problem remains: device tree > > line-names and device properties can be non-unique. > > > > What it solves is to enforce unique line names for gpio chips with > > the struct gpio_chip .names array set to some names, that each > > name in this array must be unique. > > > > This happens for example when two USB FTDI converters > > with the same GPIO lines are plugged in. Each chip can have a > > "TX" line but it can no longer have two "TX" lines. > > > > Yours, > > Linus Walleij > > about the duplicate line names, what do you think > about adding to the command "gpiofind" of libgpiod tools, > the possibility to discover all the duplicate gpio lines? > > For example, something like the following: > > # gpiofind button_1 > gpiochip0 20 > gpiochip0 22 (duplicate) This cannot happen, as the duplicate is on the same gpiochip. > gpiochip2 12 (duplicate) > # I don't like the "(duplicate)" suffix. It makes scripting harder (and more unsafe). What about outputting only the first one, unless "-a" is specified? # gpiofind -a button_1 gpiochip0 20 gpiochip1 22 gpiochip2 12 # Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Andy, > Flavio, perhaps one more rule to the gpio-line-names property has to > be added into documentation (Bart, same to DT docs?): > - names inside one chip must be unique Just a question: why "inside one chip" only? The name should be unique for all gpiochips, since "gpiofind" stops at the first name match, and doesn't show other gpios names, even if they exist, with the same name, in other gpiochip. I think that a rule as: - names must be unique (avoid duplicates in any gpiochips) > -- > With Best Regards, > Andy Shevchenko Best regards, Flavio Suligoi
Hi Geert, > > > > about the duplicate line names, what do you think > > about adding to the command "gpiofind" of libgpiod tools, > > the possibility to discover all the duplicate gpio lines? > > > > For example, something like the following: > > > > # gpiofind button_1 > > gpiochip0 20 > > gpiochip0 22 (duplicate) > > This cannot happen, as the duplicate is on the same gpiochip. Just a question: I think that a duplicate name can be present both in the same gpiochip and also in different gpiochips. The same gpio line name can be wrongly present on different gpiochips, for example caused by a mistake writing an ACPI table. > > > gpiochip2 12 (duplicate) > > # > > I don't like the "(duplicate)" suffix. It makes scripting harder (and > more unsafe). > What about outputting only the first one, unless "-a" is specified? > > # gpiofind -a button_1 > gpiochip0 20 > gpiochip1 22 > gpiochip2 12 > # Ok, I like the "-a" option! > > Gr{oetje,eeting}s, > > Geert Best regards, Flavio
Hi Flavio, On Fri, Jan 8, 2021 at 3:39 PM Flavio Suligoi <f.suligoi@asem.it> wrote: > > > about the duplicate line names, what do you think > > > about adding to the command "gpiofind" of libgpiod tools, > > > the possibility to discover all the duplicate gpio lines? > > > > > > For example, something like the following: > > > > > > # gpiofind button_1 > > > gpiochip0 20 > > > gpiochip0 22 (duplicate) > > > > This cannot happen, as the duplicate is on the same gpiochip. > > Just a question: I think that a duplicate name can be present > both in the same gpiochip and also in different gpiochips. > The same gpio line name can be wrongly present on different gpiochips, > for example caused by a mistake writing an ACPI table. A duplicate name in the same gpiochip is rejected, as per the patch that started this thread. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, > > > > about the duplicate line names, what do you think > > > > about adding to the command "gpiofind" of libgpiod tools, > > > > the possibility to discover all the duplicate gpio lines? > > > > > > > > For example, something like the following: > > > > > > > > # gpiofind button_1 > > > > gpiochip0 20 > > > > gpiochip0 22 (duplicate) > > > > > > This cannot happen, as the duplicate is on the same gpiochip. > > > > Just a question: I think that a duplicate name can be present > > both in the same gpiochip and also in different gpiochips. > > The same gpio line name can be wrongly present on different gpiochips, > > for example caused by a mistake writing an ACPI table. > > A duplicate name in the same gpiochip is rejected, as per the patch that > started this thread. right, thanks Geert! > > Gr{oetje,eeting}s, > > Geert Best regards, Flavio
On Fri, Jan 8, 2021 at 12:40 PM Flavio Suligoi <f.suligoi@asem.it> wrote: > > Hi Andy, > > > Flavio, perhaps one more rule to the gpio-line-names property has to > > be added into documentation (Bart, same to DT docs?): > > - names inside one chip must be unique > > Just a question: why "inside one chip" only? > The name should be unique for all gpiochips, since "gpiofind" > stops at the first name match, and doesn't show other gpios > names, even if they exist, with the same name, in other gpiochip. > > I think that a rule as: > > - names must be unique (avoid duplicates in any gpiochips) Definitely "no". Simple case: you have 2+ chips of the same functionality. You want to use PinX of each of them. You always don't know ahead of time how many of them you will have at runtime. -- With Best Regards, Andy Shevchenko
On Fri, Jan 8, 2021 at 4:39 PM Flavio Suligoi <f.suligoi@asem.it> wrote: ... > > > For example, something like the following: > > > > > > # gpiofind button_1 > > > gpiochip0 20 > > > gpiochip0 22 (duplicate) > > > > This cannot happen, as the duplicate is on the same gpiochip. > > Just a question: I think that a duplicate name can be present > both in the same gpiochip No. This is against common sense. Can you have the same pin numbers on one chip? > and also in different gpiochips. Yes and it's fine. > The same gpio line name can be wrongly present on different gpiochips, > for example caused by a mistake writing an ACPI table. -- With Best Regards, Andy Shevchenko
On Fri, Jan 8, 2021 at 4:04 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Jan 8, 2021 at 4:39 PM Flavio Suligoi <f.suligoi@asem.it> wrote: > > ... > > > > > For example, something like the following: > > > > > > > > # gpiofind button_1 > > > > gpiochip0 20 > > > > gpiochip0 22 (duplicate) > > > > > > This cannot happen, as the duplicate is on the same gpiochip. > > > > Just a question: I think that a duplicate name can be present > > both in the same gpiochip > > No. This is against common sense. Can you have the same pin numbers on one chip? > You're correct logically but technically this definitely can happen. As the DT examples from qualcomm show: you can have multiple pins being called "nc" for "not connected". I'm still not sure what assumptions user-space can make in this case. Should we have a list of unsupported or illegal names to look up? Sounds sketchy. Bartosz > > and also in different gpiochips. > > Yes and it's fine. > > > The same gpio line name can be wrongly present on different gpiochips, > > for example caused by a mistake writing an ACPI table. > > -- > With Best Regards, > Andy Shevchenko
On Fri, Jan 15, 2021 at 5:48 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Friday, January 15, 2021, Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: >> On Fri, Jan 8, 2021 at 4:04 PM Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >> > On Fri, Jan 8, 2021 at 4:39 PM Flavio Suligoi <f.suligoi@asem.it> wrote: >> > >> > ... >> > >> > > > > For example, something like the following: >> > > > > >> > > > > # gpiofind button_1 >> > > > > gpiochip0 20 >> > > > > gpiochip0 22 (duplicate) >> > > > >> > > > This cannot happen, as the duplicate is on the same gpiochip. >> > > >> > > Just a question: I think that a duplicate name can be present >> > > both in the same gpiochip >> > >> > No. This is against common sense. Can you have the same pin numbers on one chip? >> > >> >> You're correct logically but technically this definitely can happen. >> As the DT examples from qualcomm show: you can have multiple pins >> being called "nc" for "not connected". I'm still not sure what >> assumptions user-space can make in this case. Should we have a list of >> unsupported or illegal names to look up? Sounds sketchy. >> > > NC or “” or something like that should be done solely by framework (or at least be reserved by framework), otherwise it makes no sense to me at all. To be ignored due to gpio_chip.valid_mask? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index b02cc2abd3b6..8c0c8c5306d2 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -330,11 +330,9 @@ static struct gpio_desc *gpio_name_to_desc(const char * const name) /* * Take the names from gc->names and assign them to their GPIO descriptors. - * Warn if a name is already used for a GPIO line on a different GPIO chip. * - * Note that: - * 1. Non-unique names are still accepted, - * 2. Name collisions within the same GPIO chip are not reported. + * - Fail if a name is already used for a GPIO line on the same chip. + * - Allow names to not be globally unique but warn about it. */ static int gpiochip_set_desc_names(struct gpio_chip *gc) { @@ -343,13 +341,14 @@ static int gpiochip_set_desc_names(struct gpio_chip *gc) /* First check all names if they are unique */ for (i = 0; i != gc->ngpio; ++i) { - struct gpio_desc *gpio; + struct gpio_desc *gpiod; - gpio = gpio_name_to_desc(gc->names[i]); - if (gpio) - dev_warn(&gdev->dev, - "Detected name collision for GPIO name '%s'\n", - gc->names[i]); + gpiod = gpio_name_to_desc(gc->names[i]); + if (gpiod && (gpiod->gdev == gdev)) { + dev_err(&gdev->dev, + "GPIO name collision on the same chip, this is not allowed, fix all lines on the chip to have unique names\n"); + return -EEXIST; + } } /* Then add all names to the GPIO descriptors */
We need to make this namespace hierarchical: at least do not allow two lines on the same chip to have the same name, this is just too much flexibility. If we name a line on a chip, name it uniquely on that chip. This does not affect device tree and other gpiochips that get named from device properties: the uniqueness per-chip however affect all hotplugged devices such as GPIO expanders on USB. Cc: Geert Uytterhoeven <geert+renesas@glider.be> Cc: Johan Hovold <johan@kernel.org> Link: https://lore.kernel.org/r/20201212003447.238474-1-linus.walleij@linaro.org [Dropped warning for globally unique] Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v1->v2: - Do NOT enforce unique line names on device tree instances. Devicetrees know what they are doing and will make sure not to create conflicting naming. --- drivers/gpio/gpiolib.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) -- 2.29.2