Re: [PATCH] gpio: Revert regression in sysfs-gpio (gpiolib.c)
From: Marcelo Roberto Jimenez <hidden>
Date: 2021-12-20 20:42:11
Also in:
linux-gpio, regressions
Hi Will, On Mon, Dec 20, 2021 at 4:25 PM Will McVicker [off-list ref] wrote:
On Mon, Dec 20, 2021 at 7:14 AM Geert Uytterhoeven [off-list ref] wrote:quoted
On Mon, Dec 20, 2021 at 3:57 PM Bartosz Golaszewski [off-list ref] wrote:quoted
On Sat, Dec 18, 2021 at 7:28 AM Thorsten Leemhuis [off-list ref] wrote:quoted
[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:quoted
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 TorvaldsHi 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.