From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Tue, 9 Jul 2024 23:20:28 +0200 (CEST)
Received: by mail-lj1-f176.google.com with SMTP id
38308e7fff4ca-2eea8ea8bb0so44154181fa.1
for <dev@dpdk.org>; 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: <CAKSYD4yiEfj1B9O-dtWCbqK1f_0_sUKgNx0Q-2W-GLkcwfLpAg@mail.gmail.com>
<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 <damodharam.ammepalli@broadcom.com>
Date: Tue, 9 Jul 2024 14:20:15 -0700
Message-ID: <CAKSYD4wmfcWwqu3qS+JrJNMjxVY5Qf1RZKn0NQ_VrPg_t2fNdw@mail.gmail.com>
Subject: Re: [PATCH v4] ethdev: Add link_speed lanes support
To: Ferruh Yigit <ferruh.yigit@amd.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
On Tue, Jul 9, 2024 at 4:10=E2=80=AFAM Ferruh Yigit <ferruh.yigit@amd.com> =
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 <damodharam.ammepalli@broadcom.com>
> > ---
> > 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 <port_id> 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 <value>",
> > + .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 <port_id> speed_lanes <value>",
> > + .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; /**<Get link sp=
eed active lanes */
> > + eth_speed_lanes_set_t speed_lanes_set; /**<set the lin=
k 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 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.