From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 046EFA04FD; Thu, 10 Nov 2022 10:38:03 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E1ADD400EF; Thu, 10 Nov 2022 10:38:02 +0100 (CET) Received: from cvbackendsmtp003.nmdf.navercorp.com (cvbackendsmtp003.nmdf.navercorp.com [125.209.242.246]) by mails.dpdk.org (Postfix) with ESMTP id 9F0984003C; Tue, 8 Nov 2022 09:27:10 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=navercorp.com; s=s20171120; t=1667896029; bh=+jLACXVvSqm0QFMreCo5EyQSBcFVKVWGkRcdIkHEt+g=; h=Date:From:To:Subject:Message-ID:From:Subject:Feedback-ID: X-Works-Security; b=znsWBMG6uWQolPZksfZHAPxpigGnZv4H2fhzFJ4jFBUKrSaZ/t5zckBADx2aw6aUc 8eAhiuFdNHix/73LI/EkGHPMthso8PbbitK6GJ0i1K+KyIDoG/uZuQiL8brbmWdmxK Sfzk4Pvj4cLURX1k2b4fbbEfqNYdAIFKJFrE2itfAtnTYvAbDx+bBwAC6Mpu2huriZ cK9K+rj80blnKJmfNE+WBW5kr8qbyJ0dc+b1QRQJCGVlTQxW38COIkBl3zkLlIR9xi 27zZ6wxkOMSS/+O8BvqyiJKtg8hibxm9jdQqfvubwIvSZm6HNSF3FS9qgZOLZMa9/i mdMFCNOXqcujA== X-Session-ID: C-n6GThvQtmnD-sgRhKEXQ X-Works-Smtp-Source: VrYraAbrFoJCpNmlKoMqWHF0 Received: from dev-tkkim-git-send-email-ncl.nfra.io ([10.113.149.62]) by cvbackendsmtp003.nmdf.navercorp.com with ESMTP id C-n6GThvQtmnD-sgRhKEXQ for ; Tue, 08 Nov 2022 08:27:09 -0000 Date: Tue, 8 Nov 2022 17:27:08 +0900 From: Taekyung Kim To: "Xia, Chenbo" Cc: "Pei, Andy" , "dev@dpdk.org" , "stable@dpdk.org" , "maxime.coquelin@redhat.com" , "Wang, Xiao W" Subject: Re: [PATCH v3] vdpa/ifc: fix update_datapath error handling Message-ID: References: <20221107053411.98653-1-kim.tae.kyung@navercorp.com> <20221107085943.116357-1-kim.tae.kyung@navercorp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Mailman-Approved-At: Thu, 10 Nov 2022 10:38:01 +0100 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Nov 08, 2022 at 07:56:18AM +0000, Xia, Chenbo wrote: > > -----Original Message----- > > From: Pei, Andy > > Sent: Tuesday, November 8, 2022 3:39 PM > > To: Xia, Chenbo ; Taekyung Kim > > ; dev@dpdk.org > > Cc: stable@dpdk.org; maxime.coquelin@redhat.com; Wang, Xiao W > > > > Subject: RE: [PATCH v3] vdpa/ifc: fix update_datapath error handling > > > > Hi > > > > See my reply inline. > > > > > -----Original Message----- > > > From: Xia, Chenbo > > > Sent: Tuesday, November 8, 2022 9:47 AM > > > To: Taekyung Kim ; dev@dpdk.org > > > Cc: stable@dpdk.org; maxime.coquelin@redhat.com; Wang, Xiao W > > > > > > Subject: RE: [PATCH v3] vdpa/ifc: fix update_datapath error handling > > > > > > > -----Original Message----- > > > > From: Taekyung Kim > > > > Sent: Monday, November 7, 2022 5:00 PM > > > > To: dev@dpdk.org > > > > Cc: stable@dpdk.org; maxime.coquelin@redhat.com; Xia, Chenbo > > > > ; Wang, Xiao W ; > > > > kim.tae.kyung@navercorp.com > > > > 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: stable@dpdk.org > > > > > > > > Signed-off-by: Taekyung Kim > > > > --- > > > > 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 Hi Andy and Chenbo, Thanks for your comments. I forgot to add `rte_vdpa_unregister_device(internal->vdev)`. I will send a new patch soon. By the way, it seems that deallocation for `ifcvf_vfio_setup(internal)` is also ommitted in `ifcvf_pci_probe(...)`. I will submit another commit to split `error:` into `error2:` and `error1:`, which calls `rte_pci_unmap_device(...)` and `rte_vfio_container_destroy(...)`. Thanks, Taekyung > > > > > > > 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