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

[PATCH v2 0/9] 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 in a
shared-interrupt environment the other interrupt can still fire while
the first is masked.

The first four patches are result of the sashiko review. Does not look
critical.

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

v1…v2: https://lore.kernel.org/r/20260529-imx_mbox_rproc-v1-0-b8ffc36e11e5@linutronix.de
  - Using correct register to enable RXDB event.
  - Update commit description for the "threaded interrupt", "unmasks the
    interrupt" => "masks the interrupt event".
  - Add a shutdown field so that the interrupt does not unmask the
    interrupt if it has been already disabled because the channel is
    about to be shutdown.  A possible race mentioned by sashiko.
  - Use devm_pm_runtime_enable(). This should avoid a possible race
    sashiko mentioned.
  - Use devm_of_platform_populate().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
Sebastian Andrzej Siewior (9):
      mailbox: imx: Add a channel shutdown field
      mailbox: imx: Use devm_pm_runtime_enable()
      mailbox: imx: use devm_of_platform_populate()
      mailbox: imx: Use channel index instead of zero in imx_mu_specific_rx()
      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  | 133 ++++++++++++++++++++++++++++++-----------
 drivers/remoteproc/imx_rproc.c |  33 +---------
 2 files changed, 100 insertions(+), 66 deletions(-)
---
base-commit: 3bc831df9ee16fceee851872315161377ca1417d
change-id: 20260529-imx_mbox_rproc-7d512f5a6f78

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

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

[Sebastian Andrzej Siewior]

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.

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

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
@@ -80,6 +80,7 @@ struct imx_mu_con_priv {
 	enum imx_mu_chan_type	type;
 	struct mbox_chan	*chan;
 	struct work_struct 	txdb_work;
+	bool			shutdown;
 };
 
 struct imx_mu_priv {
@@ -219,6 +220,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);
+}
+
 static int imx_mu_generic_tx(struct imx_mu_priv *priv,
 			     struct imx_mu_con_priv *cp,
 			     void *data)
@@ -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;
@@ -604,6 +635,7 @@ static int imx_mu_startup(struct mbox_chan *chan)
 		return ret;
 	}
 
+	cp->shutdown = false;
 	switch (cp->type) {
 	case IMX_MU_TYPE_RX:
 		imx_mu_xcr_rmw(priv, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx), 0);
@@ -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;
 	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));
 		break;
 	case IMX_MU_TYPE_RST:
 		imx_mu_xcr_rmw(priv, IMX_MU_CR, IMX_MU_xCR_RST(priv->dcfg->type), 0);

-- 
2.53.0

[PATCH v2 2/9] mailbox: imx: Use devm_pm_runtime_enable()

[Sebastian Andrzej Siewior]

sashiko complained about early usage of the device while probe isn't
completed. This can be mitigated by delaying the pm_runtime_enable()
into the removal path instead doing it early. This ensures that in an
error case the device is removed (and imx_mu_shutdown()) before
pm_runtime_disable() so we don't have to do this manually.

For the order to work, lets move devm_mbox_controller_register() until
after the pm-runtime part. So the reverse order will be mbox-controller
removal followed by disabling pm runtime.

Use devm_pm_runtime_enable(), remove manual pm_runtime_disable()
invocations and move the pm_runtime handling in probe before
devm_mbox_controller_register().

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

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 34edc2f31dcb5..24cf704b4f9bd 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -965,28 +965,28 @@ static int imx_mu_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 
+	ret = devm_pm_runtime_enable(dev);
+	if (ret < 0)
+		goto disable_clk;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret < 0)
+		goto disable_clk;
+
+	ret = pm_runtime_put_sync(dev);
+	if (ret < 0)
+		goto disable_clk;
+
+	clk_disable_unprepare(priv->clk);
+
 	ret = devm_mbox_controller_register(dev, &priv->mbox);
 	if (ret)
 		goto disable_clk;
 
 	of_platform_populate(dev->of_node, NULL, NULL, dev);
 
-	pm_runtime_enable(dev);
-
-	ret = pm_runtime_resume_and_get(dev);
-	if (ret < 0)
-		goto disable_runtime_pm;
-
-	ret = pm_runtime_put_sync(dev);
-	if (ret < 0)
-		goto disable_runtime_pm;
-
-	clk_disable_unprepare(priv->clk);
-
 	return 0;
 
