diff mbox series

[v2,02/54] ethdev: change rte_eth_dev_info_get() return value to int

Message ID 1567519051-28189-3-git-send-email-arybchenko@solarflare.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: change rte_eth_dev_info_get() return value to int |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Andrew Rybchenko Sept. 3, 2019, 1:56 p.m. UTC 
  From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.com>

Change rte_eth_dev_info_get() return value from void to int and return
negative errno values in case of error conditions.
Modify rte_eth_dev_info_get() usage across the ethdev according
to new return type.

Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.com>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 doc/guides/rel_notes/deprecation.rst   |  1 -
 doc/guides/rel_notes/release_19_11.rst |  5 ++-
 lib/librte_ethdev/rte_ethdev.c         | 71 ++++++++++++++++++++++++----------
 lib/librte_ethdev/rte_ethdev.h         |  6 ++-
 4 files changed, 60 insertions(+), 23 deletions(-)

Comments

Ferruh Yigit Sept. 4, 2019, 4:40 p.m. UTC | #1 
On 9/3/2019 2:56 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.com>
> 
> Change rte_eth_dev_info_get() return value from void to int and return
> negative errno values in case of error conditions.
> Modify rte_eth_dev_info_get() usage across the ethdev according
> to new return type.
> 
> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.com>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

<...>

> @@ -1144,7 +1143,9 @@ struct rte_eth_dev *

The diff output seems not able to detect the function/block which makes it
harder to review, same patch I am getting a diff output like below:
 @@ -1144,7 +1143,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q,
uint16_t nb_tx_q,

Can you please either check a newer version of git, as far as I can see you are
using '1.8.3.1', or can you please try by removing/updating '.gitattributes' file?

>  	 */
>  	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
>  
> -	rte_eth_dev_info_get(port_id, &dev_info);
> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
> +	if (ret != 0)
> +		return ret;

'dev->data->dev_conf' should be restored from 'orig_conf' on failure, before return.

<...>

