Re: [dpdk-dev] [dpdk-stable] [PATCH] app/testpmd: set fixed flag when exact link speed is chosen

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

On 4/23/2019 4:44 PM, Andrew Rybchenko wrote:
> On 4/23/19 6:04 PM, Ferruh Yigit wrote:
>> On 4/23/2019 4:02 PM, Ferruh Yigit wrote:
>>> On 4/12/2019 2:12 PM, Andrew Rybchenko wrote:
>>>> Setting exact link speed makes sense if auto-negotiation is
>>>> disabled. Fixed flag is required to disable auto-negotiation.
>>> Hi Andrew,
>>>
>>> Fixed link speed is not supported by all PMDs and this change is breaking them
>>> to set the speed in testpmd.
> 
> Not all. sfc definitely supports it.
> It looks like bnxt, e1000, igb support it as well.

The ones we know for now as not supported are ixgbe and i40e.

> 
>>> As long as device speed set correct, I believe the command doesn't really care
>>> about if link mode is auto-negotiation or not.
> 
> Sometimes it is really important to disable auto-negotiation.

If this is the case, we should cover this option in testpmd, but perhaps as an
extension to current command, like new parameter?

> 
>>> I am for reverting this commit, is there any objection to revert it?
>> cc'ing Wenjie who reported the issue.
> 
> May be I misunderstood the purpose of the command.
> Typically if someone wants to set link speed, it is assumed that
> auto-negotiation should be disabled since user specifies what he really 
> wants.
> Of course, it is still valid request to keep auto-negotiation enabled, but
> limit set of advertised speeds (may be keep only one).

I think the purpose of the command is not clear, and what you said makes sense,
only it is breaking the some PMDs is problem I think.

> 
> So, I'm not happy to revert it completely. There is an option to revert 
> to old
> behaviour and add optional argument to disable auto-negotiation, but I
> can't say that I like it, since it would make sense if the command 
> allows more
> than one speed to be advertised (to be auto-negotiated). If it is only 
> one speed,
> the default should be autoneg disabled.

Indeed I was suggesting same thing above, I didn't get the problem with this
approach, if the 'fixed' arg added it will work as you suggested, single fixed
speed value.

> 
> Anyway documentation of the command should be improved.
> 
> Andrew.
> 
>>>> Fixes: 88fbedcd5e5a ("app/testpmd: move speed and duplex parsing in a function")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>>   app/test-pmd/cmdline.c | 12 ++++++------
>>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>>> index 2ab03c111..691d818a6 100644
>>>> --- a/app/test-pmd/cmdline.c
>>>> +++ b/app/test-pmd/cmdline.c
>>>> @@ -1440,17 +1440,17 @@ parse_and_check_speed_duplex(char *speedstr, char *duplexstr, uint32_t *speed)
>>>>   			return -1;
>>>>   		}
>>>>   		if (!strcmp(speedstr, "1000")) {
>>>> -			*speed = ETH_LINK_SPEED_1G;
>>>> +			*speed = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_FIXED;
>>>>   		} else if (!strcmp(speedstr, "10000")) {
>>>> -			*speed = ETH_LINK_SPEED_10G;
>>>> +			*speed = ETH_LINK_SPEED_10G | ETH_LINK_SPEED_FIXED;
>>>>   		} else if (!strcmp(speedstr, "25000")) {
>>>> -			*speed = ETH_LINK_SPEED_25G;
>>>> +			*speed = ETH_LINK_SPEED_25G | ETH_LINK_SPEED_FIXED;
>>>>   		} else if (!strcmp(speedstr, "40000")) {
>>>> -			*speed = ETH_LINK_SPEED_40G;
>>>> +			*speed = ETH_LINK_SPEED_40G | ETH_LINK_SPEED_FIXED;
>>>>   		} else if (!strcmp(speedstr, "50000")) {
>>>> -			*speed = ETH_LINK_SPEED_50G;
>>>> +			*speed = ETH_LINK_SPEED_50G | ETH_LINK_SPEED_FIXED;
>>>>   		} else if (!strcmp(speedstr, "100000")) {
>>>> -			*speed = ETH_LINK_SPEED_100G;
>>>> +			*speed = ETH_LINK_SPEED_100G | ETH_LINK_SPEED_FIXED;
>>>>   		} else if (!strcmp(speedstr, "auto")) {
>>>>   			*speed = ETH_LINK_SPEED_AUTONEG;
>>>>   		} else {
>>>>
>