Re: [PATCH v2 1/3] virtio: synchronize callbacks during device reset
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2026-09-06 06:49:52
Also in:
kvm, linux-remoteproc, linux-s390, linux-um, lkml, platform-driver-x86, virtualization
On Sat, Sep 05, 2026 at 05:20:57PM +0200, Karl Mehltretter wrote:
virtio_reset_device() promises that vq callbacks have finished when it returns. virtio-pci waits
it does not wait. it synchronizes.
in vp_reset(), but other transports can return with a callback still running. Call virtio_synchronize_cbs() after config->reset() and drop the duplicate waits from both PCI reset methods. Add the wait to virtio_device_shutdown() too, since it calls config->reset() directly.
so what, if it calls reset directly? what does it have to do with virtio_reset_device? is there a bug around shutdown? if yes it's a separate issue.
Keep the pre-reset call under CONFIG_VIRTIO_HARDEN_NOTIFICATION so callbacks see vq->broken.
This kind of slop is what LLMs write. u do not need to document all the things you did not change.
Always take irq_lock in the classic virtio-ccw interrupt handler so it pairs with synchronize_cbs even without notification hardening. Use is_thinint to choose the lock: airq_info can stay allocated after a fallback to classic interrupts.
I can't even read this, half sentences out of context. Can you pls write the commit log yourself? I suspect what this is trying to say is that synchronize_cbs is buggy on ccw and this is trying to fix it? A separate patch then.
The transport reset must still stop new callbacks before this wait.
Fixes: d9679d0013a6 ("virtio: wrap config->reset calls")
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Assisted-by: LLMcodex likes putting this in but it is the wrong format.
quoted hunk ↗ jump to hunk
Signed-off-by: Karl Mehltretter <redacted> --- drivers/s390/virtio/virtio_ccw.c | 6 +----- drivers/virtio/virtio.c | 2 ++ drivers/virtio/virtio_pci_legacy.c | 2 -- drivers/virtio/virtio_pci_modern.c | 3 --- include/linux/virtio_config.h | 6 +++--- 5 files changed, 6 insertions(+), 13 deletions(-)diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c index bab6cad3fd5c..552d77998012 100644 --- a/drivers/s390/virtio/virtio_ccw.c +++ b/drivers/s390/virtio/virtio_ccw.c@@ -1062,7 +1062,7 @@ static void virtio_ccw_synchronize_cbs(struct virtio_device *vdev) struct virtio_ccw_device *vcdev = to_vc_device(vdev); struct airq_info *info = vcdev->airq_info; - if (info) { + if (vcdev->is_thinint && info) { /* * This device uses adapter interrupts: synchronize with * vring_interrupt() called by virtio_airq_handler()@@ -1204,13 +1204,11 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev, vcdev->err = -EIO; } virtio_ccw_check_activity(vcdev, activity); -#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION /* * Paired with virtio_ccw_synchronize_cbs() and interrupts are * disabled here. */ read_lock(&vcdev->irq_lock); -#endif for_each_set_bit(i, indicators(vcdev), sizeof(*indicators(vcdev)) * BITS_PER_BYTE) { /* The bit clear must happen before the vring kick. */@@ -1219,9 +1217,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev, vq = virtio_ccw_vq_by_ind(vcdev, i); vring_interrupt(0, vq); } -#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION read_unlock(&vcdev->irq_lock); -#endif if (test_bit(0, indicators2(vcdev))) { virtio_config_changed(&vcdev->vdev); clear_bit(0, indicators2(vcdev));diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 75bb4ffe3b87..ad1c50b8a94e 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c@@ -264,6 +264,7 @@ void virtio_reset_device(struct virtio_device *dev) #endif dev->config->reset(dev); + virtio_synchronize_cbs(dev); } EXPORT_SYMBOL_GPL(virtio_reset_device);@@ -424,6 +425,7 @@ void virtio_device_shutdown(struct virtio_device *dev) * Some devices get wedged if this happens, so reset to make sure it does not. */ dev->config->reset(dev); + virtio_synchronize_cbs(dev); } EXPORT_SYMBOL_GPL(virtio_device_shutdown);diff --git a/drivers/virtio/virtio_pci_legacy.c b/drivers/virtio/virtio_pci_legacy.c index d9cbb02b35a1..8115aa39e01e 100644 --- a/drivers/virtio/virtio_pci_legacy.c +++ b/drivers/virtio/virtio_pci_legacy.c@@ -98,8 +98,6 @@ static void vp_reset(struct virtio_device *vdev) /* Flush out the status write, and flush in device writes, * including MSi-X interrupts, if any. */ vp_legacy_get_status(&vp_dev->ldev); - /* Flush pending VQ/configuration callbacks. */ - vp_synchronize_vectors(vdev); } 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..c9e21317c51a 100644 --- a/drivers/virtio/virtio_pci_modern.c +++ b/drivers/virtio/virtio_pci_modern.c@@ -559,9 +559,6 @@ static void vp_reset(struct virtio_device *vdev) msleep(1); vp_modern_avq_cleanup(vdev); - - /* Flush pending VQ/configuration callbacks. */ - vp_synchronize_vectors(vdev); } static int vp_active_vq(struct virtqueue *vq, u16 msix_vec)diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h index 69f84ea85d71..8684a1e268ee 100644 --- a/include/linux/virtio_config.h +++ b/include/linux/virtio_config.h@@ -71,9 +71,9 @@ struct virtqueue_info { * Returns 0 on success or error status * @del_vqs: free virtqueues found by find_vqs(). * @synchronize_cbs: synchronize with the virtqueue callbacks (optional) - * The function guarantees that all memory operations on the - * queue before it are visible to the vring_interrupt() that is - * called after it. + * Wait for running callbacks to complete. Memory operations on the + * queue before this call must be visible to vring_interrupt() calls + * that follow it. * vdev: the virtio_device * @get_features: get the array of feature bits for this device. * vdev: the virtio_device-- 2.39.5 (Apple Git-154)