On 9/17/2026 10:45 AM, Mathieu Poirier wrote:
> On Wed, 16 Sept 2026 at 09:51, Mathieu Poirier
> <[email protected]> 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 <[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.
>>>>
>>>
>>> Hi Mathieu,
>>>
>>> The previous patch sent in this matter was doing the same:
>>> https://lore.kernel.org/linux-remoteproc/[email protected]/
>>>
>>
>> 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.
>
Hi Mathieu,
Thanks for the suggestion. I looked into it, and I think it can work. I
will implement this suggestion, and will send new patch.
Tanmay
>>> 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/[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
>>>>>>>>>
>>>>>>>
>>>>>
>>>
