On Wed, 2026-09-02 at 05:30 -0400, Michael S. Tsirkin wrote: > On Tue, Sep 01, 2026 at 10:07:41PM -0400, Radu Rendec wrote: > > Under certain conditions(*) vp_modern_get_status() can always return > > 0xff. This has a ripple effect that ultimately makes the system fail to > > boot if the drivers are built-in and the virtio devices are probed > > during boot: > > * vp_reset() for the virtio-net-pci device spins forever in the loop > > around vp_modern_get_status(mdev), which always returns 0xff. > > * vp_reset() is called in the context of the probe() function for the > > virtio-net-pci device, so probe() never returns. > > * the init task gets stuck in wait_for_device_probe() before it > > executes the real init binary. > > > > Add a timeout to the reset function and make the device probing fail if > > the reset function times out. > > Generally, I like using gdb stub in qemu to debug kernel
That's what I did too. It took a while, because init and the kthread that runs probe() are different tasks, and it's not obvious why init doesn't run while the kernel is still alive. It would've been *much* faster/easier if the probe() function complained instead of silently waiting indefinitely. > and anything that will make things time out and fail > is unwelcome. So a probe() that never returns and makes the system fail to fully boot is preferable? That's... surprising. IMHO it's also not what most probe() functions do when there is something wrong with the device. > > (*) This is not a theoretical problem. It happens on imx8mp-evk under > > Qemu due to a relatively recent change to the dw_pcie driver that's > > incompatible with the dw_pcie emulation in Qemu. I reported that bug > > separately as a follow up to the patch that introduced it: > > https://lore.kernel.org/all/[email protected]/ > > So presumably it will be fixed in qemu? Or find a different way, > e.g. poke at device/vendor id of virtio. or add a workaround > in the driver if you prefer. Yes, one of those (probably qemu). But this patch is not about fixing it. In fact, it doesn't even fix it. It just aims to make life a bit easier for the next poor soul who runs into a similar problem. > > Signed-off-by: Radu Rendec <[email protected]> > > --- > > drivers/remoteproc/remoteproc_virtio.c | 4 +++- > > drivers/virtio/virtio.c | 8 +++++--- > > drivers/virtio/virtio_mmio.c | 5 ++++- > > drivers/virtio/virtio_pci_legacy.c | 4 +++- > > drivers/virtio/virtio_pci_modern.c | 15 ++++++++++++--- > > include/linux/virtio.h | 2 +- > > include/linux/virtio_config.h | 2 +- > > 7 files changed, 29 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/remoteproc/remoteproc_virtio.c > > b/drivers/remoteproc/remoteproc_virtio.c > > index d5e9ff045a28..d103b80b1faf 100644 > > --- a/drivers/remoteproc/remoteproc_virtio.c > > +++ b/drivers/remoteproc/remoteproc_virtio.c > > @@ -231,7 +231,7 @@ static void rproc_virtio_set_status(struct > > virtio_device *vdev, u8 status) > > dev_dbg(&vdev->dev, "status: %d\n", status); > > } > > > > -static void rproc_virtio_reset(struct virtio_device *vdev) > > +static int rproc_virtio_reset(struct virtio_device *vdev) > > { > > struct rproc_vdev *rvdev = vdev_to_rvdev(vdev); > > struct fw_rsc_vdev *rsc; > > @@ -240,6 +240,8 @@ static void rproc_virtio_reset(struct virtio_device > > *vdev) > > > > rsc->status = 0; > > dev_dbg(&vdev->dev, "reset !\n"); > > + > > + return 0; > > } > > > > /* provide the vdev features as retrieved from the firmware */ > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > > index 75bb4ffe3b87..9542e4f7cb31 100644 > > --- a/drivers/virtio/virtio.c > > +++ b/drivers/virtio/virtio.c > > @@ -250,7 +250,7 @@ static int virtio_features_ok(struct virtio_device *dev) > > * call/workqueue/bh. Invoking virtio_break_device then flushing any such > > * contexts is one way to handle that. > > * */ > > -void virtio_reset_device(struct virtio_device *dev) > > +int virtio_reset_device(struct virtio_device *dev) > > { > > #ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION > > /* > > @@ -263,7 +263,7 @@ void virtio_reset_device(struct virtio_device *dev) > > virtio_synchronize_cbs(dev); > > #endif > > > > - dev->config->reset(dev); > > + return dev->config->reset(dev); > > } > > EXPORT_SYMBOL_GPL(virtio_reset_device); > > > > @@ -567,7 +567,9 @@ int register_virtio_device(struct virtio_device *dev) > > > > /* We always start by resetting the device, in case a previous > > * driver messed it up. This also tests that code path a little. */ > > - virtio_reset_device(dev); > > + err = virtio_reset_device(dev); > > + if (err) > > + goto out_of_node_put; > > > > /* Acknowledge that we've seen the device. */ > > virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); > > diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c > > index 316f03b97356..537d38841331 100644 > > --- a/drivers/virtio/virtio_mmio.c > > +++ b/drivers/virtio/virtio_mmio.c > > @@ -249,7 +249,7 @@ static void vm_set_status(struct virtio_device *vdev, > > u8 status) > > writel(status, vm_dev->base + VIRTIO_MMIO_STATUS); > > } > > > > -static void vm_reset(struct virtio_device *vdev) > > +static int vm_reset(struct virtio_device *vdev) > > { > > struct virtio_mmio_device *vm_dev = to_virtio_mmio_device(vdev); > > > > @@ -260,7 +260,10 @@ static void vm_reset(struct virtio_device *vdev) > > /* Wait for reset to complete. */ > > while (vm_get_status(vdev)) > > fsleep(1000); > > + /* TODO: add timeout */ > > } > > + > > + return 0; > > } > > > > > > diff --git a/drivers/virtio/virtio_pci_legacy.c > > b/drivers/virtio/virtio_pci_legacy.c > > index d9cbb02b35a1..2882756c7084 100644 > > --- a/drivers/virtio/virtio_pci_legacy.c > > +++ b/drivers/virtio/virtio_pci_legacy.c > > @@ -90,7 +90,7 @@ static void vp_set_status(struct virtio_device *vdev, u8 > > status) > > vp_legacy_set_status(&vp_dev->ldev, status); > > } > > > > -static void vp_reset(struct virtio_device *vdev) > > +static int vp_reset(struct virtio_device *vdev) > > { > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > /* 0 status means a reset. */ > > @@ -100,6 +100,8 @@ static void vp_reset(struct virtio_device *vdev) > > vp_legacy_get_status(&vp_dev->ldev); > > /* Flush pending VQ/configuration callbacks. */ > > vp_synchronize_vectors(vdev); > > + > > + return 0; > > } > > > > static u16 vp_config_vector(struct virtio_pci_device *vp_dev, u16 vector) > > diff --git a/drivers/virtio/virtio_pci_modern.c > > b/drivers/virtio/virtio_pci_modern.c > > index 6d8ae2a6a8ca..4a6906a18cfa 100644 > > --- a/drivers/virtio/virtio_pci_modern.c > > +++ b/drivers/virtio/virtio_pci_modern.c > > @@ -21,6 +21,7 @@ > > #include "virtio_pci_common.h" > > > > #define VIRTIO_AVQ_SGS_MAX 4 > > +#define VIRTIO_PCI_RESET_TIMEOUT_MS 1000 > > > > static void vp_get_features(struct virtio_device *vdev, u64 *features) > > { > > @@ -543,10 +544,11 @@ static void vp_set_status(struct virtio_device *vdev, > > u8 status) > > vp_modern_avq_activate(vdev); > > } > > > > -static void vp_reset(struct virtio_device *vdev) > > +static int vp_reset(struct virtio_device *vdev) > > { > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > struct virtio_pci_modern_device *mdev = &vp_dev->mdev; > > + int timeout = VIRTIO_PCI_RESET_TIMEOUT_MS; > > > > /* 0 status means a reset. */ > > vp_modern_set_status(mdev, 0); > > @@ -555,13 +557,20 @@ static void vp_reset(struct virtio_device *vdev) > > * This will flush out the status write, and flush in device writes, > > * including MSI-X interrupts, if any. > > */ > > - while (vp_modern_get_status(mdev)) > > - msleep(1); > > + while (vp_modern_get_status(mdev)) { > > + if (!timeout--) { > > + dev_warn(&vdev->dev, "reset timeout"); > > + return -ETIMEDOUT; > > + } > > + msleep(1000); > > > 1 second is a long time. normally devices reset immediately. Sorry, the msleep() should have been unchanged. It was something I did for debugging and forgot to change back. The intention was to use the existing 1ms sleep and spin at most VIRTIO_PCI_RESET_TIMEOUT_MS times. This is what happens when you send patches at 10pm :) > > + } > > > > vp_modern_avq_cleanup(vdev); > > > > /* Flush pending VQ/configuration callbacks. */ > > vp_synchronize_vectors(vdev); > > + > > + return 0; > > } > > > > static int vp_active_vq(struct virtqueue *vq, u16 msix_vec) > > > So now if the device does not reset, during cleanup we > previously blocked and now it will corrupt memory. That was not the intention, obviously. The patch as it is also adds a TODO comment to vm_reset() (instead of adding a max count), which is clearly unacceptable for a real patch. This is a RFC patch, and my intention was essentially to ask if something like this is desirable/acceptable. In all honesty, I didn't even test it (other than make sure it compiles). I intentionally didn't spend too much time on it because one of the two possible answers to my question could be "no", and then that time would be wasted time. If that changes anything and a polished and properly tested version of the patch makes sense, I can work on that. If not, that's fine too - at least now I know what to look for if I see a system hang during boot and virtio is used. In any case, thanks for looking into this and for the feedback! > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > > index f923e42cfd01..568e1bd95e4c 100644 > > --- a/include/linux/virtio.h > > +++ b/include/linux/virtio.h > > @@ -212,7 +212,7 @@ void virtio_config_driver_enable(struct virtio_device > > *dev); > > int virtio_device_freeze(struct virtio_device *dev); > > int virtio_device_restore(struct virtio_device *dev); > > #endif > > -void virtio_reset_device(struct virtio_device *dev); > > +int virtio_reset_device(struct virtio_device *dev); > > void virtio_device_shutdown(struct virtio_device *dev); > > int virtio_device_reset_prepare(struct virtio_device *dev); > > int virtio_device_reset_done(struct virtio_device *dev); > > diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h > > index 69f84ea85d71..153426b0ae79 100644 > > --- a/include/linux/virtio_config.h > > +++ b/include/linux/virtio_config.h > > @@ -117,7 +117,7 @@ struct virtio_config_ops { > > u32 (*generation)(struct virtio_device *vdev); > > u8 (*get_status)(struct virtio_device *vdev); > > void (*set_status)(struct virtio_device *vdev, u8 status); > > - void (*reset)(struct virtio_device *vdev); > > + int (*reset)(struct virtio_device *vdev); > > int (*find_vqs)(struct virtio_device *vdev, unsigned int nvqs, > > struct virtqueue *vqs[], > > struct virtqueue_info vqs_info[], > > -- > > 2.55.0 -- Best regards, Radu
