[PATCH net v2] ethtool: reset #lanes when lanes is omitted

[PATCH net v2] ethtool: reset #lanes when lanes is omitted

[Andy Roulin]

If the number of lanes was forced and then subsequently the user
omits this parameter, the ksettings->lanes is reset. The driver
should then reset the number of lanes to the device's default
for the specified speed.

However, although the ksettings->lanes is set to 0, the mod variable
is not set to true to indicate the driver and userspace should be
notified of the changes.

The consequence is that the same ethtool operation will produce
different results based on the initial state.

If the initial state is:
$ ethtool swp1 | grep -A 3 'Speed: '
        Speed: 500000Mb/s
        Lanes: 2
        Duplex: Full
        Auto-negotiation: on

then executing 'ethtool -s swp1 speed 50000 autoneg off' will yield:
$ ethtool swp1 | grep -A 3 'Speed: '
        Speed: 500000Mb/s
        Lanes: 2
        Duplex: Full
        Auto-negotiation: off

While if the initial state is:
$ ethtool swp1 | grep -A 3 'Speed: '
        Speed: 500000Mb/s
        Lanes: 1
        Duplex: Full
        Auto-negotiation: off

executing the same 'ethtool -s swp1 speed 50000 autoneg off' results in:
$ ethtool swp1 | grep -A 3 'Speed: '
        Speed: 500000Mb/s
        Lanes: 1
        Duplex: Full
        Auto-negotiation: off

This patch fixes this behavior. Omitting lanes will always results in
the driver choosing the default lane width for the chosen speed. In this
scenario, regardless of the initial state, the end state will be, e.g.,

$ ethtool swp1 | grep -A 3 'Speed: '
        Speed: 500000Mb/s
        Lanes: 2
        Duplex: Full
        Auto-negotiation: off

Fixes: 012ce4dd3102 ("ethtool: Extend link modes settings uAPI with lanes")
Signed-off-by: Andy Roulin <aroulin@nvidia.com>
Reviewed-by: Danielle Ratson <danieller@nvidia.com>
---

Notes:
    v2: add before/after examples in the commit message

 net/ethtool/linkmodes.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index fab66c169b9f..20165e07ef90 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -270,11 +270,12 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
 					    "lanes configuration not supported by device");
 			return -EOPNOTSUPP;
 		}
-	} else if (!lsettings->autoneg) {
-		/* If autoneg is off and lanes parameter is not passed from user,
-		 * set the lanes parameter to 0.
+	} else if (!lsettings->autoneg && ksettings->lanes) {
+		/* If autoneg is off and lanes parameter is not passed from user but
+		 * it was defined previously then set the lanes parameter to 0.
 		 */
 		ksettings->lanes = 0;
+		*mod = true;
 	}
 
 	ret = ethnl_update_bitset(ksettings->link_modes.advertising,
-- 
2.20.1

Re: [PATCH net v2] ethtool: reset #lanes when lanes is omitted

[Ido Schimmel]

On Mon, Apr 03, 2023 at 02:20:53PM -0700, Andy Roulin wrote:
> If the number of lanes was forced and then subsequently the user
> omits this parameter, the ksettings->lanes is reset. The driver
> should then reset the number of lanes to the device's default
> for the specified speed.
> 
> However, although the ksettings->lanes is set to 0, the mod variable
> is not set to true to indicate the driver and userspace should be
> notified of the changes.
> 
> The consequence is that the same ethtool operation will produce
> different results based on the initial state.
> 
> If the initial state is:
> $ ethtool swp1 | grep -A 3 'Speed: '
>         Speed: 500000Mb/s
>         Lanes: 2
>         Duplex: Full
>         Auto-negotiation: on
> 
> then executing 'ethtool -s swp1 speed 50000 autoneg off' will yield:
> $ ethtool swp1 | grep -A 3 'Speed: '
>         Speed: 500000Mb/s
>         Lanes: 2
>         Duplex: Full
>         Auto-negotiation: off
> 
> While if the initial state is:
> $ ethtool swp1 | grep -A 3 'Speed: '
>         Speed: 500000Mb/s
>         Lanes: 1
>         Duplex: Full
>         Auto-negotiation: off
> 
> executing the same 'ethtool -s swp1 speed 50000 autoneg off' results in:
> $ ethtool swp1 | grep -A 3 'Speed: '
>         Speed: 500000Mb/s
>         Lanes: 1
>         Duplex: Full
>         Auto-negotiation: off
> 
> This patch fixes this behavior. Omitting lanes will always results in
> the driver choosing the default lane width for the chosen speed. In this
> scenario, regardless of the initial state, the end state will be, e.g.,
> 
> $ ethtool swp1 | grep -A 3 'Speed: '
>         Speed: 500000Mb/s
>         Lanes: 2
>         Duplex: Full
>         Auto-negotiation: off
> 
> Fixes: 012ce4dd3102 ("ethtool: Extend link modes settings uAPI with lanes")
> Signed-off-by: Andy Roulin <aroulin@nvidia.com>
> Reviewed-by: Danielle Ratson <danieller@nvidia.com>

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

Re: [PATCH net v2] ethtool: reset #lanes when lanes is omitted

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 3 Apr 2023 14:20:53 -0700 you wrote:
> If the number of lanes was forced and then subsequently the user
> omits this parameter, the ksettings->lanes is reset. The driver
> should then reset the number of lanes to the device's default
> for the specified speed.
> 
> However, although the ksettings->lanes is set to 0, the mod variable
> is not set to true to indicate the driver and userspace should be
> notified of the changes.
> 
> [...]

Here is the summary with links:
  - [net,v2] ethtool: reset #lanes when lanes is omitted
    https://git.kernel.org/netdev/net/c/e847c7675e19

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html