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

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

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(). 
 
> 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
> >>>>>>
> >>>>
> >>
>