Re: [PATCH REPOST REPOST v6] remoteproc: imx_rproc: Invoke the callback directly

I'll pick this up when RC1 comes out.

Thanks,
Mathieu

On Thu, 13 Aug 2026 at 02:22, Sebastian Andrzej Siewior
<[email protected]> wrote:
>
> 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 <[email protected]>
> Reviewed-by: Mathieu Poirier <[email protected]>
> Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
> ---
> 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.
>
> v5…v6: https://patch.msgid.link/[email protected]
>   - Repost. Adding Greg and akpm, hoping someone will apply it.
>
> v4…v5: https://lore.kernel.org/all/[email protected]/
>   - Repost
>
> v3…v4: https://lore.kernel.org/r/[email protected]
>   - 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/[email protected]
>   - 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/[email protected]
>   - 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: 2ee859ebf156157609f71060ae472711c8cbc326
> change-id: 20260529-imx_mbox_rproc-7d512f5a6f78
>
> Best regards,
> --
> Sebastian Andrzej Siewior <[email protected]>