[PATCH 0/5] mailbox: imx: Use threaded handler to avoid kworker in imx's remoteproc

[PATCH 0/5] mailbox: imx: Use threaded handler to avoid kworker in imx's remoteproc

[Sebastian Andrzej Siewior]

The imx's remoteproc driver uses a kworker from its mailbox callback to
complete the request. The reason is that the imx mailbox driver invokes
the callback from its interrupt handler and the remoteproc callback (at
least the rpmsg-tty) requires a preemptible context.

This works but is problematic in a PREEMPT_RT environment where the
latency of the invocation is important. By scheduling a kworker the
high task priority from the threaded handler is lost and the kworker
competes for CPU ressources with every SCHED_OTHER task in the system.
This can lead to long delays on a busy system with other RT threads
which are less important than the completion of this request.

Looking over other mailbox driver, like the arm_mhu for instance, they
use a threaded interrupt handler to invoke the callback. This avoids the
kworker detour.

The here suggested change utilises a threaded interrupt to invoke the
callback. The primary handler mask the interrupt source so that the
handler can run without getting interrupted by the interrupt again.
Doing so avoids marking the interrupt IRQF_ONESHOT so that that in a
shared-interrupt environment the other interrupt can still fire while
the first one is masked.

This change was tested on a im93 board with rpmsg-tty driver.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Sebastian Andrzej Siewior (5):
      mailbox: imx: Start splitting the IRQ handler in primary and threaded handler
      mailbox: imx: Move the RX part of the mailbox into the threaded handler
      mailbox: imx: Move the RXDB part of the mailbox into the threaded handler
      mailbox: imx: Don't force-thread the primary handler
      remoteproc: imx_rproc: Invoke the callback directly

 drivers/mailbox/imx-mailbox.c  | 65 +++++++++++++++++++++++++++++++++---------
 drivers/remoteproc/imx_rproc.c | 33 +--------------------
 2 files changed, 53 insertions(+), 45 deletions(-)
---
base-commit: 9e171fc1d7d7ab847a750c03571c87ac3c17bd84
change-id: 20260529-imx_mbox_rproc-7d512f5a6f78

Best regards,
-- 
Sebastian Andrzej Siewior <bigeasy@linutronix.de>

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

[Sebastian Andrzej Siewior]

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.

