On Tue, Sep 29, 2020 at 06:44:55PM +0200, Michal Kubecek wrote:

On Tue, Sep 29, 2020 at 07:02:47PM +0300, Ido Schimmel wrote:

quoted

From: Ido Schimmel <idosch@nvidia.com>

With the ioctl interface, when autoneg is enabled, but without
specifying speed, duplex or link modes, the advertised link modes are
set to the supported link modes by the ethtool user space utility.

[...] 

quoted

With the netlink interface, the same thing is done by the kernel, but
only if speed or duplex are specified. In which case, the advertised
link modes are set by traversing the supported link modes and picking
the ones matching the specified speed or duplex.

However, if speed nor duplex are specified, the driver is passed an
empty advertised link modes bitmap. This causes the mlxsw driver to
return an error. Other drivers might also be affected. Example:

This is not completely correct. What actually happens is that the
advertised modes bitmap is left untouched. The reason why advertised
modes are cleared for mlxsw is that this driver reports empty advertised
modes whenever autonegotiation is disabled.

Correct. I'll reword.

This is similar to recent discussions about distinguishing between
configuration and state. One of them was related to EEE settings, one to
pause settings, one to WoL settings vs. general wakeup enable/disable:

  http://lkml.kernel.org/r/20200511132258.GT1551@shell.armlinux.org.uk
  http://lkml.kernel.org/r/20200512185503.GD1551@shell.armlinux.org.uk
  http://lkml.kernel.org/r/20200521192342.GE8771@lion.mk-sys.cz

All of these are about common problem: we have a settings A and B such
that B is only effective if A is enabled. Should we report B as disabled
whenever A is disabled? I believe - and the consensus in those
discussions seemed to be - that we should report and set A and B
independently to distinguish between configuration (what user wants) and
state (how the device behaves). There is also practical aspect: (1) if
we don't do this, switching A off and on would reset the value of B even
if no change of B was requested and (2) commands setting A and B must be
issued in a specific order for changes of B to take effect.

Unfortunately there are drivers like mlxsw (I'm not sure how many) which
report zero advertised bitmap whenever autonegotiation is off.

quoted

diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index 7044a2853886..a9458c76209e 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c

@@ -288,9 +288,9 @@ linkmodes_set_policy[ETHTOOL_A_LINKMODES_MAX + 1] = {
 };
 
 /* Set advertised link modes to all supported modes matching requested speed
- * and duplex values. Called when autonegotiation is on, speed or duplex is
- * requested but no link mode change. This is done in userspace with ioctl()
- * interface, move it into kernel for netlink.
+ * and duplex values, if specified. Called when autonegotiation is on, but no
+ * link mode change. This is done in userspace with ioctl() interface, move it
+ * into kernel for netlink.
  * Returns true if advertised modes bitmap was modified.
  */
 static bool ethnl_auto_linkmodes(struct ethtool_link_ksettings *ksettings,

@@ -381,7 +381,6 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
 	ethnl_update_u8(&lsettings->master_slave_cfg, master_slave_cfg, mod);
 
 	if (!tb[ETHTOOL_A_LINKMODES_OURS] && lsettings->autoneg &&
-	    (req_speed || req_duplex) &&
 	    ethnl_auto_linkmodes(ksettings, req_speed, req_duplex))
 		*mod = true;

I'm afraid we will have to cope with existing drivers hiding advertised
mode setting when autonegotiation is off. Could we at least limit the
call to such drivers, i.e. replacing that line with something like

	(req_speed || req_duplex || (!old_autoneg && advertised_empty))

where old_autoneg would be the original value of lsettings->autoneg and
advertised_empty would be set if currently reported advertised modes are
zero?

I don't think so. Doing:

# ethtool -s eth0 autoneg

Is a pretty established behavior to enable all the supported advertise
bits. Here is an example with an unpatched kernel, two ethtool versions
(ioctl & netlink) and e1000 in QEMU.

Ioctl:

# ethtool --version
ethtool version 5.4

# ethtool -s eth0 advertise 0xC autoneg on

# ethtool eth0
Settings for eth0:
        Supported ports: [ TP ]
        Supported link modes:   10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  100baseT/Half 100baseT/Full 
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Advertised FEC modes: Not reported
        Speed: 1000Mb/s
        Duplex: Full
        Port: Twisted Pair
        PHYAD: 0
        Transceiver: internal
        Auto-negotiation: on
        MDI-X: off (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: yes

# ethtool -s eth0 autoneg on

# ethtool eth0
Settings for eth0:
        Supported ports: [ TP ]
        Supported link modes:   10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  10baseT/Half 10baseT/Full 
                                100baseT/Half 100baseT/Full 
                                1000baseT/Full 
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Advertised FEC modes: Not reported
        Speed: 1000Mb/s
        Duplex: Full
        Port: Twisted Pair
        PHYAD: 0
        Transceiver: internal
        Auto-negotiation: on
        MDI-X: off (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: yes

Netlink:

# ethtool --version
ethtool version 5.8

# ethtool -s eth0 advertise 0xC autoneg on

# ethtool eth0
Settings for eth0:
        Supported ports: [ TP ]
        Supported link modes:   10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
                                1000baseT/Full
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  100baseT/Half 100baseT/Full
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Advertised FEC modes: Not reported
        Speed: 1000Mb/s
        Duplex: Full
        Auto-negotiation: on
        Port: Twisted Pair
        PHYAD: 0
        Transceiver: internal
        MDI-X: off (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: yes

# ethtool -s eth0 autoneg on

# ethtool eth0
Settings for eth0:
        Supported ports: [ TP ]
        Supported link modes:   10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
                                1000baseT/Full
        Supported pause frame use: No
        Supports auto-negotiation: Yes
        Supported FEC modes: Not reported
        Advertised link modes:  100baseT/Half 100baseT/Full
        Advertised pause frame use: No
        Advertised auto-negotiation: Yes
        Advertised FEC modes: Not reported
        Speed: 1000Mb/s
        Duplex: Full
        Auto-negotiation: on
        Port: Twisted Pair
        PHYAD: 0
        Transceiver: internal
        MDI-X: off (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: yes