Messages in this thread | | | Subject | Re: [PATCH] ifcvf: move IRQ request/free to status change handlers | From | Jason Wang <> | Date | Tue, 12 May 2020 13:51:25 +0800 |
| |
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
| |