Re: [PATCH v3] ethdev: Add link_speed lanes support

[huangdengdui <huangdengdui@huawei.com>]

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 {