[PATCH] gpio: omap: use dynamic allocation of base

[PATCH] gpio: omap: use dynamic allocation of base

[Andreas Kemnade]

Static allocatin is deprecated and may cause probe mess,
if probe order is unusual.

like this example
[    2.553833] twl4030_gpio twl4030-gpio: gpio (irq 145) chaining IRQs 161..178
[    2.561401] gpiochip_find_base: found new base at 160
[    2.564392] gpio gpiochip5: (twl4030): added GPIO chardev (254:5)
[    2.564544] gpio gpiochip5: registered GPIOs 160 to 177 on twl4030
[...]
[    2.692169] omap-gpmc 6e000000.gpmc: GPMC revision 5.0
[    2.697357] gpmc_mem_init: disabling cs 0 mapped at 0x0-0x1000000
[    2.703643] gpiochip_find_base: found new base at 178
[    2.704376] gpio gpiochip6: (omap-gpmc): added GPIO chardev (254:6)
[    2.704589] gpio gpiochip6: registered GPIOs 178 to 181 on omap-gpmc
[...]
[    2.840393] gpio gpiochip7: Static allocation of GPIO base is deprecated, use dynamic allocation.
[    2.849365] gpio gpiochip7: (gpio-160-191): GPIO integer space overlap, cannot add chip
[    2.857513] gpiochip_add_data_with_key: GPIOs 160..191 (gpio-160-191) failed to register, -16
[    2.866149] omap_gpio 48310000.gpio: error -EBUSY: Could not register gpio chip

So probing was done in an unusual order, causing mess
and chips not getting their gpio in the end.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
maybe CC stable? not sure about good fixes tag.

 drivers/gpio/gpio-omap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 80ddc43fd875..f5f3d4b22452 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1020,7 +1020,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc,
 		if (!label)
 			return -ENOMEM;
 		bank->chip.label = label;
-		bank->chip.base = gpio;
+		bank->chip.base = -1;
 	}
 	bank->chip.ngpio = bank->width;
 
-- 
2.30.2

Re: [PATCH] gpio: omap: use dynamic allocation of base

[Bartosz Golaszewski]

On Fri, Jan 13, 2023 at 9:59 PM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> Static allocatin is deprecated and may cause probe mess,
> if probe order is unusual.
>
> like this example
> [    2.553833] twl4030_gpio twl4030-gpio: gpio (irq 145) chaining IRQs 161..178
> [    2.561401] gpiochip_find_base: found new base at 160
> [    2.564392] gpio gpiochip5: (twl4030): added GPIO chardev (254:5)
> [    2.564544] gpio gpiochip5: registered GPIOs 160 to 177 on twl4030
> [...]
> [    2.692169] omap-gpmc 6e000000.gpmc: GPMC revision 5.0
> [    2.697357] gpmc_mem_init: disabling cs 0 mapped at 0x0-0x1000000
> [    2.703643] gpiochip_find_base: found new base at 178
> [    2.704376] gpio gpiochip6: (omap-gpmc): added GPIO chardev (254:6)
> [    2.704589] gpio gpiochip6: registered GPIOs 178 to 181 on omap-gpmc
> [...]
> [    2.840393] gpio gpiochip7: Static allocation of GPIO base is deprecated, use dynamic allocation.
> [    2.849365] gpio gpiochip7: (gpio-160-191): GPIO integer space overlap, cannot add chip
> [    2.857513] gpiochip_add_data_with_key: GPIOs 160..191 (gpio-160-191) failed to register, -16
> [    2.866149] omap_gpio 48310000.gpio: error -EBUSY: Could not register gpio chip
>
> So probing was done in an unusual order, causing mess
> and chips not getting their gpio in the end.
>
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> maybe CC stable? not sure about good fixes tag.
>
>  drivers/gpio/gpio-omap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 80ddc43fd875..f5f3d4b22452 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1020,7 +1020,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc,
>                 if (!label)
>                         return -ENOMEM;
>                 bank->chip.label = label;
> -               bank->chip.base = gpio;
> +               bank->chip.base = -1;
>         }
>         bank->chip.ngpio = bank->width;
>
> --
> 2.30.2
>