-disable_runtime_pm:
-	pm_runtime_disable(dev);
 disable_clk:
 	clk_disable_unprepare(priv->clk);
 	return ret;
@@ -994,9 +994,6 @@ static int imx_mu_probe(struct platform_device *pdev)
 
 static void imx_mu_remove(struct platform_device *pdev)
 {
-	struct imx_mu_priv *priv = platform_get_drvdata(pdev);
-
-	pm_runtime_disable(priv->dev);
 }
 
 static const struct imx_mu_dcfg imx_mu_cfg_imx6sx = {

-- 
2.53.0

[PATCH v2 3/9] mailbox: imx: use devm_of_platform_populate()

[Sebastian Andrzej Siewior]

The driver uses of_platform_populate() but does not remove the added
devices on removal. This can lead to "double devices" on module removal
followed by adding the module again.

Use devm_of_platform_populate() to remove the populated devices once the
parent device is removed.

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

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 24cf704b4f9bd..1dda1437b87f6 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -983,7 +983,7 @@ static int imx_mu_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_clk;
 
-	of_platform_populate(dev->of_node, NULL, NULL, dev);
+	devm_of_platform_populate(dev);
 
 	return 0;
 

-- 
2.53.0

[PATCH v2 4/9] mailbox: imx: Use channel index instead of zero in imx_mu_specific_rx()

[Sebastian Andrzej Siewior]

imx_mu_specific_rx() masks channel 0 and unmasks it again at the end of
the function. Given that at startup the channel index got unmasked it
should do the right job.

This here either unmasks the actual channel or another one but should
have no impact given that it reverses its doing at the end.

Use the channel index instead of zero.

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

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index 1dda1437b87f6..d1de07cc0ed62 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -380,7 +380,7 @@ 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));
+	imx_mu_xcr_rmw(priv, IMX_MU_RCR, 0, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
 	*data++ = imx_mu_read(priv, priv->dcfg->xRR);
 
 	if (priv->dcfg->type & IMX_MU_V2_S4) {
@@ -407,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_set_act(priv, cp, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, 0));
+	imx_mu_xcr_set_act(priv, cp, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
 	mbox_chan_received_data(cp->chan, (void *)priv->msg);
 
 	return 0;

-- 
2.53.0

[PATCH v2 5/9] 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 masks the
interrupt event 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 | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index d1de07cc0ed62..006aa76b74b62 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -81,6 +81,7 @@ struct imx_mu_con_priv {
 	struct mbox_chan	*chan;
 	struct work_struct 	txdb_work;
 	bool			shutdown;
+	bool			pending;
 };
 
 struct imx_mu_priv {
@@ -539,11 +540,35 @@ 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 = false;
+		mbox_chan_txdone(chan, 0);
+		break;
+
+	default:
+		dev_warn_ratelimited(priv->dev, "Unhandled channel type %d\n",
+				     cp->type);
+		return IRQ_NONE;
+	}
+	return IRQ_HANDLED;
+}
+
 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) {
@@ -579,7 +604,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 = true;
+		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);
@@ -594,7 +620,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)
@@ -629,7 +655,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 v2 6/9] 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 006aa76b74b62..6fe202593592f 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -381,7 +381,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, cp->idx));
 	*data++ = imx_mu_read(priv, priv->dcfg->xRR);
 
 	if (priv->dcfg->type & IMX_MU_V2_S4) {
@@ -408,7 +407,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_set_act(priv, cp, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
 	mbox_chan_received_data(cp->chan, (void *)priv->msg);
 
 	return 0;
@@ -555,6 +553,12 @@ static irqreturn_t imx_mu_isr_th(int irq, void *p)
 		mbox_chan_txdone(chan, 0);
 		break;
 
+	case IMX_MU_TYPE_RX:
+		cp->pending = false;
+		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));
+		break;
+
 	default:
 		dev_warn_ratelimited(priv->dev, "Unhandled channel type %d\n",
 				     cp->type);
