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

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

On Fri, Jul 5, 2024 at 10:33 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 6/26/2024 3:19 AM, huangdengdui wrote:
> >
> > On 2024/6/26 5:07, Damodharam Ammepalli wrote:
> >> On Wed, Jun 19, 2024 at 8:23 PM huangdengdui <huangdengdui@huawei.com> wrote:
> >>>
> >>> Hi Damodharam
> >>> Here are some suggestions. See below.
> >>>
> >> Thank you for the review.
> >>
> >>> On 2024/6/18 4:34, Damodharam Ammepalli wrote:
> >>>> Update the eth_dev_ops structure with new function vectors
> >>>> to get, get capabilities and set ethernet link speed lanes.
> >>>> Update the testpmd to provide required config and information
> >>>> display infrastructure.
> >>>>
> >>>> The supporting ethernet controller driver will register callbacks
> >>>> to avail link speed lanes config and get services. This lanes
> >>>> configuration is applicable only when the nic is forced to fixed
> >>>> speeds. In Autonegiation mode, the hardware automatically
> >>>> negotiates the number of lanes.
> >>>>
> >>>
> >>>
> >>>> +
> >>>>  /* *** configure txq/rxq, txd/rxd *** */
> >>>>  struct cmd_config_rx_tx {
> >>>>       cmdline_fixed_string_t port;
> >>>> @@ -13238,6 +13459,9 @@ static cmdline_parse_ctx_t builtin_ctx[] = {
> >>>>       (cmdline_parse_inst_t *)&cmd_set_port_setup_on,
> >
> > cut
> >
> >>>>
> >>>> @@ -993,7 +1022,7 @@ port_summary_display(portid_t port_id)
> >>>>       if (ret != 0)
> >>>>               return;
> >>>>
> >>>> -     printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %s\n",
> >>>> +     printf("%-4d " RTE_ETHER_ADDR_PRT_FMT " %-12s %-14s %-8s %-8s\n",
> >>>
> >>> Does the lanes need to be printed?
> >> Ferruh in the previous comment, asked not to print.
> >>
> >
> > OK
> >
> >>>
> >>>>               port_id, RTE_ETHER_ADDR_BYTES(&mac_addr), name,
> >>>>               dev_info.driver_name, (link.link_status) ? ("up") : ("down"),
> >>>>               rte_eth_link_speed_to_str(link.link_speed));
> >>>> @@ -7244,3 +7273,35 @@ show_mcast_macs(portid_t port_id)
> >>>>               printf("  %s\n", buf);
> >>>>       }
> >>>>  }
> >>>> +
> >>>> +int
> >>>> +parse_speed_lanes(uint32_t lane, uint32_t *speed_lane)
> >>>> +{
> >>>> +     uint8_t i;
> >>>> +
> >>>> +     for (i = 0; i < RTE_DIM(speed_lane_name); i++) {
> >>>> +             if (speed_lane_name[i].value == lane) {
> >>>> +                     *speed_lane = lane;
> >>>> +                     return 0;
> >>>> +             }
> >>>> +     }
> >>>> +     return -1;
> >>>> +}
> >>>> +
> >
> > cut
> >
> >>>>
> >>>> +/**
> >>>> + * This enum indicates the possible link speed lanes of an ethdev port.
> >>>> + */
> >>>> +enum rte_eth_speed_lanes {
> >>>> +     RTE_ETH_SPEED_LANE_NOLANE = 0,  /**< speed lanes unsupported mode or default */
> >>>> +     RTE_ETH_SPEED_LANE_1,           /**< Link speed lane  1 */
> >>>> +     RTE_ETH_SPEED_LANE_2,           /**< Link speed lanes 2 */
> >>>> +     RTE_ETH_SPEED_LANE_4,           /**< Link speed lanes 4 */
> >>>> +     RTE_ETH_SPEED_LANE_8,           /**< Link speed lanes 8 */
> >>>> +     RTE_ETH_SPEED_LANE_MAX,
> >>>> +};
> >>>
> >>> Is it better to make the index equal to the lanes num?
> >>> enum rte_eth_speed_lanes {
> >>>         RTE_ETH_SPEED_LANE_NOLANE = 0,      /**< speed lanes unsupported mode or default */
> >>>         RTE_ETH_SPEED_LANE_1 = 1,           /**< Link speed lane  1 */
> >>>         RTE_ETH_SPEED_LANE_2 = 2,           /**< Link speed lanes 2 */
> >>>         RTE_ETH_SPEED_LANE_4 = 4,           /**< Link speed lanes 4 */
> >>>         RTE_ETH_SPEED_LANE_8 = 8,           /**< Link speed lanes 8 */
> >>>         RTE_ETH_SPEED_LANE_MAX,
> >>> };
> >>>
> >> I followed the existing enums code convention in rtelib. Your point
> >> makes sense too.
> >>
> >
> > I looked at the other enum code in the lib. There are many similar code styles.
> > Make the index meaningful to avoid conversion. For example, the parse_speed_lanes() function in this patch
> >
> >>> In addition, when lanes = 0, is it better to define it as Unknown?
> >>> If default lanes can return 0 lanes, The active lanes are different for each NIC,
> >>> users may be confused.
> >>>
> >> Ack. Are you proposing a new enum RTE_ETH_SPEED_LANE_UKNOWN or rename
> >> RTE_ETH_SPEED_LANE_NOLANE?
> >>
> >
> > I suggest changing the name to RTE_ETH_SPEED_LANE_UKNOWN,
> > Also change the comment to describe it as an unknown lane.
> >
> > This prevents the driver from always returning lanes=0
> > even if the driver knows the number of active lanes.
> >
> >>>> +
> >>>> +/* Translate from link speed lanes to speed lanes capa */
> >>>> +#define RTE_ETH_SPEED_LANES_TO_CAPA(x) RTE_BIT32(x)
> >>>> +
> >>>> +/* This macro indicates link speed lanes capa mask */
> >>>> +#define RTE_ETH_SPEED_LANES_CAPA_MASK(x) RTE_BIT32(RTE_ETH_SPEED_ ## x)
> >>>> +
> >>>> +/* A structure used to get and set lanes capabilities per link speed */
> >>>> +struct rte_eth_speed_lanes_capa {
> >>>> +     uint32_t speed;
> >>>> +     uint32_t capa;
> >>>> +};
> >>>> +
> >
> > cut
> >
> >>>> +__rte_experimental
> >>>> +int rte_eth_speed_lanes_set(uint16_t port_id, uint32_t speed_lanes_capa);
> >>>> +
> >>>
> >>> Is it better to name 'speed_lanes'?
> >> Are you proposing to rename to rte_speed_lanes_set() function name or
> >> rename variable "speed_lanes_capa" name ?
> >>
> >
> > rename variable "speed_lanes_capa" name
> >
>
> Hi Damodharam,
>
> I was hoping to get this feature for -rc2, as outstanding comments are
> not very big, if it can be possible to get new version in next few days
> we can proceed with it for this release, otherwise can wait for next
> release.
>
> Btw, other issue is testing this new feature, as it is not supported by
> many vendors, I expect this test case not included in many test plans.
> And because of the HW dependency adding unit test won't be sufficient.
> At least for the short term, can we rely Broadcom and Huawei to test it
> with the first version this feature is merged?
>
> And I am not sure about what can be the long term solution, any
> suggestion is welcome. My concern is as number of this kind of features
> are increasing, dpdk is becoming more fragile.
>
>
>
Hi Ferruh,
Yes, I will push a new patch with review comments addressed in a day.
Sure, Broadcom will test it and provide the feedback. Infact, I am unit testing
internally with the brcm driver for every revision of the patch that is pushed.

Thanks
Damo

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