Hello,
Thank you for your patch. Please find my comments below:
On 6/19/2026 2:48 AM, Runyu Xiao wrote:
> zynqmp_r5_setup_mbox() installs zynqmp_r5_mb_rx_cb() as the mailbox RX
> callback before requesting the mailbox channels, but initializes
> ipi->mbox_work only after both channels have been requested. Once the RX
> channel is active, a notification delivered before the late INIT_WORK()
> would make the callback queue an uninitialized work item.
>
> Initialize the work item and its owning R5 core pointer before requesting
> channels. Also drain the work before freeing the mailbox state, after the
> channels have been released so no new callbacks can queue it.
>
> This issue was found by our static analysis tool and then confirmed by
> manual review of the mailbox setup sequence. The callback is published
> before the channel requests complete, so the work item and the state used
> by the work handler should be ready before the mailbox provider can invoke
> it.
>
> A QEMU PoC modeled a mailbox notification delivered after the RX callback
> became reachable but before the delayed INIT_WORK(). DEBUG_OBJECTS reported
> queueing an uninitialized work item from the zynqmp_r5_setup_mbox() path.
>
> This is sent as an RFC because the practical trigger depends on the ZynqMP
> IPI mailbox provider and firmware delivery timing. If the provider cannot
> invoke the RX callback until after setup returns, this is a defensive
> lifecycle cleanup rather than a reachable race on current systems.
>
> Fixes: 5dfb28c257b7 ("remoteproc: xilinx: Add mailbox channels for rpmsg")
> Signed-off-by: Runyu Xiao <[email protected]>
> ---
> drivers/remoteproc/xlnx_r5_remoteproc.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c
> b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index 0b7b173d0d26..1477fc542afb 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> @@ -260,8 +260,9 @@ static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl,
> void *msg)
> * Function to setup mailboxes related properties
> * return : NULL if failed else pointer to mbox_info
> */
> -static struct mbox_info *zynqmp_r5_setup_mbox(struct device *cdev)
> +static struct mbox_info *zynqmp_r5_setup_mbox(struct zynqmp_r5_core *r5_core)
Why this is needed in this patch?
I have another patch that I will post which moves mabilbox setup
operation to rproc::ops::prepare(). I think the interface should be
changed in that patch.
> {
> + struct device *cdev = r5_core->dev;
> struct mbox_client *mbox_cl;
> struct mbox_info *ipi;
>
> @@ -269,6 +270,9 @@ static struct mbox_info *zynqmp_r5_setup_mbox(struct
> device *cdev)
> if (!ipi)
> return NULL;
>
> + ipi->r5_core = r5_core;
> + INIT_WORK(&ipi->mbox_work, handle_event_notified);
> +
I agree with this part. IMO This patch can simply move INIT_WORK before
requesting channels and cancel_work() in the zynqmp_r5_free_mbox().
Thank You,
Tanmay
> mbox_cl = &ipi->mbox_cl;
> mbox_cl->rx_callback = zynqmp_r5_mb_rx_cb;
> mbox_cl->tx_block = false;
> @@ -295,8 +299,6 @@ static struct mbox_info *zynqmp_r5_setup_mbox(struct
> device *cdev)
> return NULL;
> }
>
> - INIT_WORK(&ipi->mbox_work, handle_event_notified);
> -
> return ipi;
> }
>
> @@ -315,6 +317,8 @@ static void zynqmp_r5_free_mbox(struct mbox_info *ipi)
> ipi->rx_chan = NULL;
> }
>
> + cancel_work_sync(&ipi->mbox_work);
> +
> kfree(ipi);
> }
>
> @@ -1388,10 +1392,9 @@ static int zynqmp_r5_cluster_init(struct
> zynqmp_r5_cluster *cluster)
> * If mailbox nodes are disabled using "status" property then
> * setting up mailbox channels will fail.
> */
> - ipi = zynqmp_r5_setup_mbox(&child_pdev->dev);
> + ipi = zynqmp_r5_setup_mbox(r5_cores[i]);
> if (ipi) {
> r5_cores[i]->ipi = ipi;
> - ipi->r5_core = r5_cores[i];
> }
>
> /*
