Re: [PATCH] remoteproc: remoteproc_virtio: add acknowledged vdev reset

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

On 9/17/2026 10:45 AM, Mathieu Poirier wrote:
> On Wed, 16 Sept 2026 at 09:51, Mathieu Poirier
> <mathieu.poirier@linaro.org> wrote:
>>
>> On Tue, Sep 15, 2026 at 12:20:34PM -0500, Shah, Tanmay wrote:
>>>
>>>
>>> On 9/14/2026 11:47 AM, Mathieu Poirier wrote:
>>>> On Fri, 11 Sept 2026 at 12:03, Shah, Tanmay <tanmays@amd.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 9/11/2026 9:57 AM, Mathieu Poirier wrote:
>>>>>> On Tue, Sep 08, 2026 at 02:21:53PM -0500, Shah, Tanmay wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> Thank you for the reviews.
>>>>>>>
>>>>>>> On 9/8/2026 1:02 PM, Mathieu Poirier wrote:
>>>>>>>> Good day,
>>>>>>>>
>>>>>>>> On Wed, Sep 02, 2026 at 02:44:54PM -0700, Tanmay Shah wrote:
>>>>>>>>> The existing remoteproc virtio reset path clears the vdev status locally
>>>>>>>>> without notifying the remote processor. As a result, the host cannot tell
>>>>>>>>> whether the remote side has observed the reset request or completed its
>>>>>>>>> cleanup.
>>>>>>>>>
>>>>>>>>> Add a new resource type, RSC_VDEV_V2, for virtio vdevs that support an
>>>>>>>>> acknowledged reset protocol. For these resources, encode a reset request
>>>>>>>>> in the virtio status byte, kick the remote processor using the vdev notify
>>>>>>>>> ID, and wait for the remote side to clear the status back to 0.
>>>>>>>>>
>>>>>>>>> Keep the existing RSC_VDEV behavior for backwards compatibility by
>>>>>>>>> clearing the status locally. Also reset remoteproc-created virtio
>>>>>>>>> devices before unregistering them, and expose RSC_VDEV_V2 reset state
>>>>>>>>> in debugfs.
>>>>>>>>>
>>>>>>>>> Assisted-by: Codex:GPT-5
>>>>>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/remoteproc/remoteproc_core.c     |  3 +-
>>>>>>>>>  drivers/remoteproc/remoteproc_debugfs.c  | 29 +++++++++++++++-
>>>>>>>>>  drivers/remoteproc/remoteproc_internal.h | 21 +++++++++++
>>>>>>>>>  drivers/remoteproc/remoteproc_virtio.c   | 44 ++++++++++++++++++++++--
>>>>>>>>>  include/linux/rsc_table.h                |  5 ++-
>>>>>>>>>  5 files changed, 97 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>>>>>>> index 1ed406714849..31d79684977c 100644
>>>>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>>>>>> @@ -471,6 +471,7 @@ void rproc_remove_rvdev(struct rproc_vdev *rvdev)
>>>>>>>>>  static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>>>>>>>>>                         int offset, int avail)
>>>>>>>>>  {
>>>>>>>>> +  struct fw_rsc_hdr *hdr = ptr - sizeof(*hdr);
>>>>>>>>
>>>>>>>> Spurious change.
>>>>>>>>
>>>>>>>
>>>>>>> Ack will remove it.
>>>>>>>
>>>>>>>>>    struct fw_rsc_vdev *rsc = ptr;
>>>>>>>>>    struct device *dev = &rproc->dev;
>>>>>>>>>    struct rproc_vdev *rvdev;
>>>>>>>>> @@ -485,7 +486,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr,
>>>>>>>>>            return -EINVAL;
>>>>>>>>>    }
>>>>>>>>>
>>>>>>>>> -  /* make sure reserved bytes are zeroes */
>>>>>>>>
>>>>>>>> Same
>>>>>>>
>>>>>>> Ack, will be removed.
>>>>>>>
>>>>>>>>
>>>>>>>>>    if (rsc->reserved[0] || rsc->reserved[1]) {
>>>>>>>>>            dev_err(dev, "vdev rsc has non zero reserved bytes\n");
>>>>>>>>>            return -EINVAL;
>>>>>>>>> @@ -1009,6 +1009,7 @@ static rproc_handle_resource_t rproc_loading_handlers[RSC_LAST] = {
>>>>>>>>>    [RSC_DEVMEM] = rproc_handle_devmem,
>>>>>>>>>    [RSC_TRACE] = rproc_handle_trace,
>>>>>>>>>    [RSC_VDEV] = rproc_handle_vdev,
>>>>>>>>> +  [RSC_VDEV_V2] = rproc_handle_vdev,
>>>>>>>>>  };
>>>>>>>>>
>>>>>>>>>  struct rproc_rsc_cb_data {
>>>>>>>>> diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c
>>>>>>>>> index b86c1d09c70c..1fe99749f5b4 100644
>>>>>>>>> --- a/drivers/remoteproc/remoteproc_debugfs.c
>>>>>>>>> +++ b/drivers/remoteproc/remoteproc_debugfs.c
>>>>>>>>> @@ -274,7 +274,7 @@ static const struct file_operations rproc_crash_ops = {
>>>>>>>>>  /* Expose resource table content via debugfs */
>>>>>>>>>  static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>>>>>>>>>  {
>>>>>>>>> -  static const char * const types[] = {"carveout", "devmem", "trace", "vdev"};
>>>>>>>>> +  static const char * const types[] = {"carveout", "devmem", "trace", "vdev", "vdev_v2"};
>>>>>>>>>    struct rproc *rproc = seq->private;
>>>>>>>>>    struct resource_table *table = rproc->table_ptr;
>>>>>>>>>    struct fw_rsc_carveout *c;
>>>>>>>>> @@ -336,6 +336,33 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p)
>>>>>>>>>                    seq_printf(seq, "  Reserved (should be zero) [%d][%d]\n\n",
>>>>>>>>>                               v->reserved[0], v->reserved[1]);
>>>>>>>>>
>>>>>>>>> +                  for (j = 0; j < v->num_of_vrings; j++) {
>>>>>>>>> +                          seq_printf(seq, "  Vring %d\n", j);
>>>>>>>>> +                          seq_printf(seq, "    Device Address 0x%x\n", v->vring[j].da);
>>>>>>>>> +                          seq_printf(seq, "    Alignment %d\n", v->vring[j].align);
>>>>>>>>> +                          seq_printf(seq, "    Number of buffers %d\n", v->vring[j].num);
>>>>>>>>> +                          seq_printf(seq, "    Notify ID %d\n", v->vring[j].notifyid);
>>>>>>>>> +                          seq_printf(seq, "    Physical Address 0x%x\n\n",
>>>>>>>>> +                                     v->vring[j].pa);
>>>>>>>>> +                  }
>>>>>>>>> +                  break;
>>>>>>>>> +          case RSC_VDEV_V2:
>>>>>>>>> +                  v = rsc;
>>>>>>>>> +                  seq_printf(seq, "Entry %d is of type %s\n", i, types[hdr->type]);
>>>>>>>>> +
>>>>>>>>> +                  seq_printf(seq, "  ID %d\n", v->id);
>>>>>>>>> +                  seq_printf(seq, "  Notify ID %d\n", v->notifyid);
>>>>>>>>> +                  seq_printf(seq, "  Device features 0x%x\n", v->dfeatures);
>>>>>>>>> +                  seq_printf(seq, "  Guest features 0x%x\n", v->gfeatures);
>>>>>>>>> +                  seq_printf(seq, "  Config length 0x%x\n", v->config_len);
>>>>>>>>> +                  seq_printf(seq, "  Status 0x%x\n", v->status);
>>>>>>>>> +                  seq_printf(seq, "  Number of vrings %d\n", v->num_of_vrings);
>>>>>>>>> +                  seq_printf(seq, "  Reset request pending %s\n",
>>>>>>>>> +                             rproc_rsc_vdev_reset_requested(v->status) ?
>>>>>>>>> +                             "yes" : "no");
>>>>>>>>> +                  seq_printf(seq, "  Reserved (should be zero) [%d][%d]\n\n",
>>>>>>>>> +                             v->reserved[0], v->reserved[1]);
>>>>>>>>> +
>>>>>>>>>                    for (j = 0; j < v->num_of_vrings; j++) {
>>>>>>>>>                            seq_printf(seq, "  Vring %d\n", j);
>>>>>>>>>                            seq_printf(seq, "    Device Address 0x%x\n", v->vring[j].da);
>>>>>>>>> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
>>>>>>>>> index 3a742ef6ef60..f07a96ff82a4 100644
>>>>>>>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>>>>>>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>>>>>>>> @@ -14,6 +14,7 @@
>>>>>>>>>
>>>>>>>>>  #include <linux/irqreturn.h>
>>>>>>>>>  #include <linux/firmware.h>
>>>>>>>>> +#include <linux/virtio_config.h>
>>>>>>>>>  #ifdef CONFIG_HAS_IOMEM
>>>>>>>>>  #include <linux/io.h>
>>>>>>>>>  #endif
>>>>>>>>> @@ -42,6 +43,26 @@ struct rproc_vdev_data {
>>>>>>>>>    struct fw_rsc_vdev *rsc;
>>>>>>>>>  };
>>>>>>>>>
>>>>>>>>> +/*
>>>>>>>>> + * RSC_VDEV_V2 requests an acknowledged reset by writing an otherwise
>>>>>>>>> + * impossible virtio status pattern: DRIVER and FAILED set while
>>>>>>>>> + * ACKNOWLEDGE is clear. Other status bits are left unchanged.
>>>>>>>>> + */
>>>>>>>>> +static inline u8 rproc_rsc_vdev_reset_status(u8 status)
>>>>>>>>> +{
>>>>>>>>> +  status |= VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_FAILED;
>>>>>>>>> +  status &= ~VIRTIO_CONFIG_S_ACKNOWLEDGE;
>>>>>>>>> +
>>>>>>>>> +  return status;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +static inline bool rproc_rsc_vdev_reset_requested(u8 status)
>>>>>>>>> +{
>>>>>>>>> +  return !(status & VIRTIO_CONFIG_S_ACKNOWLEDGE) &&
>>>>>>>>> +         (status & VIRTIO_CONFIG_S_DRIVER) &&
>>>>>>>>> +         (status & VIRTIO_CONFIG_S_FAILED);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>  static inline bool rproc_has_feature(struct rproc *rproc, unsigned int feature)
>>>>>>>>>  {
>>>>>>>>>    return test_bit(feature, rproc->features);
>>>>>>>>> diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c
>>>>>>>>> index d5e9ff045a28..e682caa546b2 100644
>>>>>>>>> --- a/drivers/remoteproc/remoteproc_virtio.c
>>>>>>>>> +++ b/drivers/remoteproc/remoteproc_virtio.c
>>>>>>>>> @@ -13,6 +13,7 @@
>>>>>>>>>  #include <linux/dma-map-ops.h>
>>>>>>>>>  #include <linux/dma-mapping.h>
>>>>>>>>>  #include <linux/export.h>
>>>>>>>>> +#include <linux/iopoll.h>
>>>>>>>>>  #include <linux/of_reserved_mem.h>
>>>>>>>>>  #include <linux/platform_device.h>
>>>>>>>>>  #include <linux/remoteproc.h>
>>>>>>>>> @@ -234,12 +235,48 @@ static void rproc_virtio_set_status(struct virtio_device *vdev, u8 status)
>>>>>>>>>  static void rproc_virtio_reset(struct virtio_device *vdev)
>>>>>>>>>  {
>>>>>>>>>    struct rproc_vdev *rvdev = vdev_to_rvdev(vdev);
>>>>>>>>> +  struct rproc *rproc = rvdev->rproc;
>>>>>>>>>    struct fw_rsc_vdev *rsc;
>>>>>>>>> +  struct fw_rsc_hdr *hdr;
>>>>>>>>> +  int ret;
>>>>>>>>> +  u8 val;
>>>>>>>>> +
>>>>>>>>> +  /*
>>>>>>>>> +   * During crash recovery, vdev can be stopped. But the driver can't reset
>>>>>>>>> +   * the device, as device is already crashed. In this case, reset becomes
>>>>>>>>> +   * no op.
>>>>>>>>> +   */
>>>>>>>>> +  if (rproc->state == RPROC_CRASHED)
>>>>>>>>> +          return;
>>>>>>>>>
>>>>>>>>>    rsc = (void *)rvdev->rproc->table_ptr + rvdev->rsc_offset;
>>>>>>>>> +  hdr = (void *)rsc - sizeof(*hdr);
>>>>>>>>> +
>>>>>>>>> +  if (hdr->type == RSC_VDEV_V2) {
>>>>>>>>> +          /*
>>>>>>>>> +           * RSC_VDEV_V2 encodes an acknowledged reset request in the
>>>>>>>>> +           * status byte. The remote is expected to complete the reset
>>>>>>>>> +           * and then clear status back to 0.
>>>>>>>>> +           */
>>>>>>>>> +          rsc->status = rproc_rsc_vdev_reset_status(rsc->status);
>>>>>>>>> +
>>>>>>>>> +          /* after setting reset request, kick the device */
>>>>>>>>> +          rproc->ops->kick(rproc, rsc->notifyid);
>>>>>>>>>
>>>>>>>>> -  rsc->status = 0;
>>>>>>>>> -  dev_dbg(&vdev->dev, "reset !\n");
>>>>>>>>> +          /*
>>>>>>>>> +           * When device completes reset, it is expected to set status
>>>>>>>>> +           * to 0.
>>>>>>>>> +           */
>>>>>>>>> +          ret = readb_poll_timeout(&rsc->status, val, val == 0,
>>>>>>>>> +                                   1000,      /* 1ms between reads */
>>>>>>>>> +                                   3000000);  /* 3s total timeout */
>>>>>>>>> +          if (ret)
>>>>>>>>> +                  dev_warn(&vdev->dev, "vdev reset timed out\n");
>>>>>>>>
>>>>>>>> The problem here is that we are introducing behavior that is not compliant with
>>>>>>>> the virtio specifications.  One way to acheive the same behavior could be for
>>>>>>>> the remote processor to check rsc->status before sending a interrupt of using
>>>>>>>> the virtqueues.
>>>>>>>>
>>>>>>>
>>>>>>> That is what remote is supposed to do. But what if remote do not
>>>>>>> respond? If remote is deadlocked for some reason, then the Linux will
>>>>>>> hang at this point too. That is why we need some kind of timeout.
>>>>>>
>>>>>> If the remote is dead then a watchdog timer should fire at some point.
>>>>>> Moreover, that situation won't be different from other circumstances where a
>>>>>> remote processor locks up.
>>>>>>
>>>>>
>>>>> There are few concerns:
>>>>>
>>>>> 1) Heterogeneous system where Linux is handling many remotes, the
>>>>> watchdog might not be available to all the remotes or watchdog mechanism
>>>>> is not implemented at all on the remote side.
>>>>
>>>> If a watchdog is not available adding a timeout upon resetting
>>>> rsc-status won't help.
>>>>
>>>>>
>>>>> 2) Let's say watchdog is configured for 10s, or so then for that long
>>>>> Linux will be stuck too. I am trying to avoid this case where Linux gets
>>>>> stuck for long time.
>>>>
>>>> Same resoning as above - if the remote processor dies and a watchdog
>>>> timeout is set for 10 seconds, adding a shorter timeout when
>>>> rsc->status is modified will do very little.
>>>>
>>>>>> Looking at your patch, sending a kick() won't do anything for a dead remote
>>>>>> processor.  If the remote processor is alive, it should monitor rsc->status and
>>>>>> take action when it is set to '0' by the host.  If it is locked-up, the normal
>>>>>> lockup procedure should apply.
>>>>>>
>>>>>
>>>>> Notifying virtio device on the status change is standard virtio
>>>>> mechanism. In the virtio statck it's done via virtqueue_notify so I am
>>>>> trying to do the same. It also helps remote to avoid polling on status.
>>>>>
>>>>
>>>> Can you point me to that code?  Having the same mental picture will help.
>>>>
>>>>>> I'm not sure what problem this patch is trying to address.
>>>>>>
>>>>>
>>>>> Some platforms allow Linux and Remote boot independently.
>>>>>
>>>>> Let's say Linux reboots without reseting the remote then during next
>>>>> boot Linux will find virtio status is not in the reset state.
>>>>>
>>>>
>>>> That should be handled via the attach()/detach() state machine.
>>>>
>>>>> In such case, linux need to issue virtio device reset, and wait until
>>>>> RPU completes the reset and start the device again. The virtio framework
>>>>> already issues the reset during boot here:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/virtio/virtio.c?h=for-next#n570
>>>>>
>>>>> However, the virtio_reset implementation for remoteproc_virtio simply
>>>>> set the status to 0, and doesn't wait for the remote to complete the
>>>>> reset. Due to this, attach operation becomes successfull, but the rpmsg
>>>>> channels are not created on the linux side.
>>>>>
>>>>
>>>> I think this situation should be handled in driver code rather than
>>>> the remoteproc framework.  We can consider adding this to the
>>>> remoteproc framework if/when several platforms implement the same
>>>> logic.  Otherwise I fear we'll bloat the framework with something that
>>>> isn't generic.
>>>>
>>>
>>> Hi Mathieu,
>>>
>>> The previous patch sent in this matter was doing the same:
>>> https://lore.kernel.org/linux-remoteproc/20260317201251.3920841-1-tanmay.shah@amd.com/
>>>
>>
>> I think this is a much better approach.  That said, I would really like to see
>> something like wait_for_completion_timeout() being used rather than
>> usleep_range().
>>
> 
> Thinking back on this, function wait_event_timeout() would be a better choice.
> 

Hi Mathieu,

Thanks for the suggestion. I looked into it, and I think it can work. I
will implement this suggestion, and will send new patch.

Tanmay

>>> If you are okay, can I resend it ? I think if that is accepted then we
>>> don't need this patch atleast for now.
>>>
>>> Thank You,
>>> Tanmay
>>>
>>>>> This patch solves this issue. It changes the reset mechanism while
>>>>> maintaining the backward compatibility for old way of reseting the device.
>>>>>
>>>>> I had sent a different patch regarding this before:
>>>>> https://lore.kernel.org/linux-remoteproc/20260317201251.3920841-1-tanmay.shah@amd.com/
>>>>>
>>>>> Old patch was rejected because we decided to modify the reset mechanism
>>>>> instead:
>>>>> https://lists.openampproject.org/archives/list/openamp-rp@lists.openampproject.org/thread/DDIFUMGQQ2R7CQZJHK7EB6UDO3ISAAVU/
>>>>>
>>>>> Thank You,
>>>>> Tanmay
>>>>>
>>>>>
>>>>>>>
>>>>>>> I think timeout mechanism is better for AMP systems over waiting forever
>>>>>>> for remote to clear the status.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Tanmay
>>>>>>>
>>>>>>>
>>>>>>>>> +  } else {
>>>>>>>>> +          /* back compatible for RSC_VDEV type of rsc vdev */
>>>>>>>>> +          rsc->status = 0;
>>>>>>>>> +  }
>>>>>>>>> +  dev_info(&vdev->dev, "reset !\n");
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>>  /* provide the vdev features as retrieved from the firmware */
>>>>>>>>> @@ -469,6 +506,9 @@ static int rproc_remove_virtio_dev(struct device *dev, void *data)
>>>>>>>>>  {
>>>>>>>>>    struct virtio_device *vdev = dev_to_virtio(dev);
>>>>>>>>>
>>>>>>>>> +  /* reset virtio device before unregister */
>>>>>>>>> +  virtio_reset_device(vdev);
>>>>>>>>> +
>>>>>>>>
>>>>>>>> Regardless of this feature, I think it is wise to reset the device before
>>>>>>>> unregistering with the virtio subsystem.
>>>>>>>>
>>>>>>>
>>>>>>> Agreed. I intend to keep this.
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Mathieu
>>>>>>>>
>>>>>>>>>    unregister_virtio_device(vdev);
>>>>>>>>>    return 0;
>>>>>>>>>  }
>>>>>>>>> diff --git a/include/linux/rsc_table.h b/include/linux/rsc_table.h
>>>>>>>>> index 71b60125310e..2398a6d7033e 100644
>>>>>>>>> --- a/include/linux/rsc_table.h
>>>>>>>>> +++ b/include/linux/rsc_table.h
>>>>>>>>> @@ -66,6 +66,8 @@ struct fw_rsc_hdr {
>>>>>>>>>   *                    the remote processor will be writing logs.
>>>>>>>>>   * @RSC_VDEV:       declare support for a virtio device, and serve as its
>>>>>>>>>   *                    virtio header.
>>>>>>>>> + * @RSC_VDEV_V2:    declare support for a virtio device whose reset request is
>>>>>>>>> + *                    encoded in the virtio status byte.
>>>>>>>>>   * @RSC_LAST:       just keep this one at the end of standard resources
>>>>>>>>>   * @RSC_VENDOR_START:     start of the vendor specific resource types range
>>>>>>>>>   * @RSC_VENDOR_END:       end of the vendor specific resource types range
>>>>>>>>> @@ -83,7 +85,8 @@ enum fw_resource_type {
>>>>>>>>>    RSC_DEVMEM              = 1,
>>>>>>>>>    RSC_TRACE               = 2,
>>>>>>>>>    RSC_VDEV                = 3,
>>>>>>>>> -  RSC_LAST                = 4,
>>>>>>>>> +  RSC_VDEV_V2             = 4,
>>>>>>>>> +  RSC_LAST                = 5,
>>>>>>>>>    RSC_VENDOR_START        = 128,
>>>>>>>>>    RSC_VENDOR_END          = 512,
>>>>>>>>>  };
>>>>>>>>>
>>>>>>>>> base-commit: d4d61a4b0a52e8f3cdb3e1578602850a3452ec3e
>>>>>>>>> --
>>>>>>>>> 2.43.0
>>>>>>>>>
>>>>>>>
>>>>>
>>>