Re: [PATCH v2 5/9] mailbox: imx: Start splitting the IRQ handler in primary and threaded handler

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Double `clk_disable_unprepare()` on `devm_mbox_controller_register()` error path.

Pre-existing issues:
- [High] Unchecked return value of `pm_runtime_get_sync()` can lead to synchronous external aborts.
--

commit 2c0acf5c8fb62883e0037f32b817dbaada81aae9
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

mailbox: imx: Start splitting the IRQ handler in primary and threaded handler

This commit splits the mailbox IRQ handling into a primary handler and a
threaded handler. The primary handler masks the interrupt event, allowing the
threaded handler to run in a preemptible context without raising the interrupt
again. As a first step, it prepares the logic and moves the TX handling part.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index d1de07cc0ed62..006aa76b74b62 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c

[ ... ]

> @@ -629,7 +655,8 @@ static int imx_mu_startup(struct mbox_chan *chan)
>  	if (!(priv->dcfg->type & IMX_MU_V2_IRQ))
>  		irq_flag |= IRQF_SHARED;
>  
> -	ret = request_irq(priv->irq[cp->type], imx_mu_isr, irq_flag, cp->irq_desc, chan);
> +	ret = request_threaded_irq(priv->irq[cp->type], imx_mu_isr, imx_mu_isr_th,
> +				   irq_flag, cp->irq_desc, chan);

[Severity: High]
This isn't a bug introduced by this patch, but looking at imx_mu_startup()
in drivers/mailbox/imx-mailbox.c, the return value of pm_runtime_get_sync()
is ignored:

	pm_runtime_get_sync(priv->dev);
	if (cp->type == IMX_MU_TYPE_TXDB_V2)
		return 0;

If the PM domain fails to resume (for example, returning -ENOMEM or
-ETIMEDOUT), will the function proceed to interact with hardware registers?
On i.MX platforms, accessing unclocked or unpowered hardware registers
typically results in a synchronous external abort and a kernel panic.

>  	if (ret) {
>  		dev_err(priv->dev, "Unable to acquire IRQ %d\n", priv->irq[cp->type]);
>  		return ret;

[Severity: Medium]
Looking at the driver changes introduced earlier in this patch series
(specifically "mailbox: imx: Use devm_pm_runtime_enable()"), there appears
to be a double clock disable issue on the error path:

drivers/mailbox/imx-mailbox.c:imx_mu_probe() {
...
	clk_disable_unprepare(priv->clk);

	ret = devm_mbox_controller_register(dev, &priv->mbox);
	if (ret)
		goto disable_clk;
...
disable_clk:
	clk_disable_unprepare(priv->clk);
	return ret;
}

Could this cause the clock reference count to underflow and trigger a
warning in the clock framework if devm_mbox_controller_register() fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-imx_mbox_rproc-v2-0-a0059dc3b69a@linutronix.de?part=5