As a first step, prepare the logic and move TX handling part.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mailbox/imx-mailbox.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

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
@@ -80,6 +80,7 @@ struct imx_mu_con_priv {
 	enum imx_mu_chan_type	type;
 	struct mbox_chan	*chan;
 	struct work_struct 	txdb_work;
+	unsigned int		pending;
 };
 
 struct imx_mu_priv {
@@ -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);
+		return IRQ_HANDLED;
+
+	default:
+		dev_warn_ratelimited(priv->dev, "Unhandled channel type %d\n",
+				     cp->type);
+		return IRQ_NONE;
+	}
+}
+
 static irqreturn_t imx_mu_isr(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;
+	irqreturn_t ret = IRQ_HANDLED;
 	u32 val, ctrl;
 
 	switch (cp->type) {
@@ -548,7 +572,8 @@ static irqreturn_t imx_mu_isr(int irq, void *p)
 	if ((val == IMX_MU_xSR_TEn(priv->dcfg->type, cp->idx)) &&
 	    (cp->type == IMX_MU_TYPE_TX)) {
 		imx_mu_xcr_rmw(priv, IMX_MU_TCR, 0, IMX_MU_xCR_TIEn(priv->dcfg->type, cp->idx));
-		mbox_chan_txdone(chan, 0);
+		cp->pending = 1;
+		ret = IRQ_WAKE_THREAD;
 	} else if ((val == IMX_MU_xSR_RFn(priv->dcfg->type, cp->idx)) &&
 		   (cp->type == IMX_MU_TYPE_RX)) {
 		priv->dcfg->rx(priv, cp);
@@ -563,7 +588,7 @@ static irqreturn_t imx_mu_isr(int irq, void *p)
 	if (priv->suspend)
 		pm_system_wakeup();
 
-	return IRQ_HANDLED;
+	return ret;
 }
 
 static int imx_mu_send_data(struct mbox_chan *chan, void *data)
@@ -598,7 +623,8 @@ static int imx_mu_startup(struct mbox_chan *chan)
 	if (!(priv->dcfg->type & IMX_MU_V2_IRQ))
 		irq_flag |= IRQF_SHARED;
 
-	ret = request_irq(priv->irq[cp->type], imx_mu_isr, irq_flag, cp->irq_desc, chan);
+	ret = request_threaded_irq(priv->irq[cp->type], imx_mu_isr, imx_mu_isr_th,
+				   irq_flag, cp->irq_desc, chan);
 	if (ret) {
 		dev_err(priv->dev, "Unable to acquire IRQ %d\n", priv->irq[cp->type]);
 		return ret;

-- 
2.53.0

[PATCH 2/5] mailbox: imx: Move the RX part of the mailbox into the threaded handler

[Sebastian Andrzej Siewior]

Move RX callback handling into the threaded handler. This is similar to
the TX side except that we explicitly mask the source interrupt in the
primary handler and unmask it in the threaded handler again after
success. This was done automatically in the TX part.

The masking/ unmasking can be removed from imx_mu_specific_rx() since it
already happens in the primary/ threaded handler before invoking the
channel specific callback.

Move RX channel handling into threaded handler.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mailbox/imx-mailbox.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 2809965677bd7..30a52c609e56b 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -350,7 +350,6 @@ static int imx_mu_specific_rx(struct imx_mu_priv *priv, struct imx_mu_con_priv *
 
 	data = (u32 *)priv->msg;
 
-	imx_mu_xcr_rmw(priv, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(priv->dcfg->type, 0));
 	*data++ = imx_mu_read(priv, priv->dcfg->xRR);
 
 	if (priv->dcfg->type & IMX_MU_V2_S4) {
@@ -377,7 +376,6 @@ 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);
 	mbox_chan_received_data(cp->chan, (void *)priv->msg);
 
 	return 0;
@@ -524,6 +522,12 @@ static irqreturn_t imx_mu_isr_th(int irq, void *p)
 		mbox_chan_txdone(chan, 0);
 		return IRQ_HANDLED;
 
+	case IMX_MU_TYPE_RX:
+		cp->pending = 0;
+		if (!priv->dcfg->rx(priv, cp))
+			imx_mu_xcr_rmw(priv, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx), 0);
+		return IRQ_HANDLED;
+
 	default:
 		dev_warn_ratelimited(priv->dev, "Unhandled channel type %d\n",
 				     cp->type);
@@ -576,7 +580,9 @@ static irqreturn_t imx_mu_isr(int irq, void *p)
 		ret = IRQ_WAKE_THREAD;
 	} else if ((val == IMX_MU_xSR_RFn(priv->dcfg->type, cp->idx)) &&
 		   (cp->type == IMX_MU_TYPE_RX)) {
-		priv->dcfg->rx(priv, cp);
+		cp->pending = 1;
+		imx_mu_xcr_rmw(priv, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
+		ret = IRQ_WAKE_THREAD;
 	} else if ((val == IMX_MU_xSR_GIPn(priv->dcfg->type, cp->idx)) &&
 		   (cp->type == IMX_MU_TYPE_RXDB)) {
 		priv->dcfg->rxdb(priv, cp);

-- 
2.53.0

[PATCH 3/5] mailbox: imx: Move the RXDB part of the mailbox into the threaded handler

[Sebastian Andrzej Siewior]

Move RXDB callback handling into the threaded handler. This similar to
the RX side except that we unmask it unconditionally in threaded
handler.

Move RXDB callback handling into the threaded handler.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mailbox/imx-mailbox.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 30a52c609e56b..9bf6484af45ed 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -528,6 +528,12 @@ static irqreturn_t imx_mu_isr_th(int irq, void *p)
 			imx_mu_xcr_rmw(priv, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx), 0);
 		return IRQ_HANDLED;
 
+	case IMX_MU_TYPE_RXDB:
+		cp->pending = 0;
+		priv->dcfg->rxdb(priv, cp);
+		imx_mu_xcr_rmw(priv, IMX_MU_GIER, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx), 0);
+		return IRQ_HANDLED;
+
 	default:
 		dev_warn_ratelimited(priv->dev, "Unhandled channel type %d\n",
 				     cp->type);
@@ -585,7 +591,9 @@ static irqreturn_t imx_mu_isr(int irq, void *p)
 		ret = IRQ_WAKE_THREAD;
 	} else if ((val == IMX_MU_xSR_GIPn(priv->dcfg->type, cp->idx)) &&
 		   (cp->type == IMX_MU_TYPE_RXDB)) {
-		priv->dcfg->rxdb(priv, cp);
+		cp->pending = 1;
+		imx_mu_xcr_rmw(priv, IMX_MU_GIER, 0, IMX_MU_xCR_GIEn(priv->dcfg->type, cp->idx));
+		ret = IRQ_WAKE_THREAD;
 	} else {
 		dev_warn_ratelimited(priv->dev, "Not handled interrupt\n");
 		return IRQ_NONE;

-- 
2.53.0

[PATCH 4/5] mailbox: imx: Don't force-thread the primary handler

[Sebastian Andrzej Siewior]

The primary interrupt handler (imx_mu_isr()) no longer invokes any
callbacks it only masks the interrupt source and returns. In a
forced-threaded environment the IRQ-core will force-thread the primary
handler which can be avoided.

The primary handler uses a spinlock_t to protect the RMW operation in
imx_mu_xcr_rmw() - nothing that may introduce long latencies.

The lock can be turned into a raw_spinlock_t and then the primary
handler can run in hardirq context even on PREEMPT_RT skipping one
thread.

Make struct imx_mu_priv::xcr_lock a raw_spinlock_t and skip
force-threading the primrary handler by marking it IRQF_NO_THREAD.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/mailbox/imx-mailbox.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 9bf6484af45ed..0a710316d791b 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -87,7 +87,7 @@ struct imx_mu_priv {
 	struct device		*dev;
 	void __iomem		*base;
 	void			*msg;
-	spinlock_t		xcr_lock; /* control register lock */
+	raw_spinlock_t		xcr_lock; /* control register lock */
 
 	struct mbox_controller	mbox;
 	struct mbox_chan	mbox_chans[IMX_MU_CHANS];
@@ -207,15 +207,14 @@ static int imx_mu_rx_waiting_read(struct imx_mu_priv *priv, u32 *val, u32 idx)
 
 static u32 imx_mu_xcr_rmw(struct imx_mu_priv *priv, enum imx_mu_xcr type, u32 set, u32 clr)
 {
-	unsigned long flags;
 	u32 val;
 
-	spin_lock_irqsave(&priv->xcr_lock, flags);
+	guard(raw_spinlock_irqsave)(&priv->xcr_lock);
+
 	val = imx_mu_read(priv, priv->dcfg->xCR[type]);
 	val &= ~clr;
 	val |= set;
 	imx_mu_write(priv, val, priv->dcfg->xCR[type]);
-	spin_unlock_irqrestore(&priv->xcr_lock, flags);
 
 	return val;
 }
@@ -617,7 +616,7 @@ static int imx_mu_startup(struct mbox_chan *chan)
 {
 	struct imx_mu_priv *priv = to_imx_mu_priv(chan->mbox);
 	struct imx_mu_con_priv *cp = chan->con_priv;
-	unsigned long irq_flag = 0;
+	unsigned long irq_flag = IRQF_NO_THREAD;
 	int ret;
 
 	pm_runtime_get_sync(priv->dev);
@@ -964,7 +963,7 @@ static int imx_mu_probe(struct platform_device *pdev)
 		goto disable_clk;
 	}
 
-	spin_lock_init(&priv->xcr_lock);
+	raw_spin_lock_init(&priv->xcr_lock);
 
 	priv->mbox.dev = dev;
 	priv->mbox.ops = &imx_mu_ops;

-- 
2.53.0

[PATCH 5/5] remoteproc: imx_rproc: Invoke the callback directly

[Sebastian Andrzej Siewior]

The imx-mailbox driver moved the callback invocation into the threaded
IRQ handler. This means the callback is invoked in preemptible context
and there is no need to schedule the kworker for the
imx_rproc_notified_idr_cb() invocation.

This was tested with the rpmsg-tty driver on imx93.

Remove the workqueue handling and invoke the imx_rproc_notified_idr_cb()
callback directly.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/remoteproc/imx_rproc.c | 33 +--------------------------------
 1 file changed, 1 insertion(+), 32 deletions(-)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 0dd80e688b0ea..c97bc1c401655 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -24,7 +24,6 @@
 #include <linux/regmap.h>
 #include <linux/remoteproc.h>
 #include <linux/scmi_imx_protocol.h>
-#include <linux/workqueue.h>
 
 #include "imx_rproc.h"
 #include "remoteproc_internal.h"
@@ -115,8 +114,6 @@ struct imx_rproc {
 	struct mbox_client		cl;
 	struct mbox_chan		*tx_ch;
 	struct mbox_chan		*rx_ch;
-	struct work_struct		rproc_work;
-	struct workqueue_struct		*workqueue;
 	void __iomem			*rsc_table;
 	struct imx_sc_ipc		*ipc_handle;
 	struct notifier_block		rproc_nb;
@@ -835,21 +832,11 @@ static int imx_rproc_notified_idr_cb(int id, void *ptr, void *data)
 	return 0;
 }
 
-static void imx_rproc_vq_work(struct work_struct *work)
-{
-	struct imx_rproc *priv = container_of(work, struct imx_rproc,
-					      rproc_work);
-	struct rproc *rproc = priv->rproc;
-
-	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);
-}
-
 static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
 {
 	struct rproc *rproc = dev_get_drvdata(cl->dev);
-	struct imx_rproc *priv = rproc->priv;
 
-	queue_work(priv->workqueue, &priv->rproc_work);
+	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);
 }
 
 static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block)
