[PATCH] gpiolib: Disallow identical line names in the same chip

[PATCH] gpiolib: Disallow identical line names in the same chip

[Linus Walleij]

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.

I don't know what happens if we just apply this, I *hope* there
are not a lot of systems out there breaking this simple and
intuitive rule.

As a side effect, this makes the device tree naming code
scream a bit if names are not globally unique.

I think there are not super-many device trees out there naming
their lines so let's fix this before the problem becomes
widespread.

Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Johan Hovold <johan@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
This may be just the first step in tightening this up.
Googling gives at hand that the colission warning doesn't
happen much so we might go as far as to say the name can
be required to be globally unique, but that creates a flat
namespace so I don't know if that is desireable.
---
 drivers/gpio/gpiolib.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5ce0c14c637b..be4b3e9032b2 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,19 @@ 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)
+		gpiod = gpio_name_to_desc(gc->names[i]);
+		if (gpiod) {
 			dev_warn(&gdev->dev,
 				 "Detected name collision for GPIO name '%s'\n",
 				 gc->names[i]);
+			if (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 -EINVAL;
+			}
+		}
 	}
 
 	/* Then add all names to the GPIO descriptors */
@@ -402,8 +406,22 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip)
 		return ret;
 	}
 
-	for (i = 0; i < count; i++)
+	for (i = 0; i < count; i++) {
+		struct gpio_desc *gpiod;
+
+		gpiod = gpio_name_to_desc(names[i]);
+		if (gpiod) {
+			dev_warn(&gdev->dev,
+                                 "Detected name collision for GPIO name '%s'\n",
+                                 names[i]);
+			if (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 -EINVAL;
+			}
+		}
 		gdev->descs[i].name = names[i];
+	}
 
 	kfree(names);
 
-- 
2.28.0

Re: [PATCH] gpiolib: Disallow identical line names in the same chip

[Geert Uytterhoeven]

Hi Linus,

On Sat, Dec 12, 2020 at 1:36 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.
>
> I don't know what happens if we just apply this, I *hope* there
> are not a lot of systems out there breaking this simple and
> intuitive rule.
>
> As a side effect, this makes the device tree naming code
> scream a bit if names are not globally unique.
>
> I think there are not super-many device trees out there naming
> their lines so let's fix this before the problem becomes
> widespread.
>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> This may be just the first step in tightening this up.
> Googling gives at hand that the colission warning doesn't
> happen much so we might go as far as to say the name can
> be required to be globally unique, but that creates a flat
> namespace so I don't know if that is desireable.

As the names are specified in DT, I think the biggest "use case" for
collisions is GPIO chips on expansion boards, if multiple instances
of the same board can be connected.

This is a bit similar to clock outputs, cfr. commit f491276a51685987
("clk: vc5: Allow Versaclock driver to support multiple instances"), but
in the clock case, the name of the clock output is dictated by the
driver, not by DT.

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

Re: [PATCH] gpiolib: Disallow identical line names in the same chip

[Linus Walleij]

On Sat, Dec 12, 2020 at 10:23 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

> As the names are specified in DT, I think the biggest "use case" for
> collisions is GPIO chips on expansion boards, if multiple instances
> of the same board can be connected.

The actual case that happens in reality is GPIO chips on USB.
For example if you plug in two FTDI adapters (these have GPIOs)
and they have named their lines statically in the driver.

This is what Johan points out and also what I find from Googling.

So I would say hot-pluggable buses. Greybus would have the
same issue I think.

> This is a bit similar to clock outputs, cfr. commit f491276a51685987
> ("clk: vc5: Allow Versaclock driver to support multiple instances"), but
> in the clock case, the name of the clock output is dictated by the
> driver, not by DT.

Yeah actually the collisions we have seen in GPIO is the same
type, where we assign the names in gc->names and not in the
device tree (or ACPI).

But I think it is good to establish this habit already so we don't
end up depending on having to support flat namespaces with
several lines on the same chip named the same at least.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: Disallow identical line names in the same chip

[Johan Hovold]

On Sat, Dec 12, 2020 at 01:51:59PM +0100, Linus Walleij wrote:
> On Sat, Dec 12, 2020 at 10:23 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> 
> > As the names are specified in DT, I think the biggest "use case" for
> > collisions is GPIO chips on expansion boards, if multiple instances
> > of the same board can be connected.
> 
> The actual case that happens in reality is GPIO chips on USB.
> For example if you plug in two FTDI adapters (these have GPIOs)
> and they have named their lines statically in the driver.
> 
> This is what Johan points out and also what I find from Googling.
> 
> So I would say hot-pluggable buses. Greybus would have the
> same issue I think.

But it could be anything that provides default names, such as some
controller connected over i2c.

Having the driver provide default names that then can (but does not have
to be) overridden in devicetree if you really want to make sure the
lines are unique (or just named for the particular application).
 
> > This is a bit similar to clock outputs, cfr. commit f491276a51685987
> > ("clk: vc5: Allow Versaclock driver to support multiple instances"), but
> > in the clock case, the name of the clock output is dictated by the
> > driver, not by DT.
> 
> Yeah actually the collisions we have seen in GPIO is the same
> type, where we assign the names in gc->names and not in the
> device tree (or ACPI).
> 
> But I think it is good to establish this habit already so we don't
> end up depending on having to support flat namespaces with

I think you meant non-flat here?

> several lines on the same chip named the same at least.

