diff mbox series

[RFC,net-next,v3] ethtool: Improve compatibility between netlink and ioctl interfaces

Message ID 20201027145114.226918-1-idosch@idosch.org
State New
Headers show
Series [RFC,net-next,v3] ethtool: Improve compatibility between netlink and ioctl interfaces | expand

Commit Message

Ido Schimmel Oct. 27, 2020, 2:51 p.m. UTC 
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 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: Unknown (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: no

# 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: Unknown (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: no

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.

# 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: Unknown (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: no

# 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: Unknown (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: no

Fix this incompatibility problem by introducing a new flag in the
ethtool netlink request header: 'ETHTOOL_FLAG_LEGACY'. The purpose of
the flag is to indicate to the kernel that it needs to be compatible
with the legacy ioctl interface. A patch to the ethtool user space
utility will make sure the flag is set, when supported by the kernel.

The first use case for the flag is to have the kernel set the advertised
link modes to all supported link modes in case autoneg is enabled and no
other parameter is specified. Example with a patched kernel and ethtool:

# 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: Unknown (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: no

# 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
        Auto-negotiation: on
        Port: Twisted Pair
        PHYAD: 0
        Transceiver: internal
        MDI-X: Unknown (auto)
        Supports Wake-on: umbg
        Wake-on: d
        Current message level: 0x00000007 (7)
                               drv probe link
        Link detected: no

Changes since RFC v2:
* Reword commit message
* Rebase on top of Jakub's patches

Changes since RFC v1:
* Reword commit message
* Introduce 'ETHTOOL_FLAG_LEGACY' to explicitly indicate to the kernel
  that it needs to be compatible with legacy ioctl interface

Signed-off-by: Ido Schimmel <idosch@nvidia.com>
Suggested-by: Michal Kubecek <mkubecek@suse.cz>
---
 Documentation/networking/ethtool-netlink.rst |  9 +++++++--
 include/uapi/linux/ethtool_netlink.h         |  5 ++++-
 net/ethtool/linkmodes.c                      | 14 ++++++++------
 net/ethtool/netlink.c                        |  2 +-
 4 files changed, 20 insertions(+), 10 deletions(-)

Comments

Jakub Kicinski Oct. 27, 2020, 9:53 p.m. UTC | #1 
On Tue, 27 Oct 2020 16:51:14 +0200 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.


> Fix this incompatibility problem by introducing a new flag in the

> ethtool netlink request header: 'ETHTOOL_FLAG_LEGACY'. The purpose of

> the flag is to indicate to the kernel that it needs to be compatible

> with the legacy ioctl interface. A patch to the ethtool user space

> utility will make sure the flag is set, when supported by the kernel.


I did not look at the legacy code but I'm confused by what you wrote.

IIUC for ioctl it's the user space that sets the advertised.
For netlink it's the kernel.
So how does the legacy flag make the kernel behave like it used to?

If anything LEGACY should mean - don't populate advertised at all,
user space will do it.

Also the semantics of a "LEGACY" flag are a little loose for my taste,
IMHO a new flag attr would be cleaner. ETHTOOL_A_LINKMODES_AUTO_POPULATE?
But no strong feelings.
Michal Kubecek Oct. 28, 2020, 12:53 a.m. UTC | #2 
On Tue, Oct 27, 2020 at 02:53:05PM -0700, Jakub Kicinski wrote:
> On Tue, 27 Oct 2020 16:51:14 +0200 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.
> 
> > Fix this incompatibility problem by introducing a new flag in the
> > ethtool netlink request header: 'ETHTOOL_FLAG_LEGACY'. The purpose of
> > the flag is to indicate to the kernel that it needs to be compatible
> > with the legacy ioctl interface. A patch to the ethtool user space
> > utility will make sure the flag is set, when supported by the kernel.
> 
> I did not look at the legacy code but I'm confused by what you wrote.
> 
> IIUC for ioctl it's the user space that sets the advertised.
> For netlink it's the kernel.
> So how does the legacy flag make the kernel behave like it used to?

The idea why I suggested "legacy" as the name was that it allowed
ethtool to preserve the old behaviour (without having to query for
supported modes first). But from this point of view it's indeed a bit
confusing.

> If anything LEGACY should mean - don't populate advertised at all,
> user space will do it.

I would prefer not inverting the flag so that at least for the netlink
API, the default semantics would be that ETHTOOL_A_LINKMODES_AUTONEG=1
without other attributes means "enable autonegotiation" as expected
(without touching other settings).

> Also the semantics of a "LEGACY" flag are a little loose for my taste,
> IMHO a new flag attr would be cleaner. ETHTOOL_A_LINKMODES_AUTO_POPULATE?
> But no strong feelings.

Actually, when I suggested using a flag, I had a request specific flag
in mind, not a global one. As for the name, how about
ETHTOOL_A_LINKMODES_ADVERTISE_ALL? It should be probably forbidden to
combine it with ETHTOOL_A_LINKMODES_OURS then.

Michal
Ido Schimmel Oct. 28, 2020, 5:34 p.m. UTC | #3 
On Wed, Oct 28, 2020 at 01:53:39AM +0100, Michal Kubecek wrote:
> On Tue, Oct 27, 2020 at 02:53:05PM -0700, Jakub Kicinski wrote:
> > On Tue, 27 Oct 2020 16:51:14 +0200 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.
> > 
> > > Fix this incompatibility problem by introducing a new flag in the
> > > ethtool netlink request header: 'ETHTOOL_FLAG_LEGACY'. The purpose of
> > > the flag is to indicate to the kernel that it needs to be compatible
> > > with the legacy ioctl interface. A patch to the ethtool user space
> > > utility will make sure the flag is set, when supported by the kernel.
> > 
> > I did not look at the legacy code but I'm confused by what you wrote.
> > 
> > IIUC for ioctl it's the user space that sets the advertised.
> > For netlink it's the kernel.
> > So how does the legacy flag make the kernel behave like it used to?
> 
> The idea why I suggested "legacy" as the name was that it allowed
> ethtool to preserve the old behaviour (without having to query for
> supported modes first). But from this point of view it's indeed a bit
> confusing.

I think it would be best to solve this by having user space query the
kernel for supported link modes if autoneg is being enabled without
additional parameters. Then user space will issue a set request with
ETHTOOL_A_LINKMODES_OURS being set to all supported link modes.

It does not require kernel changes and would be easier on users that
currently need to resort to old ethtool despite having a kernel that
supports netlink-based ethtool.
Michal Kubecek Oct. 29, 2020, 9:03 a.m. UTC | #4 
On Wed, Oct 28, 2020 at 07:34:36PM +0200, Ido Schimmel wrote:
> On Wed, Oct 28, 2020 at 01:53:39AM +0100, Michal Kubecek wrote:
> > On Tue, Oct 27, 2020 at 02:53:05PM -0700, Jakub Kicinski wrote:
> > > 
> > > I did not look at the legacy code but I'm confused by what you wrote.
> > > 
> > > IIUC for ioctl it's the user space that sets the advertised.
> > > For netlink it's the kernel.
> > > So how does the legacy flag make the kernel behave like it used to?
> > 
> > The idea why I suggested "legacy" as the name was that it allowed
> > ethtool to preserve the old behaviour (without having to query for
> > supported modes first). But from this point of view it's indeed a bit
> > confusing.
> 
> I think it would be best to solve this by having user space query the
> kernel for supported link modes if autoneg is being enabled without
> additional parameters. Then user space will issue a set request with
> ETHTOOL_A_LINKMODES_OURS being set to all supported link modes.
> 
> It does not require kernel changes and would be easier on users that
> currently need to resort to old ethtool despite having a kernel that
> supports netlink-based ethtool.

That would certainly be a solution. I'm not exactly happy about having
to issue two requests but (1) it would be limited to specific case with
"autoneg on" without advertise, speed and duplex (and lanes, when/if
it's introduced), (2) we would need an extra request to check support of
the flag anyway and (3) supported modes of a device are unlikely to
change so that we don't have to worry about races.

Michal
diff mbox series

Patch

diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst
index 30b98245979f..2993bcaa93ca 100644
--- a/Documentation/networking/ethtool-netlink.rst
+++ b/Documentation/networking/ethtool-netlink.rst
@@ -65,11 +65,12 @@  all devices providing it (each device in a separate message).
 types. The interpretation of these flags is the same for all request types but
 the flags may not apply to requests. Recognized flags are:
 
-  =================================  ===================================
+  =================================  =========================================
   ``ETHTOOL_FLAG_COMPACT_BITSETS``   use compact format bitsets in reply
   ``ETHTOOL_FLAG_OMIT_REPLY``        omit optional reply (_SET and _ACT)
   ``ETHTOOL_FLAG_STATS``             include optional device statistics
-  =================================  ===================================
+  ``ETHTOOL_FLAG_LEGACY``            be compatible with legacy ioctl interface
+  =================================  =========================================
 
 New request flags should follow the general idea that if the flag is not set,
 the behaviour is backward compatible, i.e. requests from old clients not aware
@@ -442,6 +443,10 @@  autoselection is done on ethtool side with ioctl interface, netlink interface
 is supposed to allow requesting changes without knowing what exactly kernel
 supports.
 
+If autonegotiation is on (either set now or kept from before), no other
+parameter is specified (e.g., speed) and ``ETHTOOL_FLAG_LEGACY`` flag is set,
+kernel adjusts advertised modes to all supported modes. This autoselection is
+done on ethtool side with ioctl interface.
 
 LINKSTATE_GET
 =============
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index e2bf36e6964b..923c2379ade6 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -94,10 +94,13 @@  enum {
 #define ETHTOOL_FLAG_OMIT_REPLY	(1 << 1)
 /* request statistics, if supported by the driver */
 #define ETHTOOL_FLAG_STATS		(1 << 2)
+/* be compatible with legacy ioctl interface */
+#define ETHTOOL_FLAG_LEGACY		(1 << 3)
 
 #define ETHTOOL_FLAG_ALL (ETHTOOL_FLAG_COMPACT_BITSETS | \
 			  ETHTOOL_FLAG_OMIT_REPLY | \
-			  ETHTOOL_FLAG_STATS)
+			  ETHTOOL_FLAG_STATS | \
+			  ETHTOOL_FLAG_LEGACY)
 
 enum {
 	ETHTOOL_A_HEADER_UNSPEC,
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index c5bcb9abc8b9..f6c0f1b54b45 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -277,9 +277,9 @@  const struct nla_policy ethnl_linkmodes_set_policy[] = {
 };
 
 /* 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, speed,
+ * duplex or legacy behavior is requested 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,
@@ -327,7 +327,7 @@  static bool ethnl_validate_master_slave_cfg(u8 cfg)
 
 static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
 				  struct ethtool_link_ksettings *ksettings,
-				  bool *mod)
+				  bool req_legacy, bool *mod)
 {
 	struct ethtool_link_settings *lsettings = &ksettings->base;
 	bool req_speed, req_duplex;
@@ -370,7 +370,7 @@  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) &&
+	    (req_speed || req_duplex || req_legacy) &&
 	    ethnl_auto_linkmodes(ksettings, req_speed, req_duplex))
 		*mod = true;
 
@@ -384,6 +384,7 @@  int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info)
 	struct nlattr **tb = info->attrs;
 	struct net_device *dev;
 	bool mod = false;
+	bool req_legacy;
 	int ret;
 
 	ret = ethnl_parse_header_dev_get(&req_info,
@@ -392,6 +393,7 @@  int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info)
 					 true);
 	if (ret < 0)
 		return ret;
+	req_legacy = req_info.flags & ETHTOOL_FLAG_LEGACY;
 	dev = req_info.dev;
 	ret = -EOPNOTSUPP;
 	if (!dev->ethtool_ops->get_link_ksettings ||
@@ -409,7 +411,7 @@  int ethnl_set_linkmodes(struct sk_buff *skb, struct genl_info *info)
 		goto out_ops;
 	}
 
-	ret = ethnl_update_linkmodes(info, tb, &ksettings, &mod);
+	ret = ethnl_update_linkmodes(info, tb, &ksettings, req_legacy, &mod);
 	if (ret < 0)
 		goto out_ops;
 
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 50d3c8896f91..ca01205c5629 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -10,7 +10,7 @@  static bool ethnl_ok __read_mostly;
 static u32 ethnl_bcast_seq;
 
 #define ETHTOOL_FLAGS_BASIC (ETHTOOL_FLAG_COMPACT_BITSETS |	\
-			     ETHTOOL_FLAG_OMIT_REPLY)
+			     ETHTOOL_FLAG_OMIT_REPLY | ETHTOOL_FLAG_LEGACY)
 #define ETHTOOL_FLAGS_STATS (ETHTOOL_FLAGS_BASIC | ETHTOOL_FLAG_STATS)
 
 const struct nla_policy ethnl_header_policy[] = {