@@ -1214,13 +1201,6 @@ static int imx_rproc_sys_off_handler(struct sys_off_data *data)
 	return NOTIFY_DONE;
 }
 
-static void imx_rproc_destroy_workqueue(void *data)
-{
-	struct workqueue_struct *workqueue = data;
-
-	destroy_workqueue(workqueue);
-}
-
 static int imx_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1249,17 +1229,6 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		priv->ops = dcfg->ops;
 
 	dev_set_drvdata(dev, rproc);
-	priv->workqueue = create_workqueue(dev_name(dev));
-	if (!priv->workqueue) {
-		dev_err(dev, "cannot create workqueue\n");
-		return -ENOMEM;
-	}
-
-	ret = devm_add_action_or_reset(dev, imx_rproc_destroy_workqueue, priv->workqueue);
-	if (ret)
-		return dev_err_probe(dev, ret, "Failed to add devm destroy workqueue action\n");
-
-	INIT_WORK(&priv->rproc_work, imx_rproc_vq_work);
 
 	ret = imx_rproc_xtr_mbox_init(rproc, true);
 	if (ret)

-- 
2.53.0

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

[sashiko-bot]

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

Re: [PATCH 2/5] mailbox: imx: Move the RX part of the mailbox into the threaded handler

[sashiko-bot]

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

