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 EBBABA0547; Fri, 12 Mar 2021 09:45:13 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9D343160849; Fri, 12 Mar 2021 09:45:13 +0100 (CET) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mails.dpdk.org (Postfix) with ESMTP id 561A54067E for ; Fri, 12 Mar 2021 09:45:11 +0100 (CET) IronPort-SDR: Y2QYYQgC6IisuNhhwjYa1Fw3xCVJRIgVPnSlyafGe8S45R4HUsQXpKhqOwKwxO3Q4kYe6o0+ZG 4QHCsK/fJONQ== X-IronPort-AV: E=McAfee;i="6000,8403,9920"; a="168721111" X-IronPort-AV: E=Sophos;i="5.81,243,1610438400"; d="scan'208";a="168721111" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Mar 2021 00:45:10 -0800 IronPort-SDR: 4tFjySzpdfPe0MwTbvEfwZ/IKoeSJRwxDEf0OJFhw5sSY+HUYl/vJ/DrWWlC9MNmelPDu0woyU 5CtxFVOzp4Kg== X-IronPort-AV: E=Sophos;i="5.81,243,1610438400"; d="scan'208";a="510275232" Received: from ianimash-mobl.ger.corp.intel.com (HELO [10.252.11.60]) ([10.252.11.60]) by fmsmga001-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Mar 2021 00:45:09 -0800 From: Ferruh Yigit To: Andrew Rybchenko , Ajit Khaparde , dev@dpdk.org Cc: Qi Zhang , Thomas Monjalon References: <20210222191824.40230-1-ajit.khaparde@broadcom.com> <3a90da98-da41-70fb-2b6b-99e68c260b62@intel.com> <1dbfb659-4f91-90fe-0e0c-23d49d53f03b@intel.com> X-User: ferruhy Message-ID: <43d67012-7f6f-2de8-e5ee-50cbe025d3eb@intel.com> Date: Fri, 12 Mar 2021 08:45:01 +0000 MIME-Version: 1.0 In-Reply-To: <1dbfb659-4f91-90fe-0e0c-23d49d53f03b@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] app/testpmd: add support for forced ethernet speed 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 Sender: "dev" On 2/26/2021 11:21 AM, Ferruh Yigit wrote: > On 2/26/2021 6:43 AM, Andrew Rybchenko wrote: >> On 2/25/21 9:25 PM, Ferruh Yigit wrote: >>> On 2/22/2021 7:18 PM, Ajit Khaparde wrote: >>>> Add support for forced ethernet speed setting. >>>> Currently testpmd tries to configure the Ethernet port in autoneg mode. >>>> It is not possible to set the Ethernet port to a specific speed while >>>> starting testpmd. In some cases capability to configure a forced speed >>>> for the Ethernet port during initialization may be necessary. This patch >>>> tries to add this support. >>>> >>>> The patch assumes full duplex setting and does not attempt to change >>>> that. >>>> So speeds like 10M, 100M are not configurable using this method. >>>> >>>> The command line to configure a forced speed of 10G: >>>> dpdk-testpmd -c 0xff  -- -i  --eth-link-speed  10000 >>>> >>>> The command line to configure a forced speed of 50G: >>>> dpdk-testpmd -c 0xff  -- -i  --eth-link-speed  50000 >>>> >>>> Signed-off-by: Ajit Khaparde >>>> --- >>>>    app/test-pmd/parameters.c             | 42 +++++++++++++++++++++++++++ >>>>    app/test-pmd/testpmd.c                |  4 +++ >>>>    app/test-pmd/testpmd.h                |  1 + >>>>    doc/guides/testpmd_app_ug/run_app.rst | 11 +++++++ >>>>    4 files changed, 58 insertions(+) >>> >>> Can you also update the release notes to document the new parameter? >>> >>>> >>>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c >>>> index c8acd5d1b7..e10f7d38fb 100644 >>>> --- a/app/test-pmd/parameters.c >>>> +++ b/app/test-pmd/parameters.c >>>> @@ -224,6 +224,7 @@ usage(char* progname) >>>>        printf("  --hairpin-mode=0xXX: bitmask set the hairpin port >>>> mode.\n " >>>>               "    0x10 - explicit Tx rule, 0x02 - hairpin ports paired\n" >>>>               "    0x01 - hairpin ports loop, 0x00 - hairpin port >>>> self\n"); >>>> +    printf("  --eth-link-speed: forced link speed.\n"); >>>>    } >>>>      #ifdef RTE_LIB_CMDLINE >>>> @@ -485,6 +486,41 @@ parse_event_printing_config(const char *optarg, >>>> int enable) >>>>        return 0; >>>>    } >>>>    +static int >>>> +parse_link_speed(int n) >>>> +{ >>>> +    uint32_t speed; >>>> + >>>> +    switch (n) { >>> >>> OK to not support "10M, 100M", not sure if anybody really uses them, but >>> what do you think checking them and return an error? >>> >>>> +    case 1000: >>>> +        speed = ETH_LINK_SPEED_1G; >>>> +        break; >>>> +    case 10000: >>>> +        speed = ETH_LINK_SPEED_10G; >>>> +        break; >>>> +    case 25000: >>>> +        speed = ETH_LINK_SPEED_25G; >>>> +        break; >>>> +    case 40000: >>>> +        speed = ETH_LINK_SPEED_40G; >>>> +        break; >>>> +    case 50000: >>>> +        speed = ETH_LINK_SPEED_50G; >>>> +        break; >>>> +    case 100000: >>>> +        speed = ETH_LINK_SPEED_100G; >>>> +        break; >>>> +    case 200000: >>>> +        speed = ETH_LINK_SPEED_200G; >>>> +        break; >>>> +    default: >>>> +        speed = ETH_LINK_SPEED_AUTONEG; >>>> +        break; >>> >>> Isn't this function to set a fixed link speed, why falling back to autoneg? >>> >>> Also shouldn't this function set 'ETH_LINK_SPEED_FIXED' too? >> >> It should. Previous time I've tried to fix corresponding >> bug in CLI commands, it ended up with rollback because >> of Intel drivers do not handle it correctly. >> >> See "app/testpmd: set fixed flag for exact link speed" and >> corresponding revert. >> > > Thanks for the reminder Andrew, you have a good memory :) > For reference: > http://inbox.dpdk.org/dev/20190507100928.pOyue5JiSaPL-NSHiueAU3HlgisgF9bYynJGpTjyvMw@z/ > > > It seems that patch reverted with the pressure of the release, what do you think > applying it again while we have enough time to fix the PMDs before release? > > From previous discussions, long term actions listed as: > " > 1) Implement 'fixed' link speed support in the missing drivers. > 2) Send a new version of the testpmd patch with a "fixed" argument, so that we > can support all three above > " > > Not sure having (2) explicitly is required, we have already "auto" speed, not > having it implies the fixed speed. > So we can just re-apply your old patch. Andrew, if you are OK with above, can you please send your old patch (the reverted one) on top of latest head?