Right, enforcing per-chip-unique names seems perfectly reasonable.

The only "use case" of non-unique names that comes to mind would be to
provide some kind of grouping like "input"/"output" but then the names
can still be made unique by adding an index (e.g. "input0").

Johan

Re: [PATCH] gpiolib: Disallow identical line names in the same chip

[Andy Shevchenko]

On Sun, Dec 13, 2020 at 3:56 PM 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.
>
> I don't know what happens if we just apply this, I *hope* there
> are not a lot of systems out there breaking this simple and
> intuitive rule.
>
> As a side effect, this makes the device tree naming code
> scream a bit if names are not globally unique.
>
> I think there are not super-many device trees out there naming
> their lines so let's fix this before the problem becomes
> widespread.

I think it is a right direction but the name space should be attached
to the GPIO chip, globally it may be several GPIO chips on some boards
which are doing the same thing semantically. So, the compound of
gpiochipN:name should be unique globally.

...

> +                               return -EINVAL;

> +                               return -EINVAL;

A nit: -EEXIST ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib: Disallow identical line names in the same chip

[Bartosz Golaszewski]

On Sun, Dec 13, 2020 at 4:00 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Dec 13, 2020 at 3:56 PM 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.
> >
> > I don't know what happens if we just apply this, I *hope* there
> > are not a lot of systems out there breaking this simple and
> > intuitive rule.
> >
> > As a side effect, this makes the device tree naming code
> > scream a bit if names are not globally unique.
> >
> > I think there are not super-many device trees out there naming
> > their lines so let's fix this before the problem becomes
> > widespread.
>
> I think it is a right direction but the name space should be attached
> to the GPIO chip, globally it may be several GPIO chips on some boards
> which are doing the same thing semantically. So, the compound of
> gpiochipN:name should be unique globally.

I too like the idea of having the line names unique per chip. It'll
also make things easier for libgpiod.

Bartosz

>
> ...
>
> > +                               return -EINVAL;
>
> > +                               return -EINVAL;
>
> A nit: -EEXIST ?
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] gpiolib: Disallow identical line names in the same chip

[Linus Walleij]

On Sat, Dec 12, 2020 at 1:36 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.
>
> I don't know what happens if we just apply this, I *hope* there
> are not a lot of systems out there breaking this simple and
> intuitive rule.
>
> As a side effect, this makes the device tree naming code
> scream a bit if names are not globally unique.
>
> I think there are not super-many device trees out there naming
> their lines so let's fix this before the problem becomes
> widespread.
>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

I applied this patch for v5.11 with replacing -EINVAL with -EEXIST
as suggested by Andy.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: Disallow identical line names in the same chip

[Johan Hovold]

On Sat, Dec 12, 2020 at 01:34:47AM +0100, Linus Walleij 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.
> 
> I don't know what happens if we just apply this, I *hope* there
> are not a lot of systems out there breaking this simple and
> intuitive rule.
> 
> As a side effect, this makes the device tree naming code
> scream a bit if names are not globally unique.
> 
> I think there are not super-many device trees out there naming
> their lines so let's fix this before the problem becomes
> widespread.
> 
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> This may be just the first step in tightening this up.
> Googling gives at hand that the colission warning doesn't
> happen much so we might go as far as to say the name can
> be required to be globally unique, but that creates a flat
> namespace so I don't know if that is desireable.
> ---
>  drivers/gpio/gpiolib.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 5ce0c14c637b..be4b3e9032b2 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.

As I mentioned earlier, the not-globally unique warning only makes sense
for cases where all names comes from something like devicetree, and we
shouldn't warn if we plug in two identical USB devices that provide
default line names.

Not sure it's even warranted to warn even if those default names collide
with the names provided in devicetree. Perhaps best to just drop the
flat-namespace assumption entirely and make sure gpiolib and the
userspace library can handle that.

Johan

Re: [PATCH] gpiolib: Disallow identical line names in the same chip

[Linus Walleij]

On Mon, Dec 14, 2020 at 10:29 AM Johan Hovold <johan@kernel.org> wrote:

> As I mentioned earlier, the not-globally unique warning only makes sense
> for cases where all names comes from something like devicetree, and we
> shouldn't warn if we plug in two identical USB devices that provide
> default line names.
>
> Not sure it's even warranted to warn even if those default names collide
> with the names provided in devicetree. Perhaps best to just drop the
> flat-namespace assumption entirely and make sure gpiolib and the
> userspace library can handle that.

You're right. (As usual.)

I dropped the global collision warning when applying.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: Disallow identical line names in the same chip

[Geert Uytterhoeven]

Hi Linus,

On Mon, Dec 14, 2020 at 11:40 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Dec 14, 2020 at 10:29 AM Johan Hovold <johan@kernel.org> wrote:
> > As I mentioned earlier, the not-globally unique warning only makes sense
> > for cases where all names comes from something like devicetree, and we
> > shouldn't warn if we plug in two identical USB devices that provide
> > default line names.
> >
> > Not sure it's even warranted to warn even if those default names collide
> > with the names provided in devicetree. Perhaps best to just drop the
> > flat-namespace assumption entirely and make sure gpiolib and the
> > userspace library can handle that.
>
> You're right. (As usual.)
>
> I dropped the global collision warning when applying.

Commit 2cd64ae98f352873 ("gpiolib: Disallow identical line
names in the same chip") still mentions the global collision warning
in the comments:

    Allow names to not be globally unique but warn about it

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