1. Patchew
  2. linux
  3. [PATCH 0/4] pinctrl: qcom: several fixes for the pinctrl-msm code
  4. View patch

[PATCH 1/4] pinctrl: qcom: don't crash on enabling GPIO HOG pins

Dmitry Baryshkov posted 4 patches 9 months, 1 week ago
There is a newer version of this series
[PATCH 1/4] pinctrl: qcom: don't crash on enabling GPIO HOG pins
Posted by Dmitry Baryshkov 9 months, 1 week ago 
On Qualcomm platforms if the board uses GPIO hogs msm_pinmux_request()
calls gpiochip_line_is_valid(). After commit 8015443e24e7 ("gpio: Hide
valid_mask from direct assignments") gpiochip_line_is_valid() uses
gc->gpiodev, which is NULL when GPIO hog pins are being processed.
Thus after this commit using GPIO hogs causes the following crash. In
order to fix this, verify that gpiochip->gpiodev is not NULL.

Note: it is not possible to reorder calls (e.g. by calling
msm_gpio_init() before pinctrl registration or by splitting
pinctrl_register() into _and_init() and pinctrl_enable() and calling the
latter function after msm_gpio_init()) because GPIO chip registration
would fail with EPROBE_DEFER if pinctrl is not enabled at the time of
registration.

pc : gpiochip_line_is_valid+0x4/0x28
lr : msm_pinmux_request+0x24/0x40
sp : ffff8000808eb870
x29: ffff8000808eb870 x28: 0000000000000000 x27: 0000000000000000
x26: 0000000000000000 x25: ffff726240f9d040 x24: 0000000000000000
x23: ffff7262438c0510 x22: 0000000000000080 x21: ffff726243ea7000
x20: ffffab13f2c4e698 x19: 0000000000000080 x18: 00000000ffffffff
x17: ffff726242ba6000 x16: 0000000000000100 x15: 0000000000000028
x14: 0000000000000000 x13: 0000000000002948 x12: 0000000000000003
x11: 0000000000000078 x10: 0000000000002948 x9 : ffffab13f50eb5e8
x8 : 0000000003ecb21b x7 : 000000000000002d x6 : 0000000000000b68
x5 : 0000007fffffffff x4 : ffffab13f52f84a8 x3 : ffff8000808eb804
x2 : ffffab13f1de8190 x1 : 0000000000000080 x0 : 0000000000000000
Call trace:
 gpiochip_line_is_valid+0x4/0x28 (P)
 pin_request+0x208/0x2c0
 pinmux_enable_setting+0xa0/0x2e0
 pinctrl_commit_state+0x150/0x26c
 pinctrl_enable+0x6c/0x2a4
 pinctrl_register+0x3c/0xb0
 devm_pinctrl_register+0x58/0xa0
 msm_pinctrl_probe+0x2a8/0x584
 sdm845_pinctrl_probe+0x20/0x88
 platform_probe+0x68/0xc0
 really_probe+0xbc/0x298
 __driver_probe_device+0x78/0x12c
 driver_probe_device+0x3c/0x160
 __device_attach_driver+0xb8/0x138
 bus_for_each_drv+0x84/0xe0
 __device_attach+0x9c/0x188
 device_initial_probe+0x14/0x20
 bus_probe_device+0xac/0xb0
 deferred_probe_work_func+0x8c/0xc8
 process_one_work+0x208/0x5e8
 worker_thread+0x1b4/0x35c
 kthread+0x144/0x220
 ret_from_fork+0x10/0x20
Code: b5fffba0 17fffff2 9432ec27 f9400400 (f9428800)

Fixes: 8015443e24e7 ("gpio: Hide valid_mask from direct assignments")
Reported-by: Doug Anderson <dianders@chromium.org>
Closes: https://lore.kernel.org/r/CAD=FV=Vg8_ZOLgLoC4WhFPzhVsxXFC19NrF38W6cW_W_3nFjbw@mail.gmail.com
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/pinctrl/qcom/pinctrl-msm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 9ec15ae4a104cbeb9a7d819b964d341f3bba58ea..a99275f3c4a66a39f4d9318fe918101127ef4487 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -149,6 +149,13 @@ static int msm_pinmux_request(struct pinctrl_dev *pctldev, unsigned offset)
 	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	struct gpio_chip *chip = &pctrl->chip;
 
+	/*
+	 * hog pins are requested before registering GPIO chip, don't crash in
+	 * gpiochip_line_is_valid().
+	 */
+	if (!chip->gpiodev)
+		return 0;
+
 	return gpiochip_line_is_valid(chip, offset) ? 0 : -EINVAL;
 }
 

