Re: [dpdk-dev] [PATCH v2 01/11] ethdev: change eth dev stop function to return int

["Kinsella, Ray" <mdr@ashroe.eu>]

On 16/10/2020 18:13, Andrew Rybchenko wrote:
> On 10/16/20 2:20 PM, Kinsella, Ray wrote:
>> On 15/10/2020 14:30, Andrew Rybchenko wrote:
>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>
>>> Change rte_eth_dev_stop() return value from void to int
>>> and return negative errno values in case of error conditions.
>>> Also update the usage of the function in ethdev according to
>>> the new return type.
>>>
>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> Acked-by: Thomas Monjalon <thomas@monjalon.net>
>>> ---
>>>  doc/guides/rel_notes/deprecation.rst   |  1 -
>>>  doc/guides/rel_notes/release_20_11.rst |  3 +++
>>>  lib/librte_ethdev/rte_ethdev.c         | 27 +++++++++++++++++++-------
>>>  lib/librte_ethdev/rte_ethdev.h         |  5 ++++-
>>>  4 files changed, 27 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>>> index d1f5ed39db..2e04e24374 100644
>>> --- a/doc/guides/rel_notes/deprecation.rst
>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>> @@ -127,7 +127,6 @@ Deprecation Notices
>>>    negative errno values to indicate various error conditions (e.g.
>>>    invalid port ID, unsupported operation, failed operation):
>>>  
>>> -  - ``rte_eth_dev_stop``
>>>    - ``rte_eth_dev_close``
>>>  
>>>  * ethdev: New offload flags ``DEV_RX_OFFLOAD_FLOW_MARK`` will be added in 19.11.
>>> diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst
>>> index f8686a50db..c8c30937fa 100644
>>> --- a/doc/guides/rel_notes/release_20_11.rst
>>> +++ b/doc/guides/rel_notes/release_20_11.rst
>>> @@ -355,6 +355,9 @@ API Changes
>>>  * vhost: Add a new function ``rte_vhost_crypto_driver_start`` to be called
>>>    instead of ``rte_vhost_driver_start`` by crypto applications.
>>>  
>>> +* ethdev: changed ``rte_eth_dev_stop`` return value from ``void`` to
>>> +  ``int`` to provide a way to report various error conditions.
>>> +
>>>  
>>>  ABI Changes
>>>  -----------
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index d9b82df073..b8cf04ef4d 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -1661,7 +1661,7 @@ rte_eth_dev_start(uint16_t port_id)
>>>  	struct rte_eth_dev *dev;
>>>  	struct rte_eth_dev_info dev_info;
>>>  	int diag;
>>> -	int ret;
>>> +	int ret, ret_stop;
>>>  
>>>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>>>  
>>> @@ -1695,7 +1695,13 @@ rte_eth_dev_start(uint16_t port_id)
>>>  		RTE_ETHDEV_LOG(ERR,
>>>  			"Error during restoring configuration for device (port %u): %s\n",
>>>  			port_id, rte_strerror(-ret));
>>> -		rte_eth_dev_stop(port_id);
>>> +		ret_stop = rte_eth_dev_stop(port_id);
>>> +		if (ret_stop != 0) {
>>> +			RTE_ETHDEV_LOG(ERR,
>>> +				"Failed to stop device (port %u): %s\n",
>>> +				port_id, rte_strerror(-ret_stop));
>>> +		}
>>> +
>>>  		return ret;
>>>  	}
>>>  
>>> @@ -1708,26 +1714,28 @@ rte_eth_dev_start(uint16_t port_id)
>>>  	return 0;
>>>  }
>>>  
>>> -void
>>> +int
>>>  rte_eth_dev_stop(uint16_t port_id)
>>>  {
>>>  	struct rte_eth_dev *dev;
>>>  
>>> -	RTE_ETH_VALID_PORTID_OR_RET(port_id);
>>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>  	dev = &rte_eth_devices[port_id];
>>>  
>>> -	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_stop);
>>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_stop, -ENOTSUP);
>>>  
>>>  	if (dev->data->dev_started == 0) {
>>>  		RTE_ETHDEV_LOG(INFO,
>>>  			"Device with port_id=%"PRIu16" already stopped\n",
>>>  			port_id);
>>> -		return;
>>> +		return 0;
>>>  	}
>>>  
>>>  	dev->data->dev_started = 0;
>>>  	(*dev->dev_ops->dev_stop)(dev);
>>>  	rte_ethdev_trace_stop(port_id);
>>> +
>>> +	return 0;
>>>  }
>>>  
>>>  int
>>> @@ -1783,7 +1791,12 @@ rte_eth_dev_reset(uint16_t port_id)
>>>  
>>>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_reset, -ENOTSUP);
>>>  
>>> -	rte_eth_dev_stop(port_id);
>>> +	ret = rte_eth_dev_stop(port_id);
>>> +	if (ret != 0) {
>>> +		RTE_ETHDEV_LOG(ERR,
>>> +			"Failed to stop device (port %u) before reset: %s - ignore\n",
>>> +			port_id, rte_strerror(-ret));
>> ABI change is 100%,
>> Just question the logic of continuing here to do a reset, if you failed to stop the device.
> 
> In the case of reset I'm sure that we should ignore stop failure here.
> Typically reset is required to recover from bad state etc and stop
> failure in such condition could definitely happen.

Ok - thanks for that.