| Message ID | 20250503-pinctrl-msm-fix-v1-0-da9b7f6408f4@oss.qualcomm.com |
|---|---|
| Headers | show |
| Series | pinctrl: qcom: several fixes for the pinctrl-msm code | expand |
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
On 03/05/2025 08:32, Dmitry Baryshkov wrote: > In order to simplify cleanup actions, use devres-enabled version of > gpiochip_add_data(). > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 88dd462516c76d58b43d49accbddeea38af8f1ec..b2e8f7b3f3e3d5d232b2bd60e5cace62b21ffb03 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -1449,7 +1449,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > girq->handler = handle_bad_irq; > girq->parents[0] = pctrl->irq; > > - ret = gpiochip_add_data(&pctrl->chip, pctrl); > + ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl); > if (ret) { > dev_err(pctrl->dev, "Failed register gpiochip\n"); > return ret; > @@ -1470,7 +1470,6 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > dev_name(pctrl->dev), 0, 0, chip->ngpio); > if (ret) { > dev_err(pctrl->dev, "Failed to add pin range\n"); > - gpiochip_remove(&pctrl->chip); > return ret; > } > } > @@ -1608,9 +1607,6 @@ EXPORT_SYMBOL(msm_pinctrl_probe); > > void msm_pinctrl_remove(struct platform_device *pdev) > { > - struct msm_pinctrl *pctrl = platform_get_drvdata(pdev); > - > - gpiochip_remove(&pctrl->chip); > } > EXPORT_SYMBOL(msm_pinctrl_remove); I suppose this should also make the time window when the GPIO chip has gone, but pinmux is still there (to call the 'gpiochip_line_is_valid()') smaller. FWIW: Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> Yours, -- Matti
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: > > In order to simplify cleanup actions, use devres-enabled version of > gpiochip_add_data(). > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 88dd462516c76d58b43d49accbddeea38af8f1ec..b2e8f7b3f3e3d5d232b2bd60e5cace62b21ffb03 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -1449,7 +1449,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > girq->handler = handle_bad_irq; > girq->parents[0] = pctrl->irq; > > - ret = gpiochip_add_data(&pctrl->chip, pctrl); > + ret = devm_gpiochip_add_data(pctrl->dev, &pctrl->chip, pctrl); > if (ret) { > dev_err(pctrl->dev, "Failed register gpiochip\n"); > return ret; > @@ -1470,7 +1470,6 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > dev_name(pctrl->dev), 0, 0, chip->ngpio); > if (ret) { > dev_err(pctrl->dev, "Failed to add pin range\n"); > - gpiochip_remove(&pctrl->chip); > return ret; > } > } > @@ -1608,9 +1607,6 @@ EXPORT_SYMBOL(msm_pinctrl_probe); > > void msm_pinctrl_remove(struct platform_device *pdev) > { > - struct msm_pinctrl *pctrl = platform_get_drvdata(pdev); > - > - gpiochip_remove(&pctrl->chip); > } > EXPORT_SYMBOL(msm_pinctrl_remove); > > > -- > 2.39.5 > If you're at it then why not remove this function here and the callback assignment throughout the pinctrl/qcom/ directory? Bart
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 >
Fix/rework several issues in the pinctrl-msm common code. The most important fix is the one for the gpio-hog handling. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> --- Dmitry Baryshkov (4): pinctrl: qcom: don't crash on enabling GPIO HOG pins pinctrl: qcom: switch to devm_register_sys_off_handler() pinctrl: qcom: switch to devm_gpiochip_add_data() pinctrl: qcom: drop msm_pinctrl_remove() drivers/pinctrl/qcom/pinctrl-apq8064.c | 1 - drivers/pinctrl/qcom/pinctrl-apq8084.c | 1 - drivers/pinctrl/qcom/pinctrl-ipq4019.c | 1 - drivers/pinctrl/qcom/pinctrl-ipq5018.c | 1 - drivers/pinctrl/qcom/pinctrl-ipq5332.c | 1 - drivers/pinctrl/qcom/pinctrl-ipq5424.c | 1 - drivers/pinctrl/qcom/pinctrl-ipq6018.c | 1 - drivers/pinctrl/qcom/pinctrl-ipq8064.c | 1 - drivers/pinctrl/qcom/pinctrl-ipq8074.c | 1 - drivers/pinctrl/qcom/pinctrl-ipq9574.c | 1 - drivers/pinctrl/qcom/pinctrl-mdm9607.c | 1 - drivers/pinctrl/qcom/pinctrl-mdm9615.c | 1 - drivers/pinctrl/qcom/pinctrl-msm.c | 41 ++++++++++++++++----------------- drivers/pinctrl/qcom/pinctrl-msm.h | 1 - drivers/pinctrl/qcom/pinctrl-msm8226.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8660.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8909.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8916.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8917.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8953.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8960.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8976.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8994.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8996.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8998.c | 1 - drivers/pinctrl/qcom/pinctrl-msm8x74.c | 1 - drivers/pinctrl/qcom/pinctrl-qcm2290.c | 1 - drivers/pinctrl/qcom/pinctrl-qcs404.c | 1 - drivers/pinctrl/qcom/pinctrl-qcs615.c | 1 - drivers/pinctrl/qcom/pinctrl-qcs8300.c | 1 - drivers/pinctrl/qcom/pinctrl-qdf2xxx.c | 1 - drivers/pinctrl/qcom/pinctrl-qdu1000.c | 1 - drivers/pinctrl/qcom/pinctrl-sa8775p.c | 1 - drivers/pinctrl/qcom/pinctrl-sar2130p.c | 1 - drivers/pinctrl/qcom/pinctrl-sc7180.c | 1 - drivers/pinctrl/qcom/pinctrl-sc7280.c | 1 - drivers/pinctrl/qcom/pinctrl-sc8180x.c | 1 - drivers/pinctrl/qcom/pinctrl-sc8280xp.c | 1 - drivers/pinctrl/qcom/pinctrl-sdm660.c | 1 - drivers/pinctrl/qcom/pinctrl-sdm670.c | 1 - drivers/pinctrl/qcom/pinctrl-sdm845.c | 1 - drivers/pinctrl/qcom/pinctrl-sdx55.c | 1 - drivers/pinctrl/qcom/pinctrl-sdx65.c | 1 - drivers/pinctrl/qcom/pinctrl-sdx75.c | 1 - drivers/pinctrl/qcom/pinctrl-sm4450.c | 1 - drivers/pinctrl/qcom/pinctrl-sm6115.c | 1 - drivers/pinctrl/qcom/pinctrl-sm6125.c | 1 - drivers/pinctrl/qcom/pinctrl-sm6350.c | 1 - drivers/pinctrl/qcom/pinctrl-sm6375.c | 1 - drivers/pinctrl/qcom/pinctrl-sm7150.c | 1 - drivers/pinctrl/qcom/pinctrl-sm8150.c | 1 - drivers/pinctrl/qcom/pinctrl-sm8250.c | 1 - drivers/pinctrl/qcom/pinctrl-sm8350.c | 1 - drivers/pinctrl/qcom/pinctrl-sm8450.c | 1 - drivers/pinctrl/qcom/pinctrl-sm8550.c | 1 - drivers/pinctrl/qcom/pinctrl-sm8650.c | 1 - drivers/pinctrl/qcom/pinctrl-sm8750.c | 1 - drivers/pinctrl/qcom/pinctrl-x1e80100.c | 1 - 58 files changed, 20 insertions(+), 78 deletions(-) --- base-commit: 6ac908f24cd7ddae52c496bbc888e97ee7b033ac change-id: 20250503-pinctrl-msm-fix-b225fc7ead2c Best regards,