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 7757B455CE; Tue, 9 Jul 2024 23:20:31 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id EF878402A9; Tue, 9 Jul 2024 23:20:30 +0200 (CEST) Received: from mail-lj1-f176.google.com (mail-lj1-f176.google.com [209.85.208.176]) by mails.dpdk.org (Postfix) with ESMTP id A0A6A402A9 for ; Tue, 9 Jul 2024 23:20:28 +0200 (CEST) Received: by mail-lj1-f176.google.com with SMTP id 38308e7fff4ca-2eea8ea8bb0so44154181fa.1 for ; Tue, 09 Jul 2024 14:20:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; t=1720560028; x=1721164828; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=ApU+j+II4e0oZvKOncvJlWBrlO9qfg0X4tHDw/HhIfE=; b=Ys97i6lT4xKzUajpqJaFJ78lkj3mpLx51PCHqFyNsV2yGr1szOEWgLmfqVfh7wH+4m 6nG+O+cGyZYivS5oHGev0m/tTlk+XDnoCWkBiOusBYsl+N5HFOBCns0Q3bQ2IqTqsWsT QYQTAZ1/xy1Y/VlEcChB35VuNveRmXhNSmBbk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720560028; x=1721164828; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ApU+j+II4e0oZvKOncvJlWBrlO9qfg0X4tHDw/HhIfE=; b=KcyFwLn2bLGwAzyy3PgCI/b4ZDg743os43rMkvKcDNOib7e5L5Y1Rg0Lzzic0Z5xY4 IWn35kTf7g7u6bJvwZ8mnr1vnNSyk3HMdNC/wUCinU0FjGh2nFfAI5Ue5Vp/4Zuvw7ph TdNAlMOrkP0pZJIcZ5F9FK8KplBmoL2RjQPoKbymyqAksR/9EOSo/jhjeZYchcmzh720 PcQdVZtDkX+R69V7XX0wc20M3yqBhdphiH5Pd+OdqiIoHXt8xxe9HuQ0BW+cEbN96pQQ 7/wgWJA4YIJWPYr6odoGKhDoVKdigX7nRmySF+6P6UBTlXb5ZcmHUIIMTNh0uLP5JYDY Ip0g== X-Forwarded-Encrypted: i=1; AJvYcCUMTh4ugNgFN/IVjGT9fomC5vACh017Nnt3MmRItZpzfnc2UyTxsxZ6lWZkz2Xc3qztiLSSXOjun1ySl1E= X-Gm-Message-State: AOJu0YyWlWKQFCuliJgvhOcEFs+WOPC5cXO/G0RbkfqThWy7Y8WPcCA5 5di7Bh7qjEnwtrXPgxj+T43XOC5M1viON0h+TUuvJxDZMXwofiH6SXDIMDZ/QCaIJTg0GxjodH3 ZfVtdKs7RMzX02JpdwNAEX5HjnhwegPee6TlYS5yxS4J8btjZQEfxWE3Ac+nxC/nCwnd8PDY9Xm DqVcha X-Google-Smtp-Source: AGHT+IE+1RXt3bb+m2lXScchSgvMCzb3x6Oeea+IiqiTDnPswfzj4kNL2ZyiaQMeyi471KNb0M0+4nkvs3erEPAivN8= X-Received: by 2002:a05:651c:a09:b0:2ec:4d8a:785a with SMTP id 38308e7fff4ca-2eeb30ba739mr39437851fa.4.1720560027667; Tue, 09 Jul 2024 14:20:27 -0700 (PDT) MIME-Version: 1.0 References: <20240708232351.491529-1-damodharam.ammepalli@broadcom.com> <3577879f-4652-4633-8ea8-badd36aaeb8a@amd.com> In-Reply-To: <3577879f-4652-4633-8ea8-badd36aaeb8a@amd.com> From: Damodharam Ammepalli Date: Tue, 9 Jul 2024 14:20:15 -0700 Message-ID: Subject: Re: [PATCH v4] ethdev: Add link_speed lanes support To: Ferruh Yigit Cc: ajit.khaparde@broadcom.com, dev@dpdk.org, huangdengdui@huawei.com, kalesh-anakkur.purayil@broadcom.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Tue, Jul 9, 2024 at 4:10=E2=80=AFAM Ferruh Yigit = wrote: > > On 7/9/2024 12:22 AM, 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. > > > > 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 > > > > Is there a requirement to set speed before speed_lane? > Because I expect driver will verify if a speed_lane value is valid or > not for a specific speed value. In above usage, driver will verify based > on existing speed, whatever it is, later chaning speed may cause invalid > speed_lane configuration. > > There is no requirement to set speed before speed_lane. If the controller supports lanes configuration capability, if no lanes are given (which is 0) the driver will pick up the lowest speed (eg: 200 gbps with NRZ mode), if a fixed speed already exists or is configured in tandem with speed_lanes. If speed is already Auto, test-pmd's speed_lane config is ignored. > > 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 > > --- > > v2->v3 Consolidating the testpmd and rtelib patches into a single patch > > as requested. > > v3->v4 Addressed comments and fix help string and documentation. > > > > app/test-pmd/cmdline.c | 230 +++++++++++++++++++++++++++++++++++++ > > app/test-pmd/config.c | 69 ++++++++++- > > app/test-pmd/testpmd.h | 4 + > > lib/ethdev/ethdev_driver.h | 77 +++++++++++++ > > lib/ethdev/rte_ethdev.c | 51 ++++++++ > > lib/ethdev/rte_ethdev.h | 92 +++++++++++++++ > > lib/ethdev/version.map | 5 + > > 7 files changed, 526 insertions(+), 2 deletions(-) > > > > diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c > > index b7759e38a8..a507df31d8 100644 > > --- a/app/test-pmd/cmdline.c > > +++ b/app/test-pmd/cmdline.c > > @@ -284,6 +284,9 @@ static void cmd_help_long_parsed(void *parsed_resul= t, > > > > "dump_log_types\n" > > " Dumps the log level for all the dpdk modules= \n\n" > > + > > + "show port (port_id) speed_lanes capabilities" > > + " Show speed lanes capabilities of a port.\= n\n" > > ); > > } > > > > @@ -823,6 +826,9 @@ static void cmd_help_long_parsed(void *parsed_resul= t, > > "port config (port_id) txq (queue_id) affinity (v= alue)\n" > > " Map a Tx queue with an aggregated port " > > "of the DPDK port\n\n" > > + > > + "port config (port_id|all) speed_lanes (0|1|4|8)\= n" > > + " Set number of lanes for all ports or port_id= for a forced speed\n\n" > > ); > > } > > > > @@ -1560,6 +1566,110 @@ static cmdline_parse_inst_t cmd_config_speed_sp= ecific =3D { > > }, > > }; > > > > +static int > > +parse_speed_lanes_cfg(portid_t pid, uint32_t lanes) > > +{ > > + int ret; > > + uint32_t lanes_capa; > > + > > + ret =3D 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 =3D rte_eth_speed_lanes_set(pid, lanes_capa); > > + if (ret =3D=3D -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 =3D 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 =3D rte_eth_speed_lanes_get_capability(res->cmd_pid, NULL, 0)= ; > > + if (ret =3D=3D -ENOTSUP) { > > + fprintf(stderr, "Function not implemented\n"); > > + return; > > + } else if (ret < 0) { > > + fprintf(stderr, "Get speed lanes capability failed: %d\n"= , ret); > > + return; > > + } > > + > > + num =3D (unsigned int)ret; > > + speed_lanes_capa =3D calloc(num, sizeof(*speed_lanes_capa)); > > + if (speed_lanes_capa =3D=3D NULL) { > > + fprintf(stderr, "Failed to alloc speed lanes capability b= uffer\n"); > > + return; > > + } > > + > > + ret =3D rte_eth_speed_lanes_get_capability(res->cmd_pid, speed_la= nes_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 =3D > > + TOKEN_STRING_INITIALIZER(struct cmd_show_speed_lanes_result, > > + cmd_show, "show"); > > +static cmdline_parse_token_string_t cmd_show_speed_lanes_port =3D > > + TOKEN_STRING_INITIALIZER(struct cmd_show_speed_lanes_result, > > + cmd_port, "port"); > > +static cmdline_parse_token_num_t cmd_show_speed_lanes_pid =3D > > + TOKEN_NUM_INITIALIZER(struct cmd_show_speed_lanes_result, > > + cmd_pid, RTE_UINT16); > > +static cmdline_parse_token_string_t cmd_show_speed_lanes_keyword =3D > > + 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 = =3D > > + TOKEN_STRING_INITIALIZER(struct cmd_show_speed_lanes_result, > > + cmd_keyword, "capabilities"); > > + > > +static cmdline_parse_inst_t cmd_show_speed_lanes =3D { > > + .f =3D cmd_show_speed_lanes_parsed, > > + .data =3D NULL, > > + .help_str =3D "show port speed_lanes capabilities", > > + .tokens =3D { > > + (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 +1786,123 @@ static cmdline_parse_inst_t cmd_config_loopback= _specific =3D { > > }, > > }; > > > > +/* *** 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 =3D 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 = =3D > > + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, port,= "port"); > > +static cmdline_parse_token_string_t cmd_config_speed_lanes_all_keyword= =3D > > + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, keywo= rd, > > + "config"); > > +static cmdline_parse_token_string_t cmd_config_speed_lanes_all_all =3D > > + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, all, = "all"); > > +static cmdline_parse_token_string_t cmd_config_speed_lanes_all_item = =3D > > + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, item, > > + "speed_lanes"); > > +static cmdline_parse_token_num_t cmd_config_speed_lanes_all_lanes =3D > > + TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_all, lanes, R= TE_UINT32); > > + > > +static cmdline_parse_inst_t cmd_config_speed_lanes_all =3D { > > + .f =3D cmd_config_speed_lanes_all_parsed, > > + .data =3D NULL, > > + .help_str =3D "port config all speed_lanes ", > > + .tokens =3D { > > + (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 =3D 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; > > + } > > > > There is a requirement here, that port needs to be stopped before > calling the rte_eth_speed_lanes_set(), > is this requirement documented in the API documentation? > > Speed link mode needs a phy reset, hence port stop is a requirement. I will update this in the documentation in the next patch. > > + > > + 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_po= rt =3D > > + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, = port, > > + "port"); > > +static cmdline_parse_token_string_t cmd_config_speed_lanes_specific_ke= yword =3D > > + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, = keyword, > > + "config"); > > +static cmdline_parse_token_num_t cmd_config_speed_lanes_specific_id = =3D > > + TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_specific, por= t_id, > > + RTE_UINT16); > > +static cmdline_parse_token_string_t cmd_config_speed_lanes_specific_it= em =3D > > + TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, = item, > > + "speed_lanes"); > > +static cmdline_parse_token_num_t cmd_config_speed_lanes_specific_lanes= =3D > > + TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_specific, lan= es, > > + RTE_UINT32); > > + > > +static cmdline_parse_inst_t cmd_config_speed_lanes_specific =3D { > > + .f =3D cmd_config_speed_lanes_specific_parsed, > > + .data =3D NULL, > > + .help_str =3D "port config speed_lanes ", > > + .tokens =3D { > > + (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 +13465,9 @@ static cmdline_parse_ctx_t builtin_ctx[] =3D { > > (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..498a7db467 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[] =3D { > > + { > > + .lane =3D RTE_ETH_SPEED_LANE_UNKNOWN, > > + .value =3D 0, > > + }, > > + { > > + .lane =3D RTE_ETH_SPEED_LANE_1, > > + .value =3D 1, > > + }, > > + { > > + .lane =3D RTE_ETH_SPEED_LANE_2, > > + .value =3D 2, > > + }, > > + { > > + .lane =3D RTE_ETH_SPEED_LANE_4, > > + .value =3D 4, > > + }, > > + { > > + .lane =3D RTE_ETH_SPEED_LANE_8, > > + .value =3D 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,12 @@ port_infos_display(portid_t port_id) > > > > printf("\nLink status: %s\n", (link.link_status) ? ("up") : ("dow= n")); > > printf("Link speed: %s\n", rte_eth_link_speed_to_str(link.link_sp= eed)); > > + if (rte_eth_speed_lanes_get(port_id, &lanes) =3D=3D 0) { > > + if (lanes > 0) > > + printf("Active Lanes: %d\n", lanes); > > + else > > + printf("Active Lanes: %s\n", "Unknown"); > > > > What can be the 'else' case? > As 'lanes' is unsigned, only option is it being zero. Is API allowed to > return zero as lane number? > > Yes. link is down, but controller supports speed_lanes capability, then we can show "unknown" Other cases from brcm spec. 1gb 1Gb link speed < no lane info > (theoretically it can't be zero, but we need to show what controller provides in the query). 10Gb (NRZ: 10G per lane, 1 lane) link speed > > + } > > printf("Link duplex: %s\n", (link.link_duplex =3D=3D RTE_ETH_LINK= _FULL_DUPLEX) ? > > ("full-duplex") : ("half-duplex")); > > printf("Autoneg status: %s\n", (link.link_autoneg =3D=3D RTE_ETH_= LINK_AUTONEG) ? > > @@ -962,7 +995,7 @@ port_summary_header_display(void) > > > > port_number =3D 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 +1026,7 @@ port_summary_display(portid_t port_id) > > if (ret !=3D 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", > > > > Summary updates are irrelevant in the patch, can you please drop them. > > Sure I will. > > port_id, RTE_ETHER_ADDR_BYTES(&mac_addr), name, > > dev_info.driver_name, (link.link_status) ? ("up") : ("dow= n"), > > rte_eth_link_speed_to_str(link.link_speed)); > > @@ -7244,3 +7277,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 =3D 0; i < RTE_DIM(speed_lane_name); i++) { > > + if (speed_lane_name[i].value =3D=3D lane) { > > + *speed_lane =3D lane; > > > > This converts from 8 -> 8, 4 -> 4 .... > > Why not completely eliminate this fucntion? See below. > Sure, will evaluate and do the needful. > > + return 0; > > + } > > + } > > + return -1; > > +} > > + > > +void > > +show_speed_lanes_capability(unsigned int num, struct rte_eth_speed_lan= es_capa *speed_lanes_capa) > > +{ > > + unsigned int i, j; > > + > > + printf("\n%-15s %-10s", "Supported-speeds", "Valid-lanes"); > > + printf("\n-----------------------------------\n"); > > + for (i =3D 0; i < num; i++) { > > + printf("%-17s ", rte_eth_link_speed_to_str(speed_lanes_ca= pa[i].speed)); > > + > > + for (j =3D 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)= ; > > + } > > To eliminate both RTE_ETH_SPEED_LANE_MAX & speed_lane_name, what do you > think about: > > capa =3D speed_lanes_capa[i].capa; > int s =3D 0; > while (capa) { > if (capa & 0x1) > printf("%-2d ", 1 << s); > s++; > capa =3D capa >> 1; > } > Am new to the DPDK world. Followed the FEC driver conventions for consistency. Will update it as you suggested and it makes sense. > > + 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 *res= ult, 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_l= anes_capa); > > +int parse_speed_lanes(uint32_t lane, uint32_t *speed_lane); > > + > > > > These functions only called in 'test-pmd/cmdline.c', what do you think > move functions to that file and make them static? > > Ack > > 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 st= ruct rte_eth_dev *dev, > > uint16_t queue_id, uint16_t offse= t, > > 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. > > > > Is above '-ENOTSUP' & '-EIO' return values are valid? > Normally we expect those two from ethdev API, not from dev_ops. > In which case a dev_ops expected to return these? > > Same comment for all three new APIs. > > code snippet from our driver .speed_lanes_get =3D bnxt_speed_lanes_get, The ethdev dev_ops returns -ENOTSUP if capability is not supported. Is this= ok? static int bnxt_speed_lanes_get(struct rte_eth_dev *dev, uint32_t *speed_lanes) { struct bnxt *bp =3D dev->data->dev_private; if (!BNXT_LINK_SPEEDS_V2(bp)) return -ENOTSUP; -EIO - will remove I will check and update other functions also. > > + */ > > +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. > > > > I can understand what above says but I think it can be clarified more, > what do you think? > Ack > > + * @param num > > + * a number of elements in an speed_speed_lanes_capa array. > > > > 'a number of elements' or 'number of elements' ? > Ack > > + * > > + * @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 v= alue > > + * is the number of entries filled in the speed lanes array. > > + * A non-negative value higher than num: error, the given speed lane= s 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 ar= e 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 *de= v, > > + struct rte_eth_speed_lane= s_capa *speed_lanes_capa, > > + unsigned int num); > > + > > > > These new dev_ops placed just in between existing dev_ops > 'eth_rx_descriptor_dump_t' and 'eth_tx_descriptor_dump_t', > if you were looking this header file as whole, what would you think > about quality of it? > > Please group new dev_ops below link related ones. > > Ack > > /** > > * @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 st= ate */ > > + 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 po= rt_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 =3D &rte_eth_devices[port_id]; > > + > > + if (*dev->dev_ops->speed_lanes_get =3D=3D NULL) > > + return -ENOTSUP; > > + return eth_err(port_id, (*dev->dev_ops->speed_lanes_get)(dev, lan= e)); > > +} > > + > > +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 =3D &rte_eth_devices[port_id]; > > + > > + if (speed_lanes_capa =3D=3D NULL && num > 0) { > > + RTE_ETHDEV_LOG_LINE(ERR, > > + "Cannot get ethdev port %u speed lane= s capability to NULL when array size is non zero", > > + port_id); > > + return -EINVAL; > > + } > > > > According above check, "speed_lanes_capa =3D=3D NULL && num =3D=3D 0" is = a valid > input, I assume this is useful to get expected size of the > 'speed_lanes_capa' array, but this is not mentioned in the API > documentation, can you please update API doxygen comment to cover this ca= se. > > Ack > > + > > + if (*dev->dev_ops->speed_lanes_get_capa =3D=3D NULL) > > + return -ENOTSUP; > > > > About the order or the checks, should we first check if the dev_ops > exist than validating the input arguments? > If dev_ops is not available, input variables doesn't matter anyway. > Ack > > + ret =3D (*dev->dev_ops->speed_lanes_get_capa)(dev, speed_lanes_ca= pa, num); > > + > > + return ret; > > > > API returns -EIO only if it is returned with 'eth_err()', that is to > cover the hot remove case. It is missing in this function. > > Ack > > +} > > + > > +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 =3D &rte_eth_devices[port_id]; > > + > > + if (*dev->dev_ops->speed_lanes_set =3D=3D NULL) > > + return -ENOTSUP; > > + return eth_err(port_id, (*dev->dev_ops->speed_lanes_set)(dev, spe= ed_lanes_capa)); > > +} > > > > Simiar location comment with the header one, instead of adding new APIs > to the very bottom of the file, can you please group them just below the > link related APIs? > Ack > > + > > 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..35d0b81452 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 st= ring. */ > > /**@}*/ > > > > +/** > > + * This enum indicates the possible link speed lanes of an ethdev port= . > > + */ > > +enum rte_eth_speed_lanes { > > + RTE_ETH_SPEED_LANE_UNKNOWN =3D 0, /**< speed lanes unsupported m= ode or default */ > > + RTE_ETH_SPEED_LANE_1 =3D 1, /**< Link speed lane 1 */ > > + RTE_ETH_SPEED_LANE_2 =3D 2, /**< Link speed lanes 2 */ > > + RTE_ETH_SPEED_LANE_4 =3D 4, /**< Link speed lanes 4 */ > > + RTE_ETH_SPEED_LANE_8 =3D 8, /**< Link speed lanes 8 */ > > > > Do we really need enum for the lane number? Why not use it as just number= ? > As far as I can see APIs get "uint32 lanes" parameter anyway. > > Ack > > + RTE_ETH_SPEED_LANE_MAX, > > > Will take care in the upcoming new patch > This kind of MAX enum usage is causing trouble when we want to extend > the support in the future. > Like when 16 lane is required, adding it changes the value of MAX and as > this is a public structure, change is causing ABI break, making us wait > until next ABI break realease. > So better if we can prevent MAX enum usage. > Make sense. Ack > > +}; > > + > > +/* 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= ) > > > > Why is above macro needed? > > To use in parse_speed_lanes to validate user input. It's not used any more in new patches. will remove it. > > + > > +/* 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,74 @@ 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 Autonegotia= ted or Fixed speed. > > > > As these doxygen comments are API docummentation, can you please form > them as proper sentences, like start with uppercase, end with '.', etc... > Same comment for all APIs. > Ack > > + * > > + * @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. > > > > 'this speeds' ? > > Ack. "number of lanes for current 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_set(uint16_t port_id, uint32_t speed_lanes); > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior = notice > > + * > > + * Get speed lanes supported by the NIC. > > + * > > + * @param port_id > > + * The port identifier of the Ethernet device. > > + * @param speed_lanes_capa > > + * speed_lanes_capa int array with valid lanes per speed. > > + * @param num > > + * size of the speed_lanes_capa array. > > + * > > + * @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. > > + * - (-EINVAL) if *speed_lanes* invalid > > + */ > > +__rte_experimental > > +int rte_eth_speed_lanes_get_capability(uint16_t port_id, > > + struct rte_eth_speed_lanes_capa *s= peed_lanes_capa, > > + unsigned int num); > > + > > > > The bottom of the header file is for static inline functions. > Instead of adding these new APIs at the very bottom of the header, can > you please group them just below the link speed related APIs? > > Ack > > #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 { > --=20 This electronic communication and the information and any files transmitted= =20 with it, or attached to it, are confidential and are intended solely for=20 the use of the individual or entity to whom it is addressed and may contain= =20 information that is confidential, legally privileged, protected by privacy= =20 laws, or otherwise restricted from disclosure to anyone else. If you are=20 not the intended recipient or the person responsible for delivering the=20 e-mail to the intended recipient, you are hereby notified that any use,=20 copying, distributing, dissemination, forwarding, printing, or copying of= =20 this e-mail is strictly prohibited. If you received this e-mail in error,= =20 please return the e-mail to the sender, delete it from your computer, and= =20 destroy any printed copy of it.