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.ammepa...@broadcom.com>
> Reviewed-by: Kalesh AP <kalesh-anakkur.pura...@broadcom.com>
> Reviewed-by: Ajit Khaparde <ajit.khapa...@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.
