Re: [PATCH v4] ethdev: Add link_speed lanes support

[Damodharam Ammepalli <damodharam.ammepalli@broadcom.com>]

On Tue, Jul 9, 2024 at 4:10 AM 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_result,
> >
> >                       "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_result,
> >                       "port config (port_id) txq (queue_id) affinity (value)\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_specific = {
> >       },
> >  };
> >
> > +static int
> > +parse_speed_lanes_cfg(portid_t pid, uint32_t lanes)
> > +{
> > +     int ret;
> > +     uint32_t lanes_capa;
> > +
> > +     ret = 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 = rte_eth_speed_lanes_set(pid, lanes_capa);
> > +     if (ret == -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 = 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 = rte_eth_speed_lanes_get_capability(res->cmd_pid, NULL, 0);
> > +     if (ret == -ENOTSUP) {
> > +             fprintf(stderr, "Function not implemented\n");
> > +             return;
> > +     } else if (ret < 0) {
> > +             fprintf(stderr, "Get speed lanes capability failed: %d\n", ret);
> > +             return;
> > +     }
> > +
> > +     num = (unsigned int)ret;
> > +     speed_lanes_capa = calloc(num, sizeof(*speed_lanes_capa));
> > +     if (speed_lanes_capa == NULL) {
> > +             fprintf(stderr, "Failed to alloc speed lanes capability buffer\n");
> > +             return;
> > +     }
> > +
> > +     ret = rte_eth_speed_lanes_get_capability(res->cmd_pid, speed_lanes_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 =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_show_speed_lanes_result,
> > +                              cmd_show, "show");
> > +static cmdline_parse_token_string_t cmd_show_speed_lanes_port =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_show_speed_lanes_result,
> > +                              cmd_port, "port");
> > +static cmdline_parse_token_num_t cmd_show_speed_lanes_pid =
> > +     TOKEN_NUM_INITIALIZER(struct cmd_show_speed_lanes_result,
> > +                           cmd_pid, RTE_UINT16);
> > +static cmdline_parse_token_string_t cmd_show_speed_lanes_keyword =
> > +     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 =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_show_speed_lanes_result,
> > +                              cmd_keyword, "capabilities");
> > +
> > +static cmdline_parse_inst_t cmd_show_speed_lanes = {
> > +     .f = cmd_show_speed_lanes_parsed,
> > +     .data = NULL,
> > +     .help_str = "show port <port_id> speed_lanes capabilities",
> > +     .tokens = {
> > +             (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 = {
> >       },
> >  };
> >
> > +/* *** 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 = 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 =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, port, "port");
> > +static cmdline_parse_token_string_t cmd_config_speed_lanes_all_keyword =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, keyword,
> > +                              "config");
> > +static cmdline_parse_token_string_t cmd_config_speed_lanes_all_all =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, all, "all");
> > +static cmdline_parse_token_string_t cmd_config_speed_lanes_all_item =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_all, item,
> > +                              "speed_lanes");
> > +static cmdline_parse_token_num_t cmd_config_speed_lanes_all_lanes =
> > +     TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_all, lanes, RTE_UINT32);
> > +
> > +static cmdline_parse_inst_t cmd_config_speed_lanes_all = {
> > +     .f = cmd_config_speed_lanes_all_parsed,
> > +     .data = NULL,
> > +     .help_str = "port config all speed_lanes <value>",
> > +     .tokens = {
> > +             (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 = 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_port =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, port,
> > +                              "port");
> > +static cmdline_parse_token_string_t cmd_config_speed_lanes_specific_keyword =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, keyword,
> > +                              "config");
> > +static cmdline_parse_token_num_t cmd_config_speed_lanes_specific_id =
> > +     TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_specific, port_id,
> > +                           RTE_UINT16);
> > +static cmdline_parse_token_string_t cmd_config_speed_lanes_specific_item =
> > +     TOKEN_STRING_INITIALIZER(struct cmd_config_speed_lanes_specific, item,
> > +                              "speed_lanes");
> > +static cmdline_parse_token_num_t cmd_config_speed_lanes_specific_lanes =
> > +     TOKEN_NUM_INITIALIZER(struct cmd_config_speed_lanes_specific, lanes,
> > +                           RTE_UINT32);
> > +
> > +static cmdline_parse_inst_t cmd_config_speed_lanes_specific = {
> > +     .f = cmd_config_speed_lanes_specific_parsed,
> > +     .data = NULL,
> > +     .help_str = "port config <port_id> speed_lanes <value>",
> > +     .tokens = {
> > +             (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[] = {
> >       (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[] = {
> > +     {
> > +             .lane = RTE_ETH_SPEED_LANE_UNKNOWN,
> > +             .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,12 @@ 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) {
> > +             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 == RTE_ETH_LINK_FULL_DUPLEX) ?
> >              ("full-duplex") : ("half-duplex"));
> >       printf("Autoneg status: %s\n", (link.link_autoneg == RTE_ETH_LINK_AUTONEG) ?
> > @@ -962,7 +995,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 +1026,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",
> >
>
> 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") : ("down"),
> >               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 = 0; i < RTE_DIM(speed_lane_name); i++) {
> > +             if (speed_lane_name[i].value == lane) {
> > +                     *speed_lane = 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_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);
> > +             }
>
> To eliminate both RTE_ETH_SPEED_LANE_MAX & speed_lane_name, what do you
> think about:
>
> capa = speed_lanes_capa[i].capa;
> int s = 0;
> while (capa) {
>     if (capa & 0x1)
>         printf("%-2d ", 1 << s);
>     s++;
>     capa = 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 *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);
> > +
> >
>
> 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 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.
> >
>
> 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 = 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 = 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 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);
> > +
> >
>
> 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 state */
> > +     eth_speed_lanes_get_t      speed_lanes_get;       /**<Get link speed active lanes */
> > +     eth_speed_lanes_set_t      speed_lanes_set;       /**<set the link speeds supported lanes */
> > +     /** Get link speed lanes capability */
> > +     eth_speed_lanes_get_capability_t speed_lanes_get_capa;
> >       /** Check if the device was physically removed */
> >       eth_is_removed_t           is_removed;
> >
> > diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> > index f1c658f49e..07cefea307 100644
> > --- a/lib/ethdev/rte_ethdev.c
> > +++ b/lib/ethdev/rte_ethdev.c
> > @@ -7008,4 +7008,55 @@ int rte_eth_dev_map_aggr_tx_affinity(uint16_t port_id, uint16_t tx_queue_id,
> >       return ret;
> >  }
> >
> > +int
> > +rte_eth_speed_lanes_get(uint16_t port_id, uint32_t *lane)
> > +{
> > +     struct rte_eth_dev *dev;
> > +
> > +     RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +     dev = &rte_eth_devices[port_id];
> > +
> > +     if (*dev->dev_ops->speed_lanes_get == NULL)
> > +             return -ENOTSUP;
> > +     return eth_err(port_id, (*dev->dev_ops->speed_lanes_get)(dev, lane));
> > +}
> > +
> > +int
> > +rte_eth_speed_lanes_get_capability(uint16_t port_id,
> > +                                struct rte_eth_speed_lanes_capa *speed_lanes_capa,
> > +                                unsigned int num)
> > +{
> > +     struct rte_eth_dev *dev;
> > +     int ret;
> > +
> > +     RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +     dev = &rte_eth_devices[port_id];
> > +
> > +     if (speed_lanes_capa == NULL && num > 0) {
> > +             RTE_ETHDEV_LOG_LINE(ERR,
> > +                                 "Cannot get ethdev port %u speed lanes capability to NULL when array size is non zero",
> > +                                 port_id);
> > +             return -EINVAL;
> > +     }
> >
>
> According above check, "speed_lanes_capa == NULL && num == 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 case.
>
>
Ack

> > +
> > +     if (*dev->dev_ops->speed_lanes_get_capa == 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 = (*dev->dev_ops->speed_lanes_get_capa)(dev, speed_lanes_capa, 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 = &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));
> > +}
> >
>
> 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 string. */
> >  /**@}*/
> >
> > +/**
> > + * This enum indicates the possible link speed lanes of an ethdev port.
> > + */
> > +enum rte_eth_speed_lanes {
> > +     RTE_ETH_SPEED_LANE_UNKNOWN = 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 */
> >
>
> 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 Autonegotiated 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 *speed_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 {
>

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.