@@ -608,7 +612,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 = true;
+		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 v2 7/9] 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 6fe202593592f..f0a14449652f1 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -559,6 +559,12 @@ static irqreturn_t imx_mu_isr_th(int irq, void *p)
 			imx_mu_xcr_set_act(priv, cp, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
 		break;
 
+	case IMX_MU_TYPE_RXDB:
+		cp->pending = false;
+		priv->dcfg->rxdb(priv, cp);
+		imx_mu_xcr_set_act(priv, cp, IMX_MU_GIER, IMX_MU_xCR_GIEn(priv->dcfg->type, cp->idx));
+		break;
+
 	default:
 		dev_warn_ratelimited(priv->dev, "Unhandled channel type %d\n",
 				     cp->type);
@@ -617,7 +623,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 = true;
+		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 v2 8/9] 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 | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
index f0a14449652f1..f3de47908e00f 100644
--- a/drivers/mailbox/imx-mailbox.c
+++ b/drivers/mailbox/imx-mailbox.c
@@ -88,7 +88,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];
@@ -208,15 +208,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;
 }
@@ -224,31 +223,27 @@ static u32 imx_mu_xcr_rmw(struct imx_mu_priv *priv, enum imx_mu_xcr type, u32 se
 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);
+	guard(raw_spinlock_irqsave)(&priv->xcr_lock);
 	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);
+	guard(raw_spinlock_irqsave)(&priv->xcr_lock);
 	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);
 }
 
 static int imx_mu_generic_tx(struct imx_mu_priv *priv,
@@ -649,7 +644,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);
@@ -997,7 +992,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 v2 9/9] 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 v2 6/9] mailbox: imx: Move the RX part of the mailbox into the threaded handler

[Daniel Baluta]

On 6/3/26 16:05, Sebastian Andrzej Siewior wrote:
> 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.

Hi Sebastian,

It is not very clear from me from the commit message why this change is needed.
Can you please update the commit message with proper explanation?


> 
> 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 006aa76b74b62..6fe202593592f 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -381,7 +381,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, cp->idx));
>  	*data++ = imx_mu_read(priv, priv->dcfg->xRR);
>  
>  	if (priv->dcfg->type & IMX_MU_V2_S4) {
> @@ -408,7 +407,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_set_act(priv, cp, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
>  	mbox_chan_received_data(cp->chan, (void *)priv->msg);
>  
>  	return 0;
> @@ -555,6 +553,12 @@ static irqreturn_t imx_mu_isr_th(int irq, void *p)
>  		mbox_chan_txdone(chan, 0);
>  		break;
>  
> +	case IMX_MU_TYPE_RX:
> +		cp->pending = false;
> +		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));
> +		break;
> +
>  	default:
>  		dev_warn_ratelimited(priv->dev, "Unhandled channel type %d\n",
>  				     cp->type);
> @@ -608,7 +612,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 = true;
> +		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);
> 

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

[sashiko-bot]

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

Re: [PATCH v2 6/9] mailbox: imx: Move the RX part of the mailbox into the threaded handler

[Sebastian Andrzej Siewior]

On 2026-06-03 16:18:22 [+0300], Daniel Baluta wrote:
> On 6/3/26 16:05, Sebastian Andrzej Siewior wrote:
> > 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.
> 
> Hi Sebastian,
> 
> It is not very clear from me from the commit message why this change is needed.
> Can you please update the commit message with proper explanation?

The intention is to move the entire interrupt handling into the threaded
handler. The primary handler masks just event, the threaded handler
performs the RX/TX/ event.
I move the individual events step by step so the previous patch moved
the TX, here is RX and the next one does RXDB.

Is this clear now? The cover later explains why this done in the first
place.

Sebastian

Re: [PATCH v2 2/9] mailbox: imx: Use devm_pm_runtime_enable()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Double clk_disable_unprepare() on the devm_mbox_controller_register() error path.
--

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

mailbox: imx: Use devm_pm_runtime_enable()

This commit mitigates early usage of the device before probe completes
by delaying the pm_runtime_enable() into the removal path. It also moves
devm_mbox_controller_register() to occur after the pm-runtime handling.

> 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
[ ... ]
> @@ -965,28 +965,28 @@ static int imx_mu_probe(struct platform_device *pdev)
[ ... ]
>  	ret = pm_runtime_put_sync(dev);
>  	if (ret < 0)
> -		goto disable_runtime_pm;
> +		goto disable_clk;
>  
>  	clk_disable_unprepare(priv->clk);
>  
> +	ret = devm_mbox_controller_register(dev, &priv->mbox);
> +	if (ret)
> +		goto disable_clk;

[Severity: High]
Does this error path cause a double clock disable?

