On 9/17/2026 7:19 AM, Beleswar Prasad Padhi wrote:
> 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.
>
HI Beleswar,
Thanks for the reviews.
I will implement the solution within the attach() callback of the driver
as discussed in the other patch.
>> 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.
>
For now I will not address this as this is out of scope for me.
Thanks,
Tanmay
> 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
>>>>>>>
