Re: [PATCH v2 8/9] mailbox: imx: Don't force-thread the primary handler

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Double disable of priv->clk on devm_mbox_controller_register() failure.

Pre-existing issues:
- [High] Unchecked return value of pm_runtime_get_sync() leads to kernel panic.
- [High] Silent transmission failure due to ignored timeout error in imx_mu_generic_tx().
- [High] Polling jiffies and using time APIs with local interrupts disabled leads to hard lockups.
--

commit 91fcad059338175baa26f9a301cda6d0b62ba3ee
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

mailbox: imx: Don't force-thread the primary handler

This commit avoids force-threading the primary interrupt handler in the
IMX mailbox driver. It converts the xcr_lock to a raw_spinlock_t so the
primary handler can safely run in hardirq context, and marks the interrupt
with IRQF_NO_THREAD.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index f0a14449652f1..f3de47908e00f 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c

[ ... ]

> @@ -649,7 +644,7 @@ static int imx_mu_startup(struct mbox_chan *chan)
>  {
>  	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
>  	struct imx_mu_con_priv *cp = chan->con_priv;
> -	unsigned long irq_flag = 0;
> +	unsigned long irq_flag = IRQF_NO_THREAD;
>  	int ret;
>  
>  	pm_runtime_get_sync(priv->dev);

[Severity: High]
This is a pre-existing issue, but does this missing check for the return
value of pm_runtime_get_sync() risk a kernel panic?

If the device fails to resume (e.g., due to a clock enablement failure),
it remains in a suspended/unpowered state.

The function immediately proceeds to configure the device and access its
I/O registers. Accessing unpowered hardware registers can cause system
crashes, such as synchronous external aborts or bus hangs, on ARM platforms.

[ ... ]

While reviewing the driver, I noticed a few other potential regressions outside
the diff hunks above:

[Severity: High]
This is a pre-existing issue, but can the timeout loop in imx_mu_generic_tx()
lead to silent transmission failures?

When processing IMX_MU_TYPE_TXDB_V2, the code loops up to 10 times waiting
for the readl_poll_timeout() condition:

drivers/mailbox/imx-mailbox.c:imx_mu_generic_tx() {
    ...
    while (ret && (count < 10)) {
        ret = readl_poll_timeout(priv->base + priv->dcfg->xCR[IMX_MU_GCR], val,
                                 !(val & IMX_MU_xCR_GIRn(priv->dcfg->type, cp->idx)),
                                 0, 10000);
        if (ret) {
            dev_warn_ratelimited(...);
        }
    }
    break;
    ...
}

If all 10 attempts time out, ret is left as -ETIMEDOUT and the while loop
exits. The code then breaks from the switch block and unconditionally hits
return 0 at the end of the function instead of returning ret.

Falsely returning success causes the mailbox framework to drop the message,
leading to silent data loss. Should this return the error code instead?

[Severity: Medium]
Is there a double disable of priv->clk if devm_mbox_controller_register()
fails?

In imx_mu_probe(), the clock is manually disabled via clk_disable_unprepare()
after successfully syncing the PM runtime state:

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;
}

If devm_mbox_controller_register() fails, the code branches to disable_clk,
which executes clk_disable_unprepare(priv->clk) a second time. This double
disable unbalances the clock reference count and triggers a kernel warning.

[Severity: High]
This is a pre-existing issue, but does the use of time APIs and polling
with local interrupts disabled lead to hard lockups?

Mailbox transmission (priv->dcfg->tx) is invoked via mbox_send_message()
with the channel lock (spin_lock_irqsave) held, meaning local interrupts
are disabled.

Inside imx_mu_tx_waiting_write(), the code polls a hardware register while
checking time_is_after_jiffies64():

drivers/mailbox/imx-mailbox.c:imx_mu_tx_waiting_write() {
    ...
    do {
        status = imx_mu_read(priv, priv->dcfg->xSR[IMX_MU_TSR]);
        can_write = status & IMX_MU_xSR_TEn(priv->dcfg->type, idx % 4);
    } while (!can_write && time_is_after_jiffies64(timeout_time));
    ...
}

Because interrupts are disabled, jiffies will not advance on UP systems,
causing an infinite loop (deadlock).

Additionally, imx_mu_specific_tx() uses readl_poll_timeout() with a
5-second timeout in this atomic context:

drivers/mailbox/imx-mailbox.c:imx_mu_specific_tx() {
    ...
    ret = readl_poll_timeout(priv->base + priv->dcfg->xSR[IMX_MU_TSR],
                             xsr,
                             xsr & IMX_MU_xSR_TEn(priv->dcfg->type, i % num_tr),
                             0, 5 * USEC_PER_SEC);
    ...
}

Spinning for up to 5 seconds with IRQs disabled violates core kernel
locking rules and can trigger NMI watchdogs and RCU stalls.

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