Re: [PATCH v2 1/9] mailbox: imx: Add a channel shutdown field

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Timeout in imx_mu_seco_rxdb() skips clearing the hardware interrupt status, causing an infinite IRQ storm.
- [High] The patch addresses a race where an RX interrupt can be re-enabled during channel shutdown, but fails to fix the exact same race for TX channels. This can result in an unhandled interrupt and the kernel disabling the shared IRQ line.
- [High] Error paths in imx_mu_specific_rx() permanently wedge the RX channel by leaving the receive interrupt disabled.
- [High] The driver performs a multi-second busy-wait polling loop inside RT-priority threaded handlers, triggering softlockups.
--

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

mailbox: imx: Add a channel shutdown field

Introduce imx_mu_con_priv::shutdown to signal that the channel is shutting
down. This prevents a race condition where an RX event could be disabled in
shutdown() and then incorrectly re-enabled by the ISR.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 246a9a9e39520..34edc2f31dcb5 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
[ ... ]
> @@ -376,7 +407,7 @@ static int imx_mu_specific_rx(struct imx_mu_priv *priv, struct imx_mu_con_priv *
>  		*data++ = imx_mu_read(priv, priv->dcfg->xRR + (i % num_rr) * 4);
>  	}
>  
> -	imx_mu_xcr_rmw(priv, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, 0), 0);
> +	imx_mu_xcr_set_act(priv, cp, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, 0));
>  	mbox_chan_received_data(cp->chan, (void *)priv->msg);
>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but can error paths in
imx_mu_specific_rx() permanently wedge the RX channel by leaving the receive
interrupt disabled?

In the updated threaded interrupt architecture, the receive interrupt
(RIEn) is explicitly disabled in the primary handler. The threaded handler
(imx_mu_isr_th) invokes the channel callback and conditionally unmasks the
interrupt only if the callback returns 0:

    if (!priv->dcfg->rx(priv, cp))
        imx_mu_xcr_set_act(priv, cp, IMX_MU_RCR,
                           IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));

If imx_mu_specific_rx() encounters an error (such as exceeding the maximum
message size):

    if (size > max_size / 4) {
        dev_err(priv->dev, ...);
        return -EINVAL;
    }

It returns a non-zero error code. The threaded handler will evaluate the
condition as false and skip imx_mu_xcr_set_act(), leaving the receive
interrupt permanently disabled. The hardware will never generate another
receive interrupt for this channel, permanently deadlocking communication.


[Severity: High]
This isn't a bug introduced by this patch, but is it safe to perform a
multi-second busy-wait polling loop inside RT-priority threaded handlers, or
could this trigger softlockups?

The driver uses multi-second busy-wait polling in functions called from the
threaded interrupt handler. imx_mu_specific_rx() uses readl_poll_timeout()
with a 5-second timeout:

    ret = readl_poll_timeout(..., 0, 5 * USEC_PER_SEC);

Similarly, imx_mu_rx_waiting_read() loops for 3 seconds without sleeping:

    } while (!can_read && time_is_after_jiffies64(timeout_time));

Because threaded IRQ handlers run as Real-Time tasks (SCHED_FIFO,
priority 50), spinning for up to 5 seconds will entirely stall the CPU
core, starve normal priority tasks, delay RCU grace periods, and
trigger softlockup and RCU stall panics.

[ ... ]
> @@ -638,13 +670,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 isn't a bug introduced by this patch, but would the same race condition
exist for TX channels, potentially resulting in unhandled interrupts?

When shutting down a TX channel, imx_mu_shutdown() calls
imx_mu_xcr_clr_shut() to clear the TX interrupt enable bit (TIEn) and set
cp->shutdown = true. It then calls free_irq(), which waits for any running
threaded ISR to finish.

If the threaded ISR (imx_mu_isr_th) is running concurrently, it acknowledges
the interrupt and calls mbox_chan_txdone(). This can push the next queued
message via send_data(), invoking imx_mu_generic_tx() or
imx_mu_specific_tx().

Both of these TX functions unconditionally re-enable the TX interrupt using
imx_mu_xcr_rmw(), completely ignoring the cp->shutdown flag:

    imx_mu_xcr_rmw(priv, IMX_MU_TCR,
                   IMX_MU_xCR_TIEn(priv->dcfg->type, cp->idx), 0);

After the handler finishes, free_irq() unregisters the interrupt handler.
The hardware will eventually assert the TX interrupt because TIEn was
re-enabled. Since the IRQ is shared, other handlers will return IRQ_NONE,
leading to an interrupt storm and the kernel disabling the shared IRQ line.
Should the fix have used imx_mu_xcr_set_act() in the TX paths, just as it did
for RX?

>  	case IMX_MU_TYPE_RX:
> -		imx_mu_xcr_rmw(priv, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
> +		imx_mu_xcr_clr_shut(priv, cp, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
>  		break;
>  	case IMX_MU_TYPE_RXDB:
> -		imx_mu_xcr_rmw(priv, IMX_MU_GIER, 0, IMX_MU_xCR_GIEn(priv->dcfg->type, cp->idx));
> +		imx_mu_xcr_clr_shut(priv, cp, IMX_MU_GIER, IMX_MU_xCR_GIEn(priv->dcfg->type, cp->idx));

[Severity: Critical]
This isn't a bug introduced by this patch, but does a timeout in
imx_mu_seco_rxdb() skip clearing the hardware interrupt status, causing an
infinite IRQ storm?

In imx_mu_seco_rxdb(), if imx_mu_rx_waiting_read() times out, the function
jumps to the error label:

    if (err) {
        dev_err(priv->dev, "Timeout rx %d\n", i);
        goto error;
    }

This bypasses the hardware write that clears the General Purpose Interrupt
Pending (GIPn) bit. The threaded handler (imx_mu_isr_th) ignores the error
return value from rxdb() and unconditionally unmasks the interrupt:

    priv->dcfg->rxdb(priv, cp);
    imx_mu_xcr_set_act(priv, cp, IMX_MU_GIER,
                       IMX_MU_xCR_GIEn(priv->dcfg->type, cp->idx));

Because the hardware interrupt is never acknowledged or disabled, the
interrupt controller will immediately re-trigger the ISR. This cycle repeats
infinitely: trigger, thread busy-waits 3 seconds, timeout, unmask, trigger
again, completely locking up the CPU.

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