If devm_mbox_controller_register() fails here, the code jumps to the
disable_clk label. However, clk_disable_unprepare(priv->clk) has already
been called just above this registration attempt. This means the error path
will disable the clock a second time, which could cause a reference count
underflow and potentially disable shared clocks.

> +
> +	of_platform_populate(dev->of_node, NULL, NULL, dev);
> +
>  	return 0;
>  
> -disable_runtime_pm:
> -	pm_runtime_disable(dev);
>  disable_clk:
>  	clk_disable_unprepare(priv->clk);
>  	return ret;

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

Re: [PATCH v2 3/9] mailbox: imx: use devm_of_platform_populate()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unbalanced clk_disable_unprepare() on the error path for devm_mbox_controller_register() causes a double clock disable.
--

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

mailbox: imx: use devm_of_platform_populate()

The driver uses of_platform_populate() but does not remove the added
devices on removal, which can lead to duplicate devices upon module reload.
This commit switches to devm_of_platform_populate() to ensure populated
devices are removed when the parent device is removed.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index 24cf704b4f9bd..1dda1437b87f6 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c
> @@ -983,7 +983,7 @@ static int imx_mu_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto disable_clk;

[Severity: High]
Does this error path trigger a double clock disable?

Looking at imx_mu_probe(), the clock is explicitly disabled before
devm_mbox_controller_register() is called:

	clk_disable_unprepare(priv->clk);

	ret = devm_mbox_controller_register(dev, &priv->mbox);
	if (ret)
		goto disable_clk;

If the registration fails, we jump to the disable_clk label which
unconditionally calls clk_disable_unprepare(priv->clk) a second time:

disable_clk:
	clk_disable_unprepare(priv->clk);
	return ret;

Can this imbalanced clock disable cause a framework warning and potentially
break shared clocks used by other devices?

This regression was introduced by the PM runtime refactoring earlier in the
series and remains unfixed.

>  
> -	of_platform_populate(dev->of_node, NULL, NULL, dev);
> +	devm_of_platform_populate(dev);
>  
>  	return 0;

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

Re: [PATCH v2 6/9] mailbox: imx: Move the RX part of the mailbox into the threaded handler

[Daniel Baluta]

On 6/3/26 16:20, Sebastian Andrzej Siewior wrote:
> On 2026-06-03 16:18:22 [+0300], Daniel Baluta wrote:
>> On 6/3/26 16:05, Sebastian Andrzej Siewior wrote:
>>> 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.
>>
>> Hi Sebastian,
>>
>> It is not very clear from me from the commit message why this change is needed.
>> Can you please update the commit message with proper explanation?
> 
> The intention is to move the entire interrupt handling into the threaded
> handler. The primary handler masks just event, the threaded handler
> performs the RX/TX/ event.
> I move the individual events step by step so the previous patch moved
> the TX, here is RX and the next one does RXDB.
> 
> Is this clear now? The cover later explains why this done in the first
> place.

I would prefer to add the explanation for this in at least one commit
message to be there in git log.

Cover letter is not kept in git history.

Re: [PATCH v2 5/9] 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] Double `clk_disable_unprepare()` on `devm_mbox_controller_register()` error path.

Pre-existing issues:
- [High] Unchecked return value of `pm_runtime_get_sync()` can lead to synchronous external aborts.
--

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

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

This commit splits the mailbox IRQ handling into a primary handler and a
threaded handler. The primary handler masks the interrupt event, allowing the
threaded handler to run in a preemptible context without raising the interrupt
again. As a first step, it prepares the logic and moves the TX handling part.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index d1de07cc0ed62..006aa76b74b62 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c

[ ... ]

> @@ -629,7 +655,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);

[Severity: High]
This isn't a bug introduced by this patch, but looking at imx_mu_startup()
in drivers/mailbox/imx-mailbox.c, the return value of pm_runtime_get_sync()
is ignored:

	pm_runtime_get_sync(priv->dev);
	if (cp->type == IMX_MU_TYPE_TXDB_V2)
		return 0;

If the PM domain fails to resume (for example, returning -ENOMEM or
-ETIMEDOUT), will the function proceed to interact with hardware registers?
On i.MX platforms, accessing unclocked or unpowered hardware registers
typically results in a synchronous external abort and a kernel panic.

>  	if (ret) {
>  		dev_err(priv->dev, "Unable to acquire IRQ %d\n", priv->irq[cp->type]);
>  		return ret;

[Severity: Medium]
Looking at the driver changes introduced earlier in this patch series
(specifically "mailbox: imx: Use devm_pm_runtime_enable()"), there appears
to be a double clock disable issue on the error path:

drivers/mailbox/imx-mailbox.c:imx_mu_probe() {
...
	clk_disable_unprepare(priv->clk);

	ret = devm_mbox_controller_register(dev, &priv->mbox);
	if (ret)
		goto disable_clk;
...
disable_clk:
	clk_disable_unprepare(priv->clk);
	return ret;
}

Could this cause the clock reference count to underflow and trigger a
warning in the clock framework if devm_mbox_controller_register() fails?

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

Re: [PATCH v2 7/9] 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:

Pre-existing issues:
- [High] Error paths in `imx_mu_seco_rxdb` fail to clear the hardware interrupt status (`GIPn`), leading to an infinite IRQ storm because the threaded handler unconditionally unmasks the interrupt.
--

commit fc70fc114c374d42412c37d5886dc2cfe0a507bb
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Subject: mailbox: imx: Move the RXDB part of the mailbox into the threaded handler

This commit moves the RXDB callback handling from the hardirq to the threaded
interrupt handler. It also changes the behavior to unconditionally unmask
the RXDB interrupt after the callback returns.

> 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

[ ... ]

> @@ -559,6 +559,12 @@ static irqreturn_t imx_mu_isr_th(int irq, void *p)
>  			imx_mu_xcr_set_act(priv, cp, IMX_MU_RCR, IMX_MU_xCR_RIEn(priv->dcfg->type, cp->idx));
>  		break;
>  
> +	case IMX_MU_TYPE_RXDB:
> +		cp->pending = false;
> +		priv->dcfg->rxdb(priv, cp);
> +		imx_mu_xcr_set_act(priv, cp, IMX_MU_GIER, IMX_MU_xCR_GIEn(priv->dcfg->type, cp->idx));

[Severity: High]
This is a pre-existing issue, but since this handler now unconditionally
unmasks the interrupt, could this lead to an infinite interrupt storm if an
error occurs in the callback?

Looking at imx_mu_seco_rxdb(), if the received message size is invalid or a
read timeout occurs, the function jumps to the error path:

drivers/mailbox/imx-mailbox.c:imx_mu_seco_rxdb() {
    ...
        goto error;
    ...
error:
    mbox_chan_received_data(cp->chan, ERR_PTR(err));

exit:
    return err;
}

This error path skips writing to the GSR register to clear the GIPn hardware
interrupt pending bit. Since imx_mu_isr_th() ignores the return value of
rxdb() and unconditionally unmasks the interrupt via imx_mu_xcr_set_act(),
won't the uncleared hardware status bit immediately re-trigger the hardirq
and cause a livelock?

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

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

Re: [PATCH v2 8/9] mailbox: imx: Don't force-thread the primary handler

[sashiko-bot]

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

New issues:
- [Medium] Double disable of priv->clk on devm_mbox_controller_register() failure.

Pre-existing issues:
- [High] Unchecked return value of pm_runtime_get_sync() leads to kernel panic.
- [High] Silent transmission failure due to ignored timeout error in imx_mu_generic_tx().
- [High] Polling jiffies and using time APIs with local interrupts disabled leads to hard lockups.
--

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

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

This commit avoids force-threading the primary interrupt handler in the
IMX mailbox driver. It converts the xcr_lock to a raw_spinlock_t so the
primary handler can safely run in hardirq context, and marks the interrupt
with IRQF_NO_THREAD.

> diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> index f0a14449652f1..f3de47908e00f 100644
> --- a/drivers/mailbox/imx-mailbox.c
> +++ b/drivers/mailbox/imx-mailbox.c

[ ... ]

> @@ -649,7 +644,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);

[Severity: High]
This is a pre-existing issue, but does this missing check for the return
value of pm_runtime_get_sync() risk a kernel panic?

If the device fails to resume (e.g., due to a clock enablement failure),
it remains in a suspended/unpowered state.

The function immediately proceeds to configure the device and access its
I/O registers. Accessing unpowered hardware registers can cause system
crashes, such as synchronous external aborts or bus hangs, on ARM platforms.

[ ... ]