New issues:
- [High] Unconditional unmasking of the RX interrupt in the threaded IRQ handler races with `imx_mu_shutdown()`, potentially leaving the hardware interrupt permanently enabled after the IRQ handler is freed.

Pre-existing issues:
- [High] Permanent denial of service (DoS) on the RX channel due to missing interrupt unmasking on error paths.
--

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

mailbox: imx: Move the RX part of the mailbox into the threaded handler

This commit moves the RX mailbox callback handling into the threaded
interrupt handler. It updates the driver to mask the RX interrupt in the
primary handler and unmask it in the threaded handler upon success.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c

[ ... ]

> @@ -524,6 +522,12 @@ static irqreturn_t imx_mu_isr_th(int irq, void *p)
>  		mbox_chan_txdone(chan, 0);
>  		return IRQ_HANDLED;
>  
> +	case IMX_MU_TYPE_RX:
> +		cp->pending = 0;
> +		if (!priv->dcfg->rx(priv, cp))
> +			imx_mu_xcr_rmw(priv, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx), 0);

[Severity: High]
This is a pre-existing issue, but does bypassing the unmasking step on
error permanently disable the channel?

If imx_mu_specific_rx() encounters an error, such as a read timeout or
receiving a message that exceeds the maximum size, it returns a negative
error code. When this happens, the condition evaluates to false and the
interrupt remains masked.

