Re: [PATCH AUTOSEL 6.18-6.12] mailbox: imx: Add a channel shutdown field

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch fixes the teardown race for RX channels but misses the identical race condition for TX channels, allowing spurious interrupts.
--

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

mailbox: imx: Add a channel shutdown field

This commit introduces a shutdown flag to imx_mu_con_priv to coordinate
channel teardown and prevent the ISR from re-enabling interrupts after
imx_mu_shutdown() has disabled them. The flag is checked by a new function
imx_mu_xcr_set_act() before modifying the control register.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 6778afc64a048..44c2cc06b794e 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
[ ... ]
> @@ -220,6 +221,36 @@ static u32 imx_mu_xcr_rmw(struct imx_mu_priv *priv, enum imx_mu_xcr type, u32 se
>  	return val;
>  }
>  
> +static void imx_mu_xcr_clr_shut(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp,
> +				enum imx_mu_xcr type, u32 clr)
> +{
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&priv->xcr_lock, flags);
> +	cp->shutdown = true;
> +
> +	val = imx_mu_read(priv, priv->dcfg->xCR[type]);
> +	val &= ~clr;
> +	imx_mu_write(priv, val, priv->dcfg->xCR[type]);
> +	spin_unlock_irqrestore(&priv->xcr_lock, flags);
> +}
> +
> +static void imx_mu_xcr_set_act(struct imx_mu_priv *priv, struct imx_mu_con_priv *cp,
> +			       enum imx_mu_xcr type, u32 set)
> +{
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&priv->xcr_lock, flags);
> +	if (!cp->shutdown) {
> +		val = imx_mu_read(priv, priv->dcfg->xCR[type]);
> +		val |= set;
> +		imx_mu_write(priv, val, priv->dcfg->xCR[type]);
> +	}
> +	spin_unlock_irqrestore(&priv->xcr_lock, flags);
> +}
> +
[ ... ]
> @@ -377,7 +408,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 is a pre-existing issue, but while this patch successfully fixes the
teardown race for RX channels, does the identical race still exist for TX
channels?

If mbox_free_channel() is called while a TX interrupt is active and there are
pending TX messages, it calls imx_mu_shutdown() which sets cp->shutdown = true
and clears the TX interrupt enable bit, then blocks in free_irq().

When the concurrent ISR completes the current TX, it calls mbox_chan_txdone(),
triggering msg_submit() to send the next queued message via
imx_mu_generic_tx() or imx_mu_specific_tx().

Both functions unconditionally rewrite the TX interrupt enable bit using
imx_mu_xcr_rmw(), bypassing the new shutdown flag:

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

And similarly in imx_mu_specific_tx():

drivers/mailbox/imx-mailbox.c:imx_mu_specific_tx() {
    ...
		for (; i < size; i++) {
			ret = readl_poll_timeout(...);
			if (ret) {
				...
			}
			imx_mu_write(priv, *arg++, priv->dcfg->xTR + (i % num_tr) * 4);
		}

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

Should these TX paths also be updated to use imx_mu_xcr_set_act() to prevent
a spurious interrupt from disabling the shared IRQ line during teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831133314.4125787-195-sashal@kernel.org?part=1