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 {