While reviewing the driver, I noticed a few other potential regressions outside
the diff hunks above:

[Severity: High]
This is a pre-existing issue, but can the timeout loop in imx_mu_generic_tx()
lead to silent transmission failures?

When processing IMX_MU_TYPE_TXDB_V2, the code loops up to 10 times waiting
for the readl_poll_timeout() condition:

drivers/mailbox/imx-mailbox.c:imx_mu_generic_tx() {
    ...
    while (ret && (count < 10)) {
        ret = readl_poll_timeout(priv->base + priv->dcfg->xCR[IMX_MU_GCR], val,
                                 !(val & IMX_MU_xCR_GIRn(priv->dcfg->type, cp->idx)),
                                 0, 10000);
        if (ret) {
            dev_warn_ratelimited(...);
        }
    }
    break;
    ...
}

If all 10 attempts time out, ret is left as -ETIMEDOUT and the while loop
exits. The code then breaks from the switch block and unconditionally hits
return 0 at the end of the function instead of returning ret.

Falsely returning success causes the mailbox framework to drop the message,
leading to silent data loss. Should this return the error code instead?

[Severity: Medium]
Is there a double disable of priv->clk if devm_mbox_controller_register()
fails?

In imx_mu_probe(), the clock is manually disabled via clk_disable_unprepare()
after successfully syncing the PM runtime state:

drivers/mailbox/imx-mailbox.c:imx_mu_probe() {
    ...
    clk_disable_unprepare(priv->clk);

    ret = devm_mbox_controller_register(dev, &priv->mbox);
    if (ret)
        goto disable_clk;
    ...
disable_clk:
    clk_disable_unprepare(priv->clk);
    return ret;
}

If devm_mbox_controller_register() fails, the code branches to disable_clk,
which executes clk_disable_unprepare(priv->clk) a second time. This double
disable unbalances the clock reference count and triggers a kernel warning.

[Severity: High]
This is a pre-existing issue, but does the use of time APIs and polling
with local interrupts disabled lead to hard lockups?

Mailbox transmission (priv->dcfg->tx) is invoked via mbox_send_message()
with the channel lock (spin_lock_irqsave) held, meaning local interrupts
are disabled.

Inside imx_mu_tx_waiting_write(), the code polls a hardware register while
checking time_is_after_jiffies64():

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

Because interrupts are disabled, jiffies will not advance on UP systems,
causing an infinite loop (deadlock).

Additionally, imx_mu_specific_tx() uses readl_poll_timeout() with a
5-second timeout in this atomic context:

drivers/mailbox/imx-mailbox.c:imx_mu_specific_tx() {
    ...
    ret = readl_poll_timeout(priv->base + priv->dcfg->xSR[IMX_MU_TSR],
                             xsr,
                             xsr & IMX_MU_xSR_TEn(priv->dcfg->type, i % num_tr),
                             0, 5 * USEC_PER_SEC);
    ...
}

Spinning for up to 5 seconds with IRQs disabled violates core kernel
locking rules and can trigger NMI watchdogs and RCU stalls.

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing RCU read lock around `idr_for_each()` executing in a preemptible context, leading to potential use-after-free.
- [High] Error paths in mailbox initialization leave `ERR_PTR` or dangling pointers in the channel pointers, leading to a kernel panic during cleanup.
--

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

remoteproc: imx_rproc: Invoke the callback directly

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.

> 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

[ ... ]

> @@ -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);

[Severity: High]
This is a pre-existing issue, but does this idr_for_each() loop require RCU
read protection in drivers/remoteproc/imx_rproc.c?

Since imx_rproc_rx_callback() now runs in a preemptible threaded IRQ handler
(and previously ran in a preemptible kworker), it executes without holding an
RCU read lock.

If a concurrent unbind of the rproc-virtio driver removes an IDR entry via:

rproc_virtio_remove()
  rproc_free_vring()
    idr_remove()

The radix tree nodes could be scheduled for delayed RCU freeing. If the
threaded IRQ handler is preempted during the idr_for_each() iteration and the
RCU grace period expires, could the iteration traverse freed memory upon
resuming and cause a use-after-free crash?

>  }

[ ... ]

> @@ -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)

[Severity: High]
This is a pre-existing issue, but can the mailbox initialization error paths in
imx_rproc_xtr_mbox_init() lead to a use-after-free or panic during cleanup?

