* [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) @ 2021-12-17 15:35 Marcelo Roberto Jimenez 2021-12-18 6:28 ` Thorsten Leemhuis ` (2 more replies) 0 siblings, 3 replies; 28+ messages in thread From: Marcelo Roberto Jimenez @ 2021-12-17 15:35 UTC (permalink / raw) To: stable, regressions, Linus Walleij, Bartosz Golaszewski, linux-gpio, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli Cc: Marcelo Roberto Jimenez Some GPIO lines have stopped working after the patch commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") And this has supposedly been fixed in the following patches commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") But an erratic behavior where some GPIO lines work while others do not work has been introduced. This patch reverts those changes so that the sysfs-gpio interface works properly again. Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> --- Hi, My system is ARM926EJ-S rev 5 (v5l) (AT91SAM9G25), the board is an ACME Systems Arietta. The system used sysfs-gpio to manage a few gpio lines, and I have noticed that some have stopped working. The test script is very simple: #! /bin/bash cd /sys/class/gpio/ echo 24 > export cd pioA24 echo out > direction echo 0 > value cat value echo 1 > value cat value echo 0 > value cat value echo 1 > value cat value cd .. echo 24 > unexport In a "good" kernel, this script outputs 0, 1, 0, 1. In a bad kernel, the output result is 1, 1, 1, 1. Also it must be possible to run this script twice without errors, that was the issue with the gpiochip_generic_free() call that had been addressed in another patch. In my system PINCTRL is automatically selected by SOC_AT91SAM9 [=y] && ARCH_AT91 [=y] && ARCH_MULTI_V5 [=y] So it is not an option to disable it to make it work. Best regards, Marcelo. drivers/gpio/gpiolib.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index af5bb8fedfea..ac69ec8fb37a 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1804,11 +1804,6 @@ static inline void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gc) */ int gpiochip_generic_request(struct gpio_chip *gc, unsigned offset) { -#ifdef CONFIG_PINCTRL - if (list_empty(&gc->gpiodev->pin_ranges)) - return 0; -#endif - return pinctrl_gpio_request(gc->gpiodev->base + offset); } EXPORT_SYMBOL_GPL(gpiochip_generic_request); @@ -1820,11 +1815,6 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_request); */ void gpiochip_generic_free(struct gpio_chip *gc, unsigned offset) { -#ifdef CONFIG_PINCTRL - if (list_empty(&gc->gpiodev->pin_ranges)) - return; -#endif - pinctrl_gpio_free(gc->gpiodev->base + offset); } EXPORT_SYMBOL_GPL(gpiochip_generic_free); -- 2.30.2 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2021-12-17 15:35 [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) Marcelo Roberto Jimenez @ 2021-12-18 6:28 ` Thorsten Leemhuis 2021-12-20 14:57 ` Bartosz Golaszewski 2022-02-11 0:02 ` Linus Walleij 2022-03-14 15:55 ` Michael Walle 2 siblings, 1 reply; 28+ messages in thread From: Thorsten Leemhuis @ 2021-12-18 6:28 UTC (permalink / raw) To: Marcelo Roberto Jimenez, stable, regressions, Linus Walleij, Bartosz Golaszewski, linux-gpio, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli [TLDR: I'm adding this regression to regzbot, the Linux kernel regression tracking bot; most text you find below is compiled from a few templates paragraphs some of you might have seen already.] On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: > Some GPIO lines have stopped working after the patch > commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > > And this has supposedly been fixed in the following patches > commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") There seems to be a backstory here. Are there any entries and bug trackers or earlier discussions everyone that looks into this should be aware of? > But an erratic behavior where some GPIO lines work while others do not work > has been introduced. > > This patch reverts those changes so that the sysfs-gpio interface works > properly again. > > Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> > --- > > Hi, > > My system is ARM926EJ-S rev 5 (v5l) (AT91SAM9G25), the board is an ACME Systems Arietta. > > The system used sysfs-gpio to manage a few gpio lines, and I have noticed that some have stopped working. > > The test script is very simple: > > #! /bin/bash > > cd /sys/class/gpio/ > echo 24 > export > > cd pioA24 > echo out > direction > > echo 0 > value > cat value > echo 1 > value > cat value > echo 0 > value > cat value > echo 1 > value > cat value > > cd .. > echo 24 > unexport > > In a "good" kernel, this script outputs 0, 1, 0, 1. In a bad kernel, the output result is 1, 1, 1, 1. Also it must be possible to run this script twice without errors, that was the issue with the gpiochip_generic_free() call that had been addressed in another patch. > > In my system PINCTRL is automatically selected by > SOC_AT91SAM9 [=y] && ARCH_AT91 [=y] && ARCH_MULTI_V5 [=y] > > So it is not an option to disable it to make it work. > > Best regards, > Marcelo. > > > drivers/gpio/gpiolib.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index af5bb8fedfea..ac69ec8fb37a 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1804,11 +1804,6 @@ static inline void gpiochip_irqchip_free_valid_mask(struct gpio_chip *gc) > */ > int gpiochip_generic_request(struct gpio_chip *gc, unsigned offset) > { > -#ifdef CONFIG_PINCTRL > - if (list_empty(&gc->gpiodev->pin_ranges)) > - return 0; > -#endif > - > return pinctrl_gpio_request(gc->gpiodev->base + offset); > } > EXPORT_SYMBOL_GPL(gpiochip_generic_request); > @@ -1820,11 +1815,6 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_request); > */ > void gpiochip_generic_free(struct gpio_chip *gc, unsigned offset) > { > -#ifdef CONFIG_PINCTRL > - if (list_empty(&gc->gpiodev->pin_ranges)) > - return; > -#endif > - > pinctrl_gpio_free(gc->gpiodev->base + offset); > } > EXPORT_SYMBOL_GPL(gpiochip_generic_free); > Hi, this is your Linux kernel regression tracker speaking. Thanks for the report. To be sure this issue doesn't fall through the cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression tracking bot: #regzbot ^introduced 2ab73c6d8323f #regzbot title gpio: some GPIO lines have stopped working #regzbot ignore-activity Reminder: when fixing the issue, please add a 'Link:' tag with the URL to the report (the parent of this mail), as explained in 'Documentation/process/submitting-patches.rst' (reminder: you should use the kernel.org redirector). Regzbot then will automatically mark the regression as resolved once the fix lands in the appropriate tree. For more details about regzbot see footer. Sending this to everyone that got the initial report, to make all aware of the tracking. I also hope that messages like this motivate people to directly get at least the regression mailing list and ideally even regzbot involved when dealing with regressions, as messages like this wouldn't be needed then. Don't worry, I'll send further messages wrt to this regression just to the lists (with a tag in the subject so people can filter them away), as long as they are intended just for regzbot. With a bit of luck no such messages will be needed anyway. Ciao, Thorsten (wearing his 'Linux kernel regression tracker' hat). P.S.: As a Linux kernel regression tracker I'm getting a lot of reports on my table. I can only look briefly into most of them. Unfortunately therefore I sometimes will get things wrong or miss something important. I hope that's not the case here; if you think it is, don't hesitate to tell me about it in a public reply. That's in everyone's interest, as what I wrote above might be misleading to everyone reading this; any suggestion I gave thus might sent someone reading this down the wrong rabbit hole, which none of us wants. BTW, I have no personal interest in this issue, which is tracked using regzbot, my Linux kernel regression tracking bot (https://linux-regtracking.leemhuis.info/regzbot/). I'm only posting this mail to get things rolling again and hence don't need to be CC on all further activities wrt to this regression. --- Additional information about regzbot: If you want to know more about regzbot, check out its web-interface, the getting start guide, and/or the references documentation: https://linux-regtracking.leemhuis.info/regzbot/ https://gitlab.com/knurd42/regzbot/-/blob/main/docs/getting_started.md https://gitlab.com/knurd42/regzbot/-/blob/main/docs/reference.md The last two documents will explain how you can interact with regzbot yourself if your want to. Hint for reporters: when reporting a regression it's in your interest to tell #regzbot about it in the report, as that will ensure the regression gets on the radar of regzbot and the regression tracker. That's in your interest, as they will make sure the report won't fall through the cracks unnoticed. Hint for developers: you normally don't need to care about regzbot once it's involved. Fix the issue as you normally would, just remember to include a 'Link:' tag to the report in the commit message, as explained in Documentation/process/submitting-patches.rst That aspect was recently was made more explicit in commit 1f57bd42b77c: https://git.kernel.org/linus/1f57bd42b77c ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2021-12-18 6:28 ` Thorsten Leemhuis @ 2021-12-20 14:57 ` Bartosz Golaszewski 2021-12-20 15:14 ` Geert Uytterhoeven 2021-12-20 20:41 ` Marcelo Roberto Jimenez 0 siblings, 2 replies; 28+ messages in thread From: Bartosz Golaszewski @ 2021-12-20 14:57 UTC (permalink / raw) To: Marcelo Roberto Jimenez, Thorsten Leemhuis Cc: stable, regressions, Linus Walleij, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis <regressions@leemhuis.info> wrote: > > [TLDR: I'm adding this regression to regzbot, the Linux kernel > regression tracking bot; most text you find below is compiled from a few > templates paragraphs some of you might have seen already.] > > On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: > > Some GPIO lines have stopped working after the patch > > commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > > > > And this has supposedly been fixed in the following patches > > commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > > commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") > > There seems to be a backstory here. Are there any entries and bug > trackers or earlier discussions everyone that looks into this should be > aware of? > Agreed with Thorsten. I'd like to first try to determine what's wrong before reverting those, as they are correct in theory but maybe the implementation missed something. Have you tried tracing the execution on your platform in order to see what the driver is doing? Bart ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2021-12-20 14:57 ` Bartosz Golaszewski @ 2021-12-20 15:14 ` Geert Uytterhoeven 2021-12-20 19:24 ` Will McVicker 2021-12-20 20:41 ` Marcelo Roberto Jimenez 2021-12-20 20:41 ` Marcelo Roberto Jimenez 1 sibling, 2 replies; 28+ messages in thread From: Geert Uytterhoeven @ 2021-12-20 15:14 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Marcelo Roberto Jimenez, Thorsten Leemhuis, stable, regressions, Linus Walleij, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On Mon, Dec 20, 2021 at 3:57 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis > <regressions@leemhuis.info> wrote: > > [TLDR: I'm adding this regression to regzbot, the Linux kernel > > regression tracking bot; most text you find below is compiled from a few > > templates paragraphs some of you might have seen already.] > > > > On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: > > > Some GPIO lines have stopped working after the patch > > > commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > > > > > > And this has supposedly been fixed in the following patches > > > commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > > > commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") > > > > There seems to be a backstory here. Are there any entries and bug > > trackers or earlier discussions everyone that looks into this should be > > aware of? > > > > Agreed with Thorsten. I'd like to first try to determine what's wrong > before reverting those, as they are correct in theory but maybe the > implementation missed something. > > Have you tried tracing the execution on your platform in order to see > what the driver is doing? Looking at commits that have related Fixes tags: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bf781869e5cf3e4ec1a47dad69b6f0df97629cbd https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/commit/?id=e8f24c58d1b69ecf410a673c22f546dc732bb879 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2021-12-20 15:14 ` Geert Uytterhoeven @ 2021-12-20 19:24 ` Will McVicker 2021-12-20 20:41 ` Marcelo Roberto Jimenez 2021-12-20 20:41 ` Marcelo Roberto Jimenez 1 sibling, 1 reply; 28+ messages in thread From: Will McVicker @ 2021-12-20 19:24 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Bartosz Golaszewski, Marcelo Roberto Jimenez, Thorsten Leemhuis, stable, regressions, Linus Walleij, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Stephen Rothwell, Edmond Chung, Andrew Chant, Sergio Tanzilli On Mon, Dec 20, 2021 at 7:14 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Dec 20, 2021 at 3:57 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis > > <regressions@leemhuis.info> wrote: > > > [TLDR: I'm adding this regression to regzbot, the Linux kernel > > > regression tracking bot; most text you find below is compiled from a few > > > templates paragraphs some of you might have seen already.] > > > > > > On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: > > > > Some GPIO lines have stopped working after the patch > > > > commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > > > > > > > > And this has supposedly been fixed in the following patches > > > > commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > > > > commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") > > > > > > There seems to be a backstory here. Are there any entries and bug > > > trackers or earlier discussions everyone that looks into this should be > > > aware of? > > > > > > > Agreed with Thorsten. I'd like to first try to determine what's wrong > > before reverting those, as they are correct in theory but maybe the > > implementation missed something. > > > > Have you tried tracing the execution on your platform in order to see > > what the driver is doing? > > Looking at commits that have related Fixes tags: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bf781869e5cf3e4ec1a47dad69b6f0df97629cbd > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/commit/?id=e8f24c58d1b69ecf410a673c22f546dc732bb879 > > 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 Marcelo, Thanks for reporting this issue. I can give you a little context on why commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") was created. We were seeing a refcounting issue on Pixel 6. In our kernel CONFIG_PINCTRL is defined. Basically, the camera kernel module requests for a GPIO on sensor enable (when the camera sensor is turned on) and releases that GPIO on sensor disable (when the camera sensor is turned off). Before commit 6dbbf84603961, if we constantly switched between the front and back camera eventually we would hit the below error in drivers/pinctrl/pinmux.c:pin_request(): E samsung-pinctrl 10840000.pinctrl: could not increase module refcount for pin 134 In our kernel the sensor GPIOs don't have pin_ranges defined. So you would get these call stacks: Sensor Enable: gpiochip_generic_request() -> return 0 Sensor Disable: gpiochip_generic_free() -> pinctrl_gpio_free() This led to an imbalance of request vs free calls leading to the refcounting error. When we added commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined"), this issue was resolved. My recommendation would be to drill down into your driver to figure out what happens in these functions to see why you're getting the results you reported. --Will ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2021-12-20 19:24 ` Will McVicker @ 2021-12-20 20:41 ` Marcelo Roberto Jimenez 0 siblings, 0 replies; 28+ messages in thread From: Marcelo Roberto Jimenez @ 2021-12-20 20:41 UTC (permalink / raw) To: Will McVicker Cc: Geert Uytterhoeven, Bartosz Golaszewski, Thorsten Leemhuis, stable, regressions, Linus Walleij, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Stephen Rothwell, Edmond Chung, Andrew Chant, Sergio Tanzilli Hi Will, On Mon, Dec 20, 2021 at 4:25 PM Will McVicker <willmcvicker@google.com> wrote: > > On Mon, Dec 20, 2021 at 7:14 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > > On Mon, Dec 20, 2021 at 3:57 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis > > > <regressions@leemhuis.info> wrote: > > > > [TLDR: I'm adding this regression to regzbot, the Linux kernel > > > > regression tracking bot; most text you find below is compiled from a few > > > > templates paragraphs some of you might have seen already.] > > > > > > > > On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: > > > > > Some GPIO lines have stopped working after the patch > > > > > commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > > > > > > > > > > And this has supposedly been fixed in the following patches > > > > > commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > > > > > commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") > > > > > > > > There seems to be a backstory here. Are there any entries and bug > > > > trackers or earlier discussions everyone that looks into this should be > > > > aware of? > > > > > > > > > > Agreed with Thorsten. I'd like to first try to determine what's wrong > > > before reverting those, as they are correct in theory but maybe the > > > implementation missed something. > > > > > > Have you tried tracing the execution on your platform in order to see > > > what the driver is doing? > > > > Looking at commits that have related Fixes tags: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bf781869e5cf3e4ec1a47dad69b6f0df97629cbd > > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/commit/?id=e8f24c58d1b69ecf410a673c22f546dc732bb879 > > > > 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 Marcelo, > > Thanks for reporting this issue. I can give you a little context on > why commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not > defined") was created. We were seeing a refcounting issue on Pixel 6. > In our kernel CONFIG_PINCTRL is defined. Basically, the camera kernel > module requests for a GPIO on sensor enable (when the camera sensor is > turned on) and releases that GPIO on sensor disable (when the camera > sensor is turned off). Before commit 6dbbf84603961, if we constantly > switched between the front and back camera eventually we would hit the > below error in drivers/pinctrl/pinmux.c:pin_request(): > > E samsung-pinctrl 10840000.pinctrl: could not increase module > refcount for pin 134 > > In our kernel the sensor GPIOs don't have pin_ranges defined. So you > would get these call stacks: > > Sensor Enable: > gpiochip_generic_request() > -> return 0 > > Sensor Disable: > gpiochip_generic_free() > -> pinctrl_gpio_free() > > This led to an imbalance of request vs free calls leading to the > refcounting error. When we added commit 6dbbf84603961 ("gpiolib: Don't > free if pin ranges are not defined"), this issue was resolved. My > recommendation would be to drill down into your driver to figure out > what happens in these functions to see why you're getting the results > you reported. Thanks for your reply. Commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") is perfectly fine in the context and fixes a serious issue. But to revert the original patch we need to revert this patch too, for the same reason, i.e., in order to not generate a *_free() imbalance. In my case the imbalance causes problems as soon as the test script is run a second time. > > --Will Regards, Marcelo. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2021-12-20 15:14 ` Geert Uytterhoeven 2021-12-20 19:24 ` Will McVicker @ 2021-12-20 20:41 ` Marcelo Roberto Jimenez 1 sibling, 0 replies; 28+ messages in thread From: Marcelo Roberto Jimenez @ 2021-12-20 20:41 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Bartosz Golaszewski, Thorsten Leemhuis, stable, regressions, Linus Walleij, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli Hi Geert, On Mon, Dec 20, 2021 at 12:14 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Mon, Dec 20, 2021 at 3:57 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis > > <regressions@leemhuis.info> wrote: > > > [TLDR: I'm adding this regression to regzbot, the Linux kernel > > > regression tracking bot; most text you find below is compiled from a few > > > templates paragraphs some of you might have seen already.] > > > > > > On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: > > > > Some GPIO lines have stopped working after the patch > > > > commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > > > > > > > > And this has supposedly been fixed in the following patches > > > > commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > > > > commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") > > > > > > There seems to be a backstory here. Are there any entries and bug > > > trackers or earlier discussions everyone that looks into this should be > > > aware of? > > > > > > > Agreed with Thorsten. I'd like to first try to determine what's wrong > > before reverting those, as they are correct in theory but maybe the > > implementation missed something. > > > > Have you tried tracing the execution on your platform in order to see > > what the driver is doing? > > Looking at commits that have related Fixes tags: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bf781869e5cf3e4ec1a47dad69b6f0df97629cbd > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/commit/?id=e8f24c58d1b69ecf410a673c22f546dc732bb879 > Interesting. These seem to imply that gpiolib-sysfs.c should be allocating a pinctrl list. That seems very easy to do in the DTD, although I don't really know if that is the right thing to do. Doing it in the code seems more appropriate, what do you think? > Gr{oetje,eeting}s, > > Geert Regards, Marcelo. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2021-12-20 14:57 ` Bartosz Golaszewski 2021-12-20 15:14 ` Geert Uytterhoeven @ 2021-12-20 20:41 ` Marcelo Roberto Jimenez 2022-01-10 7:02 ` Thorsten Leemhuis 1 sibling, 1 reply; 28+ messages in thread From: Marcelo Roberto Jimenez @ 2021-12-20 20:41 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Thorsten Leemhuis, stable, regressions, Linus Walleij, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli Hi Bart, On Mon, Dec 20, 2021 at 11:57 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis > <regressions@leemhuis.info> wrote: > > > > [TLDR: I'm adding this regression to regzbot, the Linux kernel > > regression tracking bot; most text you find below is compiled from a few > > templates paragraphs some of you might have seen already.] > > > > On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: > > > Some GPIO lines have stopped working after the patch > > > commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > > > > > > And this has supposedly been fixed in the following patches > > > commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > > > commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") > > > > There seems to be a backstory here. Are there any entries and bug > > trackers or earlier discussions everyone that looks into this should be > > aware of? > > > > Agreed with Thorsten. I'd like to first try to determine what's wrong > before reverting those, as they are correct in theory but maybe the > implementation missed something. > > Have you tried tracing the execution on your platform in order to see > what the driver is doing? Yes. The problem is that there is no list defined for the sysfs-gpio interface. The driver will not perform pinctrl_gpio_request() and will return zero (failure). I don't know if this is the case to add something to a global DTD or to fix it in the sysfs-gpio code. > Bart Regards, Marcelo. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2021-12-20 20:41 ` Marcelo Roberto Jimenez @ 2022-01-10 7:02 ` Thorsten Leemhuis 2022-01-12 0:09 ` Marcelo Roberto Jimenez 0 siblings, 1 reply; 28+ messages in thread From: Thorsten Leemhuis @ 2022-01-10 7:02 UTC (permalink / raw) To: Marcelo Roberto Jimenez, Bartosz Golaszewski Cc: stable, regressions, Linus Walleij, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli Hi, this is your Linux kernel regression tracker speaking. On 20.12.21 21:41, Marcelo Roberto Jimenez wrote: > On Mon, Dec 20, 2021 at 11:57 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis >> <regressions@leemhuis.info> wrote: >>> >>> [TLDR: I'm adding this regression to regzbot, the Linux kernel >>> regression tracking bot; most text you find below is compiled from a few >>> templates paragraphs some of you might have seen already.] >>> >>> On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: >>>> Some GPIO lines have stopped working after the patch >>>> commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") >>>> >>>> And this has supposedly been fixed in the following patches >>>> commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") >>>> commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") >>> >>> There seems to be a backstory here. Are there any entries and bug >>> trackers or earlier discussions everyone that looks into this should be >>> aware of? >> >> Agreed with Thorsten. I'd like to first try to determine what's wrong >> before reverting those, as they are correct in theory but maybe the >> implementation missed something. >> >> Have you tried tracing the execution on your platform in order to see >> what the driver is doing? > > Yes. The problem is that there is no list defined for the sysfs-gpio > interface. The driver will not perform pinctrl_gpio_request() and will > return zero (failure). > > I don't know if this is the case to add something to a global DTD or > to fix it in the sysfs-gpio code. Out of interest, has any progress been made on this front? BTW, there was a last-minute commit for 5.16 yesterday that referenced the culprit Marcelo specified: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=master&id=c8013355ead68dce152cf426686f8a5f80d88b40 This was for a BCM283x and BCM2711 devices, so I assume it won't help. Wild guess (I don't know anything about this area of the kernel): Marcelo, do the dts files for your hardware maybe need a similar fix? Ciao, Thorsten P.S.: As a Linux kernel regression tracker I'm getting a lot of reports on my table. I can only look briefly into most of them. Unfortunately therefore I sometimes will get things wrong or miss something important. I hope that's not the case here; if you think it is, don't hesitate to tell me about it in a public reply, that's in everyone's interest. BTW, I have no personal interest in this issue, which is tracked using regzbot, my Linux kernel regression tracking bot (https://linux-regtracking.leemhuis.info/regzbot/). I'm only posting this mail to get things rolling again and hence don't need to be CC on all further activities wrt to this regression. #regzbot poke ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-01-10 7:02 ` Thorsten Leemhuis @ 2022-01-12 0:09 ` Marcelo Roberto Jimenez 2022-02-08 12:24 ` Thorsten Leemhuis 2022-02-17 19:11 ` Thierry Reding 0 siblings, 2 replies; 28+ messages in thread From: Marcelo Roberto Jimenez @ 2022-01-12 0:09 UTC (permalink / raw) To: Thorsten Leemhuis Cc: Bartosz Golaszewski, stable, regressions, Linus Walleij, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli Hi, On Mon, Jan 10, 2022 at 4:02 AM Thorsten Leemhuis <regressions@leemhuis.info> wrote: > > Hi, this is your Linux kernel regression tracker speaking. > > On 20.12.21 21:41, Marcelo Roberto Jimenez wrote: > > On Mon, Dec 20, 2021 at 11:57 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > >> On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis > >> <regressions@leemhuis.info> wrote: > >>> > >>> [TLDR: I'm adding this regression to regzbot, the Linux kernel > >>> regression tracking bot; most text you find below is compiled from a few > >>> templates paragraphs some of you might have seen already.] > >>> > >>> On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: > >>>> Some GPIO lines have stopped working after the patch > >>>> commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > >>>> > >>>> And this has supposedly been fixed in the following patches > >>>> commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > >>>> commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") > >>> > >>> There seems to be a backstory here. Are there any entries and bug > >>> trackers or earlier discussions everyone that looks into this should be > >>> aware of? > >> > >> Agreed with Thorsten. I'd like to first try to determine what's wrong > >> before reverting those, as they are correct in theory but maybe the > >> implementation missed something. > >> > >> Have you tried tracing the execution on your platform in order to see > >> what the driver is doing? > > > > Yes. The problem is that there is no list defined for the sysfs-gpio > > interface. The driver will not perform pinctrl_gpio_request() and will > > return zero (failure). > > > > I don't know if this is the case to add something to a global DTD or > > to fix it in the sysfs-gpio code. > > Out of interest, has any progress been made on this front? > > BTW, there was a last-minute commit for 5.16 yesterday that referenced > the culprit Marcelo specified: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=master&id=c8013355ead68dce152cf426686f8a5f80d88b40 > > This was for a BCM283x and BCM2711 devices, so I assume it won't help. > Wild guess (I don't know anything about this area of the kernel): > Marcelo, do the dts files for your hardware maybe need a similar fix? I have tried to add "gpio-ranges" to the gpio-controllers in at91sam9x5.dtsi, but the system deadlocks, because in pinctrl-at91.c, function at91_pinctrl_probe() we have: /* * We need all the GPIO drivers to probe FIRST, or we will not be able * to obtain references to the struct gpio_chip * for them, and we * need this to proceed. */ for (i = 0; i < gpio_banks; i++) if (gpio_chips[i]) ngpio_chips_enabled++; if (ngpio_chips_enabled < info->nactive_banks) { dev_warn(&pdev->dev, "All GPIO chips are not registered yet (%d/%d)\n", ngpio_chips_enabled, info->nactive_banks); devm_kfree(&pdev->dev, info); return -EPROBE_DEFER; } On the other hand, in gpiolib-of.c, function of_gpiochip_add_pin_range() we have: if (!pctldev) return -EPROBE_DEFER; In other words, the pinctrl needs all the gpio-controllers, and the gpio-controllers need the pinctrl. Each returns -EPROBE_DEFER and the system deadlocks. > > Ciao, Thorsten > > P.S.: As a Linux kernel regression tracker I'm getting a lot of reports > on my table. I can only look briefly into most of them. Unfortunately > therefore I sometimes will get things wrong or miss something important. > I hope that's not the case here; if you think it is, don't hesitate to > tell me about it in a public reply, that's in everyone's interest. > > BTW, I have no personal interest in this issue, which is tracked using > regzbot, my Linux kernel regression tracking bot > (https://linux-regtracking.leemhuis.info/regzbot/). I'm only posting > this mail to get things rolling again and hence don't need to be CC on > all further activities wrt to this regression. > > #regzbot poke > Regards, Marcelo. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-01-12 0:09 ` Marcelo Roberto Jimenez @ 2022-02-08 12:24 ` Thorsten Leemhuis 2022-02-17 19:11 ` Thierry Reding 1 sibling, 0 replies; 28+ messages in thread From: Thorsten Leemhuis @ 2022-02-08 12:24 UTC (permalink / raw) To: Marcelo Roberto Jimenez Cc: Bartosz Golaszewski, stable, regressions, Linus Walleij, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli Hi, this is your Linux kernel regression tracker speaking. Top-posting for once, to make this easy accessible to everyone. GPIO Maintainers and developers, what the status of this regression and getting it fixed? It looks like there was no progress for quite a while. Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) P.S.: As the Linux kernel's regression tracker I'm getting a lot of reports on my table. I can only look briefly into most of them and lack knowledge about most of the areas they concern. I thus unfortunately will sometimes get things wrong or miss something important. I hope that's not the case here; if you think it is, don't hesitate to tell me in a public reply, it's in everyone's interest to set the public record straight. #regzbot poke On 12.01.22 01:09, Marcelo Roberto Jimenez wrote: > Hi, > > On Mon, Jan 10, 2022 at 4:02 AM Thorsten Leemhuis > <regressions@leemhuis.info> wrote: >> >> Hi, this is your Linux kernel regression tracker speaking. >> >> On 20.12.21 21:41, Marcelo Roberto Jimenez wrote: >>> On Mon, Dec 20, 2021 at 11:57 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: >>>> On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis >>>> <regressions@leemhuis.info> wrote: >>>>> >>>>> [TLDR: I'm adding this regression to regzbot, the Linux kernel >>>>> regression tracking bot; most text you find below is compiled from a few >>>>> templates paragraphs some of you might have seen already.] >>>>> >>>>> On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: >>>>>> Some GPIO lines have stopped working after the patch >>>>>> commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") >>>>>> >>>>>> And this has supposedly been fixed in the following patches >>>>>> commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") >>>>>> commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") >>>>> >>>>> There seems to be a backstory here. Are there any entries and bug >>>>> trackers or earlier discussions everyone that looks into this should be >>>>> aware of? >>>> >>>> Agreed with Thorsten. I'd like to first try to determine what's wrong >>>> before reverting those, as they are correct in theory but maybe the >>>> implementation missed something. >>>> >>>> Have you tried tracing the execution on your platform in order to see >>>> what the driver is doing? >>> >>> Yes. The problem is that there is no list defined for the sysfs-gpio >>> interface. The driver will not perform pinctrl_gpio_request() and will >>> return zero (failure). >>> >>> I don't know if this is the case to add something to a global DTD or >>> to fix it in the sysfs-gpio code. >> >> Out of interest, has any progress been made on this front? >> >> BTW, there was a last-minute commit for 5.16 yesterday that referenced >> the culprit Marcelo specified: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=master&id=c8013355ead68dce152cf426686f8a5f80d88b40 >> >> This was for a BCM283x and BCM2711 devices, so I assume it won't help. >> Wild guess (I don't know anything about this area of the kernel): >> Marcelo, do the dts files for your hardware maybe need a similar fix? > > I have tried to add "gpio-ranges" to the gpio-controllers in > at91sam9x5.dtsi, but the system deadlocks, because in pinctrl-at91.c, > function at91_pinctrl_probe() we have: > > /* > * We need all the GPIO drivers to probe FIRST, or we will not be able > * to obtain references to the struct gpio_chip * for them, and we > * need this to proceed. > */ > for (i = 0; i < gpio_banks; i++) > if (gpio_chips[i]) > ngpio_chips_enabled++; > > if (ngpio_chips_enabled < info->nactive_banks) { > dev_warn(&pdev->dev, > "All GPIO chips are not registered yet (%d/%d)\n", > ngpio_chips_enabled, info->nactive_banks); > devm_kfree(&pdev->dev, info); > return -EPROBE_DEFER; > } > > On the other hand, in gpiolib-of.c, function > of_gpiochip_add_pin_range() we have: > > if (!pctldev) > return -EPROBE_DEFER; > > In other words, the pinctrl needs all the gpio-controllers, and the > gpio-controllers need the pinctrl. Each returns -EPROBE_DEFER and the > system deadlocks. > >> >> Ciao, Thorsten >> >> P.S.: As a Linux kernel regression tracker I'm getting a lot of reports >> on my table. I can only look briefly into most of them. Unfortunately >> therefore I sometimes will get things wrong or miss something important. >> I hope that's not the case here; if you think it is, don't hesitate to >> tell me about it in a public reply, that's in everyone's interest. >> >> BTW, I have no personal interest in this issue, which is tracked using >> regzbot, my Linux kernel regression tracking bot >> (https://linux-regtracking.leemhuis.info/regzbot/). I'm only posting >> this mail to get things rolling again and hence don't need to be CC on >> all further activities wrt to this regression. >> >> #regzbot poke >> > > Regards, > Marcelo. > > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-01-12 0:09 ` Marcelo Roberto Jimenez 2022-02-08 12:24 ` Thorsten Leemhuis @ 2022-02-17 19:11 ` Thierry Reding 1 sibling, 0 replies; 28+ messages in thread From: Thierry Reding @ 2022-02-17 19:11 UTC (permalink / raw) To: Marcelo Roberto Jimenez Cc: Thorsten Leemhuis, Bartosz Golaszewski, stable, regressions, Linus Walleij, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli [-- Attachment #1: Type: text/plain, Size: 8158 bytes --] On Tue, Jan 11, 2022 at 09:09:15PM -0300, Marcelo Roberto Jimenez wrote: > Hi, > > On Mon, Jan 10, 2022 at 4:02 AM Thorsten Leemhuis > <regressions@leemhuis.info> wrote: > > > > Hi, this is your Linux kernel regression tracker speaking. > > > > On 20.12.21 21:41, Marcelo Roberto Jimenez wrote: > > > On Mon, Dec 20, 2021 at 11:57 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > >> On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis > > >> <regressions@leemhuis.info> wrote: > > >>> > > >>> [TLDR: I'm adding this regression to regzbot, the Linux kernel > > >>> regression tracking bot; most text you find below is compiled from a few > > >>> templates paragraphs some of you might have seen already.] > > >>> > > >>> On 17.12.21 16:35, Marcelo Roberto Jimenez wrote: > > >>>> Some GPIO lines have stopped working after the patch > > >>>> commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > > >>>> > > >>>> And this has supposedly been fixed in the following patches > > >>>> commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > > >>>> commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") > > >>> > > >>> There seems to be a backstory here. Are there any entries and bug > > >>> trackers or earlier discussions everyone that looks into this should be > > >>> aware of? > > >> > > >> Agreed with Thorsten. I'd like to first try to determine what's wrong > > >> before reverting those, as they are correct in theory but maybe the > > >> implementation missed something. > > >> > > >> Have you tried tracing the execution on your platform in order to see > > >> what the driver is doing? > > > > > > Yes. The problem is that there is no list defined for the sysfs-gpio > > > interface. The driver will not perform pinctrl_gpio_request() and will > > > return zero (failure). > > > > > > I don't know if this is the case to add something to a global DTD or > > > to fix it in the sysfs-gpio code. > > > > Out of interest, has any progress been made on this front? > > > > BTW, there was a last-minute commit for 5.16 yesterday that referenced > > the culprit Marcelo specified: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=master&id=c8013355ead68dce152cf426686f8a5f80d88b40 > > > > This was for a BCM283x and BCM2711 devices, so I assume it won't help. > > Wild guess (I don't know anything about this area of the kernel): > > Marcelo, do the dts files for your hardware maybe need a similar fix? > > I have tried to add "gpio-ranges" to the gpio-controllers in > at91sam9x5.dtsi, but the system deadlocks, because in pinctrl-at91.c, > function at91_pinctrl_probe() we have: > > /* > * We need all the GPIO drivers to probe FIRST, or we will not be able > * to obtain references to the struct gpio_chip * for them, and we > * need this to proceed. > */ > for (i = 0; i < gpio_banks; i++) > if (gpio_chips[i]) > ngpio_chips_enabled++; > > if (ngpio_chips_enabled < info->nactive_banks) { > dev_warn(&pdev->dev, > "All GPIO chips are not registered yet (%d/%d)\n", > ngpio_chips_enabled, info->nactive_banks); > devm_kfree(&pdev->dev, info); > return -EPROBE_DEFER; > } > > On the other hand, in gpiolib-of.c, function > of_gpiochip_add_pin_range() we have: > > if (!pctldev) > return -EPROBE_DEFER; > > In other words, the pinctrl needs all the gpio-controllers, and the > gpio-controllers need the pinctrl. Each returns -EPROBE_DEFER and the > system deadlocks. Ugh, yeah, this sounds like a really bad idea. Conceptually GPIO drivers are consumers of a pinctrl device and by now making the pinctrl device depend on GPIO chips you make the dependency recursive, which is exactly what you're observing here. And it's honestly not a surprise that this breaks. It is rather surprising that this has worked before in the first place. Now, looking through some of the pin control documentation, I see that this kind of setup is actually encouraged (see "Interaction with the GPIO subsystem" in Documentation/driver-api/pin-control.rst). This may make sense for cases where the GPIO chips can be hard-coded, but I don't see how that would work in practice. Looking at a random sampling of drivers, I see a lot of those that are calling pinctrl_add_gpio_range() are setting the .gc field to point at a GPIO controller. However, in the cases that I've seen, they are all very tightly integrated with the pinctrl such that they don't have to do that same dance that AT91 does (where pinctrl and GPIO drivers are separate), so they avoid the circular dependency. A couple of examples are here: - bcm/pinctrl-bcm2835.c - pinctrl-starfive.c - pinctrl-st.c - renesas/pinctrl-rza1.c - renesas/pinctrl-rza2.c - samsung/pinctrl-samsung.c - stm32/pinctrl-stm32.c pinctrl-xway.c is another, slightly different variant that references a file-scoped struct gpio_chip, so it's again in that "tightly coupled" category. tegra/pinctrl-tegra.c gets away without setting the .gc field and instead uses a hard-coded number of GPIO lines in the range. The same goes for mvebu/pinctrl-mvebu.c which uses platform data to pass GPIO ranges information. Given all of the above, it sounds to me like the right way to fix this would be to do two things: 1) avoid the circular dependency by not waiting for all GPIO chips to get probed within the pinctrl driver's ->probe() 2) use the standard, DT-based mechanism to register GPIO ranges with the pinctrl Typically 2) would involve adding the gpio-ranges properties to the GPIO controllers' DT nodes. However, it looks like it might also be possible to avoid this in the AT91 driver by making it register the GPIO ranges from the GPIO driver's ->probe() function. That would typically be slightly tricky because you don't typically have a back-reference to the pinctrl device from the GPIO chip. *However*, it looks like on AT91- based platforms the GPIO controllers are actually children of the pin controller, which would nicely solve that problem (you can get the reference to the pin controller via the GPIO controllers' parent). I have not checked exhaustively if that's always the case, just for a couple of AT91-based DTS files, but I suspect that this is a common scheme for this type of device. Note that the fact that GPIO controllers are children of the pin controller is another indicator for why the circular dependency is a bad idea. After all you can't have children without a parent. Parents need to be "fully initialized" before they can procreate. If you wanted to make this really complicated you could perhaps achieve what that driver is currently trying by using the component driver infrastructure. That would basically involve initializing the pin controller fully so that it's ready to be used by the GPIO controllers and then once all GPIO controllers have been added, a special callback will be called, allowing you to complete the initialization of all the components (which could then be used to add the GPIO ranges). I don't think that's necessary in this case, though. Thierry > > Ciao, Thorsten > > > > P.S.: As a Linux kernel regression tracker I'm getting a lot of reports > > on my table. I can only look briefly into most of them. Unfortunately > > therefore I sometimes will get things wrong or miss something important. > > I hope that's not the case here; if you think it is, don't hesitate to > > tell me about it in a public reply, that's in everyone's interest. > > > > BTW, I have no personal interest in this issue, which is tracked using > > regzbot, my Linux kernel regression tracking bot > > (https://linux-regtracking.leemhuis.info/regzbot/). I'm only posting > > this mail to get things rolling again and hence don't need to be CC on > > all further activities wrt to this regression. > > > > #regzbot poke > > > > Regards, > Marcelo. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2021-12-17 15:35 [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) Marcelo Roberto Jimenez 2021-12-18 6:28 ` Thorsten Leemhuis @ 2022-02-11 0:02 ` Linus Walleij 2022-02-11 22:36 ` Marcelo Roberto Jimenez 2022-03-14 15:55 ` Michael Walle 2 siblings, 1 reply; 28+ messages in thread From: Linus Walleij @ 2022-02-11 0:02 UTC (permalink / raw) To: Marcelo Roberto Jimenez Cc: stable, regressions, Bartosz Golaszewski, linux-gpio, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On Fri, Dec 17, 2021 at 4:36 PM Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> wrote: > My system is ARM926EJ-S rev 5 (v5l) (AT91SAM9G25), the board is an ACME Systems Arietta. Which devicetree or boardfile in the upstream Linux kernel is this system using? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-02-11 0:02 ` Linus Walleij @ 2022-02-11 22:36 ` Marcelo Roberto Jimenez 2022-02-12 16:54 ` Linus Walleij 0 siblings, 1 reply; 28+ messages in thread From: Marcelo Roberto Jimenez @ 2022-02-11 22:36 UTC (permalink / raw) To: Linus Walleij Cc: stable, regressions, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On Thu, Feb 10, 2022 at 9:02 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Fri, Dec 17, 2021 at 4:36 PM Marcelo Roberto Jimenez > <marcelo.jimenez@gmail.com> wrote: > > > My system is ARM926EJ-S rev 5 (v5l) (AT91SAM9G25), the board is an ACME Systems Arietta. > > Which devicetree or boardfile in the upstream Linux kernel is this system > using? arch/arm/boot/dts/at91-ariettag25.dts But it is worth noting that the first lines in this file are: /* * Device Tree file for Arietta G25 * This device tree is minimal, to activate more peripherals, see: * http://dts.acmesystems.it/arietta/ */ And also that the URL in the comment above is old and now it should read: http://linux.tanzilli.com/ In any case, the upstream file should be enough to test the issue reported here. > Yours, > Linus Walleij Regards, Marcelo. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-02-11 22:36 ` Marcelo Roberto Jimenez @ 2022-02-12 16:54 ` Linus Walleij 2022-02-13 23:23 ` Marcelo Roberto Jimenez 0 siblings, 1 reply; 28+ messages in thread From: Linus Walleij @ 2022-02-12 16:54 UTC (permalink / raw) To: Marcelo Roberto Jimenez Cc: stable, regressions, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On Fri, Feb 11, 2022 at 11:36 PM Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> wrote: > > Which devicetree or boardfile in the upstream Linux kernel is this system > > using? > > arch/arm/boot/dts/at91-ariettag25.dts So this system was added in 2015 which is the same year that we marked the GPIO sysfs ABI obsolete: commit fe95046e960b4b76e73dc1486955d93f47276134 Author: Linus Walleij <linus.walleij@linaro.org> Date: Thu Oct 22 09:58:34 2015 +0200 gpio: ABI: mark the sysfs ABI as obsolete Why is this system which was clearly developed while we deprecated the sysfs ABI so dependent on it? I am curious about the usecases and how deeply you have built yourselves into this. > In any case, the upstream file should be enough to test the issue reported here. The thing is that upstream isn't super happy that you have been making yourselves dependent on features that we are actively discouraging and then demanding that we support these features. Anyway, what is wrong with using the GPIO character device and libgpiod on this system? What kind of userspace are you creating that absolutely requires that you use sysfs? I hope not one of these? https://docs.kernel.org/driver-api/gpio/drivers-on-gpio.html Here is some info about what we have been doing with the GPIO character device: https://docs.kernel.org/driver-api/gpio/using-gpio.html https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/ Here is Bartosz presenting it: https://www.youtube.com/watch?v=BK6gOLVRKuU Since I patched the kernel such that you cannot activate the sysfs ABI without also activating the character device I *know* that you have it on your system. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-02-12 16:54 ` Linus Walleij @ 2022-02-13 23:23 ` Marcelo Roberto Jimenez 2022-02-15 21:56 ` Linus Walleij 0 siblings, 1 reply; 28+ messages in thread From: Marcelo Roberto Jimenez @ 2022-02-13 23:23 UTC (permalink / raw) To: Linus Walleij Cc: stable, regressions, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On Sat, Feb 12, 2022 at 1:55 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Fri, Feb 11, 2022 at 11:36 PM Marcelo Roberto Jimenez > <marcelo.jimenez@gmail.com> wrote: > > > > Which devicetree or boardfile in the upstream Linux kernel is this system > > > using? > > > > arch/arm/boot/dts/at91-ariettag25.dts > > So this system was added in 2015 which is the same year that > we marked the GPIO sysfs ABI obsolete: > > commit fe95046e960b4b76e73dc1486955d93f47276134 > Author: Linus Walleij <linus.walleij@linaro.org> > Date: Thu Oct 22 09:58:34 2015 +0200 > > gpio: ABI: mark the sysfs ABI as obsolete > > Why is this system which was clearly developed while we deprecated > the sysfs ABI so dependent on it? The date when the device tree file was committed upstream is not the same date when the product has been released. The product was originally released in about January, 2014, as can be seen, e.g, from a post here from January 30, 2014: https://www.open-electronics.org/acme-system-launched-arietta-g25-a-new-micro-linux-board/ This is almost two years before the API has been marked obsolete. The company that produces the board (which I am not affiliated to) gave their users access to a different device tree file. > I am curious about the usecases and how deeply you have built > yourselves into this. I don't know if I understand what you mean, sorry. > > In any case, the upstream file should be enough to test the issue reported here. > > The thing is that upstream isn't super happy that you have been > making yourselves dependent on features that we are actively > discouraging and then demanding that we support these features. Hum, demanding seems to be a strong word for what I am doing here. Deprecated should not mean broken. My point is: the API seems to be currently broken. User space apps got broken, that's a fact. I even took the time to bisect the kernel and show you which commit broke it. So, no, I am not demanding. More like reporting and providing a temporary solution to those with a similar problem. Maybe it is time to remove the API, but this is up to "upstream". Leaving the API broken seems pointless and unproductive. Sorry for the "not super happiness of upstream", but maybe upstream got me wrong. We are not "making ourselves dependent on features ...". The API was there. We used it. Now it is deprecated, ok, we should move on. I got the message. > Anyway, what is wrong with using the GPIO character device and libgpiod > on this system? What kind of userspace are you creating that > absolutely requires that you use sysfs? There is nothing wrong, except for a matter of causality that seems to have been inverted here and has been explained above. > I hope not one of these? > https://docs.kernel.org/driver-api/gpio/drivers-on-gpio.html > > Here is some info about what we have been doing with the GPIO > character device: > > https://docs.kernel.org/driver-api/gpio/using-gpio.html > https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/ > > Here is Bartosz presenting it: > https://www.youtube.com/watch?v=BK6gOLVRKuU Please, don't get me wrong. I find the work done in GPIO amazing. Thanks for all the valuable information, I sincerely appreciate it and will read it all carefully. And I will also tell the dev team that they must use the GPIO char dev and libgpiod from now on and must port everything to it. And we will likely have another group of people who are not super happy, but that's life... :) > Since I patched the kernel such that you cannot activate the sysfs > ABI without also activating the character device I *know* that you > have it on your system. Smart move! > Yours, > Linus Walleij Best regards, Marcelo. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-02-13 23:23 ` Marcelo Roberto Jimenez @ 2022-02-15 21:56 ` Linus Walleij 2022-02-16 14:40 ` Bartosz Golaszewski 0 siblings, 1 reply; 28+ messages in thread From: Linus Walleij @ 2022-02-15 21:56 UTC (permalink / raw) To: Marcelo Roberto Jimenez Cc: stable, regressions, Bartosz Golaszewski, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On Mon, Feb 14, 2022 at 12:24 AM Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> wrote: > On Sat, Feb 12, 2022 at 1:55 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > I am curious about the usecases and how deeply you have built > > yourselves into this. > > I don't know if I understand what you mean, sorry. Why does the user need the sysfs ABI? What is it used for? I.e what is the actual use case? > > > In any case, the upstream file should be enough to test the issue reported here. > > > > The thing is that upstream isn't super happy that you have been > > making yourselves dependent on features that we are actively > > discouraging and then demanding that we support these features. > > Hum, demanding seems to be a strong word for what I am doing here. > > Deprecated should not mean broken. My point is: the API seems to be > currently broken. User space apps got broken, that's a fact. I even > took the time to bisect the kernel and show you which commit broke it. > So, no, I am not demanding. More like reporting and providing a > temporary solution to those with a similar problem. > > Maybe it is time to remove the API, but this is up to "upstream". > Leaving the API broken seems pointless and unproductive. > > Sorry for the "not super happiness of upstream", but maybe upstream > got me wrong. > > We are not "making ourselves dependent on features ...". The API was > there. We used it. Now it is deprecated, ok, we should move on. I got > the message. Ouch I deserved some slamming for this. I'm sorry if I came across as harsh :( I just don't know how to properly push for this. I have even pushed the option of the deprecated sysfs ABI behind the CONFIG_EXPERT option, which should mean that the kernel config has been made by someone who has checked the option "yes I am an expert I know what I am doing" yet failed to observe that this ABI is obsoleted since 5 years and hence failed to be an expert. Of course the ABI (not API really) needs to be fixed if we can find the problem. It's frustrating that fixing it seems to fix broken other features which are not deprecated, hence the annoyance on my part. > And I will also tell the dev team that they must use the GPIO char dev > and libgpiod from now on and must port everything to it. And we will > likely have another group of people who are not super happy, but > that's life... :) I'm happy to hear this! Yours, Linus Walleij ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-02-15 21:56 ` Linus Walleij @ 2022-02-16 14:40 ` Bartosz Golaszewski 2022-03-04 7:13 ` Thorsten Leemhuis 2022-05-20 9:12 ` Thorsten Leemhuis 0 siblings, 2 replies; 28+ messages in thread From: Bartosz Golaszewski @ 2022-02-16 14:40 UTC (permalink / raw) To: Linus Walleij Cc: Marcelo Roberto Jimenez, stable, regressions, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On Tue, Feb 15, 2022 at 10:56 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Mon, Feb 14, 2022 at 12:24 AM Marcelo Roberto Jimenez > <marcelo.jimenez@gmail.com> wrote: > > On Sat, Feb 12, 2022 at 1:55 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > > > I am curious about the usecases and how deeply you have built > > > yourselves into this. > > > > I don't know if I understand what you mean, sorry. > > Why does the user need the sysfs ABI? What is it used for? > > I.e what is the actual use case? > > > > > In any case, the upstream file should be enough to test the issue reported here. > > > > > > The thing is that upstream isn't super happy that you have been > > > making yourselves dependent on features that we are actively > > > discouraging and then demanding that we support these features. > > > > Hum, demanding seems to be a strong word for what I am doing here. > > > > Deprecated should not mean broken. My point is: the API seems to be > > currently broken. User space apps got broken, that's a fact. I even > > took the time to bisect the kernel and show you which commit broke it. > > So, no, I am not demanding. More like reporting and providing a > > temporary solution to those with a similar problem. > > > > Maybe it is time to remove the API, but this is up to "upstream". > > Leaving the API broken seems pointless and unproductive. > > > > Sorry for the "not super happiness of upstream", but maybe upstream > > got me wrong. > > > > We are not "making ourselves dependent on features ...". The API was > > there. We used it. Now it is deprecated, ok, we should move on. I got > > the message. > > Ouch I deserved some slamming for this. > > I'm sorry if I came across as harsh :( > > I just don't know how to properly push for this. > > I have even pushed the option of the deprecated sysfs ABI > behind the CONFIG_EXPERT option, which should mean that > the kernel config has been made by someone who has checked > the option "yes I am an expert I know what I am doing" > yet failed to observe that this ABI is obsoleted since 5 years > and hence failed to be an expert. > > Of course the ABI (not API really) needs to be fixed if we can find the > problem. It's frustrating that fixing it seems to fix broken other > features which are not deprecated, hence the annoyance on my > part. > I'm afraid we'll earn ourselves a good old LinusRant if we keep pushing the character device as a solution to the problem here. Marcelo is right after all: he used an existing user interface, the interface broke, it must be fixed. I would prefer to find a solution that fixes Marcelo's issue while keeping the offending patches in tree but it seems like the issue is more complicated and will require some rework of the sysfs interface. In which case unless there are objections I lean towards reverting the relevant commits. Bart > > And I will also tell the dev team that they must use the GPIO char dev > > and libgpiod from now on and must port everything to it. And we will > > likely have another group of people who are not super happy, but > > that's life... :) > > I'm happy to hear this! > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-02-16 14:40 ` Bartosz Golaszewski @ 2022-03-04 7:13 ` Thorsten Leemhuis 2022-03-07 9:58 ` Bartosz Golaszewski 2022-05-20 9:12 ` Thorsten Leemhuis 1 sibling, 1 reply; 28+ messages in thread From: Thorsten Leemhuis @ 2022-03-04 7:13 UTC (permalink / raw) To: Bartosz Golaszewski, Linus Walleij Cc: Marcelo Roberto Jimenez, stable, regressions, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli Hi, this is your Linux kernel regression tracker. On 16.02.22 15:40, Bartosz Golaszewski wrote: > On Tue, Feb 15, 2022 at 10:56 PM Linus Walleij <linus.walleij@linaro.org> wrote: >> >> On Mon, Feb 14, 2022 at 12:24 AM Marcelo Roberto Jimenez >> <marcelo.jimenez@gmail.com> wrote: >>> On Sat, Feb 12, 2022 at 1:55 PM Linus Walleij <linus.walleij@linaro.org> wrote: >> >>>> I am curious about the usecases and how deeply you have built >>>> yourselves into this. >>> >>> I don't know if I understand what you mean, sorry. >> >> Why does the user need the sysfs ABI? What is it used for? >> >> I.e what is the actual use case? >> >>>>> In any case, the upstream file should be enough to test the issue reported here. >>>> >>>> The thing is that upstream isn't super happy that you have been >>>> making yourselves dependent on features that we are actively >>>> discouraging and then demanding that we support these features. >>> >>> Hum, demanding seems to be a strong word for what I am doing here. >>> >>> Deprecated should not mean broken. My point is: the API seems to be >>> currently broken. User space apps got broken, that's a fact. I even >>> took the time to bisect the kernel and show you which commit broke it. >>> So, no, I am not demanding. More like reporting and providing a >>> temporary solution to those with a similar problem. >>> >>> Maybe it is time to remove the API, but this is up to "upstream". >>> Leaving the API broken seems pointless and unproductive. >>> >>> Sorry for the "not super happiness of upstream", but maybe upstream >>> got me wrong. >>> >>> We are not "making ourselves dependent on features ...". The API was >>> there. We used it. Now it is deprecated, ok, we should move on. I got >>> the message. >> >> Ouch I deserved some slamming for this. >> >> I'm sorry if I came across as harsh :( >> >> I just don't know how to properly push for this. >> >> I have even pushed the option of the deprecated sysfs ABI >> behind the CONFIG_EXPERT option, which should mean that >> the kernel config has been made by someone who has checked >> the option "yes I am an expert I know what I am doing" >> yet failed to observe that this ABI is obsoleted since 5 years >> and hence failed to be an expert. >> >> Of course the ABI (not API really) needs to be fixed if we can find the >> problem. It's frustrating that fixing it seems to fix broken other >> features which are not deprecated, hence the annoyance on my >> part. >> > > I'm afraid we'll earn ourselves a good old LinusRant if we keep > pushing the character device as a solution to the problem here. > Marcelo is right after all: he used an existing user interface, the > interface broke, it must be fixed. > > I would prefer to find a solution that fixes Marcelo's issue while > keeping the offending patches in tree but it seems like the issue is > more complicated and will require some rework of the sysfs interface. > > In which case unless there are objections I lean towards reverting the > relevant commits. Sounds good to me, but that was two weeks ago and afaics nothing happened since then. Or did the discussion continue somewhere else? >>> And I will also tell the dev team that they must use the GPIO char dev >>> and libgpiod from now on and must port everything to it. And we will >>> likely have another group of people who are not super happy, but >>> that's life... :) >> >> I'm happy to hear this! Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) P.S.: As the Linux kernel's regression tracker I'm getting a lot of reports on my table. I can only look briefly into most of them and lack knowledge about most of the areas they concern. I thus unfortunately will sometimes get things wrong or miss something important. I hope that's not the case here; if you think it is, don't hesitate to tell me in a public reply, it's in everyone's interest to set the public record straight. #regzbot poke ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-03-04 7:13 ` Thorsten Leemhuis @ 2022-03-07 9:58 ` Bartosz Golaszewski 2022-03-07 10:12 ` Thorsten Leemhuis 0 siblings, 1 reply; 28+ messages in thread From: Bartosz Golaszewski @ 2022-03-07 9:58 UTC (permalink / raw) To: Thorsten Leemhuis Cc: Linus Walleij, Marcelo Roberto Jimenez, stable, regressions, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On Fri, Mar 4, 2022 at 8:13 AM Thorsten Leemhuis <regressions@leemhuis.info> wrote: > > Hi, this is your Linux kernel regression tracker. > > On 16.02.22 15:40, Bartosz Golaszewski wrote: > > On Tue, Feb 15, 2022 at 10:56 PM Linus Walleij <linus.walleij@linaro.org> wrote: > >> > >> On Mon, Feb 14, 2022 at 12:24 AM Marcelo Roberto Jimenez > >> <marcelo.jimenez@gmail.com> wrote: > >>> On Sat, Feb 12, 2022 at 1:55 PM Linus Walleij <linus.walleij@linaro.org> wrote: > >> > >>>> I am curious about the usecases and how deeply you have built > >>>> yourselves into this. > >>> > >>> I don't know if I understand what you mean, sorry. > >> > >> Why does the user need the sysfs ABI? What is it used for? > >> > >> I.e what is the actual use case? > >> > >>>>> In any case, the upstream file should be enough to test the issue reported here. > >>>> > >>>> The thing is that upstream isn't super happy that you have been > >>>> making yourselves dependent on features that we are actively > >>>> discouraging and then demanding that we support these features. > >>> > >>> Hum, demanding seems to be a strong word for what I am doing here. > >>> > >>> Deprecated should not mean broken. My point is: the API seems to be > >>> currently broken. User space apps got broken, that's a fact. I even > >>> took the time to bisect the kernel and show you which commit broke it. > >>> So, no, I am not demanding. More like reporting and providing a > >>> temporary solution to those with a similar problem. > >>> > >>> Maybe it is time to remove the API, but this is up to "upstream". > >>> Leaving the API broken seems pointless and unproductive. > >>> > >>> Sorry for the "not super happiness of upstream", but maybe upstream > >>> got me wrong. > >>> > >>> We are not "making ourselves dependent on features ...". The API was > >>> there. We used it. Now it is deprecated, ok, we should move on. I got > >>> the message. > >> > >> Ouch I deserved some slamming for this. > >> > >> I'm sorry if I came across as harsh :( > >> > >> I just don't know how to properly push for this. > >> > >> I have even pushed the option of the deprecated sysfs ABI > >> behind the CONFIG_EXPERT option, which should mean that > >> the kernel config has been made by someone who has checked > >> the option "yes I am an expert I know what I am doing" > >> yet failed to observe that this ABI is obsoleted since 5 years > >> and hence failed to be an expert. > >> > >> Of course the ABI (not API really) needs to be fixed if we can find the > >> problem. It's frustrating that fixing it seems to fix broken other > >> features which are not deprecated, hence the annoyance on my > >> part. > >> > > > > I'm afraid we'll earn ourselves a good old LinusRant if we keep > > pushing the character device as a solution to the problem here. > > Marcelo is right after all: he used an existing user interface, the > > interface broke, it must be fixed. > > > > I would prefer to find a solution that fixes Marcelo's issue while > > keeping the offending patches in tree but it seems like the issue is > > more complicated and will require some rework of the sysfs interface. > > > > In which case unless there are objections I lean towards reverting the > > relevant commits. > > Sounds good to me, but that was two weeks ago and afaics nothing > happened since then. Or did the discussion continue somewhere else? > Now queued for fixes, thanks for the reminder. Bart > >>> And I will also tell the dev team that they must use the GPIO char dev > >>> and libgpiod from now on and must port everything to it. And we will > >>> likely have another group of people who are not super happy, but > >>> that's life... :) > >> > >> I'm happy to hear this! > > Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) > > P.S.: As the Linux kernel's regression tracker I'm getting a lot of > reports on my table. I can only look briefly into most of them and lack > knowledge about most of the areas they concern. I thus unfortunately > will sometimes get things wrong or miss something important. I hope > that's not the case here; if you think it is, don't hesitate to tell me > in a public reply, it's in everyone's interest to set the public record > straight. > > #regzbot poke ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-03-07 9:58 ` Bartosz Golaszewski @ 2022-03-07 10:12 ` Thorsten Leemhuis 0 siblings, 0 replies; 28+ messages in thread From: Thorsten Leemhuis @ 2022-03-07 10:12 UTC (permalink / raw) To: Bartosz Golaszewski Cc: Linus Walleij, Marcelo Roberto Jimenez, stable, regressions, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On 07.03.22 10:58, Bartosz Golaszewski wrote: > On Fri, Mar 4, 2022 at 8:13 AM Thorsten Leemhuis > <regressions@leemhuis.info> wrote: >> On 16.02.22 15:40, Bartosz Golaszewski wrote: >>> On Tue, Feb 15, 2022 at 10:56 PM Linus Walleij <linus.walleij@linaro.org> wrote: >>>> >>>> On Mon, Feb 14, 2022 at 12:24 AM Marcelo Roberto Jimenez >>>> <marcelo.jimenez@gmail.com> wrote: >>>>> On Sat, Feb 12, 2022 at 1:55 PM Linus Walleij <linus.walleij@linaro.org> wrote: >>>> >>>>>> I am curious about the usecases and how deeply you have built >>>>>> yourselves into this. >>>>> >>>>> I don't know if I understand what you mean, sorry. >>>> >>>> Why does the user need the sysfs ABI? What is it used for? >>>> >>>> I.e what is the actual use case? >>>> >>>>>>> In any case, the upstream file should be enough to test the issue reported here. >>>>>> >>>>>> The thing is that upstream isn't super happy that you have been >>>>>> making yourselves dependent on features that we are actively >>>>>> discouraging and then demanding that we support these features. >>>>> >>>>> Hum, demanding seems to be a strong word for what I am doing here. >>>>> >>>>> Deprecated should not mean broken. My point is: the API seems to be >>>>> currently broken. User space apps got broken, that's a fact. I even >>>>> took the time to bisect the kernel and show you which commit broke it. >>>>> So, no, I am not demanding. More like reporting and providing a >>>>> temporary solution to those with a similar problem. >>>>> >>>>> Maybe it is time to remove the API, but this is up to "upstream". >>>>> Leaving the API broken seems pointless and unproductive. >>>>> >>>>> Sorry for the "not super happiness of upstream", but maybe upstream >>>>> got me wrong. >>>>> >>>>> We are not "making ourselves dependent on features ...". The API was >>>>> there. We used it. Now it is deprecated, ok, we should move on. I got >>>>> the message. >>>> >>>> Ouch I deserved some slamming for this. >>>> >>>> I'm sorry if I came across as harsh :( >>>> >>>> I just don't know how to properly push for this. >>>> >>>> I have even pushed the option of the deprecated sysfs ABI >>>> behind the CONFIG_EXPERT option, which should mean that >>>> the kernel config has been made by someone who has checked >>>> the option "yes I am an expert I know what I am doing" >>>> yet failed to observe that this ABI is obsoleted since 5 years >>>> and hence failed to be an expert. >>>> >>>> Of course the ABI (not API really) needs to be fixed if we can find the >>>> problem. It's frustrating that fixing it seems to fix broken other >>>> features which are not deprecated, hence the annoyance on my >>>> part. >>>> >>> >>> I'm afraid we'll earn ourselves a good old LinusRant if we keep >>> pushing the character device as a solution to the problem here. >>> Marcelo is right after all: he used an existing user interface, the >>> interface broke, it must be fixed. >>> >>> I would prefer to find a solution that fixes Marcelo's issue while >>> keeping the offending patches in tree but it seems like the issue is >>> more complicated and will require some rework of the sysfs interface. >>> >>> In which case unless there are objections I lean towards reverting the >>> relevant commits. >> >> Sounds good to me, but that was two weeks ago and afaics nothing >> happened since then. Or did the discussion continue somewhere else? > > Now queued for fixes, thanks for the reminder. thx, and yw, that's what I'm here for ;-) Sadly that commit didn't use 'Link:' tags pointing to the report (the start of this thread) using lore.kernel.org/r/, as explained in 'Documentation/process/submitting-patches.rst' and 'Documentation/process/5.Posting.rst'. I'd say that would have been really wise here if someone sooner or later needs to look into the backstory of the fix. And is also means that I have to tell my regression tracking bot about this issue manually now. :-/ #regzbot fixed-by: 86528d306d1826cfe59481001d63761ba793317a But whatever, thx for taking care of this! Have a nice week everyone! Ciao, Thorsten >>>>> And I will also tell the dev team that they must use the GPIO char dev >>>>> and libgpiod from now on and must port everything to it. And we will >>>>> likely have another group of people who are not super happy, but >>>>> that's life... :) >>>> >>>> I'm happy to hear this! >> >> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) >> >> P.S.: As the Linux kernel's regression tracker I'm getting a lot of >> reports on my table. I can only look briefly into most of them and lack >> knowledge about most of the areas they concern. I thus unfortunately >> will sometimes get things wrong or miss something important. I hope >> that's not the case here; if you think it is, don't hesitate to tell me >> in a public reply, it's in everyone's interest to set the public record >> straight. >> >> #regzbot poke > ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-02-16 14:40 ` Bartosz Golaszewski 2022-03-04 7:13 ` Thorsten Leemhuis @ 2022-05-20 9:12 ` Thorsten Leemhuis 2022-05-20 17:28 ` Marcelo Roberto Jimenez 1 sibling, 1 reply; 28+ messages in thread From: Thorsten Leemhuis @ 2022-05-20 9:12 UTC (permalink / raw) To: Bartosz Golaszewski, Linus Walleij Cc: Marcelo Roberto Jimenez, stable, regressions, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli On 16.02.22 15:40, Bartosz Golaszewski wrote: > On Tue, Feb 15, 2022 at 10:56 PM Linus Walleij <linus.walleij@linaro.org> wrote: >> >> On Mon, Feb 14, 2022 at 12:24 AM Marcelo Roberto Jimenez >> <marcelo.jimenez@gmail.com> wrote: >>> On Sat, Feb 12, 2022 at 1:55 PM Linus Walleij <linus.walleij@linaro.org> wrote: >> >>>> I am curious about the usecases and how deeply you have built >>>> yourselves into this. >>> >>> I don't know if I understand what you mean, sorry. >> >> Why does the user need the sysfs ABI? What is it used for? >> >> I.e what is the actual use case? >> >>>>> In any case, the upstream file should be enough to test the issue reported here. >>>> >>>> The thing is that upstream isn't super happy that you have been >>>> making yourselves dependent on features that we are actively >>>> discouraging and then demanding that we support these features. >>> >>> Hum, demanding seems to be a strong word for what I am doing here. >>> >>> Deprecated should not mean broken. My point is: the API seems to be >>> currently broken. User space apps got broken, that's a fact. I even >>> took the time to bisect the kernel and show you which commit broke it. >>> So, no, I am not demanding. More like reporting and providing a >>> temporary solution to those with a similar problem. >>> >>> Maybe it is time to remove the API, but this is up to "upstream". >>> Leaving the API broken seems pointless and unproductive. >>> >>> Sorry for the "not super happiness of upstream", but maybe upstream >>> got me wrong. >>> >>> We are not "making ourselves dependent on features ...". The API was >>> there. We used it. Now it is deprecated, ok, we should move on. I got >>> the message. >> >> Ouch I deserved some slamming for this. >> >> I'm sorry if I came across as harsh :( >> >> I just don't know how to properly push for this. >> >> I have even pushed the option of the deprecated sysfs ABI >> behind the CONFIG_EXPERT option, which should mean that >> the kernel config has been made by someone who has checked >> the option "yes I am an expert I know what I am doing" >> yet failed to observe that this ABI is obsoleted since 5 years >> and hence failed to be an expert. >> >> Of course the ABI (not API really) needs to be fixed if we can find the >> problem. It's frustrating that fixing it seems to fix broken other >> features which are not deprecated, hence the annoyance on my >> part. >> > > I'm afraid we'll earn ourselves a good old LinusRant if we keep > pushing the character device as a solution to the problem here. > Marcelo is right after all: he used an existing user interface, the > interface broke, it must be fixed. > > I would prefer to find a solution that fixes Marcelo's issue while > keeping the offending patches in tree but it seems like the issue is > more complicated and will require some rework of the sysfs interface. > > In which case unless there are objections I lean towards reverting the > relevant commits. Reviving and old thread, hence a quick reminder: The patch at the start of this thread was applied and then reverted in 56e337f2cf13 with this text: ``` This commit - while attempting to fix a regression - has caused a number of other problems. As the fallout from it is more significant than the initial problem itself, revert it for now before we find a correct solution. ``` I still have this on my list of open regressions and that made me wonder: is anyone working on a "correct solution" (or was one even applied and I missed it)? Or is the situation so tricky that we better leave everything as it is? Marcelo, do you still care? Ciao, Thorsten ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-05-20 9:12 ` Thorsten Leemhuis @ 2022-05-20 17:28 ` Marcelo Roberto Jimenez 0 siblings, 0 replies; 28+ messages in thread From: Marcelo Roberto Jimenez @ 2022-05-20 17:28 UTC (permalink / raw) To: Thorsten Leemhuis Cc: Bartosz Golaszewski, Linus Walleij, stable, regressions, open list:GPIO SUBSYSTEM, Thierry Reding, Vidya Sagar, Geert Uytterhoeven, Stephen Rothwell, Edmond Chung, Andrew Chant, Will McVicker, Sergio Tanzilli Hi Thorsten, On Fri, May 20, 2022 at 6:12 AM Thorsten Leemhuis <regressions@leemhuis.info> wrote: > > On 16.02.22 15:40, Bartosz Golaszewski wrote: > > On Tue, Feb 15, 2022 at 10:56 PM Linus Walleij <linus.walleij@linaro.org> wrote: > >> > >> On Mon, Feb 14, 2022 at 12:24 AM Marcelo Roberto Jimenez > >> <marcelo.jimenez@gmail.com> wrote: > >>> On Sat, Feb 12, 2022 at 1:55 PM Linus Walleij <linus.walleij@linaro.org> wrote: > >> > >>>> I am curious about the usecases and how deeply you have built > >>>> yourselves into this. > >>> > >>> I don't know if I understand what you mean, sorry. > >> > >> Why does the user need the sysfs ABI? What is it used for? > >> > >> I.e what is the actual use case? > >> > >>>>> In any case, the upstream file should be enough to test the issue reported here. > >>>> > >>>> The thing is that upstream isn't super happy that you have been > >>>> making yourselves dependent on features that we are actively > >>>> discouraging and then demanding that we support these features. > >>> > >>> Hum, demanding seems to be a strong word for what I am doing here. > >>> > >>> Deprecated should not mean broken. My point is: the API seems to be > >>> currently broken. User space apps got broken, that's a fact. I even > >>> took the time to bisect the kernel and show you which commit broke it. > >>> So, no, I am not demanding. More like reporting and providing a > >>> temporary solution to those with a similar problem. > >>> > >>> Maybe it is time to remove the API, but this is up to "upstream". > >>> Leaving the API broken seems pointless and unproductive. > >>> > >>> Sorry for the "not super happiness of upstream", but maybe upstream > >>> got me wrong. > >>> > >>> We are not "making ourselves dependent on features ...". The API was > >>> there. We used it. Now it is deprecated, ok, we should move on. I got > >>> the message. > >> > >> Ouch I deserved some slamming for this. > >> > >> I'm sorry if I came across as harsh :( > >> > >> I just don't know how to properly push for this. > >> > >> I have even pushed the option of the deprecated sysfs ABI > >> behind the CONFIG_EXPERT option, which should mean that > >> the kernel config has been made by someone who has checked > >> the option "yes I am an expert I know what I am doing" > >> yet failed to observe that this ABI is obsoleted since 5 years > >> and hence failed to be an expert. > >> > >> Of course the ABI (not API really) needs to be fixed if we can find the > >> problem. It's frustrating that fixing it seems to fix broken other > >> features which are not deprecated, hence the annoyance on my > >> part. > >> > > > > I'm afraid we'll earn ourselves a good old LinusRant if we keep > > pushing the character device as a solution to the problem here. > > Marcelo is right after all: he used an existing user interface, the > > interface broke, it must be fixed. > > > > I would prefer to find a solution that fixes Marcelo's issue while > > keeping the offending patches in tree but it seems like the issue is > > more complicated and will require some rework of the sysfs interface. > > > > In which case unless there are objections I lean towards reverting the > > relevant commits. > > Reviving and old thread, hence a quick reminder: The patch at the start > of this thread was applied and then reverted in 56e337f2cf13 with this text: > > ``` > This commit - while attempting to fix a regression - has caused a number > of other problems. As the fallout from it is more significant than the > initial problem itself, revert it for now before we find a correct > solution. > ``` > > I still have this on my list of open regressions and that made me > wonder: is anyone working on a "correct solution" (or was one even > applied and I missed it)? Or is the situation so tricky that we better > leave everything as it is? Marcelo, do you still care? The purpose of my patch was to revert the patch that was causing the hardware I work with to fail. But reverting that patch had bad consequences in other hardware, so I really do not think that my patch should go in. Following Linus Walleij's advice, we are stopping using sysfs for gpio, so in the near future that patch will be irrelevant for me. On the other hand, a few people using recent kernels have tried my patch successfully, so it can be used as a temporary transition hack. Also, the patch exposes a serious problem with the sysfs gpio, which is currently broken. Maybe we should consider removing the interface in a near future release, as it has been advocated several times before, since it has long been deprecated, has a much better substitute API and, the worse part, it is broken and no one seems to have a high priority in fixing it. IIRC, the last time I read, the kernel documentation said that the API would be removed in 2020, so we are a bit late :). I know that removing an API has lots of implications, so the consequences must be carefully balanced. > Ciao, Thorsten Best regards, Marcelo. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2021-12-17 15:35 [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) Marcelo Roberto Jimenez 2021-12-18 6:28 ` Thorsten Leemhuis 2022-02-11 0:02 ` Linus Walleij @ 2022-03-14 15:55 ` Michael Walle 2022-03-15 15:32 ` Bartosz Golaszewski 2 siblings, 1 reply; 28+ messages in thread From: Michael Walle @ 2022-03-14 15:55 UTC (permalink / raw) To: marcelo.jimenez Cc: achant, brgl, edmondchung, geert, linus.walleij, linux-gpio, regressions, sfr, stable, tanzilli, treding, vidyas, willmcvicker, Horatiu Vultur, Lars Povlsen, Michael Walle Hi, > Some GPIO lines have stopped working after the patch > commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > > And this has supposedly been fixed in the following patches > commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") > > But an erratic behavior where some GPIO lines work while others do not work > has been introduced. > > This patch reverts those changes so that the sysfs-gpio interface works > properly again. > > Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> This breaks the pinctrl-microchip-sgpio driver as far as I can see. I tried to debug it and this is what I have discovered so far: (1) the sgpio driver will use the gpio_stub_drv for its child nodes. Looks like a workaround, see [1]. (2) these will have an empty gpio range (3) with the changes of this patch, pinctrl_gpio_request() will now be called and will fail with -EPROBE_DEFER. I'm not exactly sure what to do here. Saravana Kannan once suggested to use devm_of_platform_populate() to probe the child nodes [2]. But I haven't found any other driver doing that. Also, I'm not sure if there are any other other driver which get broken by this. I.e. ones falling into the gpio_stub_drv category. [1] https://lore.kernel.org/lkml/20210122193600.1415639-1-saravanak@google.com/ [2] https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/ -michael NB. this patch doesn't contain a Fixes tag. Was this on purpose? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-03-14 15:55 ` Michael Walle @ 2022-03-15 15:32 ` Bartosz Golaszewski 2022-03-15 15:45 ` Michael Walle 0 siblings, 1 reply; 28+ messages in thread From: Bartosz Golaszewski @ 2022-03-15 15:32 UTC (permalink / raw) To: Michael Walle Cc: Marcelo Roberto Jimenez, Andrew Chant, Edmond Chung, Geert Uytterhoeven, Linus Walleij, open list:GPIO SUBSYSTEM, regressions, Stephen Rothwell, stable, Sergio Tanzilli, Thierry Reding, Vidya Sagar, Will McVicker, Horatiu Vultur, Lars Povlsen On Mon, Mar 14, 2022 at 4:55 PM Michael Walle <michael@walle.cc> wrote: > > Hi, > > > Some GPIO lines have stopped working after the patch > > commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") > > > > And this has supposedly been fixed in the following patches > > commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") > > commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") > > > > But an erratic behavior where some GPIO lines work while others do not work > > has been introduced. > > > > This patch reverts those changes so that the sysfs-gpio interface works > > properly again. > > > > Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> > > This breaks the pinctrl-microchip-sgpio driver as far as I can see. > > I tried to debug it and this is what I have discovered so far: > (1) the sgpio driver will use the gpio_stub_drv for its child nodes. > Looks like a workaround, see [1]. > (2) these will have an empty gpio range > (3) with the changes of this patch, pinctrl_gpio_request() will now > be called and will fail with -EPROBE_DEFER. > > I'm not exactly sure what to do here. Saravana Kannan once suggested > to use devm_of_platform_populate() to probe the child nodes [2]. But > I haven't found any other driver doing that. > TI AEMIF driver (drivers/memory/ti-aemif.c) does something like this: 406 if (np) { 407 for_each_available_child_of_node(np, child_np) { 408 ret = of_platform_populate(child_np, NULL, 409 dev_lookup, dev); 410 if (ret < 0) { 411 of_node_put(child_np); 412 goto error; 413 } 414 } 415 } else if (pdata) { 416 for (i = 0; i < pdata->num_sub_devices; i++) { 417 pdata->sub_devices[i].dev.parent = dev; 418 ret = platform_device_register(&pdata->sub_devices[i]); 419 if (ret) { 420 dev_warn(dev, "Error register sub device %s\n", 421 pdata->sub_devices[i].name); 422 } 423 } 424 } A bunch of different devices (like NAND) get instantiated this way. Would this work? Bart > Also, I'm not sure if there are any other other driver which get > broken by this. I.e. ones falling into the gpio_stub_drv category. > > [1] https://lore.kernel.org/lkml/20210122193600.1415639-1-saravanak@google.com/ > [2] https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/ > > -michael > > NB. this patch doesn't contain a Fixes tag. Was this on purpose? ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-03-15 15:32 ` Bartosz Golaszewski @ 2022-03-15 15:45 ` Michael Walle 2022-03-17 8:37 ` Andy Shevchenko 0 siblings, 1 reply; 28+ messages in thread From: Michael Walle @ 2022-03-15 15:45 UTC (permalink / raw) To: Bartosz Golaszewski, Saravana Kannan Cc: Marcelo Roberto Jimenez, Andrew Chant, Edmond Chung, Geert Uytterhoeven, Linus Walleij, open list:GPIO SUBSYSTEM, regressions, Stephen Rothwell, stable, Sergio Tanzilli, Thierry Reding, Vidya Sagar, Will McVicker, Horatiu Vultur, Lars Povlsen [+ Saravana ] Am 2022-03-15 16:32, schrieb Bartosz Golaszewski: > On Mon, Mar 14, 2022 at 4:55 PM Michael Walle <michael@walle.cc> wrote: >> > Some GPIO lines have stopped working after the patch >> > commit 2ab73c6d8323f ("gpio: Support GPIO controllers without pin-ranges") >> > >> > And this has supposedly been fixed in the following patches >> > commit 89ad556b7f96a ("gpio: Avoid using pin ranges with !PINCTRL") >> > commit 6dbbf84603961 ("gpiolib: Don't free if pin ranges are not defined") >> > >> > But an erratic behavior where some GPIO lines work while others do not work >> > has been introduced. >> > >> > This patch reverts those changes so that the sysfs-gpio interface works >> > properly again. >> > >> > Signed-off-by: Marcelo Roberto Jimenez <marcelo.jimenez@gmail.com> >> >> This breaks the pinctrl-microchip-sgpio driver as far as I can see. >> >> I tried to debug it and this is what I have discovered so far: >> (1) the sgpio driver will use the gpio_stub_drv for its child nodes. >> Looks like a workaround, see [1]. >> (2) these will have an empty gpio range >> (3) with the changes of this patch, pinctrl_gpio_request() will now >> be called and will fail with -EPROBE_DEFER. >> >> I'm not exactly sure what to do here. Saravana Kannan once suggested >> to use devm_of_platform_populate() to probe the child nodes [2]. But >> I haven't found any other driver doing that. Oh I meant gpio/pinctrl drivers. > TI AEMIF driver (drivers/memory/ti-aemif.c) does something like this: > > 406 if (np) { > 407 for_each_available_child_of_node(np, child_np) { > 408 ret = of_platform_populate(child_np, NULL, > 409 dev_lookup, > dev); > 410 if (ret < 0) { > 411 of_node_put(child_np); > 412 goto error; > 413 } > 414 } > 415 } else if (pdata) { > 416 for (i = 0; i < pdata->num_sub_devices; i++) { > 417 pdata->sub_devices[i].dev.parent = dev; > 418 ret = > platform_device_register(&pdata->sub_devices[i]); > 419 if (ret) { > 420 dev_warn(dev, "Error register sub > device %s\n", > 421 > pdata->sub_devices[i].name); > 422 } > 423 } > 424 } > > A bunch of different devices (like NAND) get instantiated this way. > Would this work? I started to try this out, but then I was wondering if there weren't other gpio/pinctrl drivers with the same problem. And judging by the reports [1], I'd say there are. Then I wasn't sure if this is actually the correct fix here - or if that old workaround [2] doesn't work anymore because it might have that empty ranges "feature". To answer your question: I don't know. But I don't know if that is actually the correct way of fixing this either. >> Also, I'm not sure if there are any other other driver which get >> broken by this. I.e. ones falling into the gpio_stub_drv category. >> >> [1] >> https://lore.kernel.org/lkml/20210122193600.1415639-1-saravanak@google.com/ >> [2] >> https://lore.kernel.org/lkml/CAGETcx9PiX==mLxB9PO8Myyk6u2vhPVwTMsA5NkD-ywH5xhusw@mail.gmail.com/ >> >> -michael >> >> NB. this patch doesn't contain a Fixes tag. Was this on purpose? -michael [1] https://lore.kernel.org/lkml/20220314192522.GA3031157@roeck-us.net/ [2] https://lore.kernel.org/lkml/20210122193600.1415639-1-saravanak@google.com/ ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-03-15 15:45 ` Michael Walle @ 2022-03-17 8:37 ` Andy Shevchenko 2022-03-17 8:48 ` Michael Walle 0 siblings, 1 reply; 28+ messages in thread From: Andy Shevchenko @ 2022-03-17 8:37 UTC (permalink / raw) To: Michael Walle Cc: Bartosz Golaszewski, Saravana Kannan, Marcelo Roberto Jimenez, Andrew Chant, Edmond Chung, Geert Uytterhoeven, Linus Walleij, open list:GPIO SUBSYSTEM, regressions, Stephen Rothwell, stable, Sergio Tanzilli, Thierry Reding, Vidya Sagar, Will McVicker, Horatiu Vultur, Lars Povlsen On Thu, Mar 17, 2022 at 7:36 AM Michael Walle <michael@walle.cc> wrote: > Am 2022-03-15 16:32, schrieb Bartosz Golaszewski: > > On Mon, Mar 14, 2022 at 4:55 PM Michael Walle <michael@walle.cc> wrote: ... > I started to try this out, but then I was wondering if there weren't > other gpio/pinctrl drivers with the same problem. And judging by the > reports [1], I'd say there are. Then I wasn't sure if this is actually > the correct fix here - or if that old workaround [2] doesn't work > anymore because it might have that empty ranges "feature". > > To answer your question: I don't know. But I don't know if that is > actually the correct way of fixing this either. > > >> Also, I'm not sure if there are any other other driver which get > >> broken by this. I.e. ones falling into the gpio_stub_drv category. I know that OF is a mess, but I want to understand why in ACPI we haven't experienced such an issue. Any pointers would be appreciated. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) 2022-03-17 8:37 ` Andy Shevchenko @ 2022-03-17 8:48 ` Michael Walle 0 siblings, 0 replies; 28+ messages in thread From: Michael Walle @ 2022-03-17 8:48 UTC (permalink / raw) To: Andy Shevchenko Cc: Bartosz Golaszewski, Saravana Kannan, Marcelo Roberto Jimenez, Andrew Chant, Edmond Chung, Geert Uytterhoeven, Linus Walleij, open list:GPIO SUBSYSTEM, regressions, Stephen Rothwell, stable, Sergio Tanzilli, Thierry Reding, Vidya Sagar, Will McVicker, Horatiu Vultur, Lars Povlsen Am 2022-03-17 09:37, schrieb Andy Shevchenko: > On Thu, Mar 17, 2022 at 7:36 AM Michael Walle <michael@walle.cc> wrote: >> Am 2022-03-15 16:32, schrieb Bartosz Golaszewski: >> > On Mon, Mar 14, 2022 at 4:55 PM Michael Walle <michael@walle.cc> wrote: > > ... > >> I started to try this out, but then I was wondering if there weren't >> other gpio/pinctrl drivers with the same problem. And judging by the >> reports [1], I'd say there are. Then I wasn't sure if this is actually >> the correct fix here - or if that old workaround [2] doesn't work >> anymore because it might have that empty ranges "feature". >> >> To answer your question: I don't know. But I don't know if that is >> actually the correct way of fixing this either. >> >> >> Also, I'm not sure if there are any other other driver which get >> >> broken by this. I.e. ones falling into the gpio_stub_drv category. > > I know that OF is a mess, but I want to understand why in ACPI we > haven't experienced such an issue. Any pointers would be appreciated. During debugging I've seen that the pinctrl-microchip-sgpio will report itself as gpio_stub_drv. You'll find that this driver was added by the following commit: commit 4731210c09f5977300f439b6c56ba220c65b2348 Author: Saravana Kannan <saravanak@google.com> Date: Fri Jan 22 11:35:59 2021 -0800 gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default The microchip driver has actually a binding which was described in that commit message. Thus I concluded, that it makes sense this driver falls into that workaround. That is where I stopped and wrote this mail. Actually, I haven't found out yet where that fallback to gpio_stub_drv is happening. -michael ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2022-05-20 17:28 UTC | newest] Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-17 15:35 [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c) Marcelo Roberto Jimenez 2021-12-18 6:28 ` Thorsten Leemhuis 2021-12-20 14:57 ` Bartosz Golaszewski 2021-12-20 15:14 ` Geert Uytterhoeven 2021-12-20 19:24 ` Will McVicker 2021-12-20 20:41 ` Marcelo Roberto Jimenez 2021-12-20 20:41 ` Marcelo Roberto Jimenez 2021-12-20 20:41 ` Marcelo Roberto Jimenez 2022-01-10 7:02 ` Thorsten Leemhuis 2022-01-12 0:09 ` Marcelo Roberto Jimenez 2022-02-08 12:24 ` Thorsten Leemhuis 2022-02-17 19:11 ` Thierry Reding 2022-02-11 0:02 ` Linus Walleij 2022-02-11 22:36 ` Marcelo Roberto Jimenez 2022-02-12 16:54 ` Linus Walleij 2022-02-13 23:23 ` Marcelo Roberto Jimenez 2022-02-15 21:56 ` Linus Walleij 2022-02-16 14:40 ` Bartosz Golaszewski 2022-03-04 7:13 ` Thorsten Leemhuis 2022-03-07 9:58 ` Bartosz Golaszewski 2022-03-07 10:12 ` Thorsten Leemhuis 2022-05-20 9:12 ` Thorsten Leemhuis 2022-05-20 17:28 ` Marcelo Roberto Jimenez 2022-03-14 15:55 ` Michael Walle 2022-03-15 15:32 ` Bartosz Golaszewski 2022-03-15 15:45 ` Michael Walle 2022-03-17 8:37 ` Andy Shevchenko 2022-03-17 8:48 ` Michael Walle
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).