[PATCH v4] remoteproc: imx_rproc: Invoke the callback directly

[PATCH v4] 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.

Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v3…v4: https://lore.kernel.org/r/20260617-imx_mbox_rproc-v3-0-77948112defc@linutronix.de
  - The mailbox bits are part of v7.2-rc1. This is just a repost of the
    imx_rproc driver which is left.

v2…v3: https://lore.kernel.org/r/20260603-imx_mbox_rproc-v2-0-a0059dc3b69a@linutronix.de
  - Forward the error in imx_mu_generic_tx() to the caller (new patch
    #1)
  - Extend the patch description a bit for for "Start splitting the IRQ
    handler" to briefly explain why callbacks are moved to the threaded
    handler.
  - Drop imx_mu_con_priv::pending. The primary handler wakes its
    threaded handler. Once the handler is woken, the pending flag must
    be set and there is no need to set/ clear it.
  - Avoid the double clk_disable_unprepare() if
    devm_mbox_controller_register() fails.

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().
---
 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 7662ebd9d2f49..e18ae33a5cf85 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;
@@ -892,21 +889,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)
@@ -1271,13 +1258,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;
@@ -1305,17 +1285,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)

---
base-commit: b3f94b2b3f3e51ab880a51fc6510e1dafba654ed
change-id: 20260529-imx_mbox_rproc-7d512f5a6f78

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

Re: [PATCH v4] 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() and idr_find() in preemptible context.
- [High] Improper devm cleanup order leaves mailbox active during driver teardown, causing Use-After-Free.
--

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

remoteproc: imx_rproc: Invoke the callback directly

This commit removes the workqueue used for callback invocation in the
imx_rproc driver. The callback is now invoked directly from the threaded
IRQ handler since it runs in a preemptible context.

> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index 745ce52cd8226..a1f2b76ec8187 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);
>  
> -	queue_work(priv->workqueue, &priv->rproc_work);
> +	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless iteration
over the notifyids IDR lack RCU protection?

Because imx_rproc_rx_callback() now runs in a preemptible context (threaded
IRQ handler), it iterates over the rproc->notifyids IDR using idr_for_each()
without the rcu_read_lock(). 

The IDR API requires lockless readers to hold rcu_read_lock() to prevent radix
tree nodes from being freed during traversal. If a concurrent modification
like idr_remove() occurs during teardown, the RCU grace period could expire,
leading to a use-after-free on the IDR internal nodes.

>  }
>  
>  static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block)
[ ... ]
> @@ -1273,17 +1253,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 could the devm cleanup order in
imx_rproc_probe() lead to a use-after-free during driver teardown?

Earlier in imx_rproc_probe(), the mailbox cleanup action imx_rproc_free_mbox
is registered via devm_add_action_or_reset() before devm_rproc_add() is
called.

Because devm actions execute in LIFO order, devm_rproc_remove() executes
first, which tears down the remote processor and frees virtio subdevices and
their IDR entries.

This leaves a dangerous race window during driver unbind where the mailbox is
still fully active while the core remoteproc data structures are being torn
down. If a mailbox interrupt fires during this window, the RX handler will
access the notifyids IDR and rvring structures while they are concurrently
being modified or freed by rproc_del().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-imx_mbox_rproc-v4-1-67b10560a861@linutronix.de?part=1

Re: [PATCH v4] remoteproc: imx_rproc: Invoke the callback directly

[Sebastian Andrzej Siewior]

On 2026-07-03 08:45:21 [+0000], sashiko-bot@kernel.org wrote:
> > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> > index 745ce52cd8226..a1f2b76ec8187 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);
> >  
> > -	queue_work(priv->workqueue, &priv->rproc_work);
> > +	idr_for_each(&rproc->notifyids, imx_rproc_notified_idr_cb, rproc);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this lockless iteration
> over the notifyids IDR lack RCU protection?
> 
> Because imx_rproc_rx_callback() now runs in a preemptible context (threaded
> IRQ handler), it iterates over the rproc->notifyids IDR using idr_for_each()
> without the rcu_read_lock(). 
> 
> The IDR API requires lockless readers to hold rcu_read_lock() to prevent radix
> tree nodes from being freed during traversal. If a concurrent modification
> like idr_remove() occurs during teardown, the RCU grace period could expire,
> leading to a use-after-free on the IDR internal nodes.

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.

> >  }
> >  
> >  static int imx_rproc_xtr_mbox_init(struct rproc *rproc, bool tx_block)
> [ ... ]
> > @@ -1273,17 +1253,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 could the devm cleanup order in
> imx_rproc_probe() lead to a use-after-free during driver teardown?
> 
> Earlier in imx_rproc_probe(), the mailbox cleanup action imx_rproc_free_mbox
> is registered via devm_add_action_or_reset() before devm_rproc_add() is
> called.
> 
> Because devm actions execute in LIFO order, devm_rproc_remove() executes
> first, which tears down the remote processor and frees virtio subdevices and
> their IDR entries.
> 
> This leaves a dangerous race window during driver unbind where the mailbox is
> still fully active while the core remoteproc data structures are being torn
> down. If a mailbox interrupt fires during this window, the RX handler will
> access the notifyids IDR and rvring structures while they are concurrently
> being modified or freed by rproc_del().

The notifyids IDR is removed by devm_rproc_free() not by
devm_rproc_remove(). This should remove the channel which removes IRQ.
This looks fine to me.

Sebastian