Re: [PATCH 1/5] mailbox: imx: Start splitting the IRQ handler in primary and threaded handler

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The commit message claims the primary handler unmasks the interrupt, but the code explicitly masks it.

Pre-existing issues:
- [High] Race condition in teardown leaves the hardware interrupt enabled after the IRQ handler is unregistered, leading to an interrupt storm.
--

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

mailbox: imx: Start splitting the IRQ handler in primary and threaded handler

This commit splits the i.MX mailbox interrupt handling into a primary
and a threaded handler. The primary handler acknowledges the interrupt
and wakes the threaded handler to invoke the actual callback in
preemptible context, starting with the TX path.

> Split the mailbox irq handling into a primary handler (imx_mu_isr()) and
> a threaded handler (imx_mu_isr_th()). The primary handler unmasks the
> interrupt so the threaded handler can run without raising the interrupt
> again. The threaded handler can invoke the actuall callback in
> preemtible context.

[Severity: Medium]
Is this commit message description accurate regarding unmasking?

Looking at imx_mu_isr(), it calls imx_mu_xcr_rmw() passing the interrupt
enable bit as the clr parameter:

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

Since this clears the enable bit, doesn't it actually mask the interrupt
rather than unmask it? The code implementation seems structurally correct
to prevent continuous firing until the threaded handler can process the
completion, but the commit message appears reversed.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 246a9a9e39520..2809965677bd7 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
[ ... ]
> @@ -508,11 +509,34 @@ static void imx_mu_txdb_work(struct work_struct *t)
>  	mbox_chan_txdone(cp->chan, 0);
>  }
>  
> +static irqreturn_t imx_mu_isr_th(int irq, void *p)
> +{
> +	struct mbox_chan *chan = p;
> +	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
> +	struct imx_mu_con_priv *cp = chan->con_priv;
> +
> +	if (!cp->pending)
> +		return IRQ_NONE;
> +
> +	switch (cp->type) {
> +	case IMX_MU_TYPE_TX:
> +		cp->pending = 0;
> +		mbox_chan_txdone(chan, 0);

[Severity: High]
This is a pre-existing issue, but can this lead to an interrupt storm
during channel teardown?

When a client frees a mailbox channel, mbox_free_channel() first calls
the driver's shutdown() callback before calling mbox_clean_and_put_channel()
which aborts queued messages.

If imx_mu_shutdown() calls free_irq(), it synchronizes with the pending
threaded handler. The threaded handler then runs to completion while
shutdown() is blocked, calling mbox_chan_txdone():

imx_mu_isr_th()
  mbox_chan_txdone()
    tx_tick()
      msg_submit()
        imx_mu_send_data()

Because the queues haven't been aborted yet, the core will immediately
submit the next queued message, and imx_mu_send_data() will re-enable
the hardware interrupt.

After the threaded handler finishes, free_irq() completes and unregisters
the IRQ action. Since the hardware interrupt was just re-enabled, won't
the hardware assert the interrupt again, resulting in an unhandled
interrupt storm that permanently disables the shared IRQ line?

> +		return IRQ_HANDLED;
> +
> +	default:
> +		dev_warn_ratelimited(priv->dev, "Unhandled channel type %d\n",
> +				     cp->type);
> +		return IRQ_NONE;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529-imx_mbox_rproc-v1-0-b8ffc36e11e5@linutronix.de?part=1