[v3] ethdev: Add link_speed lanes support

Message ID 20240617203448.97972-1-damodharam.ammepalli@broadcom.com (mailing list archive)
State Changes Requested
Delegated to: Ferruh Yigit
Headers
Series [v3] ethdev: Add link_speed lanes support |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation fail Compilation issues
ci/intel-Testing success Testing PASS
ci/github-robot: build fail github build: failed
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS

Commit Message

Damodharam Ammepalli June 17, 2024, 8:34 p.m. UTC
  Update the eth_dev_ops structure with new function vectors
to get, get capabilities and set ethernet link speed lanes.
Update the testpmd to provide required config and information
display infrastructure.

The supporting ethernet controller driver will register callbacks
to avail link speed lanes config and get services. This lanes
configuration is applicable only when the nic is forced to fixed
speeds. In Autonegiation mode, the hardware automatically
negotiates the number of lanes.

These are the new commands.

testpmd> show port 0 speed_lanes capabilities

 Supported speeds         Valid lanes
-----------------------------------
 10 Gbps                  1
 25 Gbps                  1
 40 Gbps                  4
 50 Gbps                  1 2
 100 Gbps                 1 2 4
 200 Gbps                 2 4
 400 Gbps                 4 8
testpmd>

testpmd>
testpmd> port stop 0
testpmd> port config 0 speed_lanes 4
testpmd> port config 0 speed 200000 duplex full
testpmd> port start 0
testpmd>
testpmd> show port info 0

********************* Infos for port 0  *********************
MAC address: 14:23:F2:C3:BA:D2
Device name: 0000:b1:00.0
Driver name: net_bnxt
Firmware-version: 228.9.115.0
Connect to socket: 2
memory allocation on the socket: 2
Link status: up
Link speed: 200 Gbps
Active Lanes: 4
Link duplex: full-duplex
Autoneg status: Off

Signed-off-by: Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>
---
v2->v3 Consolidating the testpmd and rtelib patches into a single patch
as requested.

 app/test-pmd/cmdline.c     | 224 +++++++++++++++++++++++++++++++++++++
 app/test-pmd/config.c      |  65 ++++++++++-
 app/test-pmd/testpmd.h     |   4 +
 lib/ethdev/ethdev_driver.h |  77 +++++++++++++
 lib/ethdev/rte_ethdev.c    |  51 +++++++++
 lib/ethdev/rte_ethdev.h    |  90 +++++++++++++++
 lib/ethdev/version.map     |   5 +
 7 files changed, 514 insertions(+), 2 deletions(-)
  

Comments

huangdengdui June 20, 2024, 3:23 a.m. UTC | #1
Hi Damodharam
Here are some suggestions. See below.

On 2024/6/18 4:34, Damodharam Ammepalli wrote:
> Update the eth_dev_ops structure with new function vectors
> to get, get capabilities and set ethernet link speed lanes.
> Update the testpmd to provide required config and information
> display infrastructure.
> 
> The supporting ethernet controller driver will register callbacks
> to avail link speed lanes config and get services. This lanes
> configuration is applicable only when the nic is forced to fixed
> speeds. In Autonegiation mode, the hardware automatically
> negotiates the number of lanes.
> 


> +
>  /* *** configure txq/rxq, txd/rxd *** */
>  struct cmd_config_rx_tx {
>  	cmdline_fixed_string_t port;
> @@ -13238,6 +13459,9 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
>  	(cmdline_parse_inst_t *)&cmd_set_port_setup_on,
>  	(cmdline_parse_inst_t *)&cmd_config_speed_all,
>  	(cmdline_parse_inst_t *)&cmd_config_speed_specific,
> +	(cmdline_parse_inst_t *)&cmd_config_speed_lanes_all,
> +	(cmdline_parse_inst_t *)&cmd_config_speed_lanes_specific,
> +	(cmdline_parse_inst_t *)&cmd_show_speed_lanes,

Please also add to help string and update doc

>  	(cmdline_parse_inst_t *)&cmd_config_loopback_all,
>  	(cmdline_parse_inst_t *)&cmd_config_loopback_specific,
>  	(cmdline_parse_inst_t *)&cmd_config_rx_tx,
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 66c3a68c1d..cf3ea50114 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -207,6 +207,32 @@ static const struct {
>  	{"gtpu", RTE_ETH_FLOW_GTPU},
>  };
>  
> +static const struct {
> +	enum rte_eth_speed_lanes lane;
> +	const uint32_t value;
> +} speed_lane_name[] = {
> +	{
> +		.lane = RTE_ETH_SPEED_LANE_NOLANE,
> +		.value = 0,
> +	},
> +	{
> +		.lane = RTE_ETH_SPEED_LANE_1,
> +		.value = 1,
> +	},
> +	{
> +		.lane = RTE_ETH_SPEED_LANE_2,
> +		.value = 2,
> +	},
> +	{
> +		.lane = RTE_ETH_SPEED_LANE_4,
> +		.value = 4,
> +	},
> +	{
> +		.lane = RTE_ETH_SPEED_LANE_8,
> +		.value = 8,
> +	},
> +};
> +
>  static void
>  print_ethaddr(const char *name, struct rte_ether_addr *eth_addr)
>  {
> @@ -786,6 +812,7 @@ port_infos_display(portid_t port_id)
>  	char name[RTE_ETH_NAME_MAX_LEN];
>  	int ret;
>  	char fw_version[ETHDEV_FWVERS_LEN];
> +	uint32_t lanes;
>  
>  	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
>  		print_valid_ports();
> @@ -828,6 +855,8 @@ port_infos_display(portid_t port_id)
>  
>  	printf("\nLink status: %s\n", (link.link_status) ? ("up") : ("down"));
>  	printf("Link speed: %s\n", rte_eth_link_speed_to_str(link.link_speed));
> +	if (rte_eth_speed_lanes_get(port_id, &lanes) == 0)
> +		printf("Active Lanes: %d\n", lanes);

It should not be printed when lanes=0. Or print unknown when lane=0?

>  	printf("Link duplex: %s\n", (link.link_duplex == RTE_ETH_LINK_FULL_DUPLEX) ?
>  	       ("full-duplex") : ("half-duplex"));
>  	printf("Autoneg status: %s\n", (link.link_autoneg == RTE_ETH_LINK_AUTONEG) ?
> @@ -962,7 +991,7 @@ port_summary_header_display(void)
>  
>  	port_number = rte_eth_dev_count_avail();
>  	printf("Number of available ports: %i\n", port_number);
> -	printf("%-4s %-17s %-12s %-14s %-8s %s\n", "Port", "MAC Address", "Name",
> +	printf("%-4s %-17s %-12s %-14s %-8s %-8s\n", "Port", "MAC Address", "Name",
>  			"Driver", "Status", "Link");
>  }
>  
> @@ -993,7 +1022,7 @@ port_summary_display(portid_t port_id)
>  	if (ret != 0)
>  		return;
>  
> -	printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %s\n",
> +	printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %-8s\n",

Does the lanes need to be printed?

>  		port_id, RTE_ETHER_ADDR_BYTES(&mac_addr), name,
>  		dev_info.driver_name, (link.link_status) ? ("up") : ("down"),
>  		rte_eth_link_speed_to_str(link.link_speed));
> @@ -7244,3 +7273,35 @@ show_mcast_macs(portid_t port_id)
>  		printf("  %s\n", buf);
>  	}
>  }
> +
> +int
> +parse_speed_lanes(uint32_t lane, uint32_t *speed_lane)
> +{
> +	uint8_t i;
> +
> +	for (i = 0; i < RTE_DIM(speed_lane_name); i++) {
> +		if (speed_lane_name[i].value == lane) {
> +			*speed_lane = lane;
> +			return 0;
> +		}
> +	}
> +	return -1;
> +}
> +
> +void
> +show_speed_lanes_capability(unsigned int num, struct rte_eth_speed_lanes_capa *speed_lanes_capa)
> +{
> +	unsigned int i, j;
> +
> +	printf("\n%-15s %-10s", "Supported-speeds", "Valid-lanes");
> +	printf("\n-----------------------------------\n");
> +	for (i = 0; i < num; i++) {
> +		printf("%-17s ", rte_eth_link_speed_to_str(speed_lanes_capa[i].speed));
> +
> +		for (j = 0; j < RTE_ETH_SPEED_LANE_MAX; j++) {
> +			if (RTE_ETH_SPEED_LANES_TO_CAPA(j) & speed_lanes_capa[i].capa)
> +				printf("%-2d ", speed_lane_name[j].value);
> +		}
> +		printf("\n");
> +	}
> +}
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 9facd7f281..fb9ef05cc5 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -1253,6 +1253,10 @@ extern int flow_parse(const char *src, void *result, unsigned int size,
>  		      struct rte_flow_item **pattern,
>  		      struct rte_flow_action **actions);
>  
> +void show_speed_lanes_capability(uint32_t num,
> +				 struct rte_eth_speed_lanes_capa *speed_lanes_capa);
> +int parse_speed_lanes(uint32_t lane, uint32_t *speed_lane);
> +
>  uint64_t str_to_rsstypes(const char *str);
>  const char *rsstypes_to_str(uint64_t rss_type);
>  
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 883e59a927..0f10aec3a1 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -1179,6 +1179,79 @@ 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
> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param speed_lanes
> + *   Number of active lanes that the link is trained up.
> + * @return
> + *   Negative errno value on error, 0 on success.
> + *
> + * @retval 0
> + *   Success, get speed_lanes data success.
> + * @retval -ENOTSUP
> + *   Operation is not supported.
> + * @retval -EIO
> + *   Device is removed.
> + */
> +typedef int (*eth_speed_lanes_get_t)(struct rte_eth_dev *dev, uint32_t *speed_lanes);
> +
> +/**
> + * @internal
> + * Set speed lanes
> + *
> + * @param dev
> + *   ethdev handle of port.
> + * @param speed_lanes
> + *   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);
> +
> +/**
> + * @internal
> + * Get supported link speed lanes capability
> + *
> + * @param speed_lanes_capa
> + *   speed_lanes_capa is out only with per-speed capabilities.
> + * @param num
> + *   a number of elements in an speed_speed_lanes_capa array.
> + *

Some of the arguments are not documented, not just here.
You can add the 'enable_docs' to verify the document, for example
meson build  -Denable_docs=true

> + * @return
> + *   Negative errno value on error, positive value on success.
> + *
> + * @retval positive value
> + *   A non-negative value lower or equal to num: success. The return value
> + *   is the number of entries filled in the speed lanes array.
> + *   A non-negative value higher than num: error, the given speed lanes capa array
> + *   is too small. The return value corresponds to the num that should
> + *   be given to succeed. The entries in the speed lanes capa array are not valid
> + *   and shall not be used by the caller.
> + * @retval -ENOTSUP
> + *   Operation is not supported.
> + * @retval -EIO
> + *   Device is removed.
> + * @retval -EINVAL
> + *   *num* or *speed_lanes_capa* invalid.
> + */
> +typedef int (*eth_speed_lanes_get_capability_t)(struct rte_eth_dev *dev,
> +						struct rte_eth_speed_lanes_capa *speed_lanes_capa,
> +						unsigned int num);
> +
>  /**
>   * @internal
>   * Dump Tx descriptor info to a file.
> @@ -1247,6 +1320,10 @@ struct eth_dev_ops {
>  	eth_dev_close_t            dev_close;     /**< Close device */
>  	eth_dev_reset_t		   dev_reset;	  /**< Reset device */
>  	eth_link_update_t          link_update;   /**< Get device link state */
> +	eth_speed_lanes_get_t	   speed_lanes_get;	  /**<Get link speed active lanes */
> +	eth_speed_lanes_set_t      speed_lanes_set;	  /**<set the link speeds supported lanes */
> +	/** Get link speed lanes capability */
> +	eth_speed_lanes_get_capability_t speed_lanes_get_capa;
>  	/** Check if the device was physically removed */
>  	eth_is_removed_t           is_removed;
>  
> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index f1c658f49e..07cefea307 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -7008,4 +7008,55 @@ 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, uint32_t *lane)
> +{
> +	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, lane));
> +}
> +
> +int
> +rte_eth_speed_lanes_get_capability(uint16_t port_id,
> +				   struct rte_eth_speed_lanes_capa *speed_lanes_capa,
> +				   unsigned int num)
> +{
> +	struct rte_eth_dev *dev;
> +	int ret;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +	dev = &rte_eth_devices[port_id];
> +
> +	if (speed_lanes_capa == NULL && num > 0) {
> +		RTE_ETHDEV_LOG_LINE(ERR,
> +				    "Cannot get ethdev port %u speed lanes capability to NULL when array size is non zero",
> +				    port_id);
> +		return -EINVAL;
> +	}
> +
> +	if (*dev->dev_ops->speed_lanes_get_capa == NULL)
> +		return -ENOTSUP;
> +	ret = (*dev->dev_ops->speed_lanes_get_capa)(dev, speed_lanes_capa, num);
> +
> +	return ret;
> +}
> +
> +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 548fada1c7..f5bacd4ba4 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -357,6 +357,30 @@ struct rte_eth_link {
>  #define RTE_ETH_LINK_MAX_STR_LEN 40 /**< Max length of default link string. */
>  /**@}*/
>  
> +/**
> + * This enum indicates the possible link speed lanes of an ethdev port.
> + */
> +enum rte_eth_speed_lanes {
> +	RTE_ETH_SPEED_LANE_NOLANE = 0,  /**< speed lanes unsupported mode or default */
> +	RTE_ETH_SPEED_LANE_1,           /**< Link speed lane  1 */
> +	RTE_ETH_SPEED_LANE_2,           /**< Link speed lanes 2 */
> +	RTE_ETH_SPEED_LANE_4,           /**< Link speed lanes 4 */
> +	RTE_ETH_SPEED_LANE_8,           /**< Link speed lanes 8 */
> +	RTE_ETH_SPEED_LANE_MAX,
> +};

Is it better to make the index equal to the lanes num?
enum rte_eth_speed_lanes {
	RTE_ETH_SPEED_LANE_NOLANE = 0,      /**< speed lanes unsupported mode or default */
	RTE_ETH_SPEED_LANE_1 = 1,           /**< Link speed lane  1 */
	RTE_ETH_SPEED_LANE_2 = 2,           /**< Link speed lanes 2 */
	RTE_ETH_SPEED_LANE_4 = 4,           /**< Link speed lanes 4 */
	RTE_ETH_SPEED_LANE_8 = 8,           /**< Link speed lanes 8 */
	RTE_ETH_SPEED_LANE_MAX,
};

In addition, when lanes = 0, is it better to define it as Unknown?
If default lanes can return 0 lanes, The active lanes are different for each NIC,
users may be confused.

> +
> +/* Translate from link speed lanes to speed lanes capa */
> +#define RTE_ETH_SPEED_LANES_TO_CAPA(x) RTE_BIT32(x)
> +
> +/* This macro indicates link speed lanes capa mask */
> +#define RTE_ETH_SPEED_LANES_CAPA_MASK(x) RTE_BIT32(RTE_ETH_SPEED_ ## x)
> +
> +/* A structure used to get and set lanes capabilities per link speed */
> +struct rte_eth_speed_lanes_capa {
> +	uint32_t speed;
> +	uint32_t capa;
> +};
> +
>  /**
>   * A structure used to configure the ring threshold registers of an Rx/Tx
>   * queue for an Ethernet port.
> @@ -6922,6 +6946,72 @@ 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 Active lanes.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param lanes
> + *   driver populates a active lanes value whether link is Autonegotiated or Fixed speed.
> + *
> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> + *     that operation.
> + *   - (-EIO) if device is removed.
> + *   - (-ENODEV)  if *port_id* invalid.
> + */
> +__rte_experimental
> +int rte_eth_speed_lanes_get(uint16_t port_id, uint32_t *lanes);
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Set speed lanes supported by the NIC.
> + *
> + * @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.
> + *
> + * @return
> + *  - (>=0) valid input and supported by driver or hardware.

Is it better to change the description to the following:
(0) if successful.

> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> + *     that operation.
> + *   - (-EIO) if device is removed.
> + *   - (-ENODEV)  if *port_id* invalid.
> + */
> +__rte_experimental
> +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
> +

Is it better to name 'speed_lanes'?

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Set speed lanes supported by the NIC.
> + *

Set -> Get

> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param speed_lanes_bmap
> + *   speed_lanes_bmap int array updated by driver by valid lanes bmap.
> + *
> + * @return
> + *  - (>=0) valid input and supported by driver or hardware.
> + *   - (-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_get_capability(uint16_t port_id,
> +				       struct rte_eth_speed_lanes_capa *speed_lanes_capa,
> +				       unsigned int num);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> index 79f6f5293b..db9261946f 100644
> --- a/lib/ethdev/version.map
> +++ b/lib/ethdev/version.map
> @@ -325,6 +325,11 @@ EXPERIMENTAL {
>  	rte_flow_template_table_resizable;
>  	rte_flow_template_table_resize;
>  	rte_flow_template_table_resize_complete;
> +
> +	# added in 24.07
> +	rte_eth_speed_lanes_get;
> +	rte_eth_speed_lanes_get_capability;
> +	rte_eth_speed_lanes_set;
>  };
>  
>  INTERNAL {
  
Damodharam Ammepalli June 25, 2024, 9:07 p.m. UTC | #2
On Wed, Jun 19, 2024 at 8:23 PM huangdengdui <huangdengdui@huawei.com> wrote:
>
> Hi Damodharam
> Here are some suggestions. See below.
>
Thank you for the review.

> On 2024/6/18 4:34, Damodharam Ammepalli wrote:
> > Update the eth_dev_ops structure with new function vectors
> > to get, get capabilities and set ethernet link speed lanes.
> > Update the testpmd to provide required config and information
> > display infrastructure.
> >
> > The supporting ethernet controller driver will register callbacks
> > to avail link speed lanes config and get services. This lanes
> > configuration is applicable only when the nic is forced to fixed
> > speeds. In Autonegiation mode, the hardware automatically
> > negotiates the number of lanes.
> >
>
>
> > +
> >  /* *** configure txq/rxq, txd/rxd *** */
> >  struct cmd_config_rx_tx {
> >       cmdline_fixed_string_t port;
> > @@ -13238,6 +13459,9 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
> >       (cmdline_parse_inst_t *)&cmd_set_port_setup_on,
> >       (cmdline_parse_inst_t *)&cmd_config_speed_all,
> >       (cmdline_parse_inst_t *)&cmd_config_speed_specific,
> > +     (cmdline_parse_inst_t *)&cmd_config_speed_lanes_all,
> > +     (cmdline_parse_inst_t *)&cmd_config_speed_lanes_specific,
> > +     (cmdline_parse_inst_t *)&cmd_show_speed_lanes,
>
> Please also add to help string and update doc
ack
>
> >       (cmdline_parse_inst_t *)&cmd_config_loopback_all,
> >       (cmdline_parse_inst_t *)&cmd_config_loopback_specific,
> >       (cmdline_parse_inst_t *)&cmd_config_rx_tx,
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> > index 66c3a68c1d..cf3ea50114 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -207,6 +207,32 @@ static const struct {
> >       {"gtpu", RTE_ETH_FLOW_GTPU},
> >  };
> >
> > +static const struct {
> > +     enum rte_eth_speed_lanes lane;
> > +     const uint32_t value;
> > +} speed_lane_name[] = {
> > +     {
> > +             .lane = RTE_ETH_SPEED_LANE_NOLANE,
> > +             .value = 0,
> > +     },
> > +     {
> > +             .lane = RTE_ETH_SPEED_LANE_1,
> > +             .value = 1,
> > +     },
> > +     {
> > +             .lane = RTE_ETH_SPEED_LANE_2,
> > +             .value = 2,
> > +     },
> > +     {
> > +             .lane = RTE_ETH_SPEED_LANE_4,
> > +             .value = 4,
> > +     },
> > +     {
> > +             .lane = RTE_ETH_SPEED_LANE_8,
> > +             .value = 8,
> > +     },
> > +};
> > +
> >  static void
> >  print_ethaddr(const char *name, struct rte_ether_addr *eth_addr)
> >  {
> > @@ -786,6 +812,7 @@ port_infos_display(portid_t port_id)
> >       char name[RTE_ETH_NAME_MAX_LEN];
> >       int ret;
> >       char fw_version[ETHDEV_FWVERS_LEN];
> > +     uint32_t lanes;
> >
> >       if (port_id_is_invalid(port_id, ENABLED_WARN)) {
> >               print_valid_ports();
> > @@ -828,6 +855,8 @@ port_infos_display(portid_t port_id)
> >
> >       printf("\nLink status: %s\n", (link.link_status) ? ("up") : ("down"));
> >       printf("Link speed: %s\n", rte_eth_link_speed_to_str(link.link_speed));
> > +     if (rte_eth_speed_lanes_get(port_id, &lanes) == 0)
> > +             printf("Active Lanes: %d\n", lanes);
>
> It should not be printed when lanes=0. Or print unknown when lane=0?
Here rte_eth_speed_lanes_get(port_id, &lanes) returns 0 only for lanes
supporting drivers.
For lane supporting drivers, In link down condition, lanes = 0 ( ie
yet to be determined).
ack. printing "unknown" also makes sense.

>
> >       printf("Link duplex: %s\n", (link.link_duplex == RTE_ETH_LINK_FULL_DUPLEX) ?
> >              ("full-duplex") : ("half-duplex"));
> >       printf("Autoneg status: %s\n", (link.link_autoneg == RTE_ETH_LINK_AUTONEG) ?
> > @@ -962,7 +991,7 @@ port_summary_header_display(void)
> >
> >       port_number = rte_eth_dev_count_avail();
> >       printf("Number of available ports: %i\n", port_number);
> > -     printf("%-4s %-17s %-12s %-14s %-8s %s\n", "Port", "MAC Address", "Name",
> > +     printf("%-4s %-17s %-12s %-14s %-8s %-8s\n", "Port", "MAC Address", "Name",
> >                       "Driver", "Status", "Link");
> >  }
> >
> > @@ -993,7 +1022,7 @@ port_summary_display(portid_t port_id)
> >       if (ret != 0)
> >               return;
> >
> > -     printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %s\n",
> > +     printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %-8s\n",
>
> Does the lanes need to be printed?
Ferruh in the previous comment, asked not to print.

>
> >               port_id, RTE_ETHER_ADDR_BYTES(&mac_addr), name,
> >               dev_info.driver_name, (link.link_status) ? ("up") : ("down"),
> >               rte_eth_link_speed_to_str(link.link_speed));
> > @@ -7244,3 +7273,35 @@ show_mcast_macs(portid_t port_id)
> >               printf("  %s\n", buf);
> >       }
> >  }
> > +
> > +int
> > +parse_speed_lanes(uint32_t lane, uint32_t *speed_lane)
> > +{
> > +     uint8_t i;
> > +
> > +     for (i = 0; i < RTE_DIM(speed_lane_name); i++) {
> > +             if (speed_lane_name[i].value == lane) {
> > +                     *speed_lane = lane;
> > +                     return 0;
> > +             }
> > +     }
> > +     return -1;
> > +}
> > +
> > +void
> > +show_speed_lanes_capability(unsigned int num, struct rte_eth_speed_lanes_capa *speed_lanes_capa)
> > +{
> > +     unsigned int i, j;
> > +
> > +     printf("\n%-15s %-10s", "Supported-speeds", "Valid-lanes");
> > +     printf("\n-----------------------------------\n");
> > +     for (i = 0; i < num; i++) {
> > +             printf("%-17s ", rte_eth_link_speed_to_str(speed_lanes_capa[i].speed));
> > +
> > +             for (j = 0; j < RTE_ETH_SPEED_LANE_MAX; j++) {
> > +                     if (RTE_ETH_SPEED_LANES_TO_CAPA(j) & speed_lanes_capa[i].capa)
> > +                             printf("%-2d ", speed_lane_name[j].value);
> > +             }
> > +             printf("\n");
> > +     }
> > +}
> > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> > index 9facd7f281..fb9ef05cc5 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -1253,6 +1253,10 @@ extern int flow_parse(const char *src, void *result, unsigned int size,
> >                     struct rte_flow_item **pattern,
> >                     struct rte_flow_action **actions);
> >
> > +void show_speed_lanes_capability(uint32_t num,
> > +                              struct rte_eth_speed_lanes_capa *speed_lanes_capa);
> > +int parse_speed_lanes(uint32_t lane, uint32_t *speed_lane);
> > +
> >  uint64_t str_to_rsstypes(const char *str);
> >  const char *rsstypes_to_str(uint64_t rss_type);
> >
> > diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> > index 883e59a927..0f10aec3a1 100644
> > --- a/lib/ethdev/ethdev_driver.h
> > +++ b/lib/ethdev/ethdev_driver.h
> > @@ -1179,6 +1179,79 @@ 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
> > + *
> > + * @param dev
> > + *   ethdev handle of port.
> > + * @param speed_lanes
> > + *   Number of active lanes that the link is trained up.
> > + * @return
> > + *   Negative errno value on error, 0 on success.
> > + *
> > + * @retval 0
> > + *   Success, get speed_lanes data success.
> > + * @retval -ENOTSUP
> > + *   Operation is not supported.
> > + * @retval -EIO
> > + *   Device is removed.
> > + */
> > +typedef int (*eth_speed_lanes_get_t)(struct rte_eth_dev *dev, uint32_t *speed_lanes);
> > +
> > +/**
> > + * @internal
> > + * Set speed lanes
> > + *
> > + * @param dev
> > + *   ethdev handle of port.
> > + * @param speed_lanes
> > + *   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);
> > +
> > +/**
> > + * @internal
> > + * Get supported link speed lanes capability
> > + *
> > + * @param speed_lanes_capa
> > + *   speed_lanes_capa is out only with per-speed capabilities.
> > + * @param num
> > + *   a number of elements in an speed_speed_lanes_capa array.
> > + *
>
> Some of the arguments are not documented, not just here.
> You can add the 'enable_docs' to verify the document, for example
> meson build  -Denable_docs=true
>
Ack. thanks will take care.
> > + * @return
> > + *   Negative errno value on error, positive value on success.
> > + *
> > + * @retval positive value
> > + *   A non-negative value lower or equal to num: success. The return value
> > + *   is the number of entries filled in the speed lanes array.
> > + *   A non-negative value higher than num: error, the given speed lanes capa array
> > + *   is too small. The return value corresponds to the num that should
> > + *   be given to succeed. The entries in the speed lanes capa array are not valid
> > + *   and shall not be used by the caller.
> > + * @retval -ENOTSUP
> > + *   Operation is not supported.
> > + * @retval -EIO
> > + *   Device is removed.
> > + * @retval -EINVAL
> > + *   *num* or *speed_lanes_capa* invalid.
> > + */
> > +typedef int (*eth_speed_lanes_get_capability_t)(struct rte_eth_dev *dev,
> > +                                             struct rte_eth_speed_lanes_capa *speed_lanes_capa,
> > +                                             unsigned int num);
> > +
> >  /**
> >   * @internal
> >   * Dump Tx descriptor info to a file.
> > @@ -1247,6 +1320,10 @@ struct eth_dev_ops {
> >       eth_dev_close_t            dev_close;     /**< Close device */
> >       eth_dev_reset_t            dev_reset;     /**< Reset device */
> >       eth_link_update_t          link_update;   /**< Get device link state */
> > +     eth_speed_lanes_get_t      speed_lanes_get;       /**<Get link speed active lanes */
> > +     eth_speed_lanes_set_t      speed_lanes_set;       /**<set the link speeds supported lanes */
> > +     /** Get link speed lanes capability */
> > +     eth_speed_lanes_get_capability_t speed_lanes_get_capa;
> >       /** Check if the device was physically removed */
> >       eth_is_removed_t           is_removed;
> >
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index f1c658f49e..07cefea307 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -7008,4 +7008,55 @@ 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, uint32_t *lane)
> > +{
> > +     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, lane));
> > +}
> > +
> > +int
> > +rte_eth_speed_lanes_get_capability(uint16_t port_id,
> > +                                struct rte_eth_speed_lanes_capa *speed_lanes_capa,
> > +                                unsigned int num)
> > +{
> > +     struct rte_eth_dev *dev;
> > +     int ret;
> > +
> > +     RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +     dev = &rte_eth_devices[port_id];
> > +
> > +     if (speed_lanes_capa == NULL && num > 0) {
> > +             RTE_ETHDEV_LOG_LINE(ERR,
> > +                                 "Cannot get ethdev port %u speed lanes capability to NULL when array size is non zero",
> > +                                 port_id);
> > +             return -EINVAL;
> > +     }
> > +
> > +     if (*dev->dev_ops->speed_lanes_get_capa == NULL)
> > +             return -ENOTSUP;
> > +     ret = (*dev->dev_ops->speed_lanes_get_capa)(dev, speed_lanes_capa, num);
> > +
> > +     return ret;
> > +}
> > +
> > +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 548fada1c7..f5bacd4ba4 100644
> > --- a/lib/ethdev/rte_ethdev.h
> > +++ b/lib/ethdev/rte_ethdev.h
> > @@ -357,6 +357,30 @@ struct rte_eth_link {
> >  #define RTE_ETH_LINK_MAX_STR_LEN 40 /**< Max length of default link string. */
> >  /**@}*/
> >
> > +/**
> > + * This enum indicates the possible link speed lanes of an ethdev port.
> > + */
> > +enum rte_eth_speed_lanes {
> > +     RTE_ETH_SPEED_LANE_NOLANE = 0,  /**< speed lanes unsupported mode or default */
> > +     RTE_ETH_SPEED_LANE_1,           /**< Link speed lane  1 */
> > +     RTE_ETH_SPEED_LANE_2,           /**< Link speed lanes 2 */
> > +     RTE_ETH_SPEED_LANE_4,           /**< Link speed lanes 4 */
> > +     RTE_ETH_SPEED_LANE_8,           /**< Link speed lanes 8 */
> > +     RTE_ETH_SPEED_LANE_MAX,
> > +};
>
> Is it better to make the index equal to the lanes num?
> enum rte_eth_speed_lanes {
>         RTE_ETH_SPEED_LANE_NOLANE = 0,      /**< speed lanes unsupported mode or default */
>         RTE_ETH_SPEED_LANE_1 = 1,           /**< Link speed lane  1 */
>         RTE_ETH_SPEED_LANE_2 = 2,           /**< Link speed lanes 2 */
>         RTE_ETH_SPEED_LANE_4 = 4,           /**< Link speed lanes 4 */
>         RTE_ETH_SPEED_LANE_8 = 8,           /**< Link speed lanes 8 */
>         RTE_ETH_SPEED_LANE_MAX,
> };
>
I followed the existing enums code convention in rtelib. Your point
makes sense too.

> In addition, when lanes = 0, is it better to define it as Unknown?
> If default lanes can return 0 lanes, The active lanes are different for each NIC,
> users may be confused.
>
Ack. Are you proposing a new enum RTE_ETH_SPEED_LANE_UKNOWN or rename
RTE_ETH_SPEED_LANE_NOLANE?

> > +
> > +/* Translate from link speed lanes to speed lanes capa */
> > +#define RTE_ETH_SPEED_LANES_TO_CAPA(x) RTE_BIT32(x)
> > +
> > +/* This macro indicates link speed lanes capa mask */
> > +#define RTE_ETH_SPEED_LANES_CAPA_MASK(x) RTE_BIT32(RTE_ETH_SPEED_ ## x)
> > +
> > +/* A structure used to get and set lanes capabilities per link speed */
> > +struct rte_eth_speed_lanes_capa {
> > +     uint32_t speed;
> > +     uint32_t capa;
> > +};
> > +
> >  /**
> >   * A structure used to configure the ring threshold registers of an Rx/Tx
> >   * queue for an Ethernet port.
> > @@ -6922,6 +6946,72 @@ 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 Active lanes.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param lanes
> > + *   driver populates a active lanes value whether link is Autonegotiated or Fixed speed.
> > + *
> > + * @return
> > + *   - (0) if successful.
> > + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> > + *     that operation.
> > + *   - (-EIO) if device is removed.
> > + *   - (-ENODEV)  if *port_id* invalid.
> > + */
> > +__rte_experimental
> > +int rte_eth_speed_lanes_get(uint16_t port_id, uint32_t *lanes);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> > + *
> > + * Set speed lanes supported by the NIC.
> > + *
> > + * @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.
> > + *
> > + * @return
> > + *  - (>=0) valid input and supported by driver or hardware.
>
> Is it better to change the description to the following:
> (0) if successful.
>
Ack
> > + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> > + *     that operation.
> > + *   - (-EIO) if device is removed.
> > + *   - (-ENODEV)  if *port_id* invalid.
> > + */
> > +__rte_experimental
> > +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
> > +
>
> Is it better to name 'speed_lanes'?
Are you proposing to rename to rte_speed_lanes_set() function name or
rename variable "speed_lanes_capa" name ?

>
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> > + *
> > + * Set speed lanes supported by the NIC.
> > + *
>
> Set -> Get
Ack
>
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param speed_lanes_bmap
> > + *   speed_lanes_bmap int array updated by driver by valid lanes bmap.
> > + *
> > + * @return
> > + *  - (>=0) valid input and supported by driver or hardware.
> > + *   - (-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_get_capability(uint16_t port_id,
> > +                                    struct rte_eth_speed_lanes_capa *speed_lanes_capa,
> > +                                    unsigned int num);
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
> > index 79f6f5293b..db9261946f 100644
> > --- a/lib/ethdev/version.map
> > +++ b/lib/ethdev/version.map
> > @@ -325,6 +325,11 @@ EXPERIMENTAL {
> >       rte_flow_template_table_resizable;
> >       rte_flow_template_table_resize;
> >       rte_flow_template_table_resize_complete;
> > +
> > +     # added in 24.07
> > +     rte_eth_speed_lanes_get;
> > +     rte_eth_speed_lanes_get_capability;
> > +     rte_eth_speed_lanes_set;
> >  };
> >
> >  INTERNAL {
  
huangdengdui June 26, 2024, 2:19 a.m. UTC | #3
On 2024/6/26 5:07, Damodharam Ammepalli wrote:
> On Wed, Jun 19, 2024 at 8:23 PM huangdengdui <huangdengdui@huawei.com> wrote:
>>
>> Hi Damodharam
>> Here are some suggestions. See below.
>>
> Thank you for the review.
> 
>> On 2024/6/18 4:34, Damodharam Ammepalli wrote:
>>> Update the eth_dev_ops structure with new function vectors
>>> to get, get capabilities and set ethernet link speed lanes.
>>> Update the testpmd to provide required config and information
>>> display infrastructure.
>>>
>>> The supporting ethernet controller driver will register callbacks
>>> to avail link speed lanes config and get services. This lanes
>>> configuration is applicable only when the nic is forced to fixed
>>> speeds. In Autonegiation mode, the hardware automatically
>>> negotiates the number of lanes.
>>>
>>
>>
>>> +
>>>  /* *** configure txq/rxq, txd/rxd *** */
>>>  struct cmd_config_rx_tx {
>>>       cmdline_fixed_string_t port;
>>> @@ -13238,6 +13459,9 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
>>>       (cmdline_parse_inst_t *)&cmd_set_port_setup_on,

cut

>>>
>>> @@ -993,7 +1022,7 @@ port_summary_display(portid_t port_id)
>>>       if (ret != 0)
>>>               return;
>>>
>>> -     printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %s\n",
>>> +     printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %-8s\n",
>>
>> Does the lanes need to be printed?
> Ferruh in the previous comment, asked not to print.
> 

OK

>>
>>>               port_id, RTE_ETHER_ADDR_BYTES(&mac_addr), name,
>>>               dev_info.driver_name, (link.link_status) ? ("up") : ("down"),
>>>               rte_eth_link_speed_to_str(link.link_speed));
>>> @@ -7244,3 +7273,35 @@ show_mcast_macs(portid_t port_id)
>>>               printf("  %s\n", buf);
>>>       }
>>>  }
>>> +
>>> +int
>>> +parse_speed_lanes(uint32_t lane, uint32_t *speed_lane)
>>> +{
>>> +     uint8_t i;
>>> +
>>> +     for (i = 0; i < RTE_DIM(speed_lane_name); i++) {
>>> +             if (speed_lane_name[i].value == lane) {
>>> +                     *speed_lane = lane;
>>> +                     return 0;
>>> +             }
>>> +     }
>>> +     return -1;
>>> +}
>>> +

cut

>>>
>>> +/**
>>> + * This enum indicates the possible link speed lanes of an ethdev port.
>>> + */
>>> +enum rte_eth_speed_lanes {
>>> +     RTE_ETH_SPEED_LANE_NOLANE = 0,  /**< speed lanes unsupported mode or default */
>>> +     RTE_ETH_SPEED_LANE_1,           /**< Link speed lane  1 */
>>> +     RTE_ETH_SPEED_LANE_2,           /**< Link speed lanes 2 */
>>> +     RTE_ETH_SPEED_LANE_4,           /**< Link speed lanes 4 */
>>> +     RTE_ETH_SPEED_LANE_8,           /**< Link speed lanes 8 */
>>> +     RTE_ETH_SPEED_LANE_MAX,
>>> +};
>>
>> Is it better to make the index equal to the lanes num?
>> enum rte_eth_speed_lanes {
>>         RTE_ETH_SPEED_LANE_NOLANE = 0,      /**< speed lanes unsupported mode or default */
>>         RTE_ETH_SPEED_LANE_1 = 1,           /**< Link speed lane  1 */
>>         RTE_ETH_SPEED_LANE_2 = 2,           /**< Link speed lanes 2 */
>>         RTE_ETH_SPEED_LANE_4 = 4,           /**< Link speed lanes 4 */
>>         RTE_ETH_SPEED_LANE_8 = 8,           /**< Link speed lanes 8 */
>>         RTE_ETH_SPEED_LANE_MAX,
>> };
>>
> I followed the existing enums code convention in rtelib. Your point
> makes sense too.
> 

I looked at the other enum code in the lib. There are many similar code styles.
Make the index meaningful to avoid conversion. For example, the parse_speed_lanes() function in this patch

>> In addition, when lanes = 0, is it better to define it as Unknown?
>> If default lanes can return 0 lanes, The active lanes are different for each NIC,
>> users may be confused.
>>
> Ack. Are you proposing a new enum RTE_ETH_SPEED_LANE_UKNOWN or rename
> RTE_ETH_SPEED_LANE_NOLANE?
> 

I suggest changing the name to RTE_ETH_SPEED_LANE_UKNOWN,
Also change the comment to describe it as an unknown lane.

This prevents the driver from always returning lanes=0
even if the driver knows the number of active lanes.

>>> +
>>> +/* Translate from link speed lanes to speed lanes capa */
>>> +#define RTE_ETH_SPEED_LANES_TO_CAPA(x) RTE_BIT32(x)
>>> +
>>> +/* This macro indicates link speed lanes capa mask */
>>> +#define RTE_ETH_SPEED_LANES_CAPA_MASK(x) RTE_BIT32(RTE_ETH_SPEED_ ## x)
>>> +
>>> +/* A structure used to get and set lanes capabilities per link speed */
>>> +struct rte_eth_speed_lanes_capa {
>>> +     uint32_t speed;
>>> +     uint32_t capa;
>>> +};
>>> +

cut

>>> +__rte_experimental
>>> +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
>>> +
>>
>> Is it better to name 'speed_lanes'?
> Are you proposing to rename to rte_speed_lanes_set() function name or
> rename variable "speed_lanes_capa" name ?
> 

rename variable "speed_lanes_capa" name

>>
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
>>> + *
>>> + * Set speed lanes supported by the NIC.
>>> + *
>>
>> Set -> Get
> Ack
>>
>>> + * @param port_id

....
  
Ferruh Yigit July 5, 2024, 5:33 p.m. UTC | #4
On 6/26/2024 3:19 AM, huangdengdui wrote:
> 
> On 2024/6/26 5:07, Damodharam Ammepalli wrote:
>> On Wed, Jun 19, 2024 at 8:23 PM huangdengdui <huangdengdui@huawei.com> wrote:
>>>
>>> Hi Damodharam
>>> Here are some suggestions. See below.
>>>
>> Thank you for the review.
>>
>>> On 2024/6/18 4:34, Damodharam Ammepalli wrote:
>>>> Update the eth_dev_ops structure with new function vectors
>>>> to get, get capabilities and set ethernet link speed lanes.
>>>> Update the testpmd to provide required config and information
>>>> display infrastructure.
>>>>
>>>> The supporting ethernet controller driver will register callbacks
>>>> to avail link speed lanes config and get services. This lanes
>>>> configuration is applicable only when the nic is forced to fixed
>>>> speeds. In Autonegiation mode, the hardware automatically
>>>> negotiates the number of lanes.
>>>>
>>>
>>>
>>>> +
>>>>  /* *** configure txq/rxq, txd/rxd *** */
>>>>  struct cmd_config_rx_tx {
>>>>       cmdline_fixed_string_t port;
>>>> @@ -13238,6 +13459,9 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
>>>>       (cmdline_parse_inst_t *)&cmd_set_port_setup_on,
> 
> cut
> 
>>>>
>>>> @@ -993,7 +1022,7 @@ port_summary_display(portid_t port_id)
>>>>       if (ret != 0)
>>>>               return;
>>>>
>>>> -     printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %s\n",
>>>> +     printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %-8s\n",
>>>
>>> Does the lanes need to be printed?
>> Ferruh in the previous comment, asked not to print.
>>
> 
> OK
> 
>>>
>>>>               port_id, RTE_ETHER_ADDR_BYTES(&mac_addr), name,
>>>>               dev_info.driver_name, (link.link_status) ? ("up") : ("down"),
>>>>               rte_eth_link_speed_to_str(link.link_speed));
>>>> @@ -7244,3 +7273,35 @@ show_mcast_macs(portid_t port_id)
>>>>               printf("  %s\n", buf);
>>>>       }
>>>>  }
>>>> +
>>>> +int
>>>> +parse_speed_lanes(uint32_t lane, uint32_t *speed_lane)
>>>> +{
>>>> +     uint8_t i;
>>>> +
>>>> +     for (i = 0; i < RTE_DIM(speed_lane_name); i++) {
>>>> +             if (speed_lane_name[i].value == lane) {
>>>> +                     *speed_lane = lane;
>>>> +                     return 0;
>>>> +             }
>>>> +     }
>>>> +     return -1;
>>>> +}
>>>> +
> 
> cut
> 
>>>>
>>>> +/**
>>>> + * This enum indicates the possible link speed lanes of an ethdev port.
>>>> + */
>>>> +enum rte_eth_speed_lanes {
>>>> +     RTE_ETH_SPEED_LANE_NOLANE = 0,  /**< speed lanes unsupported mode or default */
>>>> +     RTE_ETH_SPEED_LANE_1,           /**< Link speed lane  1 */
>>>> +     RTE_ETH_SPEED_LANE_2,           /**< Link speed lanes 2 */
>>>> +     RTE_ETH_SPEED_LANE_4,           /**< Link speed lanes 4 */
>>>> +     RTE_ETH_SPEED_LANE_8,           /**< Link speed lanes 8 */
>>>> +     RTE_ETH_SPEED_LANE_MAX,
>>>> +};
>>>
>>> Is it better to make the index equal to the lanes num?
>>> enum rte_eth_speed_lanes {
>>>         RTE_ETH_SPEED_LANE_NOLANE = 0,      /**< speed lanes unsupported mode or default */
>>>         RTE_ETH_SPEED_LANE_1 = 1,           /**< Link speed lane  1 */
>>>         RTE_ETH_SPEED_LANE_2 = 2,           /**< Link speed lanes 2 */
>>>         RTE_ETH_SPEED_LANE_4 = 4,           /**< Link speed lanes 4 */
>>>         RTE_ETH_SPEED_LANE_8 = 8,           /**< Link speed lanes 8 */
>>>         RTE_ETH_SPEED_LANE_MAX,
>>> };
>>>
>> I followed the existing enums code convention in rtelib. Your point
>> makes sense too.
>>
> 
> I looked at the other enum code in the lib. There are many similar code styles.
> Make the index meaningful to avoid conversion. For example, the parse_speed_lanes() function in this patch
> 
>>> In addition, when lanes = 0, is it better to define it as Unknown?
>>> If default lanes can return 0 lanes, The active lanes are different for each NIC,
>>> users may be confused.
>>>
>> Ack. Are you proposing a new enum RTE_ETH_SPEED_LANE_UKNOWN or rename
>> RTE_ETH_SPEED_LANE_NOLANE?
>>
> 
> I suggest changing the name to RTE_ETH_SPEED_LANE_UKNOWN,
> Also change the comment to describe it as an unknown lane.
> 
> This prevents the driver from always returning lanes=0
> even if the driver knows the number of active lanes.
> 
>>>> +
>>>> +/* Translate from link speed lanes to speed lanes capa */
>>>> +#define RTE_ETH_SPEED_LANES_TO_CAPA(x) RTE_BIT32(x)
>>>> +
>>>> +/* This macro indicates link speed lanes capa mask */
>>>> +#define RTE_ETH_SPEED_LANES_CAPA_MASK(x) RTE_BIT32(RTE_ETH_SPEED_ ## x)
>>>> +
>>>> +/* A structure used to get and set lanes capabilities per link speed */
>>>> +struct rte_eth_speed_lanes_capa {
>>>> +     uint32_t speed;
>>>> +     uint32_t capa;
>>>> +};
>>>> +
> 
> cut
> 
>>>> +__rte_experimental
>>>> +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
>>>> +
>>>
>>> Is it better to name 'speed_lanes'?
>> Are you proposing to rename to rte_speed_lanes_set() function name or
>> rename variable "speed_lanes_capa" name ?
>>
> 
> rename variable "speed_lanes_capa" name
> 

