Re: [PATCH] remoteproc: xlnx: reset virtio status during attach

[Mathieu Poirier <mathieu.poirier@linaro.org>]

Hi,

On Thu, Sep 24, 2026 at 01:34:09PM -0700, Tanmay Shah wrote:
> On AMD-Xilinx platforms cortex-A and cortex-R can be configured as
> separate subsystems. In this case, both cores can boot independent of
> each other. This is platform management firmware configuration to manage

I'm not sure to understand what the above sentence adds to the changelog.  I
suggest either reworking or removing.

> cores. In such a configuration, if Linux went through an uncontrolled
> reboot during active rpmsg communication, then during next boot it can
> find rpmsg virtio status not in the reset state. In such case it is
> important to reset the virtio status during attach callback and wait
> for the remote to handle virtio device reset. After reset, the remote
> is expected to generate the notification to the host or the host will
> eventually timeout and continue the normal boot flow.
> 
> Assisted-by: LLM
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
>  drivers/remoteproc/xlnx_r5_remoteproc.c | 74 +++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index 630621288430..6e7e2a5ea83c 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> @@ -6,6 +6,7 @@
>  
>  #include <linux/dma-mapping.h>
>  #include <linux/firmware/xlnx-zynqmp.h>
> +#include <linux/jiffies.h>
>  #include <linux/kernel.h>
>  #include <linux/mailbox_client.h>
>  #include <linux/mailbox/zynqmp-ipi-message.h>
> @@ -15,6 +16,7 @@
>  #include <linux/of_reserved_mem.h>
>  #include <linux/platform_device.h>
>  #include <linux/remoteproc.h>
> +#include <linux/wait.h>
>  
>  #include "remoteproc_internal.h"
>  
> @@ -33,6 +35,8 @@
>  #define RSC_TBL_XLNX_MAGIC	((uint32_t)'x' << 24 | (uint32_t)'a' << 16 | \
>  				 (uint32_t)'m' << 8 | (uint32_t)'p')
>  
> +#define RPROC_ATTACH_TIMEOUT_US (1000 * 1000)
> +

Please see if you can use a kernel defined time constant instead of minting your
own.

>  /*
>   * settings for RPU cluster mode which
>   * reflects possible values of xlnx,cluster-mode dt-property
> @@ -167,6 +171,9 @@ struct xlnx_rproc_crash_report {
>   * @rsc_tbl_size: resource table size retrieved from remote
>   * @pm_domain_id: RPU CPU power domain id
>   * @ipi: pointer to mailbox information
> + * @attach_wq: wait queue for attach-time vdev reset acknowledgment

I don't understand the explanation for @attach_wq - please rework.

> + * @waiting_for_attach_ack: whether attach is waiting for remote interrupt
> + * @attach_ack: remote interrupt observed while attach wait is active
>   */
>  struct zynqmp_r5_core {
>  	struct xlnx_rproc_crash_report *crash_report;
> @@ -181,6 +188,9 @@ struct zynqmp_r5_core {
>  	u32 rsc_tbl_size;
>  	u32 pm_domain_id;
>  	struct mbox_info *ipi;
> +	wait_queue_head_t attach_wq;
> +	bool waiting_for_attach_ack;
> +	bool attach_ack;
>  };
>  
>  /**
> @@ -270,10 +280,17 @@ static void handle_event_notified(struct work_struct *work)
>  static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>  {
>  	struct zynqmp_ipi_message *ipi_msg, *buf_msg;
> +	struct zynqmp_r5_core *r5_core;
>  	struct mbox_info *ipi;
>  	size_t len;
>  
>  	ipi = container_of(cl, struct mbox_info, mbox_cl);
> +	r5_core = ipi->r5_core;

Is there really a chance that ipi->r5_core be NULL?

> +
> +	if (r5_core && READ_ONCE(r5_core->waiting_for_attach_ack)) {
> +		WRITE_ONCE(r5_core->attach_ack, true);

Why use READ_ONCE/WRITE_ONCE here - what does it give you?

> +		wake_up(&r5_core->attach_wq);
> +	}

If @rsc->status has been set to 0 in zynqmp_r5_attach() and an IPI is received
before ->kick(), the core may erroneously think the remote processor is
acknowleging the reset.

>  
>  	/* copy data from ipi buffer to r5_core if IPI is buffered. */
>  	ipi_msg = (struct zynqmp_ipi_message *)msg;
> @@ -820,6 +837,62 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
>  
>  static int zynqmp_r5_attach(struct rproc *rproc)
>  {
> +	struct zynqmp_r5_core *r5_core = rproc->priv;
> +	struct device *dev = &rproc->dev;
> +	bool wait_for_remote = false;
> +	struct fw_rsc_vdev *rsc;
> +	struct fw_rsc_hdr *hdr;
> +	int i, offset, avail;
> +	long time_left;
> +
> +	if (!rproc->table_ptr)
> +		goto attach_success;
> +
> +	for (i = 0; i < rproc->table_ptr->num; i++) {
> +		offset = rproc->table_ptr->offset[i];
> +		hdr = (void *)rproc->table_ptr + offset;
> +		avail = rproc->table_sz - offset - sizeof(*hdr);
> +		rsc = (void *)hdr + sizeof(*hdr);
> +
> +		/* make sure table isn't truncated */
> +		if (avail < 0) {
> +			dev_err(dev, "rsc table is truncated\n");
> +			return -EINVAL;
> +		}
> +
> +		if (hdr->type != RSC_VDEV)
> +			continue;
> +
> +		/*
> +		 * reset vdev status, in case previous run didn't leave it in
> +		 * a clean state.
> +		 */
> +		if (rsc->status) {
> +			rsc->status = 0;
> +			wait_for_remote = true;
> +			break;
> +		}
> +	}
> +
> +	if (wait_for_remote) {
> +		WRITE_ONCE(r5_core->attach_ack, false);
> +		WRITE_ONCE(r5_core->waiting_for_attach_ack, true);
> +	}

Again, I would like to understand the motivation behind using WRITE_ONCE()
here...  I just don't see what kind of re-ordering issue you need to guard
against.

> +
> +	/* kick remote to notify about attach */
> +	rproc->ops->kick(rproc, 0);

Will older FW be able to deal with this properly?

> +
> +	if (wait_for_remote) {
> +		time_left = wait_event_timeout(r5_core->attach_wq,
> +					       READ_ONCE(r5_core->attach_ack),
> +					       usecs_to_jiffies(RPROC_ATTACH_TIMEOUT_US));

The condition where the driver is removed or the remoteproc shut down needs also
needs to be handled as a break out condition.

Thanks,
Mathieu

> +		WRITE_ONCE(r5_core->waiting_for_attach_ack, false);
> +
> +		if (!time_left)
> +			dev_warn(dev, "timeout waiting for remote vdev reset ack\n");
> +	}
> +
> +attach_success:
>  	dev_dbg(&rproc->dev, "rproc %d attached\n", rproc->index);
>  
>  	return 0;
> @@ -920,6 +993,7 @@ static struct zynqmp_r5_core *zynqmp_r5_alloc_rproc_core(struct device *cdev)
>  	r5_core = r5_rproc->priv;
>  	r5_core->dev = cdev;
>  	r5_core->np = dev_of_node(cdev);
> +	init_waitqueue_head(&r5_core->attach_wq);
>  	if (!r5_core->np) {
>  		dev_err(cdev, "can't get device node for r5 core\n");
>  		ret = -EINVAL;
> 
> base-commit: 5f639b3018c0026a5341949724b4b921cf3a3d5d
> -- 
> 2.43.0
>