All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] vp_vdpa: harden the logic of set status
@ 2022-12-06  2:13 Longpeng(Mike)
  2022-12-06  6:49   ` Jason Wang
  2022-12-06 11:00   ` Stefano Garzarella
  0 siblings, 2 replies; 6+ messages in thread
From: Longpeng(Mike) @ 2022-12-06  2:13 UTC (permalink / raw)
  To: stefanha, mst, jasowang, sgarzare, eperezma
  Cc: cohuck, arei.gonglei, yechuan, huangzhichao, virtualization,
	linux-kernel, Longpeng

From: Longpeng <longpeng2@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@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 */
+
 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 to
+	 * 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] vp_vdpa: harden the logic of set status
  2022-12-06  2:13 [PATCH v2] vp_vdpa: harden the logic of set status Longpeng(Mike)
@ 2022-12-06  6:49   ` Jason Wang
  2022-12-06 11:00   ` Stefano Garzarella
  1 sibling, 0 replies; 6+ messages in thread
From: Jason Wang @ 2022-12-06  6:49 UTC (permalink / raw)
  To: Longpeng(Mike)
  Cc: mst, cohuck, linux-kernel, yechuan, eperezma, huangzhichao,
	stefanha, virtualization

On Tue, Dec 6, 2022 at 10:13 AM Longpeng(Mike) <longpeng2@huawei.com> wrote:
>
> From: Longpeng <longpeng2@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@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 */
> +
>  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 to
> +        * 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.

s/keep/keeping/

> +        */
> +       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;
> +               }

Any chance to use readx_poll_timeout() here?

Thanks

> +       }
> +
>         if (s & VIRTIO_CONFIG_S_DRIVER_OK)
>                 vp_vdpa_free_irq(vp_vdpa);
>
> --
> 2.23.0
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] vp_vdpa: harden the logic of set status
@ 2022-12-06  6:49   ` Jason Wang
  0 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2022-12-06  6:49 UTC (permalink / raw)
  To: Longpeng(Mike)
  Cc: stefanha, mst, sgarzare, eperezma, cohuck, arei.gonglei, yechuan,
	huangzhichao, virtualization, linux-kernel

On Tue, Dec 6, 2022 at 10:13 AM Longpeng(Mike) <longpeng2@huawei.com> wrote:
>
> From: Longpeng <longpeng2@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@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 */
> +
>  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 to
> +        * 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.

s/keep/keeping/

> +        */
> +       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;
> +               }

Any chance to use readx_poll_timeout() here?

Thanks

> +       }
> +
>         if (s & VIRTIO_CONFIG_S_DRIVER_OK)
>                 vp_vdpa_free_irq(vp_vdpa);
>
> --
> 2.23.0
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] vp_vdpa: harden the logic of set status
  2022-12-06  2:13 [PATCH v2] vp_vdpa: harden the logic of set status Longpeng(Mike)
@ 2022-12-06 11:00   ` Stefano Garzarella
  2022-12-06 11:00   ` Stefano Garzarella
  1 sibling, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2022-12-06 11:00 UTC (permalink / raw)
  To: Longpeng(Mike)
  Cc: mst, cohuck, linux-kernel, yechuan, eperezma, huangzhichao,
	stefanha, virtualization

On Tue, Dec 06, 2022 at 10:13:21AM +0800, Longpeng(Mike) wrote:
>From: Longpeng <longpeng2@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@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 to

I 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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] vp_vdpa: harden the logic of set status
@ 2022-12-06 11:00   ` Stefano Garzarella
  0 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2022-12-06 11:00 UTC (permalink / raw)
  To: Longpeng(Mike)
  Cc: stefanha, mst, jasowang, eperezma, cohuck, arei.gonglei, yechuan,
	huangzhichao, virtualization, linux-kernel

On Tue, Dec 06, 2022 at 10:13:21AM +0800, Longpeng(Mike) wrote:
>From: Longpeng <longpeng2@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@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 to

I 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] vp_vdpa: harden the logic of set status
  2022-12-06 11:00   ` Stefano Garzarella
  (?)
@ 2022-12-07  0:24   ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
  -1 siblings, 0 replies; 6+ messages in thread
From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2022-12-07  0:24 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: stefanha, mst, jasowang, eperezma, cohuck, arei.gonglei, yechuan,
	huangzhichao, virtualization, linux-kernel



在 2022/12/6 19:00, Stefano Garzarella 写道:
> On Tue, Dec 06, 2022 at 10:13:21AM +0800, Longpeng(Mike) wrote:
>> From: Longpeng <longpeng2@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@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.
> 
Okay.

>> +
>> 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 to
> 
> I think we can refer to the lates v1.2 (the section should be the same).
> 
Okay, will do in next version, thanks.

>> +     * 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
> 
> 
> .

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-12-07  0:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-06  2:13 [PATCH v2] vp_vdpa: harden the logic of set status Longpeng(Mike)
2022-12-06  6:49 ` Jason Wang
2022-12-06  6:49   ` Jason Wang
2022-12-06 11:00 ` Stefano Garzarella
2022-12-06 11:00   ` Stefano Garzarella
2022-12-07  0:24   ` Longpeng (Mike, Cloud Infrastructure Service Product Dept.)

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.