Since there does not seem to be a mechanism to automatically reset the
channel after these errors, could a buggy remote processor trigger this
to cause a permanent denial of service for the RX channel?

[Severity: High]
Can this unmasking step also race with channel shutdown?

If mbox_free_channel() is called concurrently, imx_mu_shutdown() masks the
interrupt and invokes free_irq(). The free_irq() function synchronizes
with the threaded handler and will block until imx_mu_isr_th() finishes
processing.

Because imx_mu_isr_th() proceeds to unmask the interrupt here after
processing, the hardware interrupt will be left enabled after the kernel
IRQ handler is fully unregistered. If a new message arrives afterward,
won't this cause an interrupt storm and prompt the genirq core to disable
the shared IRQ line entirely?

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

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

Re: [PATCH 3/5] mailbox: imx: Move the RXDB part of the mailbox into the threaded handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect bitmask macro used when unmasking RXDB interrupts in the threaded IRQ handler.
--

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

mailbox: imx: Move the RXDB part of the mailbox into the threaded handler

This commit moves the RXDB callback handling into the threaded handler for
the i.MX mailbox driver. It unconditionally unmasks the RXDB interrupt in
the threaded handler after processing, similar to how the standard RX
interrupts are handled.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
[ ... ]
> @@ -528,6 +528,12 @@ static irqreturn_t imx_mu_isr_th(int irq, void *p)
>  			imx_mu_xcr_rmw(priv, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx), 0);
>  		return IRQ_HANDLED;
>  
> +	case IMX_MU_TYPE_RXDB:
> +		cp->pending = 0;
> +		priv->dcfg->rxdb(priv, cp);
> +		imx_mu_xcr_rmw(priv, IMX_MU_GIER, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx), 0);

[Severity: High]
Does this code use the correct bitmask macro for unmasking the RXDB interrupt?

The IMX_MU_TYPE_RXDB case appears to be using IMX_MU_xCR_RIEn (Receive
Interrupt Enable) instead of IMX_MU_xCR_GIEn (General Purpose Interrupt
Enable) to generate the bitmask.

On non-V2 hardware, these macros target different bit ranges (bits 24-27 for
RIEn vs bits 28-31 for GIEn). Could this leave the RXDB interrupt permanently
masked after it fires once, and inadvertently set incorrect bits in the
control register?

> +		return IRQ_HANDLED;
> +
>  	default:

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

Re: [PATCH 4/5] mailbox: imx: Don't force-thread the primary handler

[sashiko-bot]

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

Pre-existing issues:
- [High] Mailbox controller is registered before initialization completes, leading to a race condition and potential system crash on probe error paths.
--

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

mailbox: imx: Don't force-thread the primary handler

The primary interrupt handler no longer invokes callbacks; it only masks
the interrupt source and returns. This patch converts the control register
lock to a raw_spinlock_t to allow the primary handler to run in hardirq
context even on PREEMPT_RT, skipping one thread.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
[ ... ]
> @@ -964,7 +963,7 @@ static int imx_mu_probe(struct platform_device *pdev)
>  		goto disable_clk;
>  	}
>  
> -	spin_lock_init(&priv->xcr_lock);
> +	raw_spin_lock_init(&priv->xcr_lock);
>  
>  	priv->mbox.dev = dev;
>  	priv->mbox.ops = &imx_mu_ops;

[Severity: High]
This is a pre-existing issue, but could the ordering in imx_mu_probe() lead
to a race condition on probe error paths?

Looking at the initialization sequence, devm_mbox_controller_register() is
called shortly after this lock initialization, but before pm_runtime_enable()
and before the probe function is guaranteed to succeed. 

If a consumer requests a channel immediately after registration and calls
startup(), it gains access to the hardware. If probe() subsequently fails
(for example, if pm_runtime_resume_and_get() fails), the error path calls
clk_disable_unprepare() to manually turn off the hardware clock.

The driver core will then trigger devres cleanup, which unregisters the
mailbox controller and calls imx_mu_shutdown() for the active channel.

