Re: [PATCH v2 1/4] lib/ethdev: Add link_speed lanes support into rte lib

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 6/2/2024 3:45 AM, Damodharam Ammepalli wrote:
> Update the eth_dev_ops structure with new function vectors
> to get and set number of speed lanes. This will help user to
> configure  the same fixed speed with different number of lanes
> based on the physical carrier type.
> 
> Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
> Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> ---
>  lib/ethdev/ethdev_driver.h | 49 +++++++++++++++++++++++++++++++++++
>  lib/ethdev/rte_ethdev.c    | 26 +++++++++++++++++++
>  lib/ethdev/rte_ethdev.h    | 52 ++++++++++++++++++++++++++++++++++++++
>  lib/ethdev/version.map     |  2 ++
>  4 files changed, 129 insertions(+)
> 
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 0dbf2dd6a2..b1f473e4de 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1179,6 +1179,51 @@ typedef int (*eth_rx_descriptor_dump_t)(const struct rte_eth_dev *dev,
>  					uint16_t queue_id, uint16_t offset,
>  					uint16_t num, FILE *file);
>  
> +/**
> + * @internal
> + * Get number of current active lanes and max supported lanes
>

There is already a 'eth_speed_lanes_get_capa' dev_ops, why max supported
lanes returned in this call, instead of capa?

> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param speed_lanes_capa
> + *   Number of active lanes that the link is trained up.
> + *   Max number of lanes supported by HW
> + * @return
> + *   Negative errno value on error, 0 on success.
> + *
> + * @retval 0
> + *   Success, get speed_lanes data success.
>

Success, speed_lanes_capa filled.

> + * @retval -ENOTSUP
> + *   Operation is not supported.
> + * @retval -EIO
> + *   Device is removed.
> + */
> +typedef int (*eth_speed_lanes_get_t)(struct rte_eth_dev *dev,
> +				     struct rte_eth_speed_lanes_capa *speed_lanes_capa);
> +
> +/**
> + * @internal
> + * Set speed lanes
>

As we are on the subject, we all understand what "speed lane" is in this
context, but I am not sure if the naming is descriptive enough, how this
is referenced in datasheet?

> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param speed_lanes_capa
> + *   Non-negative number of lanes
> + *
> + * @return
> + *   Negative errno value on error, 0 on success.
> + *
> + * @retval 0
> + *   Success, set lanes success.
> + * @retval -ENOTSUP
> + *   Operation is not supported.
> + * @retval -EINVAL
> + *   Unsupported mode requested.
> + * @retval -EIO
> + *   Device is removed.
> + */
> +typedef int (*eth_speed_lanes_set_t)(struct rte_eth_dev *dev, uint32_t speed_lanes_capa);
> +

These new dev_ops seems inserted in between 'rx_descriptor_dump' &
'tx_descriptor_dump' dev_ops.
Can you please move them just below 'eth_link_update_t'?



>  /**
>   * @internal
>   * Dump Tx descriptor info to a file.
> @@ -1474,6 +1519,10 @@ struct eth_dev_ops {
>  	eth_count_aggr_ports_t count_aggr_ports;
>  	/** Map a Tx queue with an aggregated port of the DPDK port */
>  	eth_map_aggr_tx_affinity_t map_aggr_tx_affinity;
> +	/** Get number of speed lanes supported and active lanes */
> +	eth_speed_lanes_get_t speed_lanes_get;
> +	/** Set number of speed lanes */
> +	eth_speed_lanes_set_t speed_lanes_set;
>  };
>  
>  /**
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index f1c658f49e..45e2f7645b 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -7008,4 +7008,30 @@ int rte_eth_dev_map_aggr_tx_affinity(uint16_t port_id, uint16_t tx_queue_id,
>  	return ret;
>  }
>  
> +int
> +rte_eth_speed_lanes_get(uint16_t port_id, struct rte_eth_speed_lanes_capa *capa)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (*dev->dev_ops->speed_lanes_get == NULL)
> +		return -ENOTSUP;
> +	return eth_err(port_id, (*dev->dev_ops->speed_lanes_get)(dev, capa));
>

Shouldn't we verify if 'capa' is not NULL in API?

> +}
> +
> +int
> +rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (*dev->dev_ops->speed_lanes_set == NULL)
> +		return -ENOTSUP;
> +	return eth_err(port_id, (*dev->dev_ops->speed_lanes_set)(dev, speed_lanes_capa));
> +}
> +
>  RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index 147257d6a2..caae1f27c6 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1997,6 +1997,12 @@ struct rte_eth_fec_capa {
>  	uint32_t capa;  /**< FEC capabilities bitmask */
>  };
>  
> +/* A structure used to get and set lanes capabilities per link speed */
> +struct rte_eth_speed_lanes_capa {
> +	uint32_t active_lanes;
> +	uint32_t max_lanes_cap;
> +};
> +
>  #define RTE_ETH_ALL RTE_MAX_ETHPORTS
>  
>  /* Macros to check for valid port */
> @@ -6917,6 +6923,52 @@ rte_eth_tx_queue_count(uint16_t port_id, uint16_t queue_id)
>  	return rc;
>  }
>  
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Get maximum speed lanes supported by the NIC.
>

Isn't this API to get the current lane number?

> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param speed_lanes_capa
> + *   speed_lanes_capa is out only with max speed lanes capabilities.
> + *   If set to NULL, then its assumed zero or not supported.
>

Why NULL 'capa' is supported?

> + *
> + * @return
> + *   - A non-negative value of active lanes that currently link is up with.
> + *   - A non-negative value that this HW scales up to for all speeds.
>

Isn't the return value only for status, error or success, and data
stored in 'capa' pointer?


> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> + *     that operation.
> + *   - (-EIO) if device is removed.
> + *   - (-ENODEV)  if *port_id* invalid.
> + *   - (-EINVAL)  if *speed_lanes_capa* invalid
>

This is 'get' function, how 'speed_lanes_capa' can be invalid?

> + */
> +__rte_experimental
> +int rte_eth_speed_lanes_get(uint16_t port_id, struct rte_eth_speed_lanes_capa *capa);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Set speed lanes supported by the NIC.
>

Should we document somewhere that this is only for the case Auto
Negotiation (AN) is disabled, otherwise AN will figure out the lanes.

> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param speed_lanes
> + *   speed_lanes a non-zero value of number lanes for this speeds.
>

Please reword 'this speeds'

> + *
> + * @return
> + *  - (>=0) valid input and supported by driver or hardware.
>

Lanes set successfully?


> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> + *     that operation.
> + *   - (-EIO) if device is removed.
> + *   - (-ENODEV)  if *port_id* invalid.
> + *   - (-EINVAL)  if *speed_lanes* invalid
> + */
> +__rte_experimental
> +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 79f6f5293b..9c27980f3a 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -325,6 +325,8 @@ EXPERIMENTAL {
>  	rte_flow_template_table_resizable;
>  	rte_flow_template_table_resize;
>  	rte_flow_template_table_resize_complete;
> +	rte_eth_speed_lanes_get;
> +	rte_eth_speed_lanes_set;
>

Please follow the syntax in the file, add "# added in 24.07" comment and
add new APIs under it alphabetically sorted way.