If mbox_request_channel_byname() fails for the "tx" channel, it returns an
error but assigns an ERR_PTR to priv->tx_ch:

drivers/remoteproc/imx_rproc.c:imx_rproc_xtr_mbox_init() {
    ...
	priv->tx_ch = mbox_request_channel_byname(cl, "tx");
	if (IS_ERR(priv->tx_ch))
		return dev_err_probe(cl->dev, PTR_ERR(priv->tx_ch),
				     "failed to request tx mailbox channel\n");
    ...
}

If the "rx" channel fails, it leaves the ERR_PTR in priv->rx_ch and calls
mbox_free_channel(priv->tx_ch) but does not reset priv->tx_ch to NULL,
leaving it dangling:

drivers/remoteproc/imx_rproc.c:imx_rproc_xtr_mbox_init() {
    ...
	priv->rx_ch = mbox_request_channel_byname(cl, "rx");
	if (IS_ERR(priv->rx_ch)) {
		mbox_free_channel(priv->tx_ch);
		return dev_err_probe(cl->dev, PTR_ERR(priv->rx_ch),
				     "failed to request rx mailbox channel\n");
	}
    ...
}

When the device is later unbound, the devm cleanup action imx_rproc_free_mbox()
runs. It checks if the pointers are true, which they will be for an ERR_PTR
or dangling pointer:

drivers/remoteproc/imx_rproc.c:imx_rproc_free_mbox() {
    ...
	if (priv->tx_ch) {
		mbox_free_channel(priv->tx_ch);
		priv->tx_ch = NULL;
	}

	if (priv->rx_ch) {
		mbox_free_channel(priv->rx_ch);
		priv->rx_ch = NULL;
	}
    ...
}

Since mbox_free_channel() does not check IS_ERR(), will it attempt to
dereference these invalid pointers and cause a kernel panic or a double-free?

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

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

[Sebastian Andrzej Siewior]

On 2026-06-03 13:20:28 [+0000], sashiko-bot@kernel.org wrote:
> 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?

Yes, it is intended.

> [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.

Well, yes. But it improves since the threaded handler is preemptible by
task with higher priority :)

> [ ... ]
> > @@ -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?

The TX part should have been disabled by the upper layer before
invoking/ allowing send. It should for the pending TX to complete before
removing the channel I think.

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

I see. So I guess I could add the same check I have in IMX_MU_TYPE_RX to
not unmask the interrupt again.

Sebastian

Re: [PATCH v2 2/9] mailbox: imx: Use devm_pm_runtime_enable()

[Sebastian Andrzej Siewior]

On 2026-06-03 13:27:55 [+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
> [ ... ]
> > @@ -965,28 +965,28 @@ static int imx_mu_probe(struct platform_device *pdev)
> [ ... ]
> >  	ret = pm_runtime_put_sync(dev);
> >  	if (ret < 0)
> > -		goto disable_runtime_pm;
> > +		goto disable_clk;
> >  
> >  	clk_disable_unprepare(priv->clk);
> >  
> > +	ret = devm_mbox_controller_register(dev, &priv->mbox);
> > +	if (ret)
> > +		goto disable_clk;
> 
> [Severity: High]
> Does this error path cause a double clock disable?
> 
> If devm_mbox_controller_register() fails here, the code jumps to the
> disable_clk label. However, clk_disable_unprepare(priv->clk) has already
> been called just above this registration attempt. This means the error path
> will disable the clock a second time, which could cause a reference count
> underflow and potentially disable shared clocks.

Indeed it does.

Sebastian

Re: [PATCH v2 3/9] mailbox: imx: use devm_of_platform_populate()

[Sebastian Andrzej Siewior]

On 2026-06-03 13:37:15 [+0000], sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Unbalanced clk_disable_unprepare() on the error path for devm_mbox_controller_register() causes a double clock disable.

We had this.

Sebastian

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

[Sebastian Andrzej Siewior]

On 2026-06-03 13:58:22 [+0000], sashiko-bot@kernel.org wrote:
> > diff --git a/drivers/mailbox/imx-mailbox.c b/drivers/mailbox/imx-mailbox.c
> > index d1de07cc0ed62..006aa76b74b62 100644
> > --- a/drivers/mailbox/imx-mailbox.c
> > +++ b/drivers/mailbox/imx-mailbox.c
> 
> [ ... ]
> 
> > @@ -629,7 +655,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);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but looking at imx_mu_startup()
> in drivers/mailbox/imx-mailbox.c, the return value of pm_runtime_get_sync()
> is ignored:
> 
> 	pm_runtime_get_sync(priv->dev);
> 	if (cp->type == IMX_MU_TYPE_TXDB_V2)
> 		return 0;
> 
> If the PM domain fails to resume (for example, returning -ENOMEM or
> -ETIMEDOUT), will the function proceed to interact with hardware registers?
> On i.MX platforms, accessing unclocked or unpowered hardware registers
> typically results in a synchronous external abort and a kernel panic.

