Stefano Garzarella
2022-Dec-06 11:00 UTC
[PATCH v2] vp_vdpa: harden the logic of set status
On Tue, Dec 06, 2022 at 10:13:21AM +0800, Longpeng(Mike) wrote:>From: Longpeng <longpeng2 at huawei.com> > >1. We should not set status to 0 when invoking vp_vdpa_set_status(), > trigger a warning in that case. > >2. The driver MUST wait for a read of device_status to return 0 before > reinitializing the device. But we also don't want to keep us in an > infinite loop forever, so wait for 5s if we try to reset the device. > >Signed-off-by: Longpeng <longpeng2 at huawei.com> >--- >Changes v1->v2: > - use WARN_ON instead of BUG_ON. [Stefano] > - use "warning + failed" instead of "infinite loop". [Jason, Stefano] > - use usleep_range instead of msleep (checkpatch). [Longpeng] > >--- > drivers/vdpa/virtio_pci/vp_vdpa.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > >diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c >index 13701c2a1963..a2d3b13ac646 100644 >--- a/drivers/vdpa/virtio_pci/vp_vdpa.c >+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c >@@ -214,6 +214,9 @@ static void vp_vdpa_set_status(struct vdpa_device *vdpa, u8 status) > struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa); > u8 s = vp_vdpa_get_status(vdpa); > >+ /* We should never be setting status to 0. */ >+ WARN_ON(status == 0); >+ > if (status & VIRTIO_CONFIG_S_DRIVER_OK && > !(s & VIRTIO_CONFIG_S_DRIVER_OK)) { > vp_vdpa_request_irq(vp_vdpa); >@@ -222,14 +225,33 @@ static void vp_vdpa_set_status(struct vdpa_device *vdpa, u8 status) > vp_modern_set_status(mdev, status); > } > >+#define VP_VDPA_RESET_TMOUT_MS 5000 /* 5s */What about moving this define on top of this file? Near the other macro. And I would use TIMEOUT entirely.>+ > static int vp_vdpa_reset(struct vdpa_device *vdpa, bool clear) > { > struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa); > struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa); > u8 s = vp_vdpa_get_status(vdpa); >+ unsigned long timeout; > > vp_modern_set_status(mdev, 0); > >+ /* >+ * As the virtio v1.1 spec (4.1.4.3.2) says: After writing 0 toI think we can refer to the lates v1.2 (the section should be the same).>+ * device_status, the driver MUST wait for a read of device_status >+ * to return 0 before reinitializing the device. >+ * To avoid keep us here forever, we only wait for 5 seconds. >+ */ >+ timeout = jiffies + msecs_to_jiffies(VP_VDPA_RESET_TMOUT_MS); >+ while (vp_modern_get_status(mdev)) { >+ usleep_range(1000, 1500); >+ if (time_after(jiffies, timeout)) { >+ dev_err(&mdev->pci_dev->dev, >+ "vp_vdpa: fail to reset device\n"); >+ return -ETIMEDOUT; >+ } >+ } >+ > if (s & VIRTIO_CONFIG_S_DRIVER_OK) > vp_vdpa_free_irq(vp_vdpa); > >-- >2.23.0 >The rest LGTM! Thanks, Stefano