Jason Wang
2023-May-26 01:34 UTC
[PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine
On Thu, May 25, 2023 at 5:38?PM Zhu, Lingshan <lingshan.zhu at intel.com> wrote:> > > > On 5/24/2023 4:03 PM, Jason Wang wrote: > > On Mon, May 8, 2023 at 6:05?PM Zhu Lingshan <lingshan.zhu at intel.com> wrote: > >> This commit synchronize irqs of the virtqueues > >> and config space in the reset routine. > >> Thus ifcvf_stop_hw() and reset() are refactored as well. > >> > >> Signed-off-by: Zhu Lingshan <lingshan.zhu at intel.com> > >> --- > >> drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++-------- > >> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + > >> drivers/vdpa/ifcvf/ifcvf_main.c | 46 +++++---------------------------- > >> 3 files changed, 38 insertions(+), 50 deletions(-) > >> > >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c > >> index 79e313c5e10e..1f39290baa38 100644 > >> --- a/drivers/vdpa/ifcvf/ifcvf_base.c > >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c > >> @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8 status) > >> > >> void ifcvf_reset(struct ifcvf_hw *hw) > >> { > >> - hw->config_cb.callback = NULL; > >> - hw->config_cb.private = NULL; > >> - > >> ifcvf_set_status(hw, 0); > >> - /* flush set_status, make sure VF is stopped, reset */ > >> - ifcvf_get_status(hw); > >> + while (ifcvf_get_status(hw)) > >> + msleep(1); > >> } > >> > >> u64 ifcvf_get_hw_features(struct ifcvf_hw *hw) > >> @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready) > >> vp_iowrite16(ready, &cfg->queue_enable); > >> } > >> > >> -static void ifcvf_hw_disable(struct ifcvf_hw *hw) > >> +static void ifcvf_reset_vring(struct ifcvf_hw *hw) > >> { > >> - u32 i; > >> + u16 qid; > >> + > >> + for (qid = 0; qid < hw->nr_vring; qid++) { > >> + hw->vring[qid].cb.callback = NULL; > >> + hw->vring[qid].cb.private = NULL; > >> + ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR); > >> + } > >> +} > >> > >> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw) > >> +{ > >> + hw->config_cb.callback = NULL; > >> + hw->config_cb.private = NULL; > >> ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR); > >> - for (i = 0; i < hw->nr_vring; i++) { > >> - ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR); > >> +} > >> + > >> +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw) > >> +{ > >> + u32 nvectors = hw->num_msix_vectors; > >> + struct pci_dev *pdev = hw->pdev; > >> + int i, irq; > >> + > >> + for (i = 0; i < nvectors; i++) { > >> + irq = pci_irq_vector(pdev, i); > >> + if (irq >= 0) > >> + synchronize_irq(irq); > >> } > >> } > >> > >> void ifcvf_stop_hw(struct ifcvf_hw *hw) > >> { > >> - ifcvf_hw_disable(hw); > >> - ifcvf_reset(hw); > >> + ifcvf_synchronize_irq(hw); > >> + ifcvf_reset_vring(hw); > >> + ifcvf_reset_config_handler(hw); > > Nit: > > > > So the name of this function is kind of misleading since irq > > synchronization and virtqueue/config handler are not belong to > > hardware? > > > > Maybe it would be better to call it ifcvf_stop(). > Sure, I will send a V3 with this renaming, > do you ack patch 1/5?Yes, I think I've acked to that patch. Thanks> > Thanks > Zhu Lingshan > > > > Thanks > > > >> } > >> > >> void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid) > >> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h > >> index d34d3bc0dbf4..7430f80779be 100644 > >> --- a/drivers/vdpa/ifcvf/ifcvf_base.h > >> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h > >> @@ -82,6 +82,7 @@ struct ifcvf_hw { > >> int vqs_reused_irq; > >> u16 nr_vring; > >> /* VIRTIO_PCI_CAP_DEVICE_CFG size */ > >> + u32 num_msix_vectors; > >> u32 cap_dev_config_size; > >> struct pci_dev *pdev; > >> }; > >> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > >> index 968687159e44..3401b9901dd2 100644 > >> --- a/drivers/vdpa/ifcvf/ifcvf_main.c > >> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > >> @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf) > >> ifcvf_free_vq_irq(vf); > >> ifcvf_free_config_irq(vf); > >> ifcvf_free_irq_vectors(pdev); > >> + vf->num_msix_vectors = 0; > >> } > >> > >> /* ifcvf MSIX vectors allocator, this helper tries to allocate > >> @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf) > >> if (ret) > >> return ret; > >> > >> - return 0; > >> -} > >> - > >> -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter) > >> -{ > >> - struct ifcvf_hw *vf = adapter->vf; > >> - int i; > >> - > >> - for (i = 0; i < vf->nr_vring; i++) > >> - vf->vring[i].cb.callback = NULL; > >> - > >> - ifcvf_stop_hw(vf); > >> + vf->num_msix_vectors = nvectors; > >> > >> return 0; > >> } > >> > >> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter) > >> -{ > >> - struct ifcvf_hw *vf = adapter->vf; > >> - int i; > >> - > >> - for (i = 0; i < vf->nr_vring; i++) { > >> - vf->vring[i].last_avail_idx = 0; > >> - vf->vring[i].cb.callback = NULL; > >> - vf->vring[i].cb.private = NULL; > >> - } > >> - > >> - ifcvf_reset(vf); > >> -} > >> - > >> static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device *vdpa_dev) > >> { > >> return container_of(vdpa_dev, struct ifcvf_adapter, vdpa); > >> @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status) > >> > >> static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev) > >> { > >> - struct ifcvf_adapter *adapter; > >> - struct ifcvf_hw *vf; > >> - u8 status_old; > >> - > >> - vf = vdpa_to_vf(vdpa_dev); > >> - adapter = vdpa_to_adapter(vdpa_dev); > >> - status_old = ifcvf_get_status(vf); > >> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); > >> + u8 status = ifcvf_get_status(vf); > >> > >> - if (status_old == 0) > >> - return 0; > >> + ifcvf_stop_hw(vf); > >> > >> - if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) { > >> - ifcvf_stop_datapath(adapter); > >> + if (status & VIRTIO_CONFIG_S_DRIVER_OK) > >> ifcvf_free_irq(vf); > >> - } > >> > >> - ifcvf_reset_vring(adapter); > >> + ifcvf_reset(vf); > >> > >> return 0; > >> } > >> -- > >> 2.39.1 > >> >
Zhu, Lingshan
2023-May-26 03:36 UTC
[PATCH V2 4/5] vDPA/ifcvf: synchronize irqs in the reset routine
On 5/26/2023 9:34 AM, Jason Wang wrote:> On Thu, May 25, 2023 at 5:38?PM Zhu, Lingshan <lingshan.zhu at intel.com> wrote: >> >> >> On 5/24/2023 4:03 PM, Jason Wang wrote: >>> On Mon, May 8, 2023 at 6:05?PM Zhu Lingshan <lingshan.zhu at intel.com> wrote: >>>> This commit synchronize irqs of the virtqueues >>>> and config space in the reset routine. >>>> Thus ifcvf_stop_hw() and reset() are refactored as well. >>>> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu at intel.com> >>>> --- >>>> drivers/vdpa/ifcvf/ifcvf_base.c | 41 +++++++++++++++++++++-------- >>>> drivers/vdpa/ifcvf/ifcvf_base.h | 1 + >>>> drivers/vdpa/ifcvf/ifcvf_main.c | 46 +++++---------------------------- >>>> 3 files changed, 38 insertions(+), 50 deletions(-) >>>> >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c >>>> index 79e313c5e10e..1f39290baa38 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c >>>> @@ -170,12 +170,9 @@ void ifcvf_set_status(struct ifcvf_hw *hw, u8 status) >>>> >>>> void ifcvf_reset(struct ifcvf_hw *hw) >>>> { >>>> - hw->config_cb.callback = NULL; >>>> - hw->config_cb.private = NULL; >>>> - >>>> ifcvf_set_status(hw, 0); >>>> - /* flush set_status, make sure VF is stopped, reset */ >>>> - ifcvf_get_status(hw); >>>> + while (ifcvf_get_status(hw)) >>>> + msleep(1); >>>> } >>>> >>>> u64 ifcvf_get_hw_features(struct ifcvf_hw *hw) >>>> @@ -368,20 +365,42 @@ void ifcvf_set_vq_ready(struct ifcvf_hw *hw, u16 qid, bool ready) >>>> vp_iowrite16(ready, &cfg->queue_enable); >>>> } >>>> >>>> -static void ifcvf_hw_disable(struct ifcvf_hw *hw) >>>> +static void ifcvf_reset_vring(struct ifcvf_hw *hw) >>>> { >>>> - u32 i; >>>> + u16 qid; >>>> + >>>> + for (qid = 0; qid < hw->nr_vring; qid++) { >>>> + hw->vring[qid].cb.callback = NULL; >>>> + hw->vring[qid].cb.private = NULL; >>>> + ifcvf_set_vq_vector(hw, qid, VIRTIO_MSI_NO_VECTOR); >>>> + } >>>> +} >>>> >>>> +static void ifcvf_reset_config_handler(struct ifcvf_hw *hw) >>>> +{ >>>> + hw->config_cb.callback = NULL; >>>> + hw->config_cb.private = NULL; >>>> ifcvf_set_config_vector(hw, VIRTIO_MSI_NO_VECTOR); >>>> - for (i = 0; i < hw->nr_vring; i++) { >>>> - ifcvf_set_vq_vector(hw, i, VIRTIO_MSI_NO_VECTOR); >>>> +} >>>> + >>>> +static void ifcvf_synchronize_irq(struct ifcvf_hw *hw) >>>> +{ >>>> + u32 nvectors = hw->num_msix_vectors; >>>> + struct pci_dev *pdev = hw->pdev; >>>> + int i, irq; >>>> + >>>> + for (i = 0; i < nvectors; i++) { >>>> + irq = pci_irq_vector(pdev, i); >>>> + if (irq >= 0) >>>> + synchronize_irq(irq); >>>> } >>>> } >>>> >>>> void ifcvf_stop_hw(struct ifcvf_hw *hw) >>>> { >>>> - ifcvf_hw_disable(hw); >>>> - ifcvf_reset(hw); >>>> + ifcvf_synchronize_irq(hw); >>>> + ifcvf_reset_vring(hw); >>>> + ifcvf_reset_config_handler(hw); >>> Nit: >>> >>> So the name of this function is kind of misleading since irq >>> synchronization and virtqueue/config handler are not belong to >>> hardware? >>> >>> Maybe it would be better to call it ifcvf_stop(). >> Sure, I will send a V3 with this renaming, >> do you ack patch 1/5? > Yes, I think I've acked to that patch.I will send a V3 with this renaming and your ack for patch 1/5> > Thanks > >> Thanks >> Zhu Lingshan >>> Thanks >>> >>>> } >>>> >>>> void ifcvf_notify_queue(struct ifcvf_hw *hw, u16 qid) >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h >>>> index d34d3bc0dbf4..7430f80779be 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h >>>> @@ -82,6 +82,7 @@ struct ifcvf_hw { >>>> int vqs_reused_irq; >>>> u16 nr_vring; >>>> /* VIRTIO_PCI_CAP_DEVICE_CFG size */ >>>> + u32 num_msix_vectors; >>>> u32 cap_dev_config_size; >>>> struct pci_dev *pdev; >>>> }; >>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c >>>> index 968687159e44..3401b9901dd2 100644 >>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c >>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c >>>> @@ -125,6 +125,7 @@ static void ifcvf_free_irq(struct ifcvf_hw *vf) >>>> ifcvf_free_vq_irq(vf); >>>> ifcvf_free_config_irq(vf); >>>> ifcvf_free_irq_vectors(pdev); >>>> + vf->num_msix_vectors = 0; >>>> } >>>> >>>> /* ifcvf MSIX vectors allocator, this helper tries to allocate >>>> @@ -343,36 +344,11 @@ static int ifcvf_request_irq(struct ifcvf_hw *vf) >>>> if (ret) >>>> return ret; >>>> >>>> - return 0; >>>> -} >>>> - >>>> -static int ifcvf_stop_datapath(struct ifcvf_adapter *adapter) >>>> -{ >>>> - struct ifcvf_hw *vf = adapter->vf; >>>> - int i; >>>> - >>>> - for (i = 0; i < vf->nr_vring; i++) >>>> - vf->vring[i].cb.callback = NULL; >>>> - >>>> - ifcvf_stop_hw(vf); >>>> + vf->num_msix_vectors = nvectors; >>>> >>>> return 0; >>>> } >>>> >>>> -static void ifcvf_reset_vring(struct ifcvf_adapter *adapter) >>>> -{ >>>> - struct ifcvf_hw *vf = adapter->vf; >>>> - int i; >>>> - >>>> - for (i = 0; i < vf->nr_vring; i++) { >>>> - vf->vring[i].last_avail_idx = 0; >>>> - vf->vring[i].cb.callback = NULL; >>>> - vf->vring[i].cb.private = NULL; >>>> - } >>>> - >>>> - ifcvf_reset(vf); >>>> -} >>>> - >>>> static struct ifcvf_adapter *vdpa_to_adapter(struct vdpa_device *vdpa_dev) >>>> { >>>> return container_of(vdpa_dev, struct ifcvf_adapter, vdpa); >>>> @@ -462,23 +438,15 @@ static void ifcvf_vdpa_set_status(struct vdpa_device *vdpa_dev, u8 status) >>>> >>>> static int ifcvf_vdpa_reset(struct vdpa_device *vdpa_dev) >>>> { >>>> - struct ifcvf_adapter *adapter; >>>> - struct ifcvf_hw *vf; >>>> - u8 status_old; >>>> - >>>> - vf = vdpa_to_vf(vdpa_dev); >>>> - adapter = vdpa_to_adapter(vdpa_dev); >>>> - status_old = ifcvf_get_status(vf); >>>> + struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev); >>>> + u8 status = ifcvf_get_status(vf); >>>> >>>> - if (status_old == 0) >>>> - return 0; >>>> + ifcvf_stop_hw(vf); >>>> >>>> - if (status_old & VIRTIO_CONFIG_S_DRIVER_OK) { >>>> - ifcvf_stop_datapath(adapter); >>>> + if (status & VIRTIO_CONFIG_S_DRIVER_OK) >>>> ifcvf_free_irq(vf); >>>> - } >>>> >>>> - ifcvf_reset_vring(adapter); >>>> + ifcvf_reset(vf); >>>> >>>> return 0; >>>> } >>>> -- >>>> 2.39.1 >>>>