| Message ID | 20250503-pinctrl-msm-fix-v1-1-da9b7f6408f4@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: qcom: several fixes for the pinctrl-msm code | expand |
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>
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 >
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
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
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
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; }
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(+)