Since imx_mu_shutdown() attempts to write to hardware registers to mask
interrupts, and the clock has already been disabled by the probe error path,
will this register access result in a fatal bus error or system hang?

Should the devm_mbox_controller_register() call be moved to the very end of
probe() to ensure consumers cannot interact with the hardware during an early
teardown?

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

Re: [PATCH 5/5] remoteproc: imx_rproc: Invoke the callback directly

[sashiko-bot]

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

Pre-existing issues:
- [High] Unprotected access to `rproc->notifyids` in `imx_rproc_rx_callback` via `idr_for_each` without `rcu_read_lock()` can race with `idr_remove` during `rproc-virtio` unbind, leading to Use-After-Free.
--

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

remoteproc: imx_rproc: Invoke the callback directly

The imx-mailbox driver moved callback invocation into the threaded IRQ
handler, making it run in a preemptible context. This patch removes the
workqueue and invokes the imx_rproc_notified_idr_cb() callback directly
from the RX callback.

> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 0dd80e688b0ea..c97bc1c401655 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c

[ ... ]

>  static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
>  {
>  	struct rproc *rproc = dev_get_drvdata(cl->dev);
> -	struct imx_rproc *priv = rproc->priv;
>  
> -	queue_work(priv->workqueue, &priv->rproc_work);
> +	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);

[Severity: High]
This is a pre-existing issue, but does this idr_for_each() call require
rcu_read_lock() protection?

Looking at the call chain, it also performs an idr_find() in
rproc_vq_interrupt() without rcu_read_lock():

imx_rproc_rx_callback()
  -> idr_for_each()
     -> imx_rproc_notified_idr_cb()
        -> rproc_vq_interrupt()
           -> idr_find()

If a user unbinds the rproc-virtio driver via sysfs concurrently,
rproc_virtio_remove() calls rproc_free_vring() which executes idr_remove().

Since idr_for_each() and idr_find() rely on RCU internally to protect radix
tree nodes, could running this in a preemptible context without
rcu_read_lock() race with idr_remove() and lead to a use-after-free of the
IDR nodes or the rvring structure?

>  }
>  
>  static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block)

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

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

[Sebastian Andrzej Siewior]

On 2026-05-29 16:43:42 [+0000], sashiko-bot@kernel.org wrote:
> 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.

Yeah. It masks the event rather that the interrupt.

> > 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?

Right. It should reverse the startup order and first free_irq() and then
unmask the event.

Sebastian

Re: [PATCH 2/5] mailbox: imx: Move the RX part of the mailbox into the threaded handler

[Sebastian Andrzej Siewior]

On 2026-05-29 17:26:35 [+0000], sashiko-bot@kernel.org wrote:
> commit 7ab3fb54b60e0decf23b64d1d78df0e2b4409f2a
> Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> mailbox: imx: Move the RX part of the mailbox into the threaded handler
> 
> This commit moves the RX mailbox callback handling into the threaded
> interrupt handler. It updates the driver to mask the RX interrupt in the
> primary handler and unmask it in the threaded handler upon success.
> 
> > diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> > --- a/drivers/mailbox/imx-mailbox.c
> > +++ b/drivers/mailbox/imx-mailbox.c
> 
> [ ... ]
> 
> > @@ -524,6 +522,12 @@ static irqreturn_t imx_mu_isr_th(int irq, void *p)
> >  		mbox_chan_txdone(chan, 0);
> >  		return IRQ_HANDLED;
> >  
> > +	case IMX_MU_TYPE_RX:
> > +		cp->pending = 0;
> > +		if (!priv->dcfg->rx(priv, cp))
> > +			imx_mu_xcr_rmw(priv, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx), 0);
> 
> [Severity: High]
> This is a pre-existing issue, but does bypassing the unmasking step on
> error permanently disable the channel?

Yes, I think this is the intention. The RX callback does not unmask the
event on error.

