From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 35A42A04DB; Thu, 15 Oct 2020 12:44:58 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 58FAA1DEB4; Thu, 15 Oct 2020 12:40:20 +0200 (CEST) Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by dpdk.org (Postfix) with ESMTP id ED1EB1DEA5 for ; Thu, 15 Oct 2020 12:40:17 +0200 (CEST) X-Originating-IP: 86.254.165.59 Received: from u256.net (lfbn-poi-1-843-59.w86-254.abo.wanadoo.fr [86.254.165.59]) (Authenticated sender: grive@u256.net) by relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 90B3220009; Thu, 15 Oct 2020 10:40:16 +0000 (UTC) Date: Thu, 15 Oct 2020 12:40:12 +0200 From: =?utf-8?Q?Ga=C3=ABtan?= Rivet To: Ferruh Yigit Cc: dev@dpdk.org Message-ID: <20201015104012.f4wk4vjdtg5tm56n@u256.net> References: <1602682146-4722-1-git-send-email-arybchenko@solarflare.com> <1602682146-4722-12-git-send-email-arybchenko@solarflare.com> <2e49dd52-2f48-69cf-d184-f2bccbbf0972@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2e49dd52-2f48-69cf-d184-f2bccbbf0972@intel.com> Subject: Re: [dpdk-dev] [PATCH 11/11] ethdev: change stop device callback to return int X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 14/10/20 19:08 +0100, Ferruh Yigit wrote: > On 10/14/2020 2:29 PM, Andrew Rybchenko wrote: > > From: Ivan Ilchenko > > > > Change eth_dev_stop_t return value from void to int. > > Make eth_dev_stop_t implementations across all drivers to return > > negative errno values if case of error conditions. > > > > Signed-off-by: Ivan Ilchenko > > Signed-off-by: Andrew Rybchenko > > <...> > > > @@ -599,7 +599,7 @@ atl_dev_start(struct rte_eth_dev *dev) > > /* > > * Stop device: disable rx and tx functions to allow for reconfiguring. > > */ > > -static void > > +static int > > atl_dev_stop(struct rte_eth_dev *dev) > > { > > struct rte_eth_link link; > > @@ -639,6 +639,8 @@ atl_dev_stop(struct rte_eth_dev *dev) > > rte_free(intr_handle->intr_vec); > > intr_handle->intr_vec = NULL; > > } > > + > > + return 0; > > } > > /* > > @@ -689,6 +691,7 @@ atl_dev_close(struct rte_eth_dev *dev) > > struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev); > > struct rte_intr_handle *intr_handle = &pci_dev->intr_handle; > > struct aq_hw_s *hw; > > + int ret; > > PMD_INIT_FUNC_TRACE(); > > @@ -697,7 +700,9 @@ atl_dev_close(struct rte_eth_dev *dev) > > hw = ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private); > > - atl_dev_stop(dev); > > + ret = atl_dev_stop(dev); > > + if (ret != 0) > > + return ret; > > atl_free_queues(dev); > > > > Not for this driver, but for all drivers, > > If we return immediately on the 'stop()' error, the 'close()' function won't > run to the end and it won't free all resources. > And according Thomas's patch, even if 'close()' dev_ops failed, the > resources will be released and pointers will be set to null causing a > possible resource leak. > > What do you think don't return immediately if 'stop()' failes but store the > error value and continue the 'close()', return that stored error at the end > of the 'close()', briefly as following: > > dev_close() { > ... > ret = stop() > > ... continue close .. > > return ret; > } > > What do you think? > > > <...> > > > diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c > > index 4fbb7e1da0..8db7d85b04 100644 > > --- a/drivers/net/failsafe/failsafe_ops.c > > +++ b/drivers/net/failsafe/failsafe_ops.c > > @@ -179,22 +179,32 @@ fs_set_queues_state_stop(struct rte_eth_dev *dev) > > RTE_ETH_QUEUE_STATE_STOPPED; > > } > > -static void > > +static int > > fs_dev_stop(struct rte_eth_dev *dev) > > { > > struct sub_device *sdev; > > uint8_t i; > > + int ret; > > fs_lock(dev, 0); > > PRIV(dev)->state = DEV_STARTED - 1; > > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_STARTED) { > > - rte_eth_dev_stop(PORT_ID(sdev)); > > + ret = rte_eth_dev_stop(PORT_ID(sdev)); > > + if (ret != 0) { > > + ERROR("Failed to stop device %u", > > + PORT_ID(sdev)); > > + PRIV(dev)->state = DEV_STARTED + 1; > > + fs_unlock(dev, 0); > > + return ret; > > + } > > Not sure to return after first error, or should continue the loop and return > error afterwards? > > @Gaten, what do you say? > I've sent a v2 on this patch with some nitpicks, but in this case, the logic to stop the loop was already there and the change did not introduce it, it's correct IMO. Thanks for bringing it to my attention though! > > failsafe_rx_intr_uninstall_subdevice(sdev); > > sdev->state = DEV_STARTED - 1; > > } > > failsafe_rx_intr_uninstall(dev); > > fs_set_queues_state_stop(dev); > > fs_unlock(dev, 0); > > + > > + return 0; > > } > > static int > > @@ -644,8 +654,13 @@ failsafe_eth_dev_close(struct rte_eth_dev *dev) > > fs_lock(dev, 0); > > failsafe_hotplug_alarm_cancel(dev); > > - if (PRIV(dev)->state == DEV_STARTED) > > - dev->dev_ops->dev_stop(dev); > > + if (PRIV(dev)->state == DEV_STARTED) { > > + ret = dev->dev_ops->dev_stop(dev); > > + if (ret != 0) { > > + fs_unlock(dev, 0); > > + return ret; > > + } > > + } > > ditto. > -- Gaëtan