Re: [dpdk-dev] [PATCH] app/testpmd: add support for forced ethernet speed

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 2/26/2021 4:18 PM, Andrew Boyer wrote:
> 
> 
>> On Feb 26, 2021, at 6:21 AM, Ferruh Yigit <ferruh.yigit@intel.com> 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 <ajit.khaparde@broadcom.com>
>>>>> ---
>>>>>    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.
> 
> 
> Please also see the message I sent back in November: http://inbox.dpdk.org/dev/F041DE53-0ABF-4A0A-974A-16167967ABD5@pensando.io/
> 
> I added the FIXED flag to fix my local tree and found that it causes intermittent failures in link_bonding_autotest. (In version 20.02, haven’t tested in latest branch.)
> 

Why updates in the testpmd may be affecting bonding link autotest, what am I 
missing?

> -Other Andrew
> 

:)