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]/
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
>>>>>>
>>>>
>>
