Re: [PATCH v4 08/10] gpio: mxc: use local dev variable

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] pm: devres double-put of runtime PM reference on driver unbind
- [Medium] gpio: mxc: double clock disable on driver unbind due to devm_clk_get_optional_enabled and runtime PM
--

commit 22e651b0a155c89a26d690a7e46fed22ae9cc238
Author: Peng Fan <peng.fan@nxp.com>

gpio: mxc: use local dev variable

Introduce a local 'struct device *dev' variable to replace repeated
'&pdev->dev' dereferences throughout mxc_gpio_probe(), improving
readability.

> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index f5f9ef1564cdf..9d69524e06dbf 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
[ ... ]
> @@ -481,17 +482,17 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>  		return port->irq;
>  
>  	/* the controller clock is optional */
> -	port->clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
> +	port->clk = devm_clk_get_optional_enabled(dev, NULL);

[Severity: Medium]
Does this code lead to a double clock disable during driver unbind?

By using devm_clk_get_optional_enabled() here, a devres action is queued to
disable the clock when the device is unbound. However, when the device is
removed, the driver core calls pm_runtime_put_sync() to synchronously suspend
it, which triggers mxc_gpio_runtime_suspend() and disables the clock:

drivers/gpio/gpio-mxc.c:mxc_gpio_runtime_suspend() {
    ...
    clk_disable_unprepare(port->clk);
    ...
}

Afterwards, the devres cleanup sequence runs (leaving the device suspended)
and unconditionally executes the devm clock release action. Won't this call
clk_disable_unprepare() again on an already-disabled clock and trigger a
warning in the clock core?

>  	if (IS_ERR(port->clk))
>  		return PTR_ERR(port->clk);
>  
> -	err = devm_pm_runtime_get_noresume(&pdev->dev);
> +	err = devm_pm_runtime_get_noresume(dev);

[Severity: High]
Does this introduce a devres double-put of the runtime PM reference on driver
unbind?

Using devm_pm_runtime_get_noresume() acquires a runtime PM usage count and
queues a devres action (pm_runtime_put_noidle_action) to release it upon
unbind.

>  	if (err)
> -		return dev_err_probe(&pdev->dev, err, "Failed to get PM runtime\n");
> +		return dev_err_probe(dev, err, "Failed to get PM runtime\n");
[ ... ]
> @@ -531,41 +532,41 @@ static int mxc_gpio_probe(struct platform_device *pdev)
[ ... ]
>  	list_add_tail(&port->node, &mxc_gpio_ports);
>  
>  	mxc_update_irq_chained_handler(port, true);
>  
>  	platform_set_drvdata(pdev, port);
> -	pm_runtime_put_autosuspend(&pdev->dev);
> +	pm_runtime_put_autosuspend(dev);

[Severity: High]
Since gpio-mxc has no .remove callback to re-acquire the reference, won't
unbinding the driver (e.g. via unregistering the platform device) execute the
devres action queued by devm_pm_runtime_get_noresume(), causing a double-put?

Although pm_runtime_put_noidle() uses atomic_add_unless() to prevent the usage
counter from dropping mathematically below 0, this double-put could steal a
reference if another component is holding one. Can this accounting error cause
the device to prematurely runtime-suspend while still in use, leading to
unclocked hardware register access faults?

>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007-gpio-mxc-cleanup-v4-0-c9f5a8b50df9@nxp.com?part=8