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

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

Hello,

Please see my reply below.

On 10/2/2026 11:34 AM, Mathieu Poirier wrote:
> On Thu, 1 Oct 2026 at 10:51, Shah, Tanmay <tanmays@amd.com> wrote:
>>
>>
>>
>> On 9/30/2026 11:27 AM, Mathieu Poirier wrote:
>>> 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.
>>>
>>
>> Ack, I will remove it.
>>
>>>> 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.
>>>
>>
>> Ack.
>>
>>>>  /*
>>>>   * 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?
>>>
>>
>> Recently, INIT_WORK was moved before
>> mbox_request_channel_byname(mbox_cl, "rx"), In this case, if interrupt
>> occurs between requesting "rx" channel, and assigning r5_cores to ipi,
>> then r5_core can 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?
>>>
>>
>> I think this was added by AI agent, and I think it is to maintain atomic
>> nature of variable access. But if you prefer to protect these variables
>> via locks I will do that. These variables are shared between attach()
>> context and IPI interrupt, so I think they should be protected somehow.
>>
> 
> READ_ONCE/WRITE_ONCE don't guard against concurrency, only against
> access reordering.
> 

Ack, I will remove it.

>>>> +            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.
>>>
>>
>> Ack. Probably need to set these flags after kick(). I will do that.
>>
>>>>
>>>>      /* 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.
>>>
>>
>> Ack. I will remove and introduce locks if that plan works.
>>
>>>> +
>>>> +    /* kick remote to notify about attach */
>>>> +    rproc->ops->kick(rproc, 0);
>>>
>>> Will older FW be able to deal with this properly?
>>>
> 
> This question hasn't been answered.
> 

I missed to reply.

Short answer is, the old firmware will have to be updated to support
this use case.

The old xlnx firmware isn't designed to handle this case where, the
Linux is expected to get reboot in the middle of the RPMsg
communication. It is a new use case. The current use case is limited to
reboot both cores (Cortex-A and Cortex-R) in sync i.e. if Linux sees
reboot so does the remote too. So, old firmware will never hit this use
case. Once this patch gets merged, I will modify the firmware to handle
this reset case as well.

The worst case, if someone still end-up using old firmware with new
kernel, and hit this case then, firmware will simply fail. Because the
firmware doesn't expect the Linux to reboot at all. It is always in sync
of attach() -> detach() -> re-attach() for the old firmware.

>>>> +
>>>> +    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.
>>>
>>
>> So this whole feature will be helpful only if linux gets rebooted
>> without proper cleanup. If driver is removed, it will call detach() via
>> zynqmp_cluster_exit(). If machine goes throug 'reboot' command, then the
>> driver has 'shutdown' callback registered which will call 'detach()'
>> operation. In these cases the remoteproc reset is guranteed and the
>> status will be 0 on next boot.
>>
>> But if linux didn't get chance to execute above callbacks, only then the
>> status will be left non-zero on reboot.
>>
> 
> What if you insmod the driver after an unexpected reboot and then
> rmmod the driver while waiting for the remote processor to come back
> up?
> 

I guess in this case, I will have to leave status 0, and cleanup the
waitqueue that is rproc::attach() is waiting on. Since on rmmod, we will
execute the zynqmp_r5_cluster_exit(), I will release the wq there,
before calling rproc_del().

Thanks,
Tanmay

>>> 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
>>>>
>>