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

[Mathieu Poirier]

On Fri, 11 Sept 2026 at 12:03, Shah, Tanmay <tanmays@xxxxxxx> 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@xxxxxxx>
> >>>> ---
> >>>>  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.

> 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@xxxxxxx/
>
> Old patch was rejected because we decided to modify the reset mechanism
> instead:
> https://lists.openampproject.org/archives/list/openamp-rp@xxxxxxxxxxxxxxxxxxxxxxxx/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
> >>>>
> >>
>