Re: [dpdk-dev] [PATCH 2/2] ethdev: allow unknown link speed

[Matan Azrad <matan@mellanox.com>]

Hi

From: Thomas Monjalon
> When querying the link informations, the link status is a mandatory major
> information.
> Other boolean values are supposed to be accurate:
> 	- duplex mode (half/full)
> 	- negotiation (auto/fixed)
> 
> This API update is making explicit that the link speed information is optional.
> The value ETH_SPEED_NUM_NONE (0) was already part of the API.
> The value ETH_SPEED_NUM_UNKNOWN (infinite) is added to cover two
> different cases:
> 	- speed is not known by the driver
> 	- device is virtual
> 
> Suggested-by: Morten Brørup <mb@smartsharesystems.com>
> Suggested-by: Benoit Ganne <bganne@cisco.com>
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad11..2d51fd3444 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -300,6 +300,7 @@ struct rte_eth_stats {
>  #define ETH_SPEED_NUM_50G      50000 /**<  50 Gbps */
>  #define ETH_SPEED_NUM_56G      56000 /**<  56 Gbps */
>  #define ETH_SPEED_NUM_100G    100000 /**< 100 Gbps */
> +#define ETH_SPEED_NUM_UNKNOWN UINT32_MAX /**< Unknown */
> 
>  /**
>   * A structure used to retrieve link-level information of an Ethernet port.
> @@ -2245,15 +2246,16 @@ int rte_eth_allmulticast_disable(uint16_t
> port_id);  int rte_eth_allmulticast_get(uint16_t port_id);
> 
>  /**
> - * Retrieve the status (ON/OFF), the speed (in Mbps) and the mode (HALF-
> DUPLEX
> - * or FULL-DUPLEX) of the physical link of an Ethernet device. It might need
> - * to wait up to 9 seconds in it.
> + * Retrieve the link status (up/down), the duplex mode (half/full),
> + * the negotiation (auto/fixed), and if available, the speed (Mbps).
> + *
> + * It might need to wait up to 9 seconds.
> + * @see rte_eth_link_get_nowait.
>   *
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param link
> - *   A pointer to an *rte_eth_link* structure to be filled with
> - *   the status, the speed and the mode of the Ethernet device link.
> + *   Link informations written back.
>   * @return
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if the function is not supported in PMD driver.
> @@ -2262,15 +2264,13 @@ int rte_eth_allmulticast_get(uint16_t port_id);
> int rte_eth_link_get(uint16_t port_id, struct rte_eth_link *link);
> 
>  /**
> - * Retrieve the status (ON/OFF), the speed (in Mbps) and the mode (HALF-
> DUPLEX
> - * or FULL-DUPLEX) of the physical link of an Ethernet device. It is a no-wait
> - * version of rte_eth_link_get().
> + * Retrieve the link status (up/down), the duplex mode (half/full),
> + * the negotiation (auto/fixed), and if available, the speed (Mbps).

The current API doesn’t say that link speed is optional, so no link speed information means error in API.
When making link speed optional, it may break existing app that expects to get error in API when link speed is not available in order to call the API again.
So I think it breaks API.
I agree for the change but think that this is better to integrate it in 20.11 after sending prior deprecation notice.

Matan


>   *
>   * @param port_id
>   *   The port identifier of the Ethernet device.
>   * @param link
> - *   A pointer to an *rte_eth_link* structure to be filled with
> - *   the status, the speed and the mode of the Ethernet device link.
> + *   Link informations written back.
>   * @return
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if the function is not supported in PMD driver.
> --
> 2.26.0