Hi Damodharam,

I was hoping to get this feature for -rc2, as outstanding comments are
not very big, if it can be possible to get new version in next few days
we can proceed with it for this release, otherwise can wait for next
release.

Btw, other issue is testing this new feature, as it is not supported by
many vendors, I expect this test case not included in many test plans.
And because of the HW dependency adding unit test won't be sufficient.
At least for the short term, can we rely Broadcom and Huawei to test it
with the first version this feature is merged?

And I am not sure about what can be the long term solution, any
suggestion is welcome. My concern is as number of this kind of features
are increasing, dpdk is becoming more fragile.
  
Ferruh Yigit July 5, 2024, 5:35 p.m. UTC | #5
On 6/17/2024 9:34 PM, Damodharam Ammepalli wrote:
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> + *
> + * Set speed lanes supported by the NIC.
> + *
> + * @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.
> + *
> + * @return
> + *  - (>=0) valid input and supported by driver or hardware.
> + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> + *     that operation.
> + *   - (-EIO) if device is removed.
> + *   - (-ENODEV)  if port_id invalid.
> + */
> +__rte_experimental
> +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
>

Doc build fails because of "@param speed_lanes":

rte_ethdev.h:6971:
  error: argument 'speed_lanes' of command @param is not found in the
argument list of
  rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa)
