| Message ID | 20230113205922.2312951-1-andreas@kemnade.info |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: omap: use dynamic allocation of base | expand |
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
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
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
* 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
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.
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
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.
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;
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(-)