This could potentially break some legacy user-space programs using
sysfs but whatever, let's apply it and see if anyone complains.

Bart

Re: [PATCH] gpio: omap: use dynamic allocation of base

[Linus Walleij]

On Fri, Jan 13, 2023 at 9:59 PM Andreas Kemnade <andreas@kemnade.info> wrote:

> Static allocatin is deprecated and may cause probe mess,
> if probe order is unusual.
>
> like this example
> [    2.553833] twl4030_gpio twl4030-gpio: gpio (irq 145) chaining IRQs 161..178
> [    2.561401] gpiochip_find_base: found new base at 160
> [    2.564392] gpio gpiochip5: (twl4030): added GPIO chardev (254:5)
> [    2.564544] gpio gpiochip5: registered GPIOs 160 to 177 on twl4030
> [...]
> [    2.692169] omap-gpmc 6e000000.gpmc: GPMC revision 5.0
> [    2.697357] gpmc_mem_init: disabling cs 0 mapped at 0x0-0x1000000
> [    2.703643] gpiochip_find_base: found new base at 178
> [    2.704376] gpio gpiochip6: (omap-gpmc): added GPIO chardev (254:6)
> [    2.704589] gpio gpiochip6: registered GPIOs 178 to 181 on omap-gpmc
> [...]
> [    2.840393] gpio gpiochip7: Static allocation of GPIO base is deprecated, use dynamic allocation.
> [    2.849365] gpio gpiochip7: (gpio-160-191): GPIO integer space overlap, cannot add chip
> [    2.857513] gpiochip_add_data_with_key: GPIOs 160..191 (gpio-160-191) failed to register, -16
> [    2.866149] omap_gpio 48310000.gpio: error -EBUSY: Could not register gpio chip
>
> So probing was done in an unusual order, causing mess
> and chips not getting their gpio in the end.
>
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>

Dangerous but beautiful change. Let's be brave.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> maybe CC stable? not sure about good fixes tag.

I wouldn't do that from the outset. If there are no problems
for a few kernel releases we can think about doing that.

Yours,
Linus Walleij

Re: [PATCH] gpio: omap: use dynamic allocation of base

[Andreas Kemnade]

Hi,

On Mon, 16 Jan 2023 15:24:42 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> > maybe CC stable? not sure about good fixes tag.  
> 
> I wouldn't do that from the outset. If there are no problems
> for a few kernel releases we can think about doing that.

I have the impression that numbering somehow changed here.
In earlier kernel, omap_gpmc started at >400 and gpio-twl4030 also
(both base = -1 now), so no conflicts with the static allocation of
the soc-gpios.  I have not investigated/bisected yet. But perhaps
additionally, a patch ensuring that dynamic allocation starts at
a higher number to not interfer with static numbering with be interesting.

That could then be more easily backportable.

Regards,
Andreas 

Re: [PATCH] gpio: omap: use dynamic allocation of base

[Tony Lindgren]

* Bartosz Golaszewski <brgl@bgdev.pl> [230116 08:38]:
> On Fri, Jan 13, 2023 at 9:59 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index 80ddc43fd875..f5f3d4b22452 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -1020,7 +1020,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc,
> >                 if (!label)
> >                         return -ENOMEM;
> >                 bank->chip.label = label;
> > -               bank->chip.base = gpio;
> > +               bank->chip.base = -1;
> >         }
> >         bank->chip.ngpio = bank->width;
> >
> > --
> > 2.30.2
> >
> 
> This could potentially break some legacy user-space programs using
> sysfs but whatever, let's apply it and see if anyone complains.

Worth a try for sure, fingers crossed. I guess /sys/class/gpio will
break at least.

Regards,

Tony

Re: [PATCH] gpio: omap: use dynamic allocation of base

[Richard Weinberger]

