Re: [RFC PATCH] remoteproc: xlnx: initialize mailbox work before requesting channels

["Shah, Tanmay" <tanmays@amd.com>]

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 <runyu.xiao@seu.edu.cn>
> ---
>  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];
>  		}
>  
>  		/*