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 5599CA04DB; Wed, 14 Oct 2020 20:08:41 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B30E91DA8C; Wed, 14 Oct 2020 20:08:39 +0200 (CEST) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 4D2751BDDE for ; Wed, 14 Oct 2020 20:08:35 +0200 (CEST) IronPort-SDR: PjUSiCENJ8P0lgIn2dMq4PK5AVV4GqV/9f5TiyGwYoQ019ChYfum7Ql1T80bATUq3ALIkWEVya IZQNubNSaB9Q== X-IronPort-AV: E=McAfee;i="6000,8403,9774"; a="153101483" X-IronPort-AV: E=Sophos;i="5.77,375,1596524400"; d="scan'208";a="153101483" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Oct 2020 11:08:33 -0700 IronPort-SDR: xXSoLGXvpOL6CcNMi/NcwxYvJtt0J3KUpGD3+l7D5BJsI7VZTyMkCvMqysd9F+3fUxfVmwfCwC Jlf+vDlLazUg== X-IronPort-AV: E=Sophos;i="5.77,375,1596524400"; d="scan'208";a="463973414" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.212.224]) ([10.213.212.224]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Oct 2020 11:08:19 -0700 To: Andrew Rybchenko , "John W. Linville" , Ciara Loftus , Qi Zhang , Shepard Siegel , Ed Czeck , John Miller , Igor Russkikh , Pavel Belous , Steven Webster , Matt Peters , Somalapuram Amaranath , Rasesh Mody , Shahed Shaikh , Ajit Khaparde , Somnath Kotur , Chas Williams , "Min Hu (Connor)" , Rahul Lakkireddy , Hemant Agrawal , Sachin Saxena , Jeff Guo , Haiyue Wang , Marcin Wojtas , Michal Krawczyk , Guy Tzalik , Evgeny Schemeilin , Igor Chauskin , Gagandeep Singh , John Daley , Hyong Youb Kim , Gaetan Rivet , Xiao Wang , Ziyang Xuan , Xiaoyun Wang , Guoyang Zhou , "Wei Hu (Xavier)" , Yisen Zhuang , Beilei Xing , Jingjing Wu , Qiming Yang , Alfredo Cardigliano , Rosen Xu , Shijith Thotton , Srisivasubramanian Srinivasan , Matan Azrad , Shahaf Shuler , Viacheslav Ovsiienko , Zyta Szpak , Liron Himi , Stephen Hemminger , "K. Y. Srinivasan" , Haiyang Zhang , Long Li , Martin Spinler , Heinrich Kuhn , Tetsuya Mukawa , Harman Kalra , Jerin Jacob , Nithin Dabilpuram , Kiran Kumar K , Akhil Goyal , Bruce Richardson , Andrew Rybchenko , Jasvinder Singh , Cristian Dumitrescu , Keith Wiles , Maciej Czekaj , Maxime Coquelin , Chenbo Xia , Zhihong Wang , Yong Wang , Thomas Monjalon Cc: dev@dpdk.org, Ivan Ilchenko References: <1602682146-4722-1-git-send-email-arybchenko@solarflare.com> <1602682146-4722-12-git-send-email-arybchenko@solarflare.com> From: Ferruh Yigit Message-ID: <2e49dd52-2f48-69cf-d184-f2bccbbf0972@intel.com> Date: Wed, 14 Oct 2020 19:08:15 +0100 MIME-Version: 1.0 In-Reply-To: <1602682146-4722-12-git-send-email-arybchenko@solarflare.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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 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? > 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.