lkml.org 
[lkml]   [2020]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] ifcvf: move IRQ request/free to status change handlers
From
Date

On 2020/5/12 上午11:38, Jason Wang wrote:
>>>>
>>>>   static int ifcvf_start_datapath(void *private)
>>>>   {
>>>>       struct ifcvf_hw *vf = ifcvf_private_to_vf(private);
>>>> @@ -118,9 +172,12 @@ static void ifcvf_vdpa_set_status(struct
>>>> vdpa_device *vdpa_dev, u8 status)
>>>>   {
>>>>       struct ifcvf_adapter *adapter;
>>>>       struct ifcvf_hw *vf;
>>>> +    u8 status_old;
>>>> +    int ret;
>>>>         vf  = vdpa_to_vf(vdpa_dev);
>>>>       adapter = dev_get_drvdata(vdpa_dev->dev.parent);
>>>> +    status_old = ifcvf_get_status(vf);
>>>>         if (status == 0) {
>>>>           ifcvf_stop_datapath(adapter);
>>>> @@ -128,7 +185,22 @@ static void ifcvf_vdpa_set_status(struct
>>>> vdpa_device *vdpa_dev, u8 status)
>>>>           return;
>>>>       }
>>>>   -    if (status & VIRTIO_CONFIG_S_DRIVER_OK) {
>>>> +    if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>> +        !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>> +        ifcvf_stop_datapath(adapter);
>>>> +        ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
>>>> +    }
>>>> +
>>>> +    if ((status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>>>> +        !(status_old & VIRTIO_CONFIG_S_DRIVER_OK)) {
>>>> +        ret = ifcvf_request_irq(adapter);
>>>> +        if (ret) {
>>>> +            status = ifcvf_get_status(vf);
>>>> +            status |= VIRTIO_CONFIG_S_FAILED;
>>>> +            ifcvf_set_status(vf, status);
>>>> +            return;
>>>> +        }
>>>> +
>>>
>>>
>>> Have a hard though on the logic here.
>>>
>>> This depends on the status setting from guest or userspace. Which
>>> means it can not deal with e.g when qemu or userspace is crashed? Do
>>> we need to care this or it's a over engineering?
>>>
>>> Thanks
>> If qemu crash, I guess users may re-run qmeu / re-initialize the
>> device, according to the spec, there should be a reset routine.
>> This code piece handles status change on DRIVER_OK flipping. I am not
>> sure I get your point, mind to give more hints?
>
>
> The problem is if we don't launch new qemu instance, the interrupt
> will be still there?


Ok, we reset on vhost_vdpa_release() so the following is suspicious:

With the patch, we do:

static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status)
{
        struct ifcvf_adapter *adapter;
        struct ifcvf_hw *vf;
        u8 status_old;
        int ret;

        vf  = vdpa_to_vf(vdpa_dev);
        adapter = dev_get_drvdata(vdpa_dev->dev.parent);
        status_old = ifcvf_get_status(vf);

        if (status == 0) {
                ifcvf_stop_datapath(adapter);
                ifcvf_reset_vring(adapter);
                return;
        }

        if ((status_old & VIRTIO_CONFIG_S_DRIVER_OK) &&
            !(status & VIRTIO_CONFIG_S_DRIVER_OK)) {
                ifcvf_stop_datapath(adapter);
                ifcvf_free_irq(adapter, IFCVF_MAX_QUEUE_PAIRS * 2);
        }

...

So the handling of status == 0 looks wrong.

The OK -> !OK check should already cover the datapath stopping and irq
stuffs.

We only need to deal with vring reset and only need to do it after we
stop the datapath/irq stuffs.

Thanks



>
> Thanks

\
 
 \ /
  Last update: 2020-05-12 07:53    [W:0.081 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site