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: sta...@dpdk.org
>>>>
>>>> Signed-off-by: Andrew Rybchenko <arybche...@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 {
>>>>
>
