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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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?

>   		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.