Re: [PATCH] ifcvf: move IRQ request/free to status change handlers

[Jason Wang <jasowang@xxxxxxxxxx>]

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