Re: [PATCH v3 02/10] mailbox: imx: Add a channel shutdown field

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch introduces `cp->shutdown` to fix a teardown race in the RX path but fails to apply the same fix to the TX path. If a channel is freed while TX messages are queued, the ISR can unconditionally unmask the TX interrupt after `shutdown()` has masked it, leading to a 'nobody cared' interrupt storm.
- [High] Polling jiffies with local IRQs disabled leads to potential deadlock in `imx_mu_tx_waiting_write()`.
- [Medium] Missing `pm_runtime_put_sync()` on the error path of `imx_mu_startup()` causes a permanent power management reference leak.
--

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

mailbox: imx: Add a channel shutdown field

sashiko complained about possible teardown problem. The scenario

 CPU 0                              CPU 1
  imx_mu_isr()                   imx_mu_shutdown()
                                   imx_mu_xcr_rmw(priv, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
    imx_mu_specific_rx()
      imx_mu_xcr_rmw(priv, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, 0), 0);
                                   free_irq()

The RX event remains enabled because in this short window the RX event
was disabled in ->shutdown() while the interrupt was active and then
enabled again by the ISR while ->shutdown waited in free_irq().

This race requires timing and if happens can be problematic on shared
handlers if the "removed" channel triggers an interrupt. In this case
the irq-core will shutdown the interrupt with the "nobody cared"
message.

Introduce imx_mu_con_priv::shutdown to signal that the channel is
shutting down. This flag is set with the lock held (by
imx_mu_xcr_clr_shut()). The unmask side uses imx_mu_xcr_set_act() which
only enables the event if the channel has not been shutdown and
serialises on the same lock.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 0028073be4a71..e261b382d5c90 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c

[ ... ]

> @@ -605,6 +636,7 @@ static int imx_mu_startup(struct mbox_chan *chan)
>  		return ret;
>  	}
>  
> +	cp->shutdown = false;

[Severity: Medium]
This is a pre-existing issue and was not introduced by the patch under
review, but does this error path leak the PM runtime reference?

In imx_mu_startup(), pm_runtime_get_sync() is called to increment the PM
usage counter. If request_threaded_irq() fails, the function returns
directly without calling pm_runtime_put_sync():

drivers/mailbox/imx-mailbox.c:imx_mu_startup() {
    ...
    pm_runtime_get_sync(priv->dev);
    ...
    ret = request_threaded_irq(...);
    if (ret) {
        dev_err(...);
        return ret;
    }
    ...
}

The mailbox framework handles this failure via mbox_clean_and_put_channel(),
but does not invoke the shutdown() callback when startup() fails.
Consequently, can the device's PM runtime reference be permanently leaked,
preventing the hardware from entering a suspended state?

[ ... ]

> @@ -639,13 +671,13 @@ static void imx_mu_shutdown(struct mbox_chan *chan)
>  
>  	switch (cp->type) {
>  	case IMX_MU_TYPE_TX:
> -		imx_mu_xcr_rmw(priv, IMX_MU_TCR, 0, IMX_MU_xCR_TIEn(priv->dcfg->type, cp->idx));
> +		imx_mu_xcr_clr_shut(priv, cp, IMX_MU_TCR, IMX_MU_xCR_TIEn(priv->dcfg->type, cp->idx));
>  		break;

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but does this patch leave the TX path vulnerable to a similar
teardown race?

The patch introduces cp->shutdown to fix the teardown race in the RX path,
but if a TX channel is freed while messages are queued, can the ISR
unconditionally unmask the TX interrupt after shutdown() has masked it?

When mbox_free_channel() calls imx_mu_shutdown(), it masks the TX interrupt
via imx_mu_xcr_clr_shut(), sets cp->shutdown = true, and waits in free_irq().

If a TX interrupt concurrently fires, imx_mu_isr() masks it and wakes the
threaded handler imx_mu_isr_th(). This handler calls mbox_chan_txdone() and
eventually triggers msg_submit().

Because the queue isn't purged yet, it dequeues the next message and
synchronously calls the driver's TX callback. However, imx_mu_generic_tx()
and imx_mu_specific_tx() unconditionally re-enable the TX interrupt:

drivers/mailbox/imx-mailbox.c:imx_mu_generic_tx() {
    ...
    case IMX_MU_TYPE_TX:
        imx_mu_write(priv, *arg, priv->dcfg->xTR + cp->idx * 4);
        imx_mu_xcr_rmw(priv, IMX_MU_TCR, IMX_MU_xCR_TIEn(priv->dcfg->type, cp->idx), 0);
        break;
    ...
}

Once the threaded ISR finishes, free_irq() unblocks, leaving the hardware TX
interrupt enabled but without a registered handler. Will this result in a
"nobody cared" kernel warning?

[ ... ]

[Severity: High]
This is a pre-existing issue and was not introduced by the patch under
review, but does polling jiffies with local IRQs disabled lead to a potential
deadlock here?

The imx_mu_tx_waiting_write() function uses an open-coded timeout loop based
on jiffies:

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

This function is called from the send_data callback (imx_mu_seco_tx), which
the mailbox core invokes with the channel's spinlock (chan->lock) held. This
spinlock unconditionally disables local interrupts.

Because local interrupts are disabled, the timer tick cannot fire on the
local CPU to update jiffies. On a single-core system, or if the local CPU is
responsible for updating the tick, jiffies will never increment.

If the hardware does not promptly set the status bit, will the loop spin
infinitely and cause a hard lockup?

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