[2/2] ethdev: allow unknown link speed

  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(-)
  

On Wed, Apr 8, 2020 at 3:57 AM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 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

LGTM.

>
> 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 */

IMO, It is better to add the following information in the same or
other words under @note in Doxygen comment
for the release documentation perspective.

The value ETH_SPEED_NUM_UNKNOWN (infinite) is added to cover
two different cases:
        - speed is not known by the driver
        - device is virtual
  

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, April 8, 2020 12:27 AM
> 
> 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.

informations -> information

>   * @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).
>   *
>   * @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.

informations -> information

>   * @return
>   *   - (0) if successful.
>   *   - (-ENOTSUP) if the function is not supported in PMD driver.
> --
> 2.26.0
>
  

On 4/8/20 1:26 AM, Thomas Monjalon wrote:
> 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>

I'm afraid it requires more efforts to keep things consistent,
e.g.:
 - printing of link speed in proc-info
 - comparison of rate vs link_speed in testpmd
 - printing of link speed in testpmd as %u Mbps
 - division by 1000 in test-pipeline
 - link speed printing in doc/guides/sample_app_ug/link_status_intr.rst
 - link speed printing in many-many examples

IMHO, it is not nice to print UING32_MAX and let use
guess what it means.
  

On 4/13/2020 3:26 PM, Andrew Rybchenko wrote:
> On 4/8/20 1:26 AM, Thomas Monjalon wrote:
>> 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>
> 
> I'm afraid it requires more efforts to keep things consistent,
> e.g.:
>  - printing of link speed in proc-info
>  - comparison of rate vs link_speed in testpmd
>  - printing of link speed in testpmd as %u Mbps
>  - division by 1000 in test-pipeline
>  - link speed printing in doc/guides/sample_app_ug/link_status_intr.rst
>  - link speed printing in many-many examples
> 
> IMHO, it is not nice to print UING32_MAX and let use
> guess what it means.
> 

+1

Hi Thomas,

The two patches in the set seems unrelated, and first one looks OK, do you want
to separate them so that this doesn't block that one?
  

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