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 A60DA454FA; Wed, 26 Jun 2024 04:19:51 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 531BF42DDA; Wed, 26 Jun 2024 04:19:51 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 3E834402A9 for ; Wed, 26 Jun 2024 04:19:49 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.163.174]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4W84zb0vy2zZcGW; Wed, 26 Jun 2024 10:15:23 +0800 (CST) Received: from dggpeml500011.china.huawei.com (unknown [7.185.36.84]) by mail.maildlp.com (Postfix) with ESMTPS id E9F22140382; Wed, 26 Jun 2024 10:19:46 +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; Wed, 26 Jun 2024 10:19:46 +0800 Message-ID: <4a0d1bc0-4edf-4653-8b23-f2b12a924b3d@huawei.com> Date: Wed, 26 Jun 2024 10:19:46 +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: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.193] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) 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 On 2024/6/26 5:07, Damodharam Ammepalli wrote: > On Wed, Jun 19, 2024 at 8:23 PM huangdengdui 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 ....