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: > > From: Ido Schimmel <ido...@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. > > > [...] > > > > 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.gt1...@shell.armlinux.org.uk > http://lkml.kernel.org/r/20200512185503.gd1...@shell.armlinux.org.uk > http://lkml.kernel.org/r/20200521192342.ge8...@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. > > > 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