-- 
2.39.5
Re: [PATCH 1/4] pinctrl: qcom: don't crash on enabling GPIO HOG pins
Posted by Bartosz Golaszewski 9 months, 1 week ago 
On Sat, 3 May 2025 at 07:32, Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> On Qualcomm platforms if the board uses GPIO hogs msm_pinmux_request()
> calls gpiochip_line_is_valid(). After commit 8015443e24e7 ("gpio: Hide
> valid_mask from direct assignments") gpiochip_line_is_valid() uses
> gc->gpiodev, which is NULL when GPIO hog pins are being processed.
> Thus after this commit using GPIO hogs causes the following crash. In
> order to fix this, verify that gpiochip->gpiodev is not NULL.
>
> Note: it is not possible to reorder calls (e.g. by calling
> msm_gpio_init() before pinctrl registration or by splitting
> pinctrl_register() into _and_init() and pinctrl_enable() and calling the
> latter function after msm_gpio_init()) because GPIO chip registration
> would fail with EPROBE_DEFER if pinctrl is not enabled at the time of
> registration.
>
> pc : gpiochip_line_is_valid+0x4/0x28
> lr : msm_pinmux_request+0x24/0x40
> sp : ffff8000808eb870
> x29: ffff8000808eb870 x28: 0000000000000000 x27: 0000000000000000
> x26: 0000000000000000 x25: ffff726240f9d040 x24: 0000000000000000
> x23: ffff7262438c0510 x22: 0000000000000080 x21: ffff726243ea7000
> x20: ffffab13f2c4e698 x19: 0000000000000080 x18: 00000000ffffffff
> x17: ffff726242ba6000 x16: 0000000000000100 x15: 0000000000000028
> x14: 0000000000000000 x13: 0000000000002948 x12: 0000000000000003
> x11: 0000000000000078 x10: 0000000000002948 x9 : ffffab13f50eb5e8
> x8 : 0000000003ecb21b x7 : 000000000000002d x6 : 0000000000000b68
> x5 : 0000007fffffffff x4 : ffffab13f52f84a8 x3 : ffff8000808eb804
> x2 : ffffab13f1de8190 x1 : 0000000000000080 x0 : 0000000000000000
> Call trace:
>  gpiochip_line_is_valid+0x4/0x28 (P)
>  pin_request+0x208/0x2c0
>  pinmux_enable_setting+0xa0/0x2e0
>  pinctrl_commit_state+0x150/0x26c
>  pinctrl_enable+0x6c/0x2a4
>  pinctrl_register+0x3c/0xb0
>  devm_pinctrl_register+0x58/0xa0
>  msm_pinctrl_probe+0x2a8/0x584
>  sdm845_pinctrl_probe+0x20/0x88
>  platform_probe+0x68/0xc0
>  really_probe+0xbc/0x298
>  __driver_probe_device+0x78/0x12c
>  driver_probe_device+0x3c/0x160
>  __device_attach_driver+0xb8/0x138
>  bus_for_each_drv+0x84/0xe0
>  __device_attach+0x9c/0x188
>  device_initial_probe+0x14/0x20
>  bus_probe_device+0xac/0xb0
>  deferred_probe_work_func+0x8c/0xc8
>  process_one_work+0x208/0x5e8
>  worker_thread+0x1b4/0x35c
>  kthread+0x144/0x220
>  ret_from_fork+0x10/0x20
> Code: b5fffba0 17fffff2 9432ec27 f9400400 (f9428800)
>
> Fixes: 8015443e24e7 ("gpio: Hide valid_mask from direct assignments")
> Reported-by: Doug Anderson <dianders@chromium.org>
> Closes: https://lore.kernel.org/r/CAD=FV=Vg8_ZOLgLoC4WhFPzhVsxXFC19NrF38W6cW_W_3nFjbw@mail.gmail.com
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 9ec15ae4a104cbeb9a7d819b964d341f3bba58ea..a99275f3c4a66a39f4d9318fe918101127ef4487 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -149,6 +149,13 @@ static int msm_pinmux_request(struct pinctrl_dev *pctldev, unsigned offset)
>         struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>         struct gpio_chip *chip = &pctrl->chip;
>
> +       /*
> +        * hog pins are requested before registering GPIO chip, don't crash in
> +        * gpiochip_line_is_valid().
> +        */
> +       if (!chip->gpiodev)
> +               return 0;
> +

I really dislike you dereferencing gpiodev here which is (implicitly,
I know...) very much a private structure for GPIOLIB. Can we move this
into gpiochip_line_is_valid() itself?

Treewide there's only one driver (under drivers/pinctrl/) that
accesses gc->gpiodev and I would love to fix that as well and have
nobody dereference this.

Bart

>         return gpiochip_line_is_valid(chip, offset) ? 0 : -EINVAL;
>  }
>
>
> --
> 2.39.5
>
Re: [PATCH 1/4] pinctrl: qcom: don't crash on enabling GPIO HOG pins
Posted by Linus Walleij 9 months ago 
Hi Dmitry,

