Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c)

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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 --]