* [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces
@ 2020-09-29 16:02 Ido Schimmel
2020-09-29 16:44 ` Michal Kubecek
0 siblings, 1 reply; 9+ messages in thread
From: Ido Schimmel @ 2020-09-29 16:02 UTC (permalink / raw)
To: netdev
Cc: davem, kuba, mkubecek, f.fainelli, andrew, ayal, danieller,
amcohen, mlxsw, Ido Schimmel
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.
Example:
# ethtool --version
ethtool version 5.4
# ethtool -s swp3 speed 100000 autoneg off
# ethtool swp3
Settings for swp3:
Supported ports: [ FIBRE ]
Supported link modes: 1000baseKX/Full
10000baseKR/Full
40000baseCR4/Full
40000baseSR4/Full
40000baseLR4/Full
25000baseCR/Full
25000baseSR/Full
50000baseCR2/Full
100000baseSR4/Full
100000baseCR4/Full
100000baseLR4_ER4/Full
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: Not reported
Advertised pause frame use: No
Advertised auto-negotiation: No
Advertised FEC modes: Not reported
Speed: 100000Mb/s
Duplex: Full
Port: Direct Attach Copper
PHYAD: 0
Transceiver: internal
Auto-negotiation: off
Link detected: yes
# ethtool -s swp3 autoneg on
# ethtool swp3
Settings for swp3:
Supported ports: [ FIBRE ]
Supported link modes: 1000baseKX/Full
10000baseKR/Full
40000baseCR4/Full
40000baseSR4/Full
40000baseLR4/Full
25000baseCR/Full
25000baseSR/Full
50000baseCR2/Full
100000baseSR4/Full
100000baseCR4/Full
100000baseLR4_ER4/Full
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: 1000baseKX/Full
10000baseKR/Full
40000baseCR4/Full
40000baseSR4/Full
40000baseLR4/Full
25000baseCR/Full
25000baseSR/Full
50000baseCR2/Full
100000baseSR4/Full
100000baseCR4/Full
100000baseLR4_ER4/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Speed: 100000Mb/s
Duplex: Full
Port: Direct Attach Copper
PHYAD: 0
Transceiver: internal
Auto-negotiation: on
Link detected: yes
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:
# ethtool --version
ethtool version 5.8
# ethtool -s swp3 speed 100000 autoneg off
# ethtool swp3
Settings for swp3:
Supported ports: [ FIBRE ]
Supported link modes: 1000baseKX/Full
10000baseKR/Full
40000baseCR4/Full
40000baseSR4/Full
40000baseLR4/Full
25000baseCR/Full
25000baseSR/Full
50000baseCR2/Full
100000baseSR4/Full
100000baseCR4/Full
100000baseLR4_ER4/Full
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: Not reported
Advertised pause frame use: No
Advertised auto-negotiation: No
Advertised FEC modes: Not reported
Speed: 100000Mb/s
Duplex: Full
Auto-negotiation: off
Port: Direct Attach Copper
PHYAD: 0
Transceiver: internal
Link detected: yes
# ethtool -s swp3 autoneg on
netlink error: link settings update failed
netlink error: Invalid argument
Fix this incompatibility problem by having the kernel set the advertised
link modes according to the supported link modes even if speed nor
duplex are specified. In which case, the advertised link modes are set
to the supported link modes. Example after this patch:
# ethtool --version
ethtool version 5.8
# ethtool -s swp3 speed 100000 autoneg off
# ethtool swp3
Settings for swp3:
Supported ports: [ FIBRE ]
Supported link modes: 1000baseKX/Full
10000baseKR/Full
40000baseCR4/Full
40000baseSR4/Full
40000baseLR4/Full
25000baseCR/Full
25000baseSR/Full
50000baseCR2/Full
100000baseSR4/Full
100000baseCR4/Full
100000baseLR4_ER4/Full
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: Not reported
Advertised pause frame use: No
Advertised auto-negotiation: No
Advertised FEC modes: Not reported
Speed: 100000Mb/s
Duplex: Full
Auto-negotiation: off
Port: Direct Attach Copper
PHYAD: 0
Transceiver: internal
Link detected: yes
# ethtool -s swp3 autoneg on
# ethtool swp3
Settings for swp3:
Supported ports: [ FIBRE ]
Supported link modes: 1000baseKX/Full
10000baseKR/Full
40000baseCR4/Full
40000baseSR4/Full
40000baseLR4/Full
25000baseCR/Full
25000baseSR/Full
50000baseCR2/Full
100000baseSR4/Full
100000baseCR4/Full
100000baseLR4_ER4/Full
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: 1000baseKX/Full
10000baseKR/Full
40000baseCR4/Full
40000baseSR4/Full
40000baseLR4/Full
25000baseCR/Full
25000baseSR/Full
50000baseCR2/Full
100000baseSR4/Full
100000baseCR4/Full
100000baseLR4_ER4/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Speed: 100000Mb/s
Duplex: Full
Auto-negotiation: on
Port: Direct Attach Copper
PHYAD: 0
Transceiver: internal
Link detected: yes
Fixes: bfbcfe2032e7 ("ethtool: set link modes related data with LINKMODES_SET request")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
Sending as RFC as this patch has yet to be tested in our regression
---
net/ethtool/linkmodes.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
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;
--
2.26.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces
2020-09-29 16:02 [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces Ido Schimmel
@ 2020-09-29 16:44 ` Michal Kubecek
2020-09-30 7:25 ` Ido Schimmel
0 siblings, 1 reply; 9+ messages in thread
From: Michal Kubecek @ 2020-09-29 16:44 UTC (permalink / raw)
To: netdev
Cc: Ido Schimmel, davem, kuba, f.fainelli, andrew, ayal, danieller,
amcohen, mlxsw, Ido Schimmel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces
2020-09-29 16:44 ` Michal Kubecek
@ 2020-09-30 7:25 ` Ido Schimmel
2020-09-30 8:59 ` Michal Kubecek
0 siblings, 1 reply; 9+ messages in thread
From: Ido Schimmel @ 2020-09-30 7:25 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, davem, kuba, f.fainelli, andrew, ayal, danieller,
amcohen, mlxsw, Ido Schimmel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces
2020-09-30 7:25 ` Ido Schimmel
@ 2020-09-30 8:59 ` Michal Kubecek
2020-09-30 14:19 ` Andrew Lunn
2020-09-30 20:06 ` Ido Schimmel
0 siblings, 2 replies; 9+ messages in thread
From: Michal Kubecek @ 2020-09-30 8:59 UTC (permalink / raw)
To: Ido Schimmel
Cc: netdev, davem, kuba, f.fainelli, andrew, ayal, danieller,
amcohen, mlxsw, Ido Schimmel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces
2020-09-30 8:59 ` Michal Kubecek
@ 2020-09-30 14:19 ` Andrew Lunn
2020-09-30 14:35 ` Ido Schimmel
2020-09-30 20:06 ` Ido Schimmel
1 sibling, 1 reply; 9+ messages in thread
From: Andrew Lunn @ 2020-09-30 14:19 UTC (permalink / raw)
To: Michal Kubecek
Cc: Ido Schimmel, netdev, davem, kuba, f.fainelli, ayal, danieller,
amcohen, mlxsw, Ido Schimmel
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces
2020-09-30 14:19 ` Andrew Lunn
@ 2020-09-30 14:35 ` Ido Schimmel
2020-09-30 14:43 ` Ido Schimmel
0 siblings, 1 reply; 9+ messages in thread
From: Ido Schimmel @ 2020-09-30 14:35 UTC (permalink / raw)
To: Andrew Lunn
Cc: Michal Kubecek, netdev, davem, kuba, f.fainelli, ayal, danieller,
amcohen, mlxsw, Ido Schimmel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces
2020-09-30 14:35 ` Ido Schimmel
@ 2020-09-30 14:43 ` Ido Schimmel
0 siblings, 0 replies; 9+ messages in thread
From: Ido Schimmel @ 2020-09-30 14:43 UTC (permalink / raw)
To: Andrew Lunn
Cc: Michal Kubecek, netdev, davem, kuba, f.fainelli, ayal, danieller,
amcohen, mlxsw, Ido Schimmel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces
2020-09-30 8:59 ` Michal Kubecek
2020-09-30 14:19 ` Andrew Lunn
@ 2020-09-30 20:06 ` Ido Schimmel
2020-09-30 20:45 ` Michal Kubecek
1 sibling, 1 reply; 9+ messages in thread
From: Ido Schimmel @ 2020-09-30 20:06 UTC (permalink / raw)
To: Michal Kubecek
Cc: netdev, davem, kuba, f.fainelli, andrew, ayal, danieller,
amcohen, mlxsw, Ido Schimmel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces
2020-09-30 20:06 ` Ido Schimmel
@ 2020-09-30 20:45 ` Michal Kubecek
0 siblings, 0 replies; 9+ messages in thread
From: Michal Kubecek @ 2020-09-30 20:45 UTC (permalink / raw)
To: Ido Schimmel
Cc: netdev, davem, kuba, f.fainelli, andrew, ayal, danieller,
amcohen, mlxsw, Ido Schimmel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-30 20:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 16:02 [RFC PATCH net] ethtool: Fix incompatibility between netlink and ioctl interfaces Ido Schimmel
2020-09-29 16:44 ` Michal Kubecek
2020-09-30 7:25 ` Ido Schimmel
2020-09-30 8:59 ` Michal Kubecek
2020-09-30 14:19 ` Andrew Lunn
2020-09-30 14:35 ` Ido Schimmel
2020-09-30 14:43 ` Ido Schimmel
2020-09-30 20:06 ` Ido Schimmel
2020-09-30 20:45 ` Michal Kubecek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.