Virtualization - Apr 2021 - [PATCH v2 2/3] virito_pci: add timeout to reset device operation

? 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 at 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;
>>>>
>>>
>>
>