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