(warning treated as error, aborting now)
  
Damodharam Ammepalli July 8, 2024, 8:31 p.m. UTC | #6
On Fri, Jul 5, 2024 at 10:35 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 6/17/2024 9:34 PM, Damodharam Ammepalli wrote:
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
> > + *
> > + * Set speed lanes supported by the NIC.
> > + *
> > + * @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.
> > + *
> > + * @return
> > + *  - (>=0) valid input and supported by driver or hardware.
> > + *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
> > + *     that operation.
> > + *   - (-EIO) if device is removed.
> > + *   - (-ENODEV)  if port_id invalid.
> > + */
> > +__rte_experimental
> > +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
> >
>
> Doc build fails because of "@param speed_lanes":
>
> rte_ethdev.h:6971:
>   error: argument 'speed_lanes' of command @param is not found in the
> argument list of
>   rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa)
> (warning treated as error, aborting now)
Ack
  
Damodharam Ammepalli July 8, 2024, 8:35 p.m. UTC | #7
On Fri, Jul 5, 2024 at 10:33 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 6/26/2024 3:19 AM, huangdengdui wrote:
> >
> > On 2024/6/26 5:07, Damodharam Ammepalli wrote:
> >> On Wed, Jun 19, 2024 at 8:23 PM huangdengdui <huangdengdui@huawei.com> wrote:
> >>>
> >>> Hi Damodharam
> >>> Here are some suggestions. See below.
> >>>
> >> Thank you for the review.
> >>
> >>> On 2024/6/18 4:34, Damodharam Ammepalli wrote:
> >>>> Update the eth_dev_ops structure with new function vectors
> >>>> to get, get capabilities and set ethernet link speed lanes.
> >>>> Update the testpmd to provide required config and information
> >>>> display infrastructure.
> >>>>
> >>>> The supporting ethernet controller driver will register callbacks
> >>>> to avail link speed lanes config and get services. This lanes
> >>>> configuration is applicable only when the nic is forced to fixed
> >>>> speeds. In Autonegiation mode, the hardware automatically
> >>>> negotiates the number of lanes.
> >>>>
> >>>
> >>>
> >>>> +
> >>>>  /* *** configure txq/rxq, txd/rxd *** */
> >>>>  struct cmd_config_rx_tx {
> >>>>       cmdline_fixed_string_t port;
> >>>> @@ -13238,6 +13459,9 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
> >>>>       (cmdline_parse_inst_t *)&cmd_set_port_setup_on,
> >
> > cut
> >
> >>>>
> >>>> @@ -993,7 +1022,7 @@ port_summary_display(portid_t port_id)
> >>>>       if (ret != 0)
> >>>>               return;
> >>>>
> >>>> -     printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %s\n",
> >>>> +     printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %-8s\n",
> >>>
> >>> Does the lanes need to be printed?
> >> Ferruh in the previous comment, asked not to print.
> >>
> >
> > OK
> >
> >>>
> >>>>               port_id, RTE_ETHER_ADDR_BYTES(&mac_addr), name,
> >>>>               dev_info.driver_name, (link.link_status) ? ("up") : ("down"),
> >>>>               rte_eth_link_speed_to_str(link.link_speed));
> >>>> @@ -7244,3 +7273,35 @@ show_mcast_macs(portid_t port_id)
> >>>>               printf("  %s\n", buf);
> >>>>       }
> >>>>  }
> >>>> +
> >>>> +int
> >>>> +parse_speed_lanes(uint32_t lane, uint32_t *speed_lane)
> >>>> +{
> >>>> +     uint8_t i;
> >>>> +
> >>>> +     for (i = 0; i < RTE_DIM(speed_lane_name); i++) {
> >>>> +             if (speed_lane_name[i].value == lane) {
> >>>> +                     *speed_lane = lane;
> >>>> +                     return 0;
> >>>> +             }
> >>>> +     }
> >>>> +     return -1;
> >>>> +}
> >>>> +
> >
> > cut
> >
> >>>>
> >>>> +/**
> >>>> + * This enum indicates the possible link speed lanes of an ethdev port.
> >>>> + */
> >>>> +enum rte_eth_speed_lanes {
> >>>> +     RTE_ETH_SPEED_LANE_NOLANE = 0,  /**< speed lanes unsupported mode or default */
> >>>> +     RTE_ETH_SPEED_LANE_1,           /**< Link speed lane  1 */
> >>>> +     RTE_ETH_SPEED_LANE_2,           /**< Link speed lanes 2 */
> >>>> +     RTE_ETH_SPEED_LANE_4,           /**< Link speed lanes 4 */
> >>>> +     RTE_ETH_SPEED_LANE_8,           /**< Link speed lanes 8 */
> >>>> +     RTE_ETH_SPEED_LANE_MAX,
> >>>> +};
> >>>
> >>> Is it better to make the index equal to the lanes num?
> >>> enum rte_eth_speed_lanes {
> >>>         RTE_ETH_SPEED_LANE_NOLANE = 0,      /**< speed lanes unsupported mode or default */
> >>>         RTE_ETH_SPEED_LANE_1 = 1,           /**< Link speed lane  1 */
> >>>         RTE_ETH_SPEED_LANE_2 = 2,           /**< Link speed lanes 2 */
> >>>         RTE_ETH_SPEED_LANE_4 = 4,           /**< Link speed lanes 4 */
> >>>         RTE_ETH_SPEED_LANE_8 = 8,           /**< Link speed lanes 8 */
> >>>         RTE_ETH_SPEED_LANE_MAX,
> >>> };
> >>>
> >> I followed the existing enums code convention in rtelib. Your point
> >> makes sense too.
> >>
> >
> > I looked at the other enum code in the lib. There are many similar code styles.
> > Make the index meaningful to avoid conversion. For example, the parse_speed_lanes() function in this patch
> >
> >>> In addition, when lanes = 0, is it better to define it as Unknown?
> >>> If default lanes can return 0 lanes, The active lanes are different for each NIC,
> >>> users may be confused.
> >>>
> >> Ack. Are you proposing a new enum RTE_ETH_SPEED_LANE_UKNOWN or rename
> >> RTE_ETH_SPEED_LANE_NOLANE?
> >>
> >
> > I suggest changing the name to RTE_ETH_SPEED_LANE_UKNOWN,
> > Also change the comment to describe it as an unknown lane.
> >
> > This prevents the driver from always returning lanes=0
> > even if the driver knows the number of active lanes.
> >
> >>>> +
> >>>> +/* Translate from link speed lanes to speed lanes capa */
> >>>> +#define RTE_ETH_SPEED_LANES_TO_CAPA(x) RTE_BIT32(x)
> >>>> +
> >>>> +/* This macro indicates link speed lanes capa mask */
> >>>> +#define RTE_ETH_SPEED_LANES_CAPA_MASK(x) RTE_BIT32(RTE_ETH_SPEED_ ## x)
> >>>> +
> >>>> +/* A structure used to get and set lanes capabilities per link speed */
> >>>> +struct rte_eth_speed_lanes_capa {
> >>>> +     uint32_t speed;
> >>>> +     uint32_t capa;
> >>>> +};
> >>>> +
> >
> > cut
> >
> >>>> +__rte_experimental
> >>>> +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
> >>>> +
> >>>
> >>> Is it better to name 'speed_lanes'?
> >> Are you proposing to rename to rte_speed_lanes_set() function name or
> >> rename variable "speed_lanes_capa" name ?
> >>
> >
> > rename variable "speed_lanes_capa" name
> >
>
> Hi Damodharam,
>
> I was hoping to get this feature for -rc2, as outstanding comments are
> not very big, if it can be possible to get new version in next few days
> we can proceed with it for this release, otherwise can wait for next
> release.
>
> Btw, other issue is testing this new feature, as it is not supported by
> many vendors, I expect this test case not included in many test plans.
> And because of the HW dependency adding unit test won't be sufficient.
> At least for the short term, can we rely Broadcom and Huawei to test it
> with the first version this feature is merged?
>
> And I am not sure about what can be the long term solution, any
> suggestion is welcome. My concern is as number of this kind of features
> are increasing, dpdk is becoming more fragile.
>
>
>
Hi Ferruh,
Yes, I will push a new patch with review comments addressed in a day.
Sure, Broadcom will test it and provide the feedback. Infact, I am unit testing
internally with the brcm driver for every revision of the patch that is pushed.

