| Message ID | 20200929160247.1665922-1-idosch@idosch.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,net] ethtool: Fix incompatibility between netlink and ioctl interfaces | expand |
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. 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? Michal
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
On Wed, Sep 30, 2020 at 10:25:29AM +0300, Ido Schimmel wrote: > 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: > > > 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. I didn't say that ethtool does not behave like this. I just said it was a very unfortunate idea and suggested to only preserve this unfortunate behaviour for drivers like mlxsw which reset advertised modes whenever autonegotiation is turned off (which is something that should be IMHO fixed regardless of the result of this discussion). After all, even the behaviour of ethtool -s $dev autoneg on speed $s duplex $d you mentioned earlier does in fact work with ioctl only for a limited (and very small) set of link modes, only if both speed and duplex are provided and even then it only chooses one of matching modes, completely ignoring what the device supports. If we wanted to be 100% compatible, e.g. ethtool -s <dev> autoneg on speed 100000 ethtool -s <dev> autoneg on speed 100000 duplex full would have to ignore the parameters and enable all supported modes while the same commands with "1000" instead of "100000" would work as expected (as long as the device supports 1000baseT/Full and not one the other three 1000base*/Full modes). That doesn't sound like a good idea to me. How about this compromise? Let's introduce a "legacy" flag which would allow "ethtool -s <dev> autoneg on" do what it used to do while we would not taint the kernel-userspace API with this special case so that ETHTOOL_MSG_LINKMODES_SET request with only ETHTOOL_A_LINKMODES_AUTONEG (but no other attributes like _SPEED or _DUPLEX) would leave advertised link modes untouched unless the "legacy" flag is set. If the "legacy" flag is set in the request, such request would set advertised modes to all supported. Michal
> > I don't think so. Doing: > > > > # ethtool -s eth0 autoneg > > > > Is a pretty established behavior to enable all the supported advertise > > bits. I would disagree. phylib will return -EINVAL for this. int phy_ethtool_ksettings_set(struct phy_device *phydev, const struct ethtool_link_ksettings *cmd) { __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); u8 autoneg = cmd->base.autoneg; u8 duplex = cmd->base.duplex; u32 speed = cmd->base.speed; ... linkmode_copy(advertising, cmd->link_modes.advertising); ... if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising)) return -EINVAL; You have to pass a list of modes you want it to advertise. If you are using phylink and not a copper PHY, and autoneg, that means you are using in-band signalling. The same is imposed: /* If autonegotiation is enabled, we must have an advertisement */ if (config.an_enabled && phylink_is_empty_linkmode(config.advertising)) return -EINVAL; We have consistent behaviour whenever Linux is controlling the PHY because the core is imposing that behaviour. It would be nice if drivers ignoring the PHY core where consistent with this. Andrew
On Wed, Sep 30, 2020 at 04:19:09PM +0200, Andrew Lunn wrote: > > > I don't think so. Doing: > > > > > > # ethtool -s eth0 autoneg > > > > > > Is a pretty established behavior to enable all the supported advertise > > > bits. > > I would disagree. phylib will return -EINVAL for this. This has nothing to do with the kernel / phylib. With the ioctl interface when you do: # ethtool -s eth0 autoneg on The ethtool user space utility will enable all the supported link modes: https://git.kernel.org/pub/scm/network/ethtool/ethtool.git/tree/ethtool.c#n3170 For the netlink interface this is done by the kernel: https://github.com/torvalds/linux/blob/master/net/ethtool/linkmodes.c#L2 But only if speed or duplex were specified: https://github.com/torvalds/linux/blob/master/net/ethtool/linkmodes.c#L383 Which is a problem. > > int phy_ethtool_ksettings_set(struct phy_device *phydev, > const struct ethtool_link_ksettings *cmd) > { > __ETHTOOL_DECLARE_LINK_MODE_MASK(advertising); > u8 autoneg = cmd->base.autoneg; > u8 duplex = cmd->base.duplex; > u32 speed = cmd->base.speed; > > ... > linkmode_copy(advertising, cmd->link_modes.advertising); > > ... > > if (autoneg == AUTONEG_ENABLE && linkmode_empty(advertising)) > return -EINVAL; > > You have to pass a list of modes you want it to advertise. If you are > using phylink and not a copper PHY, and autoneg, that means you are > using in-band signalling. The same is imposed: > > /* If autonegotiation is enabled, we must have an advertisement */ > if (config.an_enabled && phylink_is_empty_linkmode(config.advertising)) > return -EINVAL; > > We have consistent behaviour whenever Linux is controlling the PHY > because the core is imposing that behaviour. It would be nice if > drivers ignoring the PHY core where consistent with this. You will get an error from mlxsw as well (see example in the change log). > > Andrew
On Wed, Sep 30, 2020 at 05:35:31PM +0300, Ido Schimmel wrote:
> https://github.com/torvalds/linux/blob/master/net/ethtool/linkmodes.c#L2
Should be:
https://github.com/torvalds/linux/blob/master/net/ethtool/linkmodes.c#L290
On Wed, Sep 30, 2020 at 10:59:17AM +0200, Michal Kubecek wrote: > How about this compromise? Let's introduce a "legacy" flag which would > allow "ethtool -s <dev> autoneg on" do what it used to do while we would > not taint the kernel-userspace API with this special case so that > ETHTOOL_MSG_LINKMODES_SET request with only ETHTOOL_A_LINKMODES_AUTONEG > (but no other attributes like _SPEED or _DUPLEX) would leave advertised > link modes untouched unless the "legacy" flag is set. If the "legacy" > flag is set in the request, such request would set advertised modes to > all supported. Sorry for the delay, busy with other obligations. Regarding the "legacy" flag suggestion, do you mean that the ethtool user space utility will always set it in ETHTOOL_MSG_LINKMODES_SET request in order to maintain backward compatibility with the ioctl interface? Thanks for spending time on this issue.
On Wed, Sep 30, 2020 at 11:06:53PM +0300, Ido Schimmel wrote: > On Wed, Sep 30, 2020 at 10:59:17AM +0200, Michal Kubecek wrote: > > How about this compromise? Let's introduce a "legacy" flag which would > > allow "ethtool -s <dev> autoneg on" do what it used to do while we would > > not taint the kernel-userspace API with this special case so that > > ETHTOOL_MSG_LINKMODES_SET request with only ETHTOOL_A_LINKMODES_AUTONEG > > (but no other attributes like _SPEED or _DUPLEX) would leave advertised > > link modes untouched unless the "legacy" flag is set. If the "legacy" > > flag is set in the request, such request would set advertised modes to > > all supported. > > Sorry for the delay, busy with other obligations. Regarding the "legacy" > flag suggestion, do you mean that the ethtool user space utility will > always set it in ETHTOOL_MSG_LINKMODES_SET request in order to maintain > backward compatibility with the ioctl interface? Yes, that is what I meant. We could also do the opposite (ioctl compatibility by default and a flag for the way it works now) but I would prefer the default (on userspace-kernel API level) to be "only ETHTOOL_A_LINKMODES_AUTONEG attribute means we only want to switch autonegotiation and touch nothing else". Michal
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;