Re: [PATCH] pinctrl: stm32: Fix reset control leak in stm32_pctl_probe() error paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] pinctrl: stm32: incorrect Fixes trailer in commit message
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] pinctrl: stm32: clock leak in stm32_pctl_probe() [1]
--

commit 7844cefd15f00547f16870f8594d1153fe131bff
Author: Xiangwu Lai <laixiangwu@cmss.chinamobile.com>

> pinctrl: stm32: Fix reset control leak in stm32_pctl_probe() error paths
>
> stm32_pctl_probe() acquires an exclusive reset control for every GPIO
> bank with of_reset_control_get_exclusive(), but none of the error
> paths release them again: the bank loop returns directly on
> -EPROBE_DEFER and on a failed clock lookup, and the
> clk_bulk_prepare_enable() failure and the err_register path also
> leave the already acquired reset controls behind.
>
> On success the reset controls are kept for the lifetime of the device
> and are never released either (the driver has no .remove), but that
> is harmless.  On a failed probe, and in particular when the probe is
> retried after -EPROBE_DEFER, every attempt leaks another set of
> reset control references.
>
> Release the acquired reset controls on all error paths.
>
> Fixes: bb949ed9b16b ("pinctrl: stm32: Switch to use for_each_gpiochip_node() helper")
> Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>

[Severity: Low]
Commit bb949ed9b16b ("pinctrl: stm32: Switch to use for_each_gpiochip_node()
helper") only replaced the child-node iteration macro without changing how
bank->rstc was acquired or cleaned up.  Should the Fixes tag instead point
to 2254e77665d5 ("pinctrl: stm32: defer probe if reset resource is not yet
ready"), which moved the of_reset_control_get_exclusive() loop into
stm32_pctl_probe() and added the -EPROBE_DEFER return path?

> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 405ebb22c9eaf..02410e57f97d7 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -1963,15 +1963,17 @@ int stm32_pctl_probe(struct platform_device *pdev)
>  		bank->rstc = of_reset_control_get_exclusive(np, NULL);
>  		if (PTR_ERR(bank->rstc) == -EPROBE_DEFER) {
>  			fwnode_handle_put(child);
> -			return -EPROBE_DEFER;
> +			ret = -EPROBE_DEFER;
> +			goto err_put_rstcs;
>  		}

[Severity: Medium]
This problem wasn't introduced by this patch, but does jumping to
err_put_rstcs without calling clk_bulk_put(i, pctl->clks) leak the clock
references [1] acquired in earlier iterations of the loop when
of_reset_control_get_exclusive() returns -EPROBE_DEFER on a subsequent bank
(i > 0)?

[1] https://sashiko.dev/#/bug/linux-022b050a-8471-43dd-a330-d8c0a082f0ff

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110065722.1340-1-laixiangwu@cmss.chinamobile.com?part=1