[dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Andrew Rybchenko]

Use ENODEV as the error code if specified port ID is invalid.

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
 lib/librte_ethdev/rte_ethdev.h | 46 +++++++++++++++++++++++-----------
 2 files changed, 54 insertions(+), 36 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 5b7979a3b8..1f862f918a 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
 {
 	char *tmp;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	if (name == NULL) {
 		RTE_ETHDEV_LOG(ERR, "Null pointer is specified\n");
@@ -931,7 +931,7 @@ rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id)
 	struct rte_eth_dev *dev;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 	if (!dev->data->dev_started) {
@@ -972,7 +972,7 @@ rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id)
 	struct rte_eth_dev *dev;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
@@ -1006,7 +1006,7 @@ rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id)
 	struct rte_eth_dev *dev;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 	if (!dev->data->dev_started) {
@@ -1045,7 +1045,7 @@ rte_eth_dev_tx_queue_stop(uint16_t port_id, uint16_t tx_queue_id)
 	struct rte_eth_dev *dev;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
@@ -1278,7 +1278,7 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	int diag;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
@@ -1664,7 +1664,7 @@ rte_eth_dev_start(uint16_t port_id)
 	int diag;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
@@ -1736,7 +1736,7 @@ rte_eth_dev_set_link_up(uint16_t port_id)
 {
 	struct rte_eth_dev *dev;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
@@ -1749,7 +1749,7 @@ rte_eth_dev_set_link_down(uint16_t port_id)
 {
 	struct rte_eth_dev *dev;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
@@ -1779,7 +1779,7 @@ rte_eth_dev_reset(uint16_t port_id)
 	struct rte_eth_dev *dev;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	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_reset, -ENOTSUP);
@@ -1826,7 +1826,7 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 	struct rte_eth_rxconf local_conf;
 	void **rxq;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 	if (rx_queue_id >= dev->data->nb_rx_queues) {
@@ -1981,7 +1981,7 @@ rte_eth_rx_hairpin_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 	int i;
 	int count;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 	if (rx_queue_id >= dev->data->nb_rx_queues) {
@@ -2052,7 +2052,7 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 	void **txq;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 	if (tx_queue_id >= dev->data->nb_tx_queues) {
@@ -2153,7 +2153,7 @@ rte_eth_tx_hairpin_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 	int count;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 	if (tx_queue_id >= dev->data->nb_tx_queues) {
 		RTE_ETHDEV_LOG(ERR, "Invalid TX queue_id=%u\n", tx_queue_id);
@@ -2318,7 +2318,7 @@ rte_eth_promiscuous_get(uint16_t port_id)
 {
 	struct rte_eth_dev *dev;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 	return dev->data->promiscuous;
@@ -2369,7 +2369,7 @@ rte_eth_allmulticast_get(uint16_t port_id)
 {
 	struct rte_eth_dev *dev;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 	return dev->data->all_multicast;
@@ -2457,7 +2457,7 @@ rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
 {
 	struct rte_eth_dev *dev;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 	memset(stats, 0, sizeof(*stats));
@@ -2508,7 +2508,7 @@ get_xstats_count(uint16_t port_id)
 	struct rte_eth_dev *dev;
 	int count;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 	if (dev->dev_ops->xstats_get_names_by_id != NULL) {
 		count = (*dev->dev_ops->xstats_get_names_by_id)(dev, NULL,
@@ -2913,7 +2913,7 @@ rte_eth_xstats_get(uint16_t port_id, struct rte_eth_xstat *xstats,
 	uint16_t nb_rxqs, nb_txqs;
 	int ret;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 
 	dev = &rte_eth_devices[port_id];
 
@@ -4715,7 +4715,7 @@ rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
 	return -ENOTSUP;
 #endif
 	/* Check input parameters. */
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	if (user_cb == NULL ||
 			queue_id >= rte_eth_devices[port_id].data->nb_rx_queues)
 		return -EINVAL;
@@ -4749,7 +4749,7 @@ rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
 	return -ENOTSUP;
 #endif
 	/* Check input parameters. */
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	if (user_cb == NULL ||
 			queue_id >= rte_eth_devices[port_id].data->nb_tx_queues)
 		return -EINVAL;
@@ -5209,7 +5209,7 @@ rte_eth_dev_hairpin_capability_get(uint16_t port_id,
 {
 	struct rte_eth_dev *dev;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	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->hairpin_cap_get, -ENOTSUP);
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index f4cc5917b9..6adf5600ab 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -2033,6 +2033,7 @@ rte_eth_dev_is_removed(uint16_t port_id);
  * @return
  *   - 0: Success, receive queue correctly set up.
  *   - -EIO: if device is removed.
+ *   - -ENODEV: if *port_id* is invalid.
  *   - -EINVAL: The memory pool pointer is null or the size of network buffers
  *      which can be allocated from this memory pool does not fit the various
  *      buffer sizes allowed by the device controller.
@@ -2067,6 +2068,7 @@ int rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
  *
  * @return
  *   - (0) if successful.
+ *   - (-ENODEV) if *port_id* is invalid.
  *   - (-ENOTSUP) if hardware doesn't support.
  *   - (-EINVAL) if bad parameter.
  *   - (-ENOMEM) if unable to allocate the resources.
@@ -2148,6 +2150,7 @@ int rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
  *
  * @return
  *   - (0) if successful.
+ *   - (-ENODEV) if *port_id* is invalid.
  *   - (-ENOTSUP) if hardware doesn't support.
  *   - (-EINVAL) if bad parameter.
  *   - (-ENOMEM) if unable to allocate the resources.
@@ -2192,7 +2195,8 @@ int rte_eth_dev_is_valid_port(uint16_t port_id);
  *   to rte_eth_dev_configure().
  * @return
  *   - 0: Success, the receive queue is started.
- *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
+ *   - -ENODEV: if *port_id* is invalid.
+ *   - -EINVAL: The queue_id out of range or belong to hairpin.
  *   - -EIO: if device is removed.
  *   - -ENOTSUP: The function not supported in PMD driver.
  */
@@ -2209,7 +2213,8 @@ int rte_eth_dev_rx_queue_start(uint16_t port_id, uint16_t rx_queue_id);
  *   to rte_eth_dev_configure().
  * @return
  *   - 0: Success, the receive queue is stopped.
- *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
+ *   - -ENODEV: if *port_id* is invalid.
+ *   - -EINVAL: The queue_id out of range or belong to hairpin.
  *   - -EIO: if device is removed.
  *   - -ENOTSUP: The function not supported in PMD driver.
  */
@@ -2227,7 +2232,8 @@ int rte_eth_dev_rx_queue_stop(uint16_t port_id, uint16_t rx_queue_id);
  *   to rte_eth_dev_configure().
  * @return
  *   - 0: Success, the transmit queue is started.
- *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
+ *   - -ENODEV: if *port_id* is invalid.
+ *   - -EINVAL: The queue_id out of range or belong to hairpin.
  *   - -EIO: if device is removed.
  *   - -ENOTSUP: The function not supported in PMD driver.
  */
@@ -2244,7 +2250,8 @@ int rte_eth_dev_tx_queue_start(uint16_t port_id, uint16_t tx_queue_id);
  *   to rte_eth_dev_configure().
  * @return
  *   - 0: Success, the transmit queue is stopped.
- *   - -EINVAL: The port_id or the queue_id out of range or belong to hairpin.
+ *   - -ENODEV: if *port_id* is invalid.
+ *   - -EINVAL: The queue_id out of range or belong to hairpin.
  *   - -EIO: if device is removed.
  *   - -ENOTSUP: The function not supported in PMD driver.
  */
@@ -2344,7 +2351,7 @@ void rte_eth_dev_close(uint16_t port_id);
  *
  * @return
  *   - (0) if successful.
- *   - (-EINVAL) if port identifier is invalid.
+ *   - (-ENODEV) if *port_id* is invalid.
  *   - (-ENOTSUP) if hardware doesn't support this function.
  *   - (-EPERM) if not ran from the primary process.
  *   - (-EIO) if re-initialisation failed or device is removed.
@@ -3618,6 +3625,7 @@ int rte_eth_dev_default_mac_addr_set(uint16_t port_id,
  *   rte_eth_dev_info_get().
  * @return
  *   - (0) if successful.
+ *   - (-ENODEV) if *port_id* is invalid.
  *   - (-ENOTSUP) if hardware doesn't support.
  *   - (-EINVAL) if bad parameter.
  *   - (-EIO) if device is removed.
@@ -3639,6 +3647,7 @@ int rte_eth_dev_rss_reta_update(uint16_t port_id,
  *   rte_eth_dev_info_get().
  * @return
  *   - (0) if successful.
+ *   - (-ENODEV) if *port_id* is invalid.
  *   - (-ENOTSUP) if hardware doesn't support.
  *   - (-EINVAL) if bad parameter.
  *   - (-EIO) if device is removed.
@@ -4002,8 +4011,9 @@ rte_eth_add_tx_callback(uint16_t port_id, uint16_t queue_id,
  *
  * @return
  *   - 0: Success. Callback was removed.
+ *   - -ENODEV:  If *port_id* is invalid.
  *   - -ENOTSUP: Callback support is not available.
- *   - -EINVAL:  The port_id or the queue_id is out of range, or the callback
+ *   - -EINVAL:  The queue_id is out of range, or the callback
  *               is NULL or not found for the port/queue.
  */
 int rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
@@ -4035,8 +4045,9 @@ int rte_eth_remove_rx_callback(uint16_t port_id, uint16_t queue_id,
  *
  * @return
  *   - 0: Success. Callback was removed.
+ *   - -ENODEV:  If *port_id* is invalid.
  *   - -ENOTSUP: Callback support is not available.
- *   - -EINVAL:  The port_id or the queue_id is out of range, or the callback
+ *   - -EINVAL:  The queue_id is out of range, or the callback
  *               is NULL or not found for the port/queue.
  */
 int rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
@@ -4056,8 +4067,9 @@ int rte_eth_remove_tx_callback(uint16_t port_id, uint16_t queue_id,
  *
  * @return
  *   - 0: Success
+ *   - -ENODEV:  If *port_id* is invalid.
  *   - -ENOTSUP: routine is not supported by the device PMD.
- *   - -EINVAL:  The port_id or the queue_id is out of range, or the queue
+ *   - -EINVAL:  The queue_id is out of range, or the queue
  *               is hairpin queue.
  */
 int rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
@@ -4077,8 +4089,9 @@ int rte_eth_rx_queue_info_get(uint16_t port_id, uint16_t queue_id,
  *
  * @return
  *   - 0: Success
+ *   - -ENODEV:  If *port_id* is invalid.
  *   - -ENOTSUP: routine is not supported by the device PMD.
- *   - -EINVAL:  The port_id or the queue_id is out of range, or the queue
+ *   - -EINVAL:  The queue_id is out of range, or the queue
  *               is hairpin queue.
  */
 int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
@@ -4098,8 +4111,9 @@ int rte_eth_tx_queue_info_get(uint16_t port_id, uint16_t queue_id,
  *
  * @return
  *   - 0: Success
+ *   - -ENODEV:  If *port_id* is invalid.
  *   - -ENOTSUP: routine is not supported by the device PMD.
- *   - -EINVAL:  The port_id or the queue_id is out of range.
+ *   - -EINVAL:  The queue_id is out of range.
  */
 __rte_experimental
 int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
@@ -4119,8 +4133,9 @@ int rte_eth_rx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
  *
  * @return
  *   - 0: Success
+ *   - -ENODEV:  If *port_id* is invalid.
  *   - -ENOTSUP: routine is not supported by the device PMD.
- *   - -EINVAL:  The port_id or the queue_id is out of range.
+ *   - -EINVAL:  The queue_id is out of range.
  */
 __rte_experimental
 int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
@@ -4509,6 +4524,7 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id);
 *   Buffer of size RTE_ETH_NAME_MAX_LEN to store the name.
 * @return
 *   - (0) if successful.
+*   - (-ENODEV) if *port_id* is invalid.
 *   - (-EINVAL) on failure.
 */
 int
@@ -4710,7 +4726,8 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
  *  The queue id on the specific port.
  * @return
  *  The number of used descriptors in the specific queue, or:
- *     (-EINVAL) if *port_id* or *queue_id* is invalid
+ *   - (-ENODEV) if *port_id* is invalid.
+ *     (-EINVAL) if *queue_id* is invalid
  *     (-ENOTSUP) if the device does not support this function
  */
 static inline int
@@ -4718,7 +4735,7 @@ rte_eth_rx_queue_count(uint16_t port_id, uint16_t queue_id)
 {
 	struct rte_eth_dev *dev;
 
-	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_queue_count, -ENOTSUP);
 	if (queue_id >= dev->data->nb_rx_queues ||
@@ -5017,6 +5034,7 @@ rte_eth_tx_burst(uint16_t port_id, uint16_t queue_id,
  *   meet devices requirements with rte_errno set appropriately:
  *   - EINVAL: offload flags are not correctly set
  *   - ENOTSUP: the offload feature is not supported by the hardware
+ *   - ENODEV: if *port_id* is invalid (with debug enabled only)
  *
  */
 
@@ -5031,7 +5049,7 @@ rte_eth_tx_prepare(uint16_t port_id, uint16_t queue_id,
 #ifdef RTE_LIBRTE_ETHDEV_DEBUG
 	if (!rte_eth_dev_is_valid_port(port_id)) {
 		RTE_ETHDEV_LOG(ERR, "Invalid TX port_id=%u\n", port_id);
-		rte_errno = EINVAL;
+		rte_errno = ENODEV;
 		return 0;
 	}
 #endif
-- 
2.17.1

Re: [dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Ferruh Yigit]

On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
> Use ENODEV as the error code if specified port ID is invalid.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
>   lib/librte_ethdev/rte_ethdev.h | 46 +++++++++++++++++++++++-----------
>   2 files changed, 54 insertions(+), 36 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 5b7979a3b8..1f862f918a 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
>   {
>   	char *tmp;
>   
> -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);

Thanks Andrew, +1 to this error unification.

This will be API change without deprecation notice, cc'ed techboard for it.

If this should (almost) always return '-ENODEV', does it make sense to make 
another wrapper macro for it, to prevent later other error types used again.

And there are a few instances returning '-1', are they left intentionally?

Re: [dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Andrew Rybchenko]

On 10/13/20 6:32 PM, Ferruh Yigit wrote:
> On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
>> Use ENODEV as the error code if specified port ID is invalid.
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>   lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
>>   lib/librte_ethdev/rte_ethdev.h | 46 +++++++++++++++++++++++-----------
>>   2 files changed, 54 insertions(+), 36 deletions(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>> b/lib/librte_ethdev/rte_ethdev.c
>> index 5b7979a3b8..1f862f918a 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id,
>> char *name)
>>   {
>>       char *tmp;
>>   -    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> 
> Thanks Andrew, +1 to this error unification.
> 
> This will be API change without deprecation notice, cc'ed techboard for it.

Yes, thanks.

> 
> If this should (almost) always return '-ENODEV', does it make sense to
> make another wrapper macro for it, to prevent later other error types
> used again.

Unlikely, since most likely the line will be simply copied.
RTE_ETH_VALID_PORTID_OR_ERR_RET will remain in any case, so
it will be possible to misuse it anyway.

> 
> And there are a few instances returning '-1', are they left intentionally?

Yes. Inside ethdev it is either socket_id or fd in these cases.

Re: [dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Thomas Monjalon]

13/10/2020 17:32, Ferruh Yigit:
> On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
> > Use ENODEV as the error code if specified port ID is invalid.
> > 
> > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > ---
> >   lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
> >   lib/librte_ethdev/rte_ethdev.h | 46 +++++++++++++++++++++++-----------
> >   2 files changed, 54 insertions(+), 36 deletions(-)
> > 
> > diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> > index 5b7979a3b8..1f862f918a 100644
> > --- a/lib/librte_ethdev/rte_ethdev.c
> > +++ b/lib/librte_ethdev/rte_ethdev.c
> > @@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
> >   {
> >   	char *tmp;
> >   
> > -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> 
> Thanks Andrew, +1 to this error unification.
> 
> This will be API change without deprecation notice, cc'ed techboard for it.

This is an error code change, unifying assumptions across ethdev,
I would say yes to merge in 20.11.

Re: [dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Ferruh Yigit]

On 10/13/2020 4:39 PM, Andrew Rybchenko wrote:
> On 10/13/20 6:32 PM, Ferruh Yigit wrote:
>> On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
>>> Use ENODEV as the error code if specified port ID is invalid.
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>>    lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
>>>    lib/librte_ethdev/rte_ethdev.h | 46 +++++++++++++++++++++++-----------
>>>    2 files changed, 54 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>>> b/lib/librte_ethdev/rte_ethdev.c
>>> index 5b7979a3b8..1f862f918a 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id,
>>> char *name)
>>>    {
>>>        char *tmp;
>>>    -    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> Thanks Andrew, +1 to this error unification.
>>
>> This will be API change without deprecation notice, cc'ed techboard for it.
> 
> Yes, thanks.
> 
>>
>> If this should (almost) always return '-ENODEV', does it make sense to
>> make another wrapper macro for it, to prevent later other error types
>> used again.
> 
> Unlikely, since most likely the line will be simply copied.
> RTE_ETH_VALID_PORTID_OR_ERR_RET will remain in any case, so
> it will be possible to misuse it anyway.
> 

Agree it won't prevent misuse completely but may help, anyway I don't have a 
strong opinion here, if you think that is not needed, that is OK.

>>
>> And there are a few instances returning '-1', are they left intentionally?
> 
> Yes. Inside ethdev it is either socket_id or fd in these cases.
> 

Can't those two also updated to return '-ENODEV' when 'port_id' is not valid?

Re: [dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Andrew Rybchenko]

On 10/13/20 7:12 PM, Ferruh Yigit wrote:
> On 10/13/2020 4:39 PM, Andrew Rybchenko wrote:
>> On 10/13/20 6:32 PM, Ferruh Yigit wrote:
>>> On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
>>>> Use ENODEV as the error code if specified port ID is invalid.
>>>>
>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>>    lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
>>>>    lib/librte_ethdev/rte_ethdev.h | 46
>>>> +++++++++++++++++++++++-----------
>>>>    2 files changed, 54 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>>>> b/lib/librte_ethdev/rte_ethdev.c
>>>> index 5b7979a3b8..1f862f918a 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>> @@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id,
>>>> char *name)
>>>>    {
>>>>        char *tmp;
>>>>    -    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>>>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>
>>> Thanks Andrew, +1 to this error unification.
>>>
>>> This will be API change without deprecation notice, cc'ed techboard
>>> for it.
>>
>> Yes, thanks.
>>
>>>
>>> If this should (almost) always return '-ENODEV', does it make sense to
>>> make another wrapper macro for it, to prevent later other error types
>>> used again.
>>
>> Unlikely, since most likely the line will be simply copied.
>> RTE_ETH_VALID_PORTID_OR_ERR_RET will remain in any case, so
>> it will be possible to misuse it anyway.
>>
> 
> Agree it won't prevent misuse completely but may help, anyway I don't
> have a strong opinion here, if you think that is not needed, that is OK.
> 
>>>
>>> And there are a few instances returning '-1', are they left
>>> intentionally?
>>
>> Yes. Inside ethdev it is either socket_id or fd in these cases.
>>
> 
> Can't those two also updated to return '-ENODEV' when 'port_id' is not
> valid?

I think no.
1. rte_eth_dev_socket_id() should not return -ENODEV since it
   can return -1 even if port ID is valid if fact (I see
   printouts from time to time if I'm not mistaken) and
   typically handled as unspecified NUMA node ID.
2. rte_eth_dev_rx_intr_ctl_q_get_fd() explicitly says that -1
   is returned on error. The function is still experimental
   and we can change it, but I'd say that -1 match typical
   behavior for functions returning file descriptor.

Let's limit the changeset to switch from EINVAL to ENODEV.

Re: [dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Ferruh Yigit]

On 10/14/2020 7:16 AM, Andrew Rybchenko wrote:
> On 10/13/20 7:12 PM, Ferruh Yigit wrote:
>> On 10/13/2020 4:39 PM, Andrew Rybchenko wrote:
>>> On 10/13/20 6:32 PM, Ferruh Yigit wrote:
>>>> On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
>>>>> Use ENODEV as the error code if specified port ID is invalid.
>>>>>
>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>> ---
>>>>>     lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
>>>>>     lib/librte_ethdev/rte_ethdev.h | 46
>>>>> +++++++++++++++++++++++-----------
>>>>>     2 files changed, 54 insertions(+), 36 deletions(-)
>>>>>
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>>>>> b/lib/librte_ethdev/rte_ethdev.c
>>>>> index 5b7979a3b8..1f862f918a 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>>> @@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id,
>>>>> char *name)
>>>>>     {
>>>>>         char *tmp;
>>>>>     -    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>>>>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>
>>>> Thanks Andrew, +1 to this error unification.
>>>>
>>>> This will be API change without deprecation notice, cc'ed techboard
>>>> for it.
>>>
>>> Yes, thanks.
>>>
>>>>
>>>> If this should (almost) always return '-ENODEV', does it make sense to
>>>> make another wrapper macro for it, to prevent later other error types
>>>> used again.
>>>
>>> Unlikely, since most likely the line will be simply copied.
>>> RTE_ETH_VALID_PORTID_OR_ERR_RET will remain in any case, so
>>> it will be possible to misuse it anyway.
>>>
>>
>> Agree it won't prevent misuse completely but may help, anyway I don't
>> have a strong opinion here, if you think that is not needed, that is OK.
>>
>>>>
>>>> And there are a few instances returning '-1', are they left
>>>> intentionally?
>>>
>>> Yes. Inside ethdev it is either socket_id or fd in these cases.
>>>
>>
>> Can't those two also updated to return '-ENODEV' when 'port_id' is not
>> valid?
> 
> I think no.
> 1. rte_eth_dev_socket_id() should not return -ENODEV since it
>     can return -1 even if port ID is valid if fact (I see
>     printouts from time to time if I'm not mistaken) and
>     typically handled as unspecified NUMA node ID.
> 2. rte_eth_dev_rx_intr_ctl_q_get_fd() explicitly says that -1
>     is returned on error. The function is still experimental
>     and we can change it, but I'd say that -1 match typical
>     behavior for functions returning file descriptor.
> 
> Let's limit the changeset to switch from EINVAL to ENODEV.
> 

OK.

It looks like there is no objection on the API part, so I will proceed with it 
but will get as one of the last a few patches before -rc1 to prevent other 
existing ethdev patches to conflict.

Re: [dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Ferruh Yigit]

On 10/13/2020 4:32 PM, Ferruh Yigit wrote:
> On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
>> Use ENODEV as the error code if specified port ID is invalid.
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>   lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
>>   lib/librte_ethdev/rte_ethdev.h | 46 +++++++++++++++++++++++-----------
>>   2 files changed, 54 insertions(+), 36 deletions(-)
>>
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index 5b7979a3b8..1f862f918a 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
>>   {
>>       char *tmp;
>> -    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> 
> Thanks Andrew, +1 to this error unification.
> 
> This will be API change without deprecation notice, cc'ed techboard for it.
> 
> If this should (almost) always return '-ENODEV', does it make sense to make 
> another wrapper macro for it, to prevent later other error types used again.
> 
> And there are a few instances returning '-1', are they left intentionally?
> 

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Ferruh Yigit]

On 10/16/2020 1:05 PM, Ferruh Yigit wrote:
> On 10/13/2020 4:32 PM, Ferruh Yigit wrote:
>> On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
>>> Use ENODEV as the error code if specified port ID is invalid.
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>>   lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
>>>   lib/librte_ethdev/rte_ethdev.h | 46 +++++++++++++++++++++++-----------
>>>   2 files changed, 54 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index 5b7979a3b8..1f862f918a 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
>>>   {
>>>       char *tmp;
>>> -    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>
>> Thanks Andrew, +1 to this error unification.
>>
>> This will be API change without deprecation notice, cc'ed techboard for it.
>>
>> If this should (almost) always return '-ENODEV', does it make sense to make 
>> another wrapper macro for it, to prevent later other error types used again.
>>
>> And there are a few instances returning '-1', are they left intentionally?
>>
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
 >

Applied to dpdk-next-net/main, thanks.

Re: [dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Ferruh Yigit]

On 10/16/2020 10:58 PM, Ferruh Yigit wrote:
> On 10/16/2020 1:05 PM, Ferruh Yigit wrote:
>> On 10/13/2020 4:32 PM, Ferruh Yigit wrote:
>>> On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
>>>> Use ENODEV as the error code if specified port ID is invalid.
>>>>
>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>>   lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
>>>>   lib/librte_ethdev/rte_ethdev.h | 46 +++++++++++++++++++++++-----------
>>>>   2 files changed, 54 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>>> index 5b7979a3b8..1f862f918a 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>> @@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
>>>>   {
>>>>       char *tmp;
>>>> -    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
>>>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>
>>> Thanks Andrew, +1 to this error unification.
>>>
>>> This will be API change without deprecation notice, cc'ed techboard for it.
>>>
>>> If this should (almost) always return '-ENODEV', does it make sense to make 
>>> another wrapper macro for it, to prevent later other error types used again.
>>>
>>> And there are a few instances returning '-1', are they left intentionally?
>>>
>>
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>  >
> 
> Applied to dpdk-next-net/main, thanks.
 >

There are some bitratestats unit tests, that checks APIs with invalid port_id. 
Unit tests checks return values as '-EINVAL', they also should be updated as 
'-ENODEV' with this patch.

Adding following update to this patch in next-net

  diff --git a/app/test/test_bitratestats.c b/app/test/test_bitratestats.c
  index 39d7f734d4..fb4203c57b 100644
  --- a/app/test/test_bitratestats.c
  +++ b/app/test/test_bitratestats.c
  @@ -99,8 +99,8 @@ test_stats_bitrate_calc_invalid_portid_1(void)
          int ret = 0;

          ret = rte_stats_bitrate_calc(bitrate_data, 33);
  -       TEST_ASSERT(ret == -EINVAL, "Test Failed: Expected -%d for higher "
  -                       "portid rte_stats_bitrate_calc ret:%d", EINVAL, ret);
  +       TEST_ASSERT(ret == -ENODEV, "Test Failed: Expected -%d for higher "
  +                       "portid rte_stats_bitrate_calc ret:%d", ENODEV, ret);

          return TEST_SUCCESS;
   }
  @@ -112,8 +112,8 @@ test_stats_bitrate_calc_invalid_portid_2(void)
          int ret = 0;

          ret = rte_stats_bitrate_calc(bitrate_data, -1);
  -       TEST_ASSERT(ret == -EINVAL, "Test Failed: Expected -%d for invalid "
  -                       "portid rte_stats_bitrate_calc ret:%d", EINVAL, ret);
  +       TEST_ASSERT(ret == -ENODEV, "Test Failed: Expected -%d for invalid "
  +                       "portid rte_stats_bitrate_calc ret:%d", ENODEV, ret);

          return TEST_SUCCESS;
   }
  @@ -125,9 +125,9 @@ test_stats_bitrate_calc_non_existing_portid(void)
          int ret = 0;

          ret = rte_stats_bitrate_calc(bitrate_data, 31);
  -       TEST_ASSERT(ret ==  -EINVAL, "Test Failed: Expected -%d for "
  +       TEST_ASSERT(ret ==  -ENODEV, "Test Failed: Expected -%d for "
                          "non-existing portid rte_stats_bitrate_calc ret:%d",
  -                       EINVAL, ret);
  +                       ENODEV, ret);

          return TEST_SUCCESS;
   }

Re: [dpdk-dev] [PATCH] ethdev: unify error code if port ID is invalid

[Thomas Monjalon]

17/10/2020 01:43, Ferruh Yigit:
> On 10/16/2020 10:58 PM, Ferruh Yigit wrote:
> > On 10/16/2020 1:05 PM, Ferruh Yigit wrote:
> >> On 10/13/2020 4:32 PM, Ferruh Yigit wrote:
> >>> On 10/13/2020 3:53 PM, Andrew Rybchenko wrote:
> >>>> Use ENODEV as the error code if specified port ID is invalid.
> >>>>
> >>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >>>> ---
> >>>>   lib/librte_ethdev/rte_ethdev.c | 44 ++++++++++++++++----------------
> >>>>   lib/librte_ethdev/rte_ethdev.h | 46 +++++++++++++++++++++++-----------
> >>>>   2 files changed, 54 insertions(+), 36 deletions(-)
> >>>>
> >>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> >>>> index 5b7979a3b8..1f862f918a 100644
> >>>> --- a/lib/librte_ethdev/rte_ethdev.c
> >>>> +++ b/lib/librte_ethdev/rte_ethdev.c
> >>>> @@ -784,7 +784,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
> >>>>   {
> >>>>       char *tmp;
> >>>> -    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> >>>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >>>
> >>> Thanks Andrew, +1 to this error unification.
> >>>
> >>> This will be API change without deprecation notice, cc'ed techboard for it.
> >>>
> >>> If this should (almost) always return '-ENODEV', does it make sense to make 
> >>> another wrapper macro for it, to prevent later other error types used again.
> >>>
> >>> And there are a few instances returning '-1', are they left intentionally?
> >>>
> >>
> >> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >  >
> > 
> > Applied to dpdk-next-net/main, thanks.
>  >
> 
> There are some bitratestats unit tests, that checks APIs with invalid port_id. 
> Unit tests checks return values as '-EINVAL', they also should be updated as 
> '-ENODEV' with this patch.
> 
> Adding following update to this patch in next-net
> 
>   diff --git a/app/test/test_bitratestats.c b/app/test/test_bitratestats.c
>   index 39d7f734d4..fb4203c57b 100644
>   --- a/app/test/test_bitratestats.c
>   +++ b/app/test/test_bitratestats.c
>   @@ -99,8 +99,8 @@ test_stats_bitrate_calc_invalid_portid_1(void)
>           int ret = 0;
> 
>           ret = rte_stats_bitrate_calc(bitrate_data, 33);
>   -       TEST_ASSERT(ret == -EINVAL, "Test Failed: Expected -%d for higher "
>   -                       "portid rte_stats_bitrate_calc ret:%d", EINVAL, ret);
>   +       TEST_ASSERT(ret == -ENODEV, "Test Failed: Expected -%d for higher "
>   +                       "portid rte_stats_bitrate_calc ret:%d", ENODEV, ret);
> 
>           return TEST_SUCCESS;
>    }
>   @@ -112,8 +112,8 @@ test_stats_bitrate_calc_invalid_portid_2(void)
>           int ret = 0;
> 
>           ret = rte_stats_bitrate_calc(bitrate_data, -1);
>   -       TEST_ASSERT(ret == -EINVAL, "Test Failed: Expected -%d for invalid "
>   -                       "portid rte_stats_bitrate_calc ret:%d", EINVAL, ret);
>   +       TEST_ASSERT(ret == -ENODEV, "Test Failed: Expected -%d for invalid "
>   +                       "portid rte_stats_bitrate_calc ret:%d", ENODEV, ret);
> 
>           return TEST_SUCCESS;
>    }
>   @@ -125,9 +125,9 @@ test_stats_bitrate_calc_non_existing_portid(void)
>           int ret = 0;
> 
>           ret = rte_stats_bitrate_calc(bitrate_data, 31);
>   -       TEST_ASSERT(ret ==  -EINVAL, "Test Failed: Expected -%d for "
>   +       TEST_ASSERT(ret ==  -ENODEV, "Test Failed: Expected -%d for "
>                           "non-existing portid rte_stats_bitrate_calc ret:%d",
>   -                       EINVAL, ret);
>   +                       ENODEV, ret);
> 
>           return TEST_SUCCESS;
>    }

Thank you Ferruh for the extra checks.