Re: [PATCH v2 2/3] virito_pci: add timeout to reset device operation

From: Jason Wang <jasowang@redhat.com>
To: Max Gurtovoy <mgurtovoy@nvidia.com>,
	mst@redhat.com, kvm@vger.kernel.org,
	virtualization@lists.linux-foundation.org
Cc: oren@nvidia.com, nitzanc@nvidia.com, cohuck@redhat.com
Subject: Re: [PATCH v2 2/3] virito_pci: add timeout to reset device operation
Date: Thu, 8 Apr 2021 21:18:26 +0800	[thread overview]
Message-ID: <2b4edcaf-779d-b197-0437-c6cb8e82e8e1@redhat.com> (raw)
In-Reply-To: <7ed9cafa-498e-156d-c667-6da3fa432b18@nvidia.com>

在 2021/4/8 下午8:57, Max Gurtovoy 写道:
>
> On 4/8/2021 3:45 PM, Jason Wang wrote:
>>
>> 在 2021/4/8 下午5:44, Max Gurtovoy 写道:
>>>
>>> On 4/8/2021 12:01 PM, Jason Wang wrote:
>>>>
>>>> 在 2021/4/8 下午4:11, Max Gurtovoy 写道:
>>>>> According to the spec after writing 0 to device_status, the driver 
>>>>> MUST
>>>>> wait for a read of device_status to return 0 before reinitializing 
>>>>> the
>>>>> device. In case we have a device that won't return 0, the reset
>>>>> operation will loop forever and cause the host/vm to stuck. Set 
>>>>> timeout
>>>>> for 3 minutes before giving up on the device.
>>>>>
>>>>> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
>>>>> ---
>>>>>   drivers/virtio/virtio_pci_modern.c | 10 +++++++++-
>>>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/virtio/virtio_pci_modern.c 
>>>>> b/drivers/virtio/virtio_pci_modern.c
>>>>> index cc3412a96a17..dcee616e8d21 100644
>>>>> --- a/drivers/virtio/virtio_pci_modern.c
>>>>> +++ b/drivers/virtio/virtio_pci_modern.c
>>>>> @@ -162,6 +162,7 @@ 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;
>>>>> +    unsigned long timeout = jiffies + msecs_to_jiffies(180000);
>>>>>         /* 0 status means a reset. */
>>>>>       vp_modern_set_status(mdev, 0);
>>>>> @@ -169,9 +170,16 @@ static int vp_reset(struct virtio_device *vdev)
>>>>>        * device_status to return 0 before reinitializing the device.
>>>>>        * This will flush out the status write, and flush in device 
>>>>> writes,
>>>>>        * including MSI-X interrupts, if any.
>>>>> +     * Set a timeout before giving up on the device.
>>>>>        */
>>>>> -    while (vp_modern_get_status(mdev))
>>>>> +    while (vp_modern_get_status(mdev)) {
>>>>> +        if (time_after(jiffies, timeout)) {
>>>>
>>>>
>>>> What happens if the device finish the rest after the timeout?
>>>
>>>
>>> The driver will set VIRTIO_CONFIG_S_FAILED and one can re-probe it 
>>> later on (e.g by re-scanning the pci bus).
>>
>>
>> Ok, so do we need the flush through vp_synchronize_vectors() here?
>
> If the device didn't write 0 to status I guess we don't need that.
>
> The device shouldn't raise any interrupt before negotiation finish 
> successfully.

The reset could be triggered in other places like driver removing.

Thanks

>
> MST, is that correct ?
>
>>
>> Thanks
>>
>>
>>>
>>>
>>>>
>>>> Thanks
>>>>
>>>>
>>>>> + dev_err(&vdev->dev, "virtio: device not ready. "
>>>>> +                "Aborting. Try again later\n");
>>>>> +            return -EAGAIN;
>>>>> +        }
>>>>>           msleep(1);
>>>>> +    }
>>>>>       /* Flush pending VQ/configuration callbacks. */
>>>>>       vp_synchronize_vectors(vdev);
>>>>>       return 0;
>>>>
>>>
>>
>