On 2024/6/26 5:07, Damodharam Ammepalli wrote:
> On Wed, Jun 19, 2024 at 8:23 PM huangdengdui <huangdeng...@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
....
