Re: [PATCH v3 03/10] mailbox: imx: Use devm_pm_runtime_enable()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing error check on `pm_runtime_get_sync()` in `imx_mu_startup()` allows unclocked hardware access.
- [High] Checking `ret < 0` for `pm_runtime_put_sync()` causes spurious probe failures and resource leaks on concurrent PM access.
- [High] Missing `pm_runtime_put_sync()` when `request_irq()` fails in `imx_mu_startup()`.
--

commit 023389d6153d2d912a21b34dd94fa1a45a63f27a
Author: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>

mailbox: imx: Use devm_pm_runtime_enable()

This patch updates the imx mailbox driver to use devm_pm_runtime_enable()
and reorders the probe function to delay mailbox controller registration
until after PM runtime initialization. This ensures proper cleanup order
during device removal or probe failure, resolving issues with early device
usage.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index e261b382d5c90..516a05b64daa1 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
[ ... ]
> @@ -966,38 +966,36 @@ static int imx_mu_probe(struct platform_device *pdev)
[ ... ]
>  	ret = pm_runtime_resume_and_get(dev);
>  	if (ret < 0)
> -		goto disable_runtime_pm;
> +		goto disable_clk;
>  
>  	ret = pm_runtime_put_sync(dev);
>  	if (ret < 0)
> -		goto disable_runtime_pm;
> +		goto disable_clk;

[Severity: High]
This isn't a bug introduced by this patch, but should the return value
of pm_runtime_put_sync() be treated as a fatal error here?

pm_runtime_put_sync() can return -EAGAIN or -EBUSY if the device cannot be
suspended immediately, such as when another thread concurrently accesses the
device's PM runtime via sysfs.

If this happens, the valid non-error return value causes the driver to
spuriously abort the probe process. It also leaves the device in RPM_ACTIVE,
which leaks the clock enabled by the earlier pm_runtime_resume_and_get() call.

[ ... ]

While reviewing the PM runtime usage, I also noticed two other pre-existing
issues in imx_mu_startup().

[Severity: High]
This is a pre-existing issue, but does this code leave the clock disabled
if the PM runtime resume fails?

drivers/mailbox/imx-mailbox.c:imx_mu_startup() {
	...
	pm_runtime_get_sync(priv->dev);

	spin_lock_irqsave(&priv->xcr_lock, flags);
	imx_mu_xcr_rmw(priv, 0, IMX_MU_xCR_GIEn(cp->type, cp->idx));
	...
}

If pm_runtime_get_sync() fails (for example, if the clock fails to enable),
the return value is ignored and the device remains suspended. The function
then immediately proceeds to access hardware registers via imx_mu_xcr_rmw().
Can accessing unclocked peripheral registers cause a synchronous external
abort and lead to a system crash?

[Severity: High]
This is a pre-existing issue, but does this error path leak the PM runtime
usage counter?

drivers/mailbox/imx-mailbox.c:imx_mu_startup() {
	...
	pm_runtime_get_sync(priv->dev);
	...
	ret = request_irq(priv->irq[cp->type], imx_mu_isr, irq_flag,
			  cp->irq_desc, chan);
	if (ret) {
		dev_err(priv->dev, "Unable to acquire IRQ %d\n",
			priv->irq[cp->type]);
		return ret;
	}
	...
}

If request_irq() fails, the function returns the error directly without
calling pm_runtime_put_sync(). This would permanently leak the PM runtime
usage counter, preventing the device from ever suspending and keeping the
hardware clock continuously enabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617-imx_mbox_rproc-v3-0-77948112defc@xxxxxxxxxxxxx?part=3