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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2026-06-03 14:31:08 [+0000], sashiko-bot@kernel.org wrote:
> 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?

Guess it wouldn't hurt to forward the error to the caller.

> [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).

Interrupts are disabled. Better it does not time out…
It waits IMX_MU_SECO_TX_TOUT which is 3seconds here. I would suggest to
have a udelay(10) or so and then count the loops.

Sebastian