> -void
> +int
>  rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>  {
>  	struct rte_eth_dev *dev;
> @@ -2558,7 +2566,7 @@ struct rte_eth_dev *
>  	 */
>  	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>  
> -	RTE_ETH_VALID_PORTID_OR_RET(port_id);
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	dev = &rte_eth_devices[port_id];
>  
>  	dev_info->rx_desc_lim = lim;
> @@ -2567,13 +2575,15 @@ struct rte_eth_dev *
>  	dev_info->min_mtu = RTE_ETHER_MIN_MTU;
>  	dev_info->max_mtu = UINT16_MAX;
>  
> -	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
>  	(*dev->dev_ops->dev_infos_get)(dev, dev_info);
>  	dev_info->driver_name = dev->device->driver->name;
>  	dev_info->nb_rx_queues = dev->data->nb_rx_queues;
>  	dev_info->nb_tx_queues = dev->data->nb_tx_queues;
>  
>  	dev_info->dev_flags = &dev->data->dev_flags;
> +
> +	return 0;

Should API use "return eth_err(port_id, ret);" syntax, as other APIs do,
I am not very clear about the 'eth_err()', Thomas can provide better
description/suggestion on this.

<...>

> @@ -3100,9 +3118,13 @@ struct rte_eth_dev *
>  	struct rte_eth_dev_info dev_info;
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	unsigned i;
> +	int ret;
>  
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> -	rte_eth_dev_info_get(port_id, &dev_info);
> +
> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
> +	if (ret != 0)
> +		return ret;

This is 'get_mac_addr_index()' static function, and it should either send
negative value for error, or 'index' value in case of success. So should we be
sure here only negative value sent? Incase 'rte_eth_dev_info_get()' returns
positive value in the future.

>  
>  	for (i = 0; i < dev_info.max_mac_addrs; i++)
>  		if (memcmp(addr, &dev->data->mac_addrs[i],
> @@ -3233,8 +3255,14 @@ struct rte_eth_dev *
>  	struct rte_eth_dev_info dev_info;
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>  	unsigned i;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
> +	if (ret != 0)
> +		return ret;

Same comment as above, for this 'get_hash_mac_addr_index()' static function.
Andrew Rybchenko Sept. 6, 2019, 7:30 a.m. UTC | #2 
On 9/4/19 7:40 PM, Ferruh Yigit wrote:
> On 9/3/2019 2:56 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.com>
>>
>> Change rte_eth_dev_info_get() return value from void to int and return
>> negative errno values in case of error conditions.
>> Modify rte_eth_dev_info_get() usage across the ethdev according
>> to new return type.
>>
>> Signed-off-by: Ivan Ilchenko <Ivan.Ilchenko@oktetlabs.com>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> <...>
>
>> @@ -1144,7 +1143,9 @@ struct rte_eth_dev *
> The diff output seems not able to detect the function/block which makes it
> harder to review, same patch I am getting a diff output like below:
>   @@ -1144,7 +1143,9 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q,
> uint16_t nb_tx_q,
>
> Can you please either check a newer version of git, as far as I can see you are
> using '1.8.3.1', or can you please try by removing/updating '.gitattributes' file?

Newer Git version solves the problem.

>>   	 */
>>   	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
>>   
>> -	rte_eth_dev_info_get(port_id, &dev_info);
>> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
>> +	if (ret != 0)
>> +		return ret;
> 'dev->data->dev_conf' should be restored from 'orig_conf' on failure, before return.

Thanks, will fix in v3.

> <...>
>
>> -void
>> +int
>>   rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>>   {
>>   	struct rte_eth_dev *dev;
>> @@ -2558,7 +2566,7 @@ struct rte_eth_dev *
>>   	 */
>>   	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
>>   
>> -	RTE_ETH_VALID_PORTID_OR_RET(port_id);
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>   	dev = &rte_eth_devices[port_id];
>>   
>>   	dev_info->rx_desc_lim = lim;
>> @@ -2567,13 +2575,15 @@ struct rte_eth_dev *
>>   	dev_info->min_mtu = RTE_ETHER_MIN_MTU;
>>   	dev_info->max_mtu = UINT16_MAX;
>>   
>> -	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
>>   	(*dev->dev_ops->dev_infos_get)(dev, dev_info);
>>   	dev_info->driver_name = dev->device->driver->name;
>>   	dev_info->nb_rx_queues = dev->data->nb_rx_queues;
>>   	dev_info->nb_tx_queues = dev->data->nb_tx_queues;
>>   
>>   	dev_info->dev_flags = &dev->data->dev_flags;
>> +
>> +	return 0;
> Should API use "return eth_err(port_id, ret);" syntax, as other APIs do,
> I am not very clear about the 'eth_err()', Thomas can provide better
> description/suggestion on this.

In this particular case eth_err() is irrelevant. It makes sense when
callback is updated to return int and I'll use it there.
In any case better description would be useful.

> <...>
>
>> @@ -3100,9 +3118,13 @@ struct rte_eth_dev *
>>   	struct rte_eth_dev_info dev_info;
>>   	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>   	unsigned i;
>> +	int ret;
>>   
>>   	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> -	rte_eth_dev_info_get(port_id, &dev_info);
>> +
>> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
>> +	if (ret != 0)
>> +		return ret;
> This is 'get_mac_addr_index()' static function, and it should either send
> negative value for error, or 'index' value in case of success. So should we be
> sure here only negative value sent? Incase 'rte_eth_dev_info_get()' returns
> positive value in the future.

In fact, I think there is no necessity to check port_id here since it is 
a static
function and port_id is now checked in rte_eth_dev_info_get() anyway.
Also, I think it is better to return -1 here since it is bad to mix 
-errno and -1
return codes. -errno would be useful if caller really takes it into 
account and
have different processing for -ENOENT which makes sense in this case.

>>   
>>   	for (i = 0; i < dev_info.max_mac_addrs; i++)
>>   		if (memcmp(addr, &dev->data->mac_addrs[i],
>> @@ -3233,8 +3255,14 @@ struct rte_eth_dev *
>>   	struct rte_eth_dev_info dev_info;
>>   	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
>>   	unsigned i;
>> +	int ret;
>> +
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +
>> +	ret = rte_eth_dev_info_get(port_id, &dev_info);
>> +	if (ret != 0)
>> +		return ret;
> Same comment as above, for this 'get_hash_mac_addr_index()' static function.

Same as above.

Many thanks for review.
diff mbox series

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 0ee8533..cbb4c34 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -88,7 +88,6 @@  Deprecation Notices
   negative errno values to indicate various error conditions (e.g.
   invalid port ID, unsupported operation, failed operation):
 
-  - ``rte_eth_dev_info_get``
   - ``rte_eth_promiscuous_enable`` and ``rte_eth_promiscuous_disable``
   - ``rte_eth_allmulticast_enable`` and ``rte_eth_allmulticast_disable``
   - ``rte_eth_link_get`` and ``rte_eth_link_get_nowait``
diff --git a/doc/guides/rel_notes/release_19_11.rst b/doc/guides/rel_notes/release_19_11.rst
index 27cfbd9..152f120 100644
--- a/doc/guides/rel_notes/release_19_11.rst
+++ b/doc/guides/rel_notes/release_19_11.rst
@@ -94,6 +94,9 @@  API Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* ethdev: changed ``rte_eth_dev_infos_get`` return value from ``void`` to
+  ``int`` to provide a way to report various error conditions.
+
 
 ABI Changes
 -----------
@@ -145,7 +148,7 @@  The libraries prepended with a plus sign were incremented in this version.
      librte_distributor.so.1
      librte_eal.so.11
      librte_efd.so.1
-     librte_ethdev.so.12
+   + librte_ethdev.so.13
      librte_eventdev.so.7
      librte_flow_classify.so.1
      librte_gro.so.1
diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 17d183e..4b6cbe2 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1125,7 +1125,6 @@  struct rte_eth_dev *
 
 	dev = &rte_eth_devices[port_id];
 
-	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
 	if (dev->data->dev_started) {
@@ -1144,7 +1143,9 @@  struct rte_eth_dev *
 	 */
 	memcpy(&dev->data->dev_conf, dev_conf, sizeof(dev->data->dev_conf));
 
-	rte_eth_dev_info_get(port_id, &dev_info);
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
 
 	/* If number of queues specified by application for both Rx and Tx is
 	 * zero, use driver preferred values. This cannot be done individually
@@ -1406,6 +1407,7 @@  struct rte_eth_dev *
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
 	int diag;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1420,7 +1422,9 @@  struct rte_eth_dev *
 		return 0;
 	}
 
-	rte_eth_dev_info_get(port_id, &dev_info);
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
 
 	/* Lets restore MAC now if device does not support live change */
 	if (*dev_info.dev_flags & RTE_ETH_DEV_NOLIVE_MAC_ADDR)
@@ -1584,7 +1588,6 @@  struct rte_eth_dev *
 		return -EINVAL;
 	}
 
-	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_setup, -ENOTSUP);
 
 	/*
@@ -1592,7 +1595,10 @@  struct rte_eth_dev *
 	 * This value must be provided in the private data of the memory pool.
 	 * First check that the memory pool has a valid private data.
 	 */
-	rte_eth_dev_info_get(port_id, &dev_info);
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
+
 	if (mp->private_data_size < sizeof(struct rte_pktmbuf_pool_private)) {
 		RTE_ETHDEV_LOG(ERR, "%s private_data_size %d < %d\n",
 			mp->name, (int)mp->private_data_size,
@@ -1703,6 +1709,7 @@  struct rte_eth_dev *
 	struct rte_eth_dev_info dev_info;
 	struct rte_eth_txconf local_conf;
 	void **txq;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
@@ -1712,10 +1719,11 @@  struct rte_eth_dev *
 		return -EINVAL;
 	}
 
-	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_setup, -ENOTSUP);
 
-	rte_eth_dev_info_get(port_id, &dev_info);
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
 
 	/* Use default specified by driver, if nb_tx_desc is zero */
 	if (nb_tx_desc == 0) {
@@ -2540,7 +2548,7 @@  struct rte_eth_dev *
 							fw_version, fw_size));
 }
 
-void
+int
 rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
 {
 	struct rte_eth_dev *dev;
@@ -2558,7 +2566,7 @@  struct rte_eth_dev *
 	 */
 	memset(dev_info, 0, sizeof(struct rte_eth_dev_info));
 
-	RTE_ETH_VALID_PORTID_OR_RET(port_id);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
 	dev_info->rx_desc_lim = lim;
@@ -2567,13 +2575,15 @@  struct rte_eth_dev *
 	dev_info->min_mtu = RTE_ETHER_MIN_MTU;
 	dev_info->max_mtu = UINT16_MAX;
 
-	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	(*dev->dev_ops->dev_infos_get)(dev, dev_info);
 	dev_info->driver_name = dev->device->driver->name;
 	dev_info->nb_rx_queues = dev->data->nb_rx_queues;
 	dev_info->nb_tx_queues = dev->data->nb_tx_queues;
 
 	dev_info->dev_flags = &dev->data->dev_flags;
+
+	return 0;
 }
 
 int
@@ -2643,7 +2653,10 @@  struct rte_eth_dev *
 	 * which relies on dev->dev_ops->dev_infos_get.
 	 */
 	if (*dev->dev_ops->dev_infos_get != NULL) {
-		rte_eth_dev_info_get(port_id, &dev_info);
+		ret = rte_eth_dev_info_get(port_id, &dev_info);
+		if (ret != 0)
+			return ret;
+
 		if (mtu < dev_info.min_mtu || mtu > dev_info.max_mtu)
 			return -EINVAL;
 	}
@@ -2991,10 +3004,15 @@  struct rte_eth_dev *
 {
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info = { .flow_type_rss_offloads = 0, };
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
+
 	dev = &rte_eth_devices[port_id];
-	rte_eth_dev_info_get(port_id, &dev_info);
 	if ((dev_info.flow_type_rss_offloads | rss_conf->rss_hf) !=
 	    dev_info.flow_type_rss_offloads) {
 		RTE_ETHDEV_LOG(ERR,
@@ -3100,9 +3118,13 @@  struct rte_eth_dev *
 	struct rte_eth_dev_info dev_info;
 	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
 	unsigned i;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
-	rte_eth_dev_info_get(port_id, &dev_info);
+
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
 
 	for (i = 0; i < dev_info.max_mac_addrs; i++)
 		if (memcmp(addr, &dev->data->mac_addrs[i],
@@ -3233,8 +3255,14 @@  struct rte_eth_dev *
 	struct rte_eth_dev_info dev_info;
 	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
 	unsigned i;
+	int ret;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
 
-	rte_eth_dev_info_get(port_id, &dev_info);
 	if (!dev->data->hash_mac_addrs)
 		return -1;
 
@@ -3319,11 +3347,15 @@  int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
 	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
 	struct rte_eth_link link;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
+
 	dev = &rte_eth_devices[port_id];
-	rte_eth_dev_info_get(port_id, &dev_info);
 	link = dev->data->dev_link;
 
 	if (queue_idx > dev_info.max_tx_queues) {
@@ -4363,15 +4395,14 @@  int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
 				 uint16_t *nb_rx_desc,
 				 uint16_t *nb_tx_desc)
 {
-	struct rte_eth_dev *dev;
 	struct rte_eth_dev_info dev_info;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
-	dev = &rte_eth_devices[port_id];
-	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
-
-	rte_eth_dev_info_get(port_id, &dev_info);
+	ret = rte_eth_dev_info_get(port_id, &dev_info);
+	if (ret != 0)
+		return ret;
 
 	if (nb_rx_desc != NULL)
 		rte_eth_dev_adjust_nb_desc(nb_rx_desc, &dev_info.rx_desc_lim);
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index 8fa89bf..eda9e5c 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -2366,8 +2366,12 @@  int rte_eth_dev_set_rx_queue_stats_mapping(uint16_t port_id,
  * @param dev_info
  *   A pointer to a structure of type *rte_eth_dev_info* to be filled with
  *   the contextual information of the Ethernet device.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if support for dev_infos_get() does not exist for the device.
+ *   - (-ENODEV) if *port_id* invalid.
  */
-void rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
+int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info);
 
 /**
  * Retrieve the firmware version of a device.