On Mon, Apr 25 2022, Jason Wang <jasowang at redhat.com> wrote:> This patch allows the virtio_break_device() to accept a boolean value > then we can unbreak the virtqueue. > > Signed-off-by: Jason Wang <jasowang at redhat.com> > --- > drivers/char/virtio_console.c | 2 +- > drivers/crypto/virtio/virtio_crypto_core.c | 2 +- > drivers/s390/virtio/virtio_ccw.c | 4 ++-- > drivers/virtio/virtio_pci_common.c | 2 +- > drivers/virtio/virtio_ring.c | 4 ++-- > include/linux/virtio.h | 2 +- > 6 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index e3c430539a17..afede977f7b3 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1958,7 +1958,7 @@ static void virtcons_remove(struct virtio_device *vdev) > spin_unlock_irq(&pdrvdata_lock); > > /* Device is going away, exit any polling for buffers */ > - virtio_break_device(vdev); > + virtio_break_device(vdev, true); > if (use_multiport(portdev)) > flush_work(&portdev->control_work); > else > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c > index c6f482db0bc0..fd17f3f2e958 100644 > --- a/drivers/crypto/virtio/virtio_crypto_core.c > +++ b/drivers/crypto/virtio/virtio_crypto_core.c > @@ -215,7 +215,7 @@ static int virtcrypto_update_status(struct virtio_crypto *vcrypto) > dev_warn(&vcrypto->vdev->dev, > "Unknown status bits: 0x%x\n", status); > > - virtio_break_device(vcrypto->vdev); > + virtio_break_device(vcrypto->vdev, true); > return -EPERM; > } > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > index c19f07a82d62..9a963f5af5b5 100644 > --- a/drivers/s390/virtio/virtio_ccw.c > +++ b/drivers/s390/virtio/virtio_ccw.c > @@ -1211,7 +1211,7 @@ static void virtio_ccw_remove(struct ccw_device *cdev) > > if (vcdev && cdev->online) { > if (vcdev->device_lost) > - virtio_break_device(&vcdev->vdev); > + virtio_break_device(&vcdev->vdev, true); > unregister_virtio_device(&vcdev->vdev); > spin_lock_irqsave(get_ccwdev_lock(cdev), flags); > dev_set_drvdata(&cdev->dev, NULL); > @@ -1228,7 +1228,7 @@ static int virtio_ccw_offline(struct ccw_device *cdev) > if (!vcdev) > return 0; > if (vcdev->device_lost) > - virtio_break_device(&vcdev->vdev); > + virtio_break_device(&vcdev->vdev, true); > unregister_virtio_device(&vcdev->vdev); > spin_lock_irqsave(get_ccwdev_lock(cdev), flags); > dev_set_drvdata(&cdev->dev, NULL); > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index d724f676608b..39a711ddff30 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -583,7 +583,7 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) > * layers can abort any ongoing operation. > */ > if (!pci_device_is_present(pci_dev)) > - virtio_break_device(&vp_dev->vdev); > + virtio_break_device(&vp_dev->vdev, true); > > pci_disable_sriov(pci_dev); > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index cfb028ca238e..6da13495a70c 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -2382,7 +2382,7 @@ EXPORT_SYMBOL_GPL(virtqueue_is_broken); > * This should prevent the device from being used, allowing drivers to > * recover. You may need to grab appropriate locks to flush. > */ > -void virtio_break_device(struct virtio_device *dev) > +void virtio_break_device(struct virtio_device *dev, bool broken)I think we need to be careful to say when it is safe to unset 'broken'. The current callers set all queues to broken in case of surprise removal (ccw, pci), removal (console), or the device behaving badly (crypto). There's also code setting individual queues to broken. We do not want to undo any of these, unless the device has gone through a reset in the meanwhile. Maybe add: "It is only safe to call this function to *remove* the broken flag for a device that is (re)transitioning to becoming usable; calling it that way during normal usage may have unpredictable consequences." (Not sure how to word this; especially if we consider future usage of queue reset.)
Michael S. Tsirkin
2022-Apr-25 14:04 UTC
[PATCH V3 7/9] virtio: allow to unbreak virtqueue
On Mon, Apr 25, 2022 at 02:44:06PM +0200, Cornelia Huck wrote:> On Mon, Apr 25 2022, Jason Wang <jasowang at redhat.com> wrote: > > > This patch allows the virtio_break_device() to accept a boolean value > > then we can unbreak the virtqueue. > > > > Signed-off-by: Jason Wang <jasowang at redhat.com> > > --- > > drivers/char/virtio_console.c | 2 +- > > drivers/crypto/virtio/virtio_crypto_core.c | 2 +- > > drivers/s390/virtio/virtio_ccw.c | 4 ++-- > > drivers/virtio/virtio_pci_common.c | 2 +- > > drivers/virtio/virtio_ring.c | 4 ++-- > > include/linux/virtio.h | 2 +- > > 6 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > > index e3c430539a17..afede977f7b3 100644 > > --- a/drivers/char/virtio_console.c > > +++ b/drivers/char/virtio_console.c > > @@ -1958,7 +1958,7 @@ static void virtcons_remove(struct virtio_device *vdev) > > spin_unlock_irq(&pdrvdata_lock); > > > > /* Device is going away, exit any polling for buffers */ > > - virtio_break_device(vdev); > > + virtio_break_device(vdev, true); > > if (use_multiport(portdev)) > > flush_work(&portdev->control_work); > > else > > diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c > > index c6f482db0bc0..fd17f3f2e958 100644 > > --- a/drivers/crypto/virtio/virtio_crypto_core.c > > +++ b/drivers/crypto/virtio/virtio_crypto_core.c > > @@ -215,7 +215,7 @@ static int virtcrypto_update_status(struct virtio_crypto *vcrypto) > > dev_warn(&vcrypto->vdev->dev, > > "Unknown status bits: 0x%x\n", status); > > > > - virtio_break_device(vcrypto->vdev); > > + virtio_break_device(vcrypto->vdev, true); > > return -EPERM; > > } > > > > diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c > > index c19f07a82d62..9a963f5af5b5 100644 > > --- a/drivers/s390/virtio/virtio_ccw.c > > +++ b/drivers/s390/virtio/virtio_ccw.c > > @@ -1211,7 +1211,7 @@ static void virtio_ccw_remove(struct ccw_device *cdev) > > > > if (vcdev && cdev->online) { > > if (vcdev->device_lost) > > - virtio_break_device(&vcdev->vdev); > > + virtio_break_device(&vcdev->vdev, true); > > unregister_virtio_device(&vcdev->vdev); > > spin_lock_irqsave(get_ccwdev_lock(cdev), flags); > > dev_set_drvdata(&cdev->dev, NULL); > > @@ -1228,7 +1228,7 @@ static int virtio_ccw_offline(struct ccw_device *cdev) > > if (!vcdev) > > return 0; > > if (vcdev->device_lost) > > - virtio_break_device(&vcdev->vdev); > > + virtio_break_device(&vcdev->vdev, true); > > unregister_virtio_device(&vcdev->vdev); > > spin_lock_irqsave(get_ccwdev_lock(cdev), flags); > > dev_set_drvdata(&cdev->dev, NULL); > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > > index d724f676608b..39a711ddff30 100644 > > --- a/drivers/virtio/virtio_pci_common.c > > +++ b/drivers/virtio/virtio_pci_common.c > > @@ -583,7 +583,7 @@ static void virtio_pci_remove(struct pci_dev *pci_dev) > > * layers can abort any ongoing operation. > > */ > > if (!pci_device_is_present(pci_dev)) > > - virtio_break_device(&vp_dev->vdev); > > + virtio_break_device(&vp_dev->vdev, true); > > > > pci_disable_sriov(pci_dev); > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index cfb028ca238e..6da13495a70c 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -2382,7 +2382,7 @@ EXPORT_SYMBOL_GPL(virtqueue_is_broken); > > * This should prevent the device from being used, allowing drivers to > > * recover. You may need to grab appropriate locks to flush. > > */ > > -void virtio_break_device(struct virtio_device *dev) > > +void virtio_break_device(struct virtio_device *dev, bool broken) > > I think we need to be careful to say when it is safe to unset 'broken'. > > The current callers set all queues to broken in case of surprise removal > (ccw, pci), removal (console), or the device behaving badly > (crypto). There's also code setting individual queues to broken. We do > not want to undo any of these, unless the device has gone through a > reset in the meanwhile. Maybe add: > > "It is only safe to call this function to *remove* the broken flag for a > device that is (re)transitioning to becoming usable; calling it that way > during normal usage may have unpredictable consequences." > > (Not sure how to word this; especially if we consider future usage of > queue reset.)Right. I would prefer __virtio_unbreak_device or something similar with a bit comment explaining it's only safe to call during probe. -- MST