From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id D7E8345491; Thu, 20 Jun 2024 05:23:23 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A9ECB415D7; Thu, 20 Jun 2024 05:23:23 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 9D4EE40BA2 for ; Thu, 20 Jun 2024 05:23:21 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.163.174]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4W4Qgt1NqvzxSlh; Thu, 20 Jun 2024 11:19:06 +0800 (CST) Received: from dggpeml500011.china.huawei.com (unknown [7.185.36.84]) by mail.maildlp.com (Postfix) with ESMTPS id 15F2A140120; Thu, 20 Jun 2024 11:23:19 +0800 (CST) Received: from [10.67.121.193] (10.67.121.193) by dggpeml500011.china.huawei.com (7.185.36.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Thu, 20 Jun 2024 11:23:18 +0800 Message-ID: Date: Thu, 20 Jun 2024 11:23:18 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] ethdev: Add link_speed lanes support Content-Language: en-US To: Damodharam Ammepalli , CC: , , References: <20240617203448.97972-1-damodharam.ammepalli@broadcom.com> From: huangdengdui In-Reply-To: <20240617203448.97972-1-damodharam.ammepalli@broadcom.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.121.193] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggpeml500011.china.huawei.com (7.185.36.84) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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; /** + eth_speed_lanes_set_t speed_lanes_set; /** + /** 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 {