Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces

[Ido Schimmel <idosch@idosch.org>]

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 <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.
> > 
> [...] 
> > 
> > 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.
> 
> > 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