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
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?
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.
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.
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?
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.
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.
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>
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.
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;
}
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.