Yeah, not sure what to do here. If it fails, and we return with an error
the upper layer invokes the shutdown callback wich will free_irq, put pm
and write registers anyway.

Sebastian

Re: [PATCH v2 8/9] mailbox: imx: Don't force-thread the primary handler

[Sebastian Andrzej Siewior]

On 2026-06-03 14:31:08 [+0000], sashiko-bot@kernel.org wrote:
> While reviewing the driver, I noticed a few other potential regressions outside
> the diff hunks above:
> 
> [Severity: High]
> This is a pre-existing issue, but can the timeout loop in imx_mu_generic_tx()
> lead to silent transmission failures?
> 
> When processing IMX_MU_TYPE_TXDB_V2, the code loops up to 10 times waiting
> for the readl_poll_timeout() condition:
> 
> drivers/mailbox/imx-mailbox.c:imx_mu_generic_tx() {
>     ...
>     while (ret && (count < 10)) {
>         ret = readl_poll_timeout(priv->base + priv->dcfg->xCR[IMX_MU_GCR], val,
>                                  !(val & IMX_MU_xCR_GIRn(priv->dcfg->type, cp->idx)),
>                                  0, 10000);
>         if (ret) {
>             dev_warn_ratelimited(...);
>         }
>     }
>     break;
>     ...
> }
> 
> If all 10 attempts time out, ret is left as -ETIMEDOUT and the while loop
> exits. The code then breaks from the switch block and unconditionally hits
> return 0 at the end of the function instead of returning ret.
> 
> Falsely returning success causes the mailbox framework to drop the message,
> leading to silent data loss. Should this return the error code instead?

Guess it wouldn't hurt to forward the error to the caller.

> [Severity: High]
> This is a pre-existing issue, but does the use of time APIs and polling
> with local interrupts disabled lead to hard lockups?
> 
> Mailbox transmission (priv->dcfg->tx) is invoked via mbox_send_message()
> with the channel lock (spin_lock_irqsave) held, meaning local interrupts
> are disabled.
> 
> Inside imx_mu_tx_waiting_write(), the code polls a hardware register while
> checking time_is_after_jiffies64():
> 
> 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));
>     ...
> }
> 
> Because interrupts are disabled, jiffies will not advance on UP systems,
> causing an infinite loop (deadlock).

Interrupts are disabled. Better it does not time out…
It waits IMX_MU_SECO_TX_TOUT which is 3seconds here. I would suggest to
have a udelay(10) or so and then count the loops.

Sebastian

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

[Sebastian Andrzej Siewior]

On 2026-06-03 14:45:58 [+0000], sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing RCU read lock around `idr_for_each()` executing in a preemptible context, leading to potential use-after-free.

No. The channel is removed once the interrupt handler is gone.

> - [High] Error paths in mailbox initialization leave `ERR_PTR` or dangling pointers in the channel pointers, leading to a kernel panic during cleanup.
This, yes, sorry.

Sebastian

Re: [PATCH v2 6/9] mailbox: imx: Move the RX part of the mailbox into the threaded handler

[Sebastian Andrzej Siewior]

On 2026-06-03 16:45:16 [+0300], Daniel Baluta wrote:
> I would prefer to add the explanation for this in at least one commit
> message to be there in git log.
> 
> Cover letter is not kept in git history.

You could merge is part of the series. But I could of course add the
rational behind this to the patch where I start adding the threaded
handler and move the TX bits.

I just finished of with sashiko. Do you have other human input?
Otherwise I would fix the one thing I did wrong, propagate the one error
sashiko mentioned. I'm not sure what to do about the long time outs it
complains about, I would keep them for now. That timeout check based on
jiffies is also odd.

Sebastian