> If imx_mu_specific_rx() encounters an error, such as a read timeout or
> receiving a message that exceeds the maximum size, it returns a negative
> error code. When this happens, the condition evaluates to false and the
> interrupt remains masked.
> 
> Since there does not seem to be a mechanism to automatically reset the
> channel after these errors, could a buggy remote processor trigger this
> to cause a permanent denial of service for the RX channel?
> 
> [Severity: High]
> Can this unmasking step also race with channel shutdown?
> 
> If mbox_free_channel() is called concurrently, imx_mu_shutdown() masks the
> interrupt and invokes free_irq(). The free_irq() function synchronizes
> with the threaded handler and will block until imx_mu_isr_th() finishes
> processing.
> 
> Because imx_mu_isr_th() proceeds to unmask the interrupt here after
> processing, the hardware interrupt will be left enabled after the kernel
> IRQ handler is fully unregistered. If a new message arrives afterward,
> won't this cause an interrupt storm and prompt the genirq core to disable
> the shared IRQ line entirely?

Yes, we had this.

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

Sebastian

Re: [PATCH 3/5] mailbox: imx: Move the RXDB part of the mailbox into the threaded handler

[Sebastian Andrzej Siewior]

On 2026-05-29 17:34:02 [+0000], sashiko-bot@kernel.org wrote:
> > diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> > --- a/drivers/mailbox/imx-mailbox.c
> > +++ b/drivers/mailbox/imx-mailbox.c
> [ ... ]
> > @@ -528,6 +528,12 @@ static irqreturn_t imx_mu_isr_th(int irq, void *p)
> >  			imx_mu_xcr_rmw(priv, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx), 0);
> >  		return IRQ_HANDLED;
> >  
> > +	case IMX_MU_TYPE_RXDB:
> > +		cp->pending = 0;
> > +		priv->dcfg->rxdb(priv, cp);
> > +		imx_mu_xcr_rmw(priv, IMX_MU_GIER, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx), 0);
> 
> [Severity: High]
> Does this code use the correct bitmask macro for unmasking the RXDB interrupt?
> 
> The IMX_MU_TYPE_RXDB case appears to be using IMX_MU_xCR_RIEn (Receive
> Interrupt Enable) instead of IMX_MU_xCR_GIEn (General Purpose Interrupt
> Enable) to generate the bitmask.
> 
> On non-V2 hardware, these macros target different bit ranges (bits 24-27 for
> RIEn vs bits 28-31 for GIEn). Could this leave the RXDB interrupt permanently
> masked after it fires once, and inadvertently set incorrect bits in the
> control register?

So I copy&pasted the wrong register and the bot noticed it? I am
impressed.

Sebastian

Re: [PATCH 4/5] mailbox: imx: Don't force-thread the primary handler

[Sebastian Andrzej Siewior]

On 2026-05-29 18:13:28 [+0000], sashiko-bot@kernel.org wrote:
> [Severity: High]
> This is a pre-existing issue, but could the ordering in imx_mu_probe() lead
> to a race condition on probe error paths?
> 
> Looking at the initialization sequence, devm_mbox_controller_register() is
> called shortly after this lock initialization, but before pm_runtime_enable()
> and before the probe function is guaranteed to succeed. 
> 
> If a consumer requests a channel immediately after registration and calls
> startup(), it gains access to the hardware. If probe() subsequently fails
> (for example, if pm_runtime_resume_and_get() fails), the error path calls
> clk_disable_unprepare() to manually turn off the hardware clock.
> 
> The driver core will then trigger devres cleanup, which unregisters the
> mailbox controller and calls imx_mu_shutdown() for the active channel.
> 
> Since imx_mu_shutdown() attempts to write to hardware registers to mask
> interrupts, and the clock has already been disabled by the probe error path,
> will this register access result in a fatal bus error or system hang?
> 
> Should the devm_mbox_controller_register() call be moved to the very end of
> probe() to ensure consumers cannot interact with the hardware during an early
> teardown?

Puh. devm_mbox_controller_register() itself is harmless, there needs to
be a probe in parallel that grabs that channel before device
registration is complete. of_platform_populate() could lead to probes
and channel grabs which is okay since it happens before that
pm_runtime*().
If this fails then, then we would invoke shutdown with disabled runtime
PM. Usually one would unfold it manually in the right order.

Looking at it, not sure where there is an enable followed by resume and
put. It should be enough to just enable. But maybe a test for resume
isn't that bad…

