DPDK patches and discussions
 help / color / Atom feed
From: Gaëtan Rivet <grive@u256.net>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH 11/11] ethdev: change stop device callback to return int
Date: Thu, 15 Oct 2020 12:40:12 +0200
Message-ID: <20201015104012.f4wk4vjdtg5tm56n@u256.net> (raw)
In-Reply-To: <2e49dd52-2f48-69cf-d184-f2bccbbf0972@intel.com>

On 14/10/20 19:08 +0100, Ferruh Yigit wrote:
> On 10/14/2020 2:29 PM, Andrew Rybchenko wrote:
> > From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.ru>
> > 
> > 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 <Ivan.Ilchenko@oktetlabs.ru>
> > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> <...>
> 
> > @@ -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

  reply index

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-14 13:28 [dpdk-dev] [PATCH 00/11] ethdev: change device stop to return status Andrew Rybchenko
2020-10-14 13:28 ` [dpdk-dev] [PATCH 01/11] ethdev: change eth dev stop function to return int Andrew Rybchenko
2020-10-14 13:33   ` Thomas Monjalon
2020-10-14 13:40     ` Andrew Rybchenko
2020-10-14 17:29   ` Ferruh Yigit
2020-10-14 13:28 ` [dpdk-dev] [PATCH 02/11] test/event: check eth dev stop status Andrew Rybchenko
2020-10-14 13:28 ` [dpdk-dev] [PATCH 03/11] app: " Andrew Rybchenko
2020-10-14 17:34   ` Ferruh Yigit
2020-10-14 13:28 ` [dpdk-dev] [PATCH 04/11] examples: " Andrew Rybchenko
2020-10-14 17:38   ` Ferruh Yigit
2020-10-14 13:29 ` [dpdk-dev] [PATCH 05/11] net/bonding: " Andrew Rybchenko
2020-10-14 17:45   ` Ferruh Yigit
2020-10-14 13:29 ` [dpdk-dev] [PATCH 06/11] kni: " Andrew Rybchenko
2020-10-14 13:29 ` [dpdk-dev] [PATCH 07/11] test/bonding: " Andrew Rybchenko
2020-10-14 13:29 ` [dpdk-dev] [PATCH 08/11] app/flow-perf: " Andrew Rybchenko
2020-10-15  8:04   ` Wisam Monther
2020-10-14 13:29 ` [dpdk-dev] [PATCH 09/11] app/testpmd: " Andrew Rybchenko
2020-10-14 13:29 ` [dpdk-dev] [PATCH 10/11] net/failsafe: " Andrew Rybchenko
2020-10-15 10:37   ` [dpdk-dev] [PATCH v2] " Gaetan Rivet
2020-10-14 13:29 ` [dpdk-dev] [PATCH 11/11] ethdev: change stop device callback to return int Andrew Rybchenko
2020-10-14 18:08   ` Ferruh Yigit
2020-10-15 10:40     ` Gaëtan Rivet [this message]
2020-10-15 13:30 ` [dpdk-dev] [PATCH v2 00/11] ethdev: change device stop to return status Andrew Rybchenko
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 01/11] ethdev: change eth dev stop function to return int Andrew Rybchenko
2020-10-16  9:22     ` Ferruh Yigit
2020-10-16 11:20     ` Kinsella, Ray
2020-10-16 17:13       ` Andrew Rybchenko
2020-10-19  9:37         ` Kinsella, Ray
2020-10-16 16:20     ` Ferruh Yigit
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 02/11] test/event: check eth dev stop status Andrew Rybchenko
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 03/11] app: " Andrew Rybchenko
2020-10-16  1:28     ` Min Hu (Connor)
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 04/11] examples: " Andrew Rybchenko
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 05/11] net/bonding: " Andrew Rybchenko
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 06/11] kni: " Andrew Rybchenko
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 07/11] test/bonding: " Andrew Rybchenko
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 08/11] app/flow-perf: " Andrew Rybchenko
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 09/11] app/testpmd: " Andrew Rybchenko
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 10/11] net/failsafe: " Andrew Rybchenko
2020-10-15 13:30   ` [dpdk-dev] [PATCH v2 11/11] ethdev: change stop device callback to return int Andrew Rybchenko
2020-10-16 18:37     ` Ferruh Yigit
2020-10-18  8:55     ` Xu, Rosen
2020-10-16 18:54   ` [dpdk-dev] [PATCH v2 00/11] ethdev: change device stop to return status Ferruh Yigit

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201015104012.f4wk4vjdtg5tm56n@u256.net \
    --to=grive@u256.net \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox