| Message ID | 20201027145114.226918-1-idosch@idosch.org |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC,net-next,v3] ethtool: Improve compatibility between netlink and ioctl interfaces | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 5 this patch: 5 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | fail | Link |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
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.
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
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.
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 --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[] = {