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

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

[-- Attachment #1: Type: text/plain, Size: 25967 bytes --]

On Wed, Jul 10, 2024 at 2:06 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 7/9/2024 10:20 PM, Damodharam Ammepalli wrote:
> > 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.
> >
>
> We don't see your driver code yet, so it is hard to know the intention,
> but for your case will it work if first speed is set, later lane is set?
> (Btw, in the context of lanes, I am always assuming it is fixed speed
> configuration.)
>

Ack. Follow up series will carry the patches.

> Assume that default speed is 10G, when speed_lane is set first, is it
> expected that driver verify the lane for that speed? If so, perhaps that
> specific lane config is not valid for the default speed and fails to
> set, do you think is this a valid case?
>

No, Let me take 100G as an example which has lanes capability.
If the link is Auto Negotiated, the driver will ignore the user's lane
configuration.
Lane configuration will only be applied if the existing linked up
speed(eg: 100G) is a fixed speed.
Driver will check the lane's validity and reject it if it doesn't
match the speed's capability.

> Overall, the issue I am trying to clarify is the reason why Dengdui went
> with having different desing to couple speed and lanes. We said we will
> go with this separated design, but lets clarify relation/order in user
> perspective, since this is something that impacts driver implementation.
> And not clarifying this enough for sure will cause confusion and
> inconsistency in different driver implemntations.
>

Ack. Since the port is stopped, order of configuration does not matter.
Once the port is started, the driver will pick up up test pmds lanes and speed
configuration and re-configures the phy in one shot. The recommended steps
are to stop the port, configure a fixed speed and lane count.

> >>> 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.
> >
>
> That is OK to have the stop requirement, only it needs to be clear.
> And addition to document this in the API documentation, what do you
> think to add this check in the API, so it guarantees in coding level
> that this requirement is satified.
> So this check in application level can be dropped.
>

Ack. updated the relevant comments section and also added check in the function.

> >>> +
> >>> +     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?
> >>

Ack, in the ensuing patch, removing the else case since as per your
next comment,
driver returns either error code or 0, with lanes count > 0.

> >>
> > 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
> >
>
> I don't understand how application proccess the information that
> 'rte_eth_speed_lanes_get()' returns success but lane is zero.
>

Ack. Answered in the next comment.

> For the link down case, doesn't it make sense to return error for the
> 'rte_eth_speed_lanes_get()' API?
>
Ack.

> And for other cases, like 1G that doesn't support the lane
> configuration, does it make sense to return 1?
> And for these cases what is the capability value you are returning?
>

Ack. For speeds like 1G, checking with firmware engineers confirmed, a lane = 1
would be appropriate and the driver will always return lanes > 0.

>
> >>> +     }
> >>>       printf("Link duplex: %s\n", (link.link_duplex == RTE_ETH_LINK_FULL_DUPLEX) ?
> >>>              ("full-duplex") : ("half-duplex"));
> >>>       printf("Autoneg status: %s\n", (link.link_autoneg == RTE_ETH_LINK_AUTONEG) ?
> >>> @@ -962,7 +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.
> >
>
> FEC values goes sequential, but lane values in power of two, we can
> benefit from this fact. (though if we have lane value not power of two
> later, this logic may need to change.)
>
> Btw, I can see some of the code copies existing FEC implementation,
> which makes sense, but it seems FEC code could be improved as well but
> we missed at that time.
> Instead of copying FEC exactly, lets try to improve whatever we can for
> first merge, otherwise less likely these things will be fixed later.
>

Ack. Follow up patch will have comments addressed.
Thanks for the review and sorry for the delay, as other priorities
took the front seat.

> Thanks,
> ferruh

-- 
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.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5457 bytes --]