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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

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.