Re: [dpdk-dev] [PATCH 11/11] ethdev: change stop device callback to return int

["Gaëtan Rivet" <grive@u256.net>]

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