> -----Original Message----- > From: Pei, Andy <[email protected]> > Sent: Tuesday, November 8, 2022 3:39 PM > To: Xia, Chenbo <[email protected]>; Taekyung Kim > <[email protected]>; [email protected] > Cc: [email protected]; [email protected]; Wang, Xiao W > <[email protected]> > Subject: RE: [PATCH v3] vdpa/ifc: fix update_datapath error handling > > Hi > > See my reply inline. > > > -----Original Message----- > > From: Xia, Chenbo <[email protected]> > > Sent: Tuesday, November 8, 2022 9:47 AM > > To: Taekyung Kim <[email protected]>; [email protected] > > Cc: [email protected]; [email protected]; Wang, Xiao W > > <[email protected]> > > Subject: RE: [PATCH v3] vdpa/ifc: fix update_datapath error handling > > > > > -----Original Message----- > > > From: Taekyung Kim <[email protected]> > > > Sent: Monday, November 7, 2022 5:00 PM > > > To: [email protected] > > > Cc: [email protected]; [email protected]; Xia, Chenbo > > > <[email protected]>; Wang, Xiao W <[email protected]>; > > > [email protected] > > > Subject: [PATCH v3] vdpa/ifc: fix update_datapath error handling > > > > > > Stop and return the error code when update_datapath fails. > > > update_datapath prepares resources for the vdpa device. > > > The driver should not perform any further actions if update_datapath > > > returns an error. > > > > > > Fixes: a3f8150eac6d ("net/ifcvf: add ifcvf vDPA driver") > > > Cc: [email protected] > > > > > > Signed-off-by: Taekyung Kim <[email protected]> > > > --- > > > v3: > > > * Fix coding style > > > > > > v2: > > > * Revert the prepared resources before returning an error > > > * Rebase to 22.11 rc2 > > > * Add fixes and cc for backport > > > > > > --- > > > drivers/vdpa/ifc/ifcvf_vdpa.c | 26 ++++++++++++++++++++++---- > > > 1 file changed, 22 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c > > > b/drivers/vdpa/ifc/ifcvf_vdpa.c index 8dfd49336e..0396d49122 100644 > > > --- a/drivers/vdpa/ifc/ifcvf_vdpa.c > > > +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c > > > @@ -1098,7 +1098,12 @@ ifcvf_dev_config(int vid) > > > internal = list->internal; > > > internal->vid = vid; > > > rte_atomic32_set(&internal->dev_attached, 1); > > > - update_datapath(internal); > > > + if (update_datapath(internal) < 0) { > > > + DRV_LOG(ERR, "failed to update datapath for vDPA device %s", > > > + vdev->device->name); > > > + rte_atomic32_set(&internal->dev_attached, 0); > > > + return -1; > > > + } > > > > > > hw = &internal->hw; > > > for (i = 0; i < hw->nr_vring; i++) { @@ -1146,7 +1151,12 @@ > > > ifcvf_dev_close(int vid) > > > internal->sw_fallback_running = false; > > > } else { > > > rte_atomic32_set(&internal->dev_attached, 0); > > > - update_datapath(internal); > > > + if (update_datapath(internal) < 0) { > > > + DRV_LOG(ERR, "failed to update datapath for vDPA > > > device %s", > > > + vdev->device->name); > > > + internal->configured = 0; > > > + return -1; > > > + } > > > } > > > > > > internal->configured = 0; > > > @@ -1752,7 +1762,14 @@ ifcvf_pci_probe(struct rte_pci_driver *pci_drv > > > __rte_unused, > > > } > > > > > > rte_atomic32_set(&internal->started, 1); > > > - update_datapath(internal); > > > + if (update_datapath(internal) < 0) { > > > + DRV_LOG(ERR, "failed to update datapath %s", pci_dev->name); > > > + rte_atomic32_set(&internal->started, 0); > > > + pthread_mutex_lock(&internal_list_lock); > > > + TAILQ_REMOVE(&internal_list, list, next); > > > + pthread_mutex_unlock(&internal_list_lock); > > > + goto error; > > > + } > > > > > Is it necessary to unregister vdpa device?
Good catch, yes it's needed. Kim, please add the unregistration. Thanks, Chenbo > > > > rte_kvargs_free(kvlist); > > > return 0; > > > @@ -1781,7 +1798,8 @@ ifcvf_pci_remove(struct rte_pci_device *pci_dev) > > > > > > internal = list->internal; > > > rte_atomic32_set(&internal->started, 0); > > > - update_datapath(internal); > > > + if (update_datapath(internal) < 0) > > > + DRV_LOG(ERR, "failed to update datapath %s", pci_dev->name); > > > > > > rte_pci_unmap_device(internal->pdev); > > > rte_vfio_container_destroy(internal->vfio_container_fd); > > > -- > > > 2.34.1 > > > > Reviewed-by: Chenbo Xia <[email protected]>
