From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id F3384455C8; Mon, 8 Jul 2024 22:35:48 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 91A5B40ED3; Mon, 8 Jul 2024 22:35:48 +0200 (CEST) Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) by mails.dpdk.org (Postfix) with ESMTP id DDD2840B99 for ; Mon, 8 Jul 2024 22:35:47 +0200 (CEST) Received: by mail-lf1-f49.google.com with SMTP id 2adb3069b0e04-52ea8320d48so2305186e87.1 for ; Mon, 08 Jul 2024 13:35:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=broadcom.com; s=google; t=1720470947; x=1721075747; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=6OxmD3pNNipOU/Sg/YH8res6xmi0FKFEOMJYI45q+lw=; b=MCUccWCVNUlrNdlBkF2CyamiOOwrju6jOaUppC1oAp/5eLASULZTzSl94/3pxbtOE4 eHB1uyo55VB15SaHueNA2nw9zkNPMQjnSSssfH+ytxtxDWZY7o+85z247CYdS/Re9FlN Mdw6wg/pPRFZW7p3kBd0TsJ1b+AbAoPrvlDFw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720470947; x=1721075747; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=6OxmD3pNNipOU/Sg/YH8res6xmi0FKFEOMJYI45q+lw=; b=FEpbK8tdoQGPybF0pi7Fprr56OIHsulI86e1a0o9/vA2NijFh9y6FSfXSOO1S/5Kd3 Bnr/LTmhoXSAobvpL6waEPHaa6kYBNKUqR5hDuSCts9q1+jRi/Tt2MslU+PPLBfgsHCr qOqaIUt3GzZtWO0AP2ArgdtilDFn/qSrw4zXajbeN3qC7D4+bjkMnRTKneTCeCZMYW66 4BteQW/WiJ+jN6qjI0Ox5Ne9ZFsRwErmgSZ4Syrg9jDMz4q2+vMD7HzDxKV59yd3eA+w U8CQE8X2yIuti5pzMJfa4gEl6TVMfhvP5d3CABuMyveqvMgj9SgKHVZlO3a4CbWt12br 9D7g== X-Forwarded-Encrypted: i=1; AJvYcCWVpDB9SMbUd1E2cTf4v79KK02DfzC/EWzjTj+WuJQqFk1YF8r6wkFP1lbh5cwuRf9weIjw5ck8H5K56nk= X-Gm-Message-State: AOJu0YwuM7KxNY24GeIUEMuR67kj6nJtpgFzlTr1A3MVFaI9AsG5IodW gTWvxFPtT/X+ymfdbYhu/e/PuBt+DV0xvlHShwkqzPgtpWjiDhINMbuhJtHMI8shvLQn4O6oUvs jLeQsOTspr6qE8W71jJWZ03ZVWReb8LUAE/UoMWSwckaFENkEAZfFu/jxVwbIrIXKtLNkdpyCYJ L15Xm9 X-Google-Smtp-Source: AGHT+IHYz4Ojb8Ho9Zvu+U/9asNg7F3iVjViho55toZjuccspEfHQRZi9Sphd0aQ0aEnfpQ8SGrw8XZdd120pgzyIEE= X-Received: by 2002:a05:6512:3c81:b0:52c:ebf6:9a7f with SMTP id 2adb3069b0e04-52eb9d3c743mr124111e87.11.1720470947227; Mon, 08 Jul 2024 13:35:47 -0700 (PDT) MIME-Version: 1.0 References: <20240617203448.97972-1-damodharam.ammepalli@broadcom.com> <4a0d1bc0-4edf-4653-8b23-f2b12a924b3d@huawei.com> <9822cf6d-7c96-4e44-a210-88838b89b286@amd.com> In-Reply-To: <9822cf6d-7c96-4e44-a210-88838b89b286@amd.com> From: Damodharam Ammepalli Date: Mon, 8 Jul 2024 13:35:35 -0700 Message-ID: Subject: Re: [PATCH v3] ethdev: Add link_speed lanes support To: Ferruh Yigit Cc: huangdengdui , dev@dpdk.org, ajit.khaparde@broadcom.com, kalesh-anakkur.purayil@broadcom.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, Jul 5, 2024 at 10:33=E2=80=AFAM Ferruh Yigit = 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=E2=80=AFPM huangdengdui 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[] =3D= { > >>>> (cmdline_parse_inst_t *)&cmd_set_port_setup_on, > > > > cut > > > >>>> > >>>> @@ -993,7 +1022,7 @@ port_summary_display(portid_t port_id) > >>>> if (ret !=3D 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 =3D 0; i < RTE_DIM(speed_lane_name); i++) { > >>>> + if (speed_lane_name[i].value =3D=3D lane) { > >>>> + *speed_lane =3D lane; > >>>> + return 0; > >>>> + } > >>>> + } > >>>> + return -1; > >>>> +} > >>>> + > > > > cut > > > >>>> > >>>> +/** > >>>> + * This enum indicates the possible link speed lanes of an ethdev p= ort. > >>>> + */ > >>>> +enum rte_eth_speed_lanes { > >>>> + RTE_ETH_SPEED_LANE_NOLANE =3D 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 =3D 0, /**< speed lanes unsupp= orted mode or default */ > >>> RTE_ETH_SPEED_LANE_1 =3D 1, /**< Link speed lane 1= */ > >>> RTE_ETH_SPEED_LANE_2 =3D 2, /**< Link speed lanes 2= */ > >>> RTE_ETH_SPEED_LANE_4 =3D 4, /**< Link speed lanes 4= */ > >>> RTE_ETH_SPEED_LANE_8 =3D 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_s= peed_lanes() function in this patch > > > >>> In addition, when lanes =3D 0, is it better to define it as Unknown? > >>> If default lanes can return 0 lanes, The active lanes are different f= or 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=3D0 > > 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 spee= d */ > >>>> +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 tes= ting internally with the brcm driver for every revision of the patch that is pus= hed. Thanks Damo --=20 This electronic communication and the information and any files transmitted= =20 with it, or attached to it, are confidential and are intended solely for=20 the use of the individual or entity to whom it is addressed and may contain= =20 information that is confidential, legally privileged, protected by privacy= =20 laws, or otherwise restricted from disclosure to anyone else. If you are=20 not the intended recipient or the person responsible for delivering the=20 e-mail to the intended recipient, you are hereby notified that any use,=20 copying, distributing, dissemination, forwarding, printing, or copying of= =20 this e-mail is strictly prohibited. If you received this e-mail in error,= =20 please return the e-mail to the sender, delete it from your computer, and= =20 destroy any printed copy of it.