Anyway. Switching to devm_pm_runtime_enable() should simplify things a
bit. And delaying of_platform_populate() should do the other bit. And
while at it, using devm_of_platform_populate() should fix rmmod +
modprobe.

Sebastian

Re: [PATCH 5/5] remoteproc: imx_rproc: Invoke the callback directly

[Sebastian Andrzej Siewior]

On 2026-05-29 19:08:53 [+0000], sashiko-bot@kernel.org wrote:
> >  static void imx_rproc_rx_callback(struct mbox_client *cl, void *msg)
> >  {
> >  	struct rproc *rproc = dev_get_drvdata(cl->dev);
> > -	struct imx_rproc *priv = rproc->priv;
> >  
> > -	queue_work(priv->workqueue, &priv->rproc_work);
> > +	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);
> 
> [Severity: High]
> This is a pre-existing issue, but does this idr_for_each() call require
> rcu_read_lock() protection?
> 
> Looking at the call chain, it also performs an idr_find() in
> rproc_vq_interrupt() without rcu_read_lock():
> 
> imx_rproc_rx_callback()
>   -> idr_for_each()
>      -> imx_rproc_notified_idr_cb()
>         -> rproc_vq_interrupt()
>            -> idr_find()
> 
> If a user unbinds the rproc-virtio driver via sysfs concurrently,
> rproc_virtio_remove() calls rproc_free_vring() which executes idr_remove().
> 
> Since idr_for_each() and idr_find() rely on RCU internally to protect radix
> tree nodes, could running this in a preemptible context without
> rcu_read_lock() race with idr_remove() and lead to a use-after-free of the
> IDR nodes or the rvring structure?

If I see this right there is no RCU involved. First the interrupt gets
removed (via free_irq()) which ensures that it is no longer active and
then the channel is removed.

Sebastian

Re: [PATCH 0/5] mailbox: imx: Use threaded handler to avoid kworker in imx's remoteproc

[Mathieu Poirier]

On Fri, May 29, 2026 at 06:01:02PM +0200, Sebastian Andrzej Siewior wrote:
> The imx's remoteproc driver uses a kworker from its mailbox callback to
> complete the request. The reason is that the imx mailbox driver invokes
> the callback from its interrupt handler and the remoteproc callback (at
> least the rpmsg-tty) requires a preemptible context.
> 
> This works but is problematic in a PREEMPT_RT environment where the
> latency of the invocation is important. By scheduling a kworker the
> high task priority from the threaded handler is lost and the kworker
> competes for CPU ressources with every SCHED_OTHER task in the system.
> This can lead to long delays on a busy system with other RT threads
> which are less important than the completion of this request.
> 
> Looking over other mailbox driver, like the arm_mhu for instance, they
> use a threaded interrupt handler to invoke the callback. This avoids the
> kworker detour.
> 
> The here suggested change utilises a threaded interrupt to invoke the
> callback. The primary handler mask the interrupt source so that the
> handler can run without getting interrupted by the interrupt again.
> Doing so avoids marking the interrupt IRQF_ONESHOT so that that in a
> shared-interrupt environment the other interrupt can still fire while
> the first one is masked.
> 
> This change was tested on a im93 board with rpmsg-tty driver.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> Sebastian Andrzej Siewior (5):
>       mailbox: imx: Start splitting the IRQ handler in primary and threaded handler
>       mailbox: imx: Move the RX part of the mailbox into the threaded handler
>       mailbox: imx: Move the RXDB part of the mailbox into the threaded handler
>       mailbox: imx: Don't force-thread the primary handler
>       remoteproc: imx_rproc: Invoke the callback directly
> 
>  drivers/mailbox/imx-mailbox.c  | 65 +++++++++++++++++++++++++++++++++---------
>  drivers/remoteproc/imx_rproc.c | 33 +--------------------

For imx_rproc.c:

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>

You may also want to fix driver/remoteproc/stm32_rproc.c

>  2 files changed, 53 insertions(+), 45 deletions(-)
> ---
> base-commit: 9e171fc1d7d7ab847a750c03571c87ac3c17bd84
> change-id: 20260529-imx_mbox_rproc-7d512f5a6f78
> 
> Best regards,
> -- 
> Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>