On Mon, Jan 16, 2023 at 9:54 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Fri, Jan 13, 2023 at 9:59 PM Andreas Kemnade <andreas@kemnade.info> wrote:
> >
> > Static allocatin is deprecated and may cause probe mess,
> > if probe order is unusual.
> >
> > like this example
> > [    2.553833] twl4030_gpio twl4030-gpio: gpio (irq 145) chaining IRQs 161..178
> > [    2.561401] gpiochip_find_base: found new base at 160
> > [    2.564392] gpio gpiochip5: (twl4030): added GPIO chardev (254:5)
> > [    2.564544] gpio gpiochip5: registered GPIOs 160 to 177 on twl4030
> > [...]
> > [    2.692169] omap-gpmc 6e000000.gpmc: GPMC revision 5.0
> > [    2.697357] gpmc_mem_init: disabling cs 0 mapped at 0x0-0x1000000
> > [    2.703643] gpiochip_find_base: found new base at 178
> > [    2.704376] gpio gpiochip6: (omap-gpmc): added GPIO chardev (254:6)
> > [    2.704589] gpio gpiochip6: registered GPIOs 178 to 181 on omap-gpmc
> > [...]
> > [    2.840393] gpio gpiochip7: Static allocation of GPIO base is deprecated, use dynamic allocation.
> > [    2.849365] gpio gpiochip7: (gpio-160-191): GPIO integer space overlap, cannot add chip
> > [    2.857513] gpiochip_add_data_with_key: GPIOs 160..191 (gpio-160-191) failed to register, -16
> > [    2.866149] omap_gpio 48310000.gpio: error -EBUSY: Could not register gpio chip
> >
> > So probing was done in an unusual order, causing mess
> > and chips not getting their gpio in the end.
> >
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> > ---
> > maybe CC stable? not sure about good fixes tag.
> >
> >  drivers/gpio/gpio-omap.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index 80ddc43fd875..f5f3d4b22452 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -1020,7 +1020,7 @@ static int omap_gpio_chip_init(struct gpio_bank *bank, struct irq_chip *irqc,
> >                 if (!label)
> >                         return -ENOMEM;
> >                 bank->chip.label = label;
> > -               bank->chip.base = gpio;
> > +               bank->chip.base = -1;
> >         }
> >         bank->chip.ngpio = bank->width;
> >
> > --
> > 2.30.2
> >
>
> This could potentially break some legacy user-space programs using
> sysfs but whatever, let's apply it and see if anyone complains.

FWIW, this broke users pace on my side.
Not a super big deal, I'll just revert this patch for now.
Maybe the application in question can get rewritten to find the gpio by label.

-- 
Thanks,
//richard

Re: [PATCH] gpio: omap: use dynamic allocation of base

[Linus Walleij]

On Thu, Aug 29, 2024 at 10:52 AM Richard Weinberger
<richard.weinberger@gmail.com> wrote:

> > This could potentially break some legacy user-space programs using
> > sysfs but whatever, let's apply it and see if anyone complains.
>
> FWIW, this broke users pace on my side.
> Not a super big deal, I'll just revert this patch for now.
> Maybe the application in question can get rewritten to find the gpio by label.

Ugh we might need to back this out if the userspace is critical
and you need it.

Ideally userspace should use libgpiod for GPIO access, but I understand
if it's a higher bar.

Yours,
Linus Walleij

Re: [PATCH] gpio: omap: use dynamic allocation of base

[Richard Weinberger]

Linus,

On Sat, Aug 31, 2024 at 12:47 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Aug 29, 2024 at 10:52 AM Richard Weinberger
> <richard.weinberger@gmail.com> wrote:
>
> > > This could potentially break some legacy user-space programs using
> > > sysfs but whatever, let's apply it and see if anyone complains.
> >
> > FWIW, this broke users pace on my side.
> > Not a super big deal, I'll just revert this patch for now.
> > Maybe the application in question can get rewritten to find the gpio by label.
>
> Ugh we might need to back this out if the userspace is critical
> and you need it.
>
> Ideally userspace should use libgpiod for GPIO access, but I understand
> if it's a higher bar.

In the meanwhile I've explained to everyone involved on the project
that gpiod is the way
to go and the application will get changed. So, no worries. :-)

But I'm not sure about other applications in the wild, writing a
number to /sys/class/gpio/export is just too easy
and people assume the numbers are stable.

-- 
Thanks,
//richard