Hello! On 14/09/26 22:17, Mathieu Poirier wrote: > 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
I don't think virtio stack issues a notify on any status change[0][1], it's the virtio_rpmsg_bus that notifies[2] while starting up the remoteproc. [0]: https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/virtio/virtio.c?h=for-next#n573 [1]: https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/virtio/virtio.c?h=for-next#n280 [2]: https://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git/tree/drivers/rpmsg/virtio_rpmsg_bus.c?h=for-next#n1000 >> so I am >> trying to do the same. I see the point in trying to keep operations symmetric (on start and stop), but I have to ask: Why just virtio? Shouldn't all the layers notify of their respective teardown? (rpmsg channels, remoteproc platform teardown?) >> 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. I agree. > 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. If and when we come back to this, I'd maybe try to make this teardown notification uniform across the layers and fix the existing implementation than introducing a new resource type. Thanks, Beleswar > >> 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 >>>>>>
