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

From: Mathieu Poirier

Date: Thu Sep 17 2026 - 12:44:54 EST

On Wed, 16 Sept 2026 at 09:51, Mathieu Poirier
<mathieu.poirier@xxxxxxxxxx> 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@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.
> > >
> >
> > Hi Mathieu,
> >
> > The previous patch sent in this matter was doing the same:
> > https://lore.kernel.org/linux-remoteproc/20260317201251.3920841-1-tanmay.shah@xxxxxxx/
> >
>
> 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.

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