Thanks
Damo
  

Patch

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index b7759e38a8..6eb5ca9a82 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -1560,6 +1560,110 @@  static cmdline_parse_inst_t cmd_config_speed_specific = {
 	},
 };
 
+static int
+parse_speed_lanes_cfg(portid_t pid, uint32_t lanes)
+{
+	int ret;
+	uint32_t lanes_capa;
+
+	ret = parse_speed_lanes(lanes, &lanes_capa);
+	if (ret < 0) {
+		fprintf(stderr, "Unknown speed lane value: %d for port %d\n", lanes, pid);
+		return -1;
+	}
+
+	ret = rte_eth_speed_lanes_set(pid, lanes_capa);
+	if (ret == -ENOTSUP) {
+		fprintf(stderr, "Function not implemented\n");
+		return -1;
+	} else if (ret < 0) {
+		fprintf(stderr, "Set speed lanes failed\n");
+		return -1;
+	}
+
+	return 0;
+}
+
+/* *** display speed lanes per port capabilities *** */
+struct cmd_show_speed_lanes_result {
+	cmdline_fixed_string_t cmd_show;
+	cmdline_fixed_string_t cmd_port;
+	cmdline_fixed_string_t cmd_keyword;
+	portid_t cmd_pid;
+};
+
+static void
+cmd_show_speed_lanes_parsed(void *parsed_result,
+			    __rte_unused struct cmdline *cl,
+			    __rte_unused void *data)
+{
+	struct cmd_show_speed_lanes_result *res = parsed_result;
+	struct rte_eth_speed_lanes_capa *speed_lanes_capa;
+	unsigned int num;
+	int ret;
+
+	if (!rte_eth_dev_is_valid_port(res->cmd_pid)) {
+		fprintf(stderr, "Invalid port id %u\n", res->cmd_pid);
+		return;
+	}
+
+	ret = rte_eth_speed_lanes_get_capability(res->cmd_pid, NULL, 0);
+	if (ret == -ENOTSUP) {
+		fprintf(stderr, "Function not implemented\n");
+		return;
+	} else if (ret < 0) {
+		fprintf(stderr, "Get speed lanes capability failed: %d\n", ret);
+		return;
+	}
+
+	num = (unsigned int)ret;
+	speed_lanes_capa = calloc(num, sizeof(*speed_lanes_capa));
+	if (speed_lanes_capa == NULL) {
+		fprintf(stderr, "Failed to alloc speed lanes capability buffer\n");
+		return;
+	}
+
+	ret = rte_eth_speed_lanes_get_capability(res->cmd_pid, speed_lanes_capa, num);
+	if (ret < 0) {
+		fprintf(stderr, "Error getting speed lanes capability: %d\n", ret);
+		goto out;
+	}
+
+	show_speed_lanes_capability(num, speed_lanes_capa);
+out:
+	free(speed_lanes_capa);
+}
+
+static cmdline_parse_token_string_t cmd_show_speed_lanes_show =
+	TOKEN_STRING_INITIALIZER(struct cmd_show_speed_lanes_result,
+				 cmd_show, "show");
+static cmdline_parse_token_string_t cmd_show_speed_lanes_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_show_speed_lanes_result,
+				 cmd_port, "port");
+static cmdline_parse_token_num_t cmd_show_speed_lanes_pid =
+	TOKEN_NUM_INITIALIZER(struct cmd_show_speed_lanes_result,
+			      cmd_pid, RTE_UINT16);
+static cmdline_parse_token_string_t cmd_show_speed_lanes_keyword =
+	TOKEN_STRING_INITIALIZER(struct cmd_show_speed_lanes_result,
+				 cmd_keyword, "speed_lanes");
+static cmdline_parse_token_string_t cmd_show_speed_lanes_cap_keyword =
+	TOKEN_STRING_INITIALIZER(struct cmd_show_speed_lanes_result,
+				 cmd_keyword, "capabilities");
+
+static cmdline_parse_inst_t cmd_show_speed_lanes = {
+	.f = cmd_show_speed_lanes_parsed,
+	.data = NULL,
+	.help_str = "show port <port_id> speed_lanes capabilities",
+	.tokens = {
+		(void *)&cmd_show_speed_lanes_show,
+		(void *)&cmd_show_speed_lanes_port,
+		(void *)&cmd_show_speed_lanes_pid,
+		(void *)&cmd_show_speed_lanes_keyword,
+		(void *)&cmd_show_speed_lanes_cap_keyword,
+		NULL,
+	},
+};
+
 /* *** configure loopback for all ports *** */
 struct cmd_config_loopback_all {
 	cmdline_fixed_string_t port;
@@ -1676,6 +1780,123 @@  static cmdline_parse_inst_t cmd_config_loopback_specific = {
 	},
 };
 
