[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACMJSesqtkorg1akuXjMa9U1fe60aDhfGOSB_T6mX5CtCYDwtg@mail.gmail.com>
Date: Tue, 6 May 2025 19:28:00 +0200
From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Bjorn Andersson <andersson@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
Guenter Roeck <linux@...ck-us.net>, Josh Cartwright <joshc@...eaurora.org>,
Matti Vaittinen <mazziesaccount@...il.com>, Doug Anderson <dianders@...omium.org>,
linux-arm-msm@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] pinctrl: qcom: don't crash on enabling GPIO HOG pins
On Sat, 3 May 2025 at 07:32, Dmitry Baryshkov
<dmitry.baryshkov@....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@...omium.org>
> Closes: https://lore.kernel.org/r/CAD=FV=Vg8_ZOLgLoC4WhFPzhVsxXFC19NrF38W6cW_W_3nFjbw@mail.gmail.com
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@....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
>
Powered by blists - more mailing lists