thanks for your patch!

On Tue, May 6, 2025 at 7:28 PM Bartosz Golaszewski
<bartosz.golaszewski@linaro.org> wrote:
> On Sat, 3 May 2025 at 07:32, Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com> wrote:

> > +       /*
> > +        * hog pins are requested before registering GPIO chip, don't crash in
> > +        * gpiochip_line_is_valid().
> > +        */
> > +       if (!chip->gpiodev)
> > +               return 0;
> > +
>
> I really dislike you dereferencing gpiodev here which is (implicitly,
> I know...) very much a private structure for GPIOLIB. Can we move this
> into gpiochip_line_is_valid() itself?

I agree with Bartosz. Patch gpiochip_line_is_valid() so everyone
can benefit from the extended check.

Thanks!
Linus Walleij
Re: [PATCH 1/4] pinctrl: qcom: don't crash on enabling GPIO HOG pins
Posted by Doug Anderson 9 months ago 
Hi,

On Tue, May 13, 2025 at 2:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Dmitry,
>
> thanks for your patch!
>
> On Tue, May 6, 2025 at 7:28 PM Bartosz Golaszewski
> <bartosz.golaszewski@linaro.org> wrote:
> > On Sat, 3 May 2025 at 07:32, Dmitry Baryshkov
> > <dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> > > +       /*
> > > +        * hog pins are requested before registering GPIO chip, don't crash in
> > > +        * gpiochip_line_is_valid().
> > > +        */
> > > +       if (!chip->gpiodev)
> > > +               return 0;
> > > +
> >
> > I really dislike you dereferencing gpiodev here which is (implicitly,
> > I know...) very much a private structure for GPIOLIB. Can we move this
> > into gpiochip_line_is_valid() itself?
>
> I agree with Bartosz. Patch gpiochip_line_is_valid() so everyone
> can benefit from the extended check.

Any chance we can get a solution landed sooner rather than later?
Every time I test mainline I have to account for this bug or my device
crashes at bootup. ;-)

-Doug
Re: [PATCH 1/4] pinctrl: qcom: don't crash on enabling GPIO HOG pins
Posted by Linus Walleij 9 months ago 
On Tue, May 13, 2025 at 5:08 PM Doug Anderson <dianders@chromium.org> wrote:
> On Tue, May 13, 2025 at 2:27 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > Hi Dmitry,
> >
> > thanks for your patch!
> >
> > On Tue, May 6, 2025 at 7:28 PM Bartosz Golaszewski
> > <bartosz.golaszewski@linaro.org> wrote:
> > > On Sat, 3 May 2025 at 07:32, Dmitry Baryshkov
> > > <dmitry.baryshkov@oss.qualcomm.com> wrote:
> >
> > > > +       /*
> > > > +        * hog pins are requested before registering GPIO chip, don't crash in
> > > > +        * gpiochip_line_is_valid().
> > > > +        */
> > > > +       if (!chip->gpiodev)
> > > > +               return 0;
> > > > +
> > >
> > > I really dislike you dereferencing gpiodev here which is (implicitly,
> > > I know...) very much a private structure for GPIOLIB. Can we move this
> > > into gpiochip_line_is_valid() itself?
> >
> > I agree with Bartosz. Patch gpiochip_line_is_valid() so everyone
> > can benefit from the extended check.
>
> Any chance we can get a solution landed sooner rather than later?
> Every time I test mainline I have to account for this bug or my device
> crashes at bootup. ;-)

Normally at this point in the development cycle only super-crititcal
patches go to the -rc (v6.15) but I trust you more than most so I take
it this is one of those, so we will need to funnel this to Torvalds as
soon as there is an acceptable patch.

Yours,
Linus Walleij
Re: [PATCH 1/4] pinctrl: qcom: don't crash on enabling GPIO HOG pins
Posted by Doug Anderson 9 months, 1 week ago 
Hi,

On Fri, May 2, 2025 at 10:32 PM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> On Qualcomm platforms if the board uses GPIO hogs msm_pinmux_request()
> calls gpiochip_line_is_valid(). After commit 8015443e24e7 ("gpio: Hide
> valid_mask from direct assignments") gpiochip_line_is_valid() uses
> gc->gpiodev, which is NULL when GPIO hog pins are being processed.
> Thus after this commit using GPIO hogs causes the following crash. In
> order to fix this, verify that gpiochip->gpiodev is not NULL.
>
> Note: it is not possible to reorder calls (e.g. by calling
> msm_gpio_init() before pinctrl registration or by splitting
> pinctrl_register() into _and_init() and pinctrl_enable() and calling the
> latter function after msm_gpio_init()) because GPIO chip registration
> would fail with EPROBE_DEFER if pinctrl is not enabled at the time of
> registration.
>
> pc : gpiochip_line_is_valid+0x4/0x28
> lr : msm_pinmux_request+0x24/0x40
> sp : ffff8000808eb870
> x29: ffff8000808eb870 x28: 0000000000000000 x27: 0000000000000000
> x26: 0000000000000000 x25: ffff726240f9d040 x24: 0000000000000000
> x23: ffff7262438c0510 x22: 0000000000000080 x21: ffff726243ea7000
> x20: ffffab13f2c4e698 x19: 0000000000000080 x18: 00000000ffffffff
> x17: ffff726242ba6000 x16: 0000000000000100 x15: 0000000000000028
> x14: 0000000000000000 x13: 0000000000002948 x12: 0000000000000003
> x11: 0000000000000078 x10: 0000000000002948 x9 : ffffab13f50eb5e8
> x8 : 0000000003ecb21b x7 : 000000000000002d x6 : 0000000000000b68
> x5 : 0000007fffffffff x4 : ffffab13f52f84a8 x3 : ffff8000808eb804
> x2 : ffffab13f1de8190 x1 : 0000000000000080 x0 : 0000000000000000
> Call trace:
>  gpiochip_line_is_valid+0x4/0x28 (P)
>  pin_request+0x208/0x2c0
>  pinmux_enable_setting+0xa0/0x2e0
>  pinctrl_commit_state+0x150/0x26c
>  pinctrl_enable+0x6c/0x2a4
>  pinctrl_register+0x3c/0xb0
>  devm_pinctrl_register+0x58/0xa0
>  msm_pinctrl_probe+0x2a8/0x584
>  sdm845_pinctrl_probe+0x20/0x88
>  platform_probe+0x68/0xc0
>  really_probe+0xbc/0x298
>  __driver_probe_device+0x78/0x12c
>  driver_probe_device+0x3c/0x160
>  __device_attach_driver+0xb8/0x138
>  bus_for_each_drv+0x84/0xe0
>  __device_attach+0x9c/0x188
>  device_initial_probe+0x14/0x20
>  bus_probe_device+0xac/0xb0
>  deferred_probe_work_func+0x8c/0xc8
>  process_one_work+0x208/0x5e8
>  worker_thread+0x1b4/0x35c
>  kthread+0x144/0x220
>  ret_from_fork+0x10/0x20
> Code: b5fffba0 17fffff2 9432ec27 f9400400 (f9428800)
>
> Fixes: 8015443e24e7 ("gpio: Hide valid_mask from direct assignments")
> Reported-by: Doug Anderson <dianders@chromium.org>
> Closes: https://lore.kernel.org/r/CAD=FV=Vg8_ZOLgLoC4WhFPzhVsxXFC19NrF38W6cW_W_3nFjbw@mail.gmail.com
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>  drivers/pinctrl/qcom/pinctrl-msm.c | 7 +++++++
>  1 file changed, 7 insertions(+)

So I guess this is fine because nobody would specify a hog in their
device tree that's an invalid GPIO?

In any case, this works for me. Thanks for the fix!

Tested-by: Douglas Anderson <dianders@chromium.org>
Re: [PATCH 1/4] pinctrl: qcom: don't crash on enabling GPIO HOG pins
Posted by Matti Vaittinen 9 months, 1 week ago 
On 03/05/2025 08:32, Dmitry Baryshkov wrote:
> On Qualcomm platforms if the board uses GPIO hogs msm_pinmux_request()
> calls gpiochip_line_is_valid(). After commit 8015443e24e7 ("gpio: Hide
> valid_mask from direct assignments") gpiochip_line_is_valid() uses
> gc->gpiodev, which is NULL when GPIO hog pins are being processed.
> Thus after this commit using GPIO hogs causes the following crash. In
> order to fix this, verify that gpiochip->gpiodev is not NULL.
> 
> Note: it is not possible to reorder calls (e.g. by calling
> msm_gpio_init() before pinctrl registration or by splitting
> pinctrl_register() into _and_init() and pinctrl_enable() and calling the
> latter function after msm_gpio_init()) because GPIO chip registration
> would fail with EPROBE_DEFER if pinctrl is not enabled at the time of
> registration.
> 
> pc : gpiochip_line_is_valid+0x4/0x28
> lr : msm_pinmux_request+0x24/0x40
> sp : ffff8000808eb870
> x29: ffff8000808eb870 x28: 0000000000000000 x27: 0000000000000000
> x26: 0000000000000000 x25: ffff726240f9d040 x24: 0000000000000000
> x23: ffff7262438c0510 x22: 0000000000000080 x21: ffff726243ea7000
> x20: ffffab13f2c4e698 x19: 0000000000000080 x18: 00000000ffffffff
> x17: ffff726242ba6000 x16: 0000000000000100 x15: 0000000000000028
> x14: 0000000000000000 x13: 0000000000002948 x12: 0000000000000003
> x11: 0000000000000078 x10: 0000000000002948 x9 : ffffab13f50eb5e8
> x8 : 0000000003ecb21b x7 : 000000000000002d x6 : 0000000000000b68
> x5 : 0000007fffffffff x4 : ffffab13f52f84a8 x3 : ffff8000808eb804
> x2 : ffffab13f1de8190 x1 : 0000000000000080 x0 : 0000000000000000
> Call trace:
>   gpiochip_line_is_valid+0x4/0x28 (P)
>   pin_request+0x208/0x2c0
>   pinmux_enable_setting+0xa0/0x2e0
>   pinctrl_commit_state+0x150/0x26c
>   pinctrl_enable+0x6c/0x2a4
>   pinctrl_register+0x3c/0xb0
>   devm_pinctrl_register+0x58/0xa0
>   msm_pinctrl_probe+0x2a8/0x584
>   sdm845_pinctrl_probe+0x20/0x88
>   platform_probe+0x68/0xc0
>   really_probe+0xbc/0x298
>   __driver_probe_device+0x78/0x12c
>   driver_probe_device+0x3c/0x160
>   __device_attach_driver+0xb8/0x138
>   bus_for_each_drv+0x84/0xe0
>   __device_attach+0x9c/0x188
>   device_initial_probe+0x14/0x20
>   bus_probe_device+0xac/0xb0
>   deferred_probe_work_func+0x8c/0xc8
>   process_one_work+0x208/0x5e8
>   worker_thread+0x1b4/0x35c
>   kthread+0x144/0x220
>   ret_from_fork+0x10/0x20
> Code: b5fffba0 17fffff2 9432ec27 f9400400 (f9428800)
> 
> Fixes: 8015443e24e7 ("gpio: Hide valid_mask from direct assignments")
> Reported-by: Doug Anderson <dianders@chromium.org>
> Closes: https://lore.kernel.org/r/CAD=FV=Vg8_ZOLgLoC4WhFPzhVsxXFC19NrF38W6cW_W_3nFjbw@mail.gmail.com
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
>   drivers/pinctrl/qcom/pinctrl-msm.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index 9ec15ae4a104cbeb9a7d819b964d341f3bba58ea..a99275f3c4a66a39f4d9318fe918101127ef4487 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -149,6 +149,13 @@ static int msm_pinmux_request(struct pinctrl_dev *pctldev, unsigned offset)
>   	struct msm_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>   	struct gpio_chip *chip = &pctrl->chip;
>   
> +	/*
> +	 * hog pins are requested before registering GPIO chip, don't crash in
> +	 * gpiochip_line_is_valid().
> +	 */
> +	if (!chip->gpiodev)
> +		return 0;
> +

This should fix the reported crash at hog registration. Still, I feel 
this is only papering over a real problem, which is the dependency from 
pinmux request to the gpiodev. As far as I can say, there is no 
mechanism ensuring the gpiochip is there, right? Even though this fixes 
the reported crash, it also causes all early pinmux requests to assume 
all the GPIOs are valid, right?

Also, I suppose there will be a time window at remove path when the 
pinctrl is still there - but gpio has already gone. (I didn't really 
dive into the dirty details of the pinctrl... Perhaps if this is somehow 
prevented?) Anyways, I'm not sure how valid it is to assume the 
gpiochip_line_is_valid() will work in such case?

Unfortunately, I don't have any better suggestion how to fix this. So, 
what little it is worth, I am Ok with applying this, at least as a fix 
for the crash!

>   	return gpiochip_line_is_valid(chip, offset) ? 0 : -EINVAL;
>   }

Yours,
	-- Matti

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.