+/* *** configure speed_lanes for all ports *** */
+struct cmd_config_speed_lanes_all {
+	cmdline_fixed_string_t port;
+	cmdline_fixed_string_t keyword;
+	cmdline_fixed_string_t all;
+	cmdline_fixed_string_t item;
+	uint32_t lanes;
+};
+
+static void
+cmd_config_speed_lanes_all_parsed(void *parsed_result,
+				  __rte_unused struct cmdline *cl,
+				  __rte_unused void *data)
+{
+	struct cmd_config_speed_lanes_all *res = parsed_result;
+	portid_t pid;
+
+	if (!all_ports_stopped()) {
+		fprintf(stderr, "Please stop all ports first\n");
+		return;
+	}
+
+	RTE_ETH_FOREACH_DEV(pid) {
+		if (parse_speed_lanes_cfg(pid, res->lanes))
+			return;
+	}
+
+	cmd_reconfig_device_queue(RTE_PORT_ALL, 1, 1);
+}
+
+static cmdline_parse_token_string_t cmd_config_speed_lanes_all_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, port, "port");
+static cmdline_parse_token_string_t cmd_config_speed_lanes_all_keyword =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, keyword,
+				 "config");
+static cmdline_parse_token_string_t cmd_config_speed_lanes_all_all =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, all, "all");
+static cmdline_parse_token_string_t cmd_config_speed_lanes_all_item =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, item,
+				 "speed_lanes");
+static cmdline_parse_token_num_t cmd_config_speed_lanes_all_lanes =
+	TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_all, lanes, RTE_UINT32);
+
+static cmdline_parse_inst_t cmd_config_speed_lanes_all = {
+	.f = cmd_config_speed_lanes_all_parsed,
+	.data = NULL,
+	.help_str = "port config all speed_lanes <value>",
+	.tokens = {
+		(void *)&cmd_config_speed_lanes_all_port,
+		(void *)&cmd_config_speed_lanes_all_keyword,
+		(void *)&cmd_config_speed_lanes_all_all,
+		(void *)&cmd_config_speed_lanes_all_item,
+		(void *)&cmd_config_speed_lanes_all_lanes,
+		NULL,
+	},
+};
+
+/* *** configure speed_lanes for specific port *** */
+struct cmd_config_speed_lanes_specific {
+	cmdline_fixed_string_t port;
+	cmdline_fixed_string_t keyword;
+	uint16_t port_id;
+	cmdline_fixed_string_t item;
+	uint32_t lanes;
+};
+
+static void
+cmd_config_speed_lanes_specific_parsed(void *parsed_result,
+				       __rte_unused struct cmdline *cl,
+				       __rte_unused void *data)
+{
+	struct cmd_config_speed_lanes_specific *res = parsed_result;
+
+	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
+		return;
+
+	if (!port_is_stopped(res->port_id)) {
+		fprintf(stderr, "Please stop port %u first\n", res->port_id);
+		return;
+	}
+
+	if (parse_speed_lanes_cfg(res->port_id, res->lanes))
+		return;
+
+	cmd_reconfig_device_queue(res->port_id, 1, 1);
+}
+
+static cmdline_parse_token_string_t cmd_config_speed_lanes_specific_port =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, port,
+				 "port");
+static cmdline_parse_token_string_t cmd_config_speed_lanes_specific_keyword =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, keyword,
+				 "config");
+static cmdline_parse_token_num_t cmd_config_speed_lanes_specific_id =
+	TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_specific, port_id,
+			      RTE_UINT16);
+static cmdline_parse_token_string_t cmd_config_speed_lanes_specific_item =
+	TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, item,
+				 "speed_lanes");
+static cmdline_parse_token_num_t cmd_config_speed_lanes_specific_lanes =
+	TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_specific, lanes,
+			      RTE_UINT32);
+
+static cmdline_parse_inst_t cmd_config_speed_lanes_specific = {
+	.f = cmd_config_speed_lanes_specific_parsed,
+	.data = NULL,
+	.help_str = "port config <port_id> speed_lanes <value>",
+	.tokens = {
+		(void *)&cmd_config_speed_lanes_specific_port,
+		(void *)&cmd_config_speed_lanes_specific_keyword,
+		(void *)&cmd_config_speed_lanes_specific_id,
+		(void *)&cmd_config_speed_lanes_specific_item,
+		(void *)&cmd_config_speed_lanes_specific_lanes,
+		NULL,
+	},
+};
+
 /* *** configure txq/rxq, txd/rxd *** */
 struct cmd_config_rx_tx {
 	cmdline_fixed_string_t port;
@@ -13238,6 +13459,9 @@  static cmdline_parse_ctx_t builtin_ctx[] = {
 	(cmdline_parse_inst_t *)&cmd_set_port_setup_on,
 	(cmdline_parse_inst_t *)&cmd_config_speed_all,
 	(cmdline_parse_inst_t *)&cmd_config_speed_specific,
+	(cmdline_parse_inst_t *)&cmd_config_speed_lanes_all,
+	(cmdline_parse_inst_t *)&cmd_config_speed_lanes_specific,
+	(cmdline_parse_inst_t *)&cmd_show_speed_lanes,
 	(cmdline_parse_inst_t *)&cmd_config_loopback_all,
 	(cmdline_parse_inst_t *)&cmd_config_loopback_specific,
 	(cmdline_parse_inst_t *)&cmd_config_rx_tx,
diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 66c3a68c1d..cf3ea50114 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -207,6 +207,32 @@  static const struct {
 	{"gtpu", RTE_ETH_FLOW_GTPU},
 };
 
+static const struct {
+	enum rte_eth_speed_lanes lane;
+	const uint32_t value;
+} speed_lane_name[] = {
+	{
+		.lane = RTE_ETH_SPEED_LANE_NOLANE,
+		.value = 0,
+	},
+	{
+		.lane = RTE_ETH_SPEED_LANE_1,
+		.value = 1,
+	},
+	{
+		.lane = RTE_ETH_SPEED_LANE_2,
+		.value = 2,
+	},
+	{
+		.lane = RTE_ETH_SPEED_LANE_4,
+		.value = 4,
+	},
+	{
+		.lane = RTE_ETH_SPEED_LANE_8,
+		.value = 8,
+	},
+};
+
 static void
 print_ethaddr(const char *name, struct rte_ether_addr *eth_addr)
 {
@@ -786,6 +812,7 @@  port_infos_display(portid_t port_id)
 	char name[RTE_ETH_NAME_MAX_LEN];
 	int ret;
 	char fw_version[ETHDEV_FWVERS_LEN];
+	uint32_t lanes;
 
 	if (port_id_is_invalid(port_id, ENABLED_WARN)) {
 		print_valid_ports();
@@ -828,6 +855,8 @@  port_infos_display(portid_t port_id)
 
 	printf("\nLink status: %s\n", (link.link_status) ? ("up") : ("down"));
 	printf("Link speed: %s\n", rte_eth_link_speed_to_str(link.link_speed));
+	if (rte_eth_speed_lanes_get(port_id, &lanes) == 0)
+		printf("Active Lanes: %d\n", lanes);
 	printf("Link duplex: %s\n", (link.link_duplex == RTE_ETH_LINK_FULL_DUPLEX) ?
 	       ("full-duplex") : ("half-duplex"));
 	printf("Autoneg status: %s\n", (link.link_autoneg == RTE_ETH_LINK_AUTONEG) ?
@@ -962,7 +991,7 @@  port_summary_header_display(void)
 
 	port_number = rte_eth_dev_count_avail();
 	printf("Number of available ports: %i\n", port_number);
-	printf("%-4s %-17s %-12s %-14s %-8s %s\n", "Port", "MAC Address", "Name",
+	printf("%-4s %-17s %-12s %-14s %-8s %-8s\n", "Port", "MAC Address", "Name",
 			"Driver", "Status", "Link");
 }
 
@@ -993,7 +1022,7 @@  port_summary_display(portid_t port_id)
 	if (ret != 0)
 		return;
 
-	printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %s\n",
+	printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %-8s\n",
 		port_id, RTE_ETHER_ADDR_BYTES(&mac_addr), name,
 		dev_info.driver_name, (link.link_status) ? ("up") : ("down"),
 		rte_eth_link_speed_to_str(link.link_speed));
@@ -7244,3 +7273,35 @@  show_mcast_macs(portid_t port_id)
 		printf("  %s\n", buf);
 	}
 }
+
+int
+parse_speed_lanes(uint32_t lane, uint32_t *speed_lane)
+{
+	uint8_t i;
+
+	for (i = 0; i < RTE_DIM(speed_lane_name); i++) {
+		if (speed_lane_name[i].value == lane) {
+			*speed_lane = lane;
+			return 0;
+		}
+	}
+	return -1;
+}
+
+void
+show_speed_lanes_capability(unsigned int num, struct rte_eth_speed_lanes_capa *speed_lanes_capa)
+{
+	unsigned int i, j;
+
+	printf("\n%-15s %-10s", "Supported-speeds", "Valid-lanes");
+	printf("\n-----------------------------------\n");
+	for (i = 0; i < num; i++) {
+		printf("%-17s ", rte_eth_link_speed_to_str(speed_lanes_capa[i].speed));
+
+		for (j = 0; j < RTE_ETH_SPEED_LANE_MAX; j++) {
+			if (RTE_ETH_SPEED_LANES_TO_CAPA(j) & speed_lanes_capa[i].capa)
+				printf("%-2d ", speed_lane_name[j].value);
+		}
+		printf("\n");
+	}
+}
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 9facd7f281..fb9ef05cc5 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -1253,6 +1253,10 @@  extern int flow_parse(const char *src, void *result, unsigned int size,
 		      struct rte_flow_item **pattern,
 		      struct rte_flow_action **actions);
 
+void show_speed_lanes_capability(uint32_t num,
+				 struct rte_eth_speed_lanes_capa *speed_lanes_capa);
+int parse_speed_lanes(uint32_t lane, uint32_t *speed_lane);
+
 uint64_t str_to_rsstypes(const char *str);
 const char *rsstypes_to_str(uint64_t rss_type);
 
diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
index 883e59a927..0f10aec3a1 100644
--- a/lib/ethdev/ethdev_driver.h
+++ b/lib/ethdev/ethdev_driver.h
@@ -1179,6 +1179,79 @@  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
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param speed_lanes
+ *   Number of active lanes that the link is trained up.
+ * @return
+ *   Negative errno value on error, 0 on success.
+ *
+ * @retval 0
+ *   Success, get speed_lanes data success.
+ * @retval -ENOTSUP
+ *   Operation is not supported.
+ * @retval -EIO
+ *   Device is removed.
+ */
+typedef int (*eth_speed_lanes_get_t)(struct rte_eth_dev *dev, uint32_t *speed_lanes);
+
+/**
+ * @internal
+ * Set speed lanes
+ *
+ * @param dev
+ *   ethdev handle of port.
+ * @param speed_lanes
+ *   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);
+
+/**
+ * @internal
+ * Get supported link speed lanes capability
+ *
+ * @param speed_lanes_capa
+ *   speed_lanes_capa is out only with per-speed capabilities.
+ * @param num
+ *   a number of elements in an speed_speed_lanes_capa array.
+ *
+ * @return
+ *   Negative errno value on error, positive value on success.
+ *
+ * @retval positive value
+ *   A non-negative value lower or equal to num: success. The return value
+ *   is the number of entries filled in the speed lanes array.
+ *   A non-negative value higher than num: error, the given speed lanes capa array
+ *   is too small. The return value corresponds to the num that should
+ *   be given to succeed. The entries in the speed lanes capa array are not valid
+ *   and shall not be used by the caller.
+ * @retval -ENOTSUP
+ *   Operation is not supported.
+ * @retval -EIO
+ *   Device is removed.
+ * @retval -EINVAL
+ *   *num* or *speed_lanes_capa* invalid.
+ */
+typedef int (*eth_speed_lanes_get_capability_t)(struct rte_eth_dev *dev,
+						struct rte_eth_speed_lanes_capa *speed_lanes_capa,
+						unsigned int num);
+
 /**
  * @internal
  * Dump Tx descriptor info to a file.
@@ -1247,6 +1320,10 @@  struct eth_dev_ops {
 	eth_dev_close_t            dev_close;     /**< Close device */
 	eth_dev_reset_t		   dev_reset;	  /**< Reset device */
 	eth_link_update_t          link_update;   /**< Get device link state */
+	eth_speed_lanes_get_t	   speed_lanes_get;	  /**<Get link speed active lanes */
+	eth_speed_lanes_set_t      speed_lanes_set;	  /**<set the link speeds supported lanes */
+	/** Get link speed lanes capability */
+	eth_speed_lanes_get_capability_t speed_lanes_get_capa;
 	/** Check if the device was physically removed */
 	eth_is_removed_t           is_removed;
 
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index f1c658f49e..07cefea307 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -7008,4 +7008,55 @@  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, uint32_t *lane)
+{
+	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, lane));
+}
+
+int
+rte_eth_speed_lanes_get_capability(uint16_t port_id,
+				   struct rte_eth_speed_lanes_capa *speed_lanes_capa,
+				   unsigned int num)
+{
+	struct rte_eth_dev *dev;
+	int ret;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+	dev = &rte_eth_devices[port_id];
+
+	if (speed_lanes_capa == NULL && num > 0) {
+		RTE_ETHDEV_LOG_LINE(ERR,
+				    "Cannot get ethdev port %u speed lanes capability to NULL when array size is non zero",
+				    port_id);
+		return -EINVAL;
+	}
+
+	if (*dev->dev_ops->speed_lanes_get_capa == NULL)
+		return -ENOTSUP;
+	ret = (*dev->dev_ops->speed_lanes_get_capa)(dev, speed_lanes_capa, num);
+
+	return ret;
+}
+
+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 548fada1c7..f5bacd4ba4 100644
--- a/lib/ethdev/rte_ethdev.h
+++ b/lib/ethdev/rte_ethdev.h
@@ -357,6 +357,30 @@  struct rte_eth_link {
 #define RTE_ETH_LINK_MAX_STR_LEN 40 /**< Max length of default link string. */
 /**@}*/
 
+/**
+ * This enum indicates the possible link speed lanes of an ethdev port.
+ */
+enum rte_eth_speed_lanes {
+	RTE_ETH_SPEED_LANE_NOLANE = 0,  /**< speed lanes unsupported mode or default */
+	RTE_ETH_SPEED_LANE_1,           /**< Link speed lane  1 */
+	RTE_ETH_SPEED_LANE_2,           /**< Link speed lanes 2 */
+	RTE_ETH_SPEED_LANE_4,           /**< Link speed lanes 4 */
+	RTE_ETH_SPEED_LANE_8,           /**< Link speed lanes 8 */
+	RTE_ETH_SPEED_LANE_MAX,
+};
+
+/* Translate from link speed lanes to speed lanes capa */
+#define RTE_ETH_SPEED_LANES_TO_CAPA(x) RTE_BIT32(x)
+
+/* This macro indicates link speed lanes capa mask */
+#define RTE_ETH_SPEED_LANES_CAPA_MASK(x) RTE_BIT32(RTE_ETH_SPEED_ ## x)
+
+/* A structure used to get and set lanes capabilities per link speed */
+struct rte_eth_speed_lanes_capa {
+	uint32_t speed;
+	uint32_t capa;
+};
+
 /**
  * A structure used to configure the ring threshold registers of an Rx/Tx
  * queue for an Ethernet port.
@@ -6922,6 +6946,72 @@  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 Active lanes.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param lanes
+ *   driver populates a active lanes value whether link is Autonegotiated or Fixed speed.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
+ *     that operation.
+ *   - (-EIO) if device is removed.
+ *   - (-ENODEV)  if *port_id* invalid.
+ */
+__rte_experimental
+int rte_eth_speed_lanes_get(uint16_t port_id, uint32_t *lanes);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Set speed lanes supported by the NIC.
+ *
+ * @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.
+ *
+ * @return
+ *  - (>=0) valid input and supported by driver or hardware.
+ *   - (-ENOTSUP) if underlying hardware OR driver doesn't support.
+ *     that operation.
+ *   - (-EIO) if device is removed.
+ *   - (-ENODEV)  if *port_id* invalid.
+ */
+__rte_experimental
+int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Set speed lanes supported by the NIC.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param speed_lanes_bmap
+ *   speed_lanes_bmap int array updated by driver by valid lanes bmap.
+ *
+ * @return
+ *  - (>=0) valid input and supported by driver or hardware.
+ *   - (-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_get_capability(uint16_t port_id,
+				       struct rte_eth_speed_lanes_capa *speed_lanes_capa,
+				       unsigned int num);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
index 79f6f5293b..db9261946f 100644
--- a/lib/ethdev/version.map
+++ b/lib/ethdev/version.map
@@ -325,6 +325,11 @@  EXPERIMENTAL {
 	rte_flow_template_table_resizable;
 	rte_flow_template_table_resize;
 	rte_flow_template_table_resize_complete;
+
+	# added in 24.07
+	rte_eth_speed_lanes_get;
+	rte_eth_speed_lanes_get_capability;
+	rte_eth_speed_lanes_set;
 };
 
 INTERNAL {