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

[huangdengdui <huangdengdui@huawei.com>]

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

>>
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
>>> + *
>>> + * Set speed lanes supported by the NIC.
>>> + *
>>
>> Set -> Get
> Ack
>>
>>> + * @param port_id

....