On Fri, 11 Sept 2026 at 12:03, Shah, Tanmay <[email protected]> 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 <[email protected]> > >>>> --- > >>>> 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/[email protected]/ > > Old patch was rejected because we decided to modify the reset mechanism > instead: > https://lists.openampproject.org/archives/list/[email protected]/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 > >>>> > >> >
