2017-12-11 13:53:14

by Michal Kubecek

[permalink] [raw]
Subject: [RFC PATCH 0/9] ethtool netlink interface (WiP)

This is still work in progress and only a very small part of the ioctl
interface is reimplemented but I would like to get some comments before
the patchset becomes too big and changing things becomes too tedious.

The interface used for communication between ethtool and kernel is based on
ioctl() and suffers from many problems. The most pressing seems the be the
lack of extensibility. While some of the newer commands use structures
designed to allow future extensions (e.g. GFEATURES or TEST), most either
allow no extension at all (GPAUSEPARAM, GCOALESCE) or only limited set of
reserved fields (GDRVINFO, GEEE). Even most of those which support future
extensions limit the data types that can be used.

This series aims to provide an alternative interface based on netlink which
is what other network configuration utilities use. In particular, it uses
generic netlink (family "ethtool"). The goal is to provide an interface
which would be extensible, flexible and practical both for ethtool and for
other network configuration tools (e.g. wicked or systemd-networkd).

The interface is documented in Documentation/networking/ethtool-netlink.txt

I'll post RFC patch series for ethtool in a few days, it still needs some
more polishing.

Basic concepts:

- the interface is based on generic netlink (family name "ethtool")

- provide everything ioctl can do but allow easier future extensions

- inextensibility of ioctl interface resulted in way too many commands,
many of them obsoleted by newer ones; reduce the number by ignoring the
obsolete commands and grouping some together

- for "set" type commands, netlink allows providing only the attributes to
be changed; therefore we don't need a get-modify-set cycle, userspace can
simply say what it wants to change and how

- be less dependent on ethtool and kernel being in sync; allow e.g. saying
"ethtool -s eth0 advertise xyz off" without knowing what "xyz" means;
it's kernel's job to know what mode "xyz" is and if it exists and is
supported

Unresolved questions/tasks:

- allow dumps for "get" type requests, e.g. listing EEE settings for all
interfaces in current netns

- find reasonable format for data transfers (e.g. eeprom dump or flash)

- while the netlink interface allows easy future extensions, ethtool_ops
interface does not; some settings could be implemented using tunables and
accessed via relevant netlink messages (as well as tunables) from
userspace but in the long term, something better will be needed

- it would be nice if driver could provide useful error/warning messages to
be passed to userspace via extended ACK; example: while testing, I found
a driver which only allows values 0, 1, 3 and 10000 for certain parameter
but the only way poor user can find out is either by trying all values or
by checking driver source

Michal Kubecek (9):
netlink: introduce nla_put_bitfield32()
ethtool: introduce ethtool netlink interface
ethtool: helper functions for netlink interface
ethtool: netlink bitset handling
ethtool: implement GET_DRVINFO message
ethtool: implement GET_SETTINGS message
ethtool: implement SET_SETTINGS message
ethtool: implement GET_PARAMS message
ethtool: implement SET_PARAMS message

Documentation/networking/ethtool-netlink.txt | 466 ++++++
include/linux/ethtool_netlink.h | 12 +
include/linux/netdevice.h | 2 +
include/net/netlink.h | 15 +
include/uapi/linux/ethtool.h | 3 +
include/uapi/linux/ethtool_netlink.h | 239 +++
net/Kconfig | 7 +
net/core/Makefile | 3 +-
net/core/ethtool.c | 150 +-
net/core/ethtool_common.c | 158 ++
net/core/ethtool_common.h | 19 +
net/core/ethtool_netlink.c | 2323 ++++++++++++++++++++++++++
12 files changed, 3260 insertions(+), 137 deletions(-)
create mode 100644 Documentation/networking/ethtool-netlink.txt
create mode 100644 include/linux/ethtool_netlink.h
create mode 100644 include/uapi/linux/ethtool_netlink.h
create mode 100644 net/core/ethtool_common.c
create mode 100644 net/core/ethtool_common.h
create mode 100644 net/core/ethtool_netlink.c

--
2.15.1


2017-12-11 13:53:26

by Michal Kubecek

[permalink] [raw]
Subject: [RFC PATCH 1/9] netlink: introduce nla_put_bitfield32()

Similar to other data types, this helper puts NLA_BITFIELD32 attribute into
a netlink message. It takes separate value and selector arguments, if you
already have struct nla_bitfield32, you can use nla_put().

Signed-off-by: Michal Kubecek <[email protected]>
---
include/net/netlink.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0c154f98e987..6d4eb6bd9235 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -1064,6 +1064,21 @@ static inline int nla_put_in6_addr(struct sk_buff *skb, int attrtype,
return nla_put(skb, attrtype, sizeof(*addr), addr);
}

+/**
+ * nla_put_bitfield32 - Add a bitfield32 value/selector attribute to
+ * a socket buffer
+ * @skb: socket buffer to add attribute to
+ * @value: 32-bit value bitmap
+ * @selector: 32-bit selector bitmap
+ */
+static inline int nla_put_bitfield32(struct sk_buff *skb, int attrtype,
+ u32 value, u32 selector)
+{
+ struct nla_bitfield32 tmp = { .value = value, .selector = selector };
+
+ return nla_put(skb, attrtype, sizeof(tmp), &tmp);
+}
+
/**
* nla_get_u32 - return payload of u32 attribute
* @nla: u32 netlink attribute
--
2.15.1

2017-12-11 13:53:38

by Michal Kubecek

[permalink] [raw]
Subject: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

No function implemented yet, only genetlink and module infrastructure.
Register/unregister genetlink family "ethtool" and allow the module to be
autoloaded by genetlink code (if built as a module, distributions would
probably prefer "y").

Signed-off-by: Michal Kubecek <[email protected]>
---
Documentation/networking/ethtool-netlink.txt | 155 +++++++++++++++++++++++++++
include/linux/ethtool_netlink.h | 9 ++
include/uapi/linux/ethtool_netlink.h | 33 ++++++
net/Kconfig | 7 ++
net/core/Makefile | 1 +
net/core/ethtool_netlink.c | 46 ++++++++
6 files changed, 251 insertions(+)
create mode 100644 Documentation/networking/ethtool-netlink.txt
create mode 100644 include/linux/ethtool_netlink.h
create mode 100644 include/uapi/linux/ethtool_netlink.h
create mode 100644 net/core/ethtool_netlink.c

diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
new file mode 100644
index 000000000000..c94da66cb5fb
--- /dev/null
+++ b/Documentation/networking/ethtool-netlink.txt
@@ -0,0 +1,155 @@
+ Netlink interface for ethtool
+ =============================
+
+
+Basic information
+-----------------
+
+Netlink interface for ethtool uses generic netlink family "ethtool" (userspace
+application should use macros ETHTOOL_GENL_NAME and ETHTOOL_GENL_VERSION
+defined in <linux/ethtool_netlink.h> uapi header). This family uses a message
+header of 24 bytes:
+
+struct ethnlmsghdr {
+ __u32 ifindex;
+ __u16 flags;
+ union {
+ __u16 info_mask;
+ __u16 index;
+ }
+ char ifname[IFNAMSIZ];
+};
+
+In requests, device can be identified by ifindex or by name; if both are used,
+they must match. In replies, kernel fills both. The meaning of flags,
+info_mask and index fields depends on request type.
+
+The ethtool netlink interface uses extended ACK for error and warning
+reporting, userspace application developers are encouraged to make these
+messages available to user in a suitable way.
+
+Requests can be divided into three categories: "get" (retrieving information),
+"set" (setting parameters) and "action" (invoking an action).
+
+All "set" and "action" type requests require admin privileges (CAP_NET_ADMIN
+in the namespace). Most "get" type request are allowed for anyone but there
+are exceptions (where the response contains sensitive information). In some
+cases, the request as such is allowed for anyone but unprivileged users have
+attributes with sensitive information (e.g. wake-on-lan password) omitted.
+
+
+Conventions
+-----------
+
+Attributes which represent a boolean value usually use u8 type so that we can
+distinguish three states: "on", "off" and "not present" (meaning the
+information is not available in "get" requests or value is not to be changed
+in "set" requests). For these attributes, the "true" value should be passed as
+number 1 but any non-zero value should be understood as "true" by recipient.
+
+
+List of message types
+---------------------
+
+All constants use ETHTOOL_CMD_ prefix followed by "GET", "SET" or "ACT" to
+indicate the type.
+
+Messages of type "get" are used by userspace to request information and
+usually do not contain any attributes (that may be added later for dump
+filtering). Kernel response is in the form of corresopinding "set" message;
+the same message can be also used to set (some of) the parameters, except for
+messages marked as "response only" in the table above.
+
+Later sections describe the format and semantics of these request messages.
+
+
+Request translation
+-------------------
+
+The following table maps iosctl commands to netlink commands providing their
+functionality. Entries with "n/a" in right column are commands which do not
+have their netlink replacement yet.
+
+ioctl command netlink command
+---------------------------------------------------------------------
+ETHTOOL_GSET n/a
+ETHTOOL_SSET n/a
+ETHTOOL_GDRVINFO n/a
+ETHTOOL_GREGS n/a
+ETHTOOL_GWOL n/a
+ETHTOOL_SWOL n/a
+ETHTOOL_GMSGLVL n/a
+ETHTOOL_SMSGLVL n/a
+ETHTOOL_NWAY_RST n/a
+ETHTOOL_GLINK n/a
+ETHTOOL_GEEPROM n/a
+ETHTOOL_SEEPROM n/a
+ETHTOOL_GCOALESCE n/a
+ETHTOOL_SCOALESCE n/a
+ETHTOOL_GRINGPARAM n/a
+ETHTOOL_SRINGPARAM n/a
+ETHTOOL_GPAUSEPARAM n/a
+ETHTOOL_SPAUSEPARAM n/a
+ETHTOOL_GRXCSUM n/a
+ETHTOOL_SRXCSUM n/a
+ETHTOOL_GTXCSUM n/a
+ETHTOOL_STXCSUM n/a
+ETHTOOL_GSG n/a
+ETHTOOL_SSG n/a
+ETHTOOL_TEST n/a
+ETHTOOL_GSTRINGS n/a
+ETHTOOL_PHYS_ID n/a
+ETHTOOL_GSTATS n/a
+ETHTOOL_GTSO n/a
+ETHTOOL_STSO n/a
+ETHTOOL_GPERMADDR n/a
+ETHTOOL_GUFO n/a
+ETHTOOL_SUFO n/a
+ETHTOOL_GGSO n/a
+ETHTOOL_SGSO n/a
+ETHTOOL_GFLAGS n/a
+ETHTOOL_SFLAGS n/a
+ETHTOOL_GPFLAGS n/a
+ETHTOOL_SPFLAGS n/a
+ETHTOOL_GRXFH n/a
+ETHTOOL_SRXFH n/a
+ETHTOOL_GGRO n/a
+ETHTOOL_SGRO n/a
+ETHTOOL_GRXRINGS n/a
+ETHTOOL_GRXCLSRLCNT n/a
+ETHTOOL_GRXCLSRULE n/a
+ETHTOOL_GRXCLSRLALL n/a
+ETHTOOL_SRXCLSRLDEL n/a
+ETHTOOL_SRXCLSRLINS n/a
+ETHTOOL_FLASHDEV n/a
+ETHTOOL_RESET n/a
+ETHTOOL_SRXNTUPLE n/a
+ETHTOOL_GRXNTUPLE n/a
+ETHTOOL_GSSET_INFO n/a
+ETHTOOL_GRXFHINDIR n/a
+ETHTOOL_SRXFHINDIR n/a
+ETHTOOL_GFEATURES n/a
+ETHTOOL_SFEATURES n/a
+ETHTOOL_GCHANNELS n/a
+ETHTOOL_SCHANNELS n/a
+ETHTOOL_SET_DUMP n/a
+ETHTOOL_GET_DUMP_FLAG n/a
+ETHTOOL_GET_DUMP_DATA n/a
+ETHTOOL_GET_TS_INFO n/a
+ETHTOOL_GMODULEINFO n/a
+ETHTOOL_GMODULEEEPROM n/a
+ETHTOOL_GEEE n/a
+ETHTOOL_SEEE n/a
+ETHTOOL_GRSSH n/a
+ETHTOOL_SRSSH n/a
+ETHTOOL_GTUNABLE n/a
+ETHTOOL_STUNABLE n/a
+ETHTOOL_GPHYSTATS n/a
+ETHTOOL_PERQUEUE n/a
+ETHTOOL_GLINKSETTINGS n/a
+ETHTOOL_SLINKSETTINGS n/a
+ETHTOOL_PHY_GTUNABLE n/a
+ETHTOOL_PHY_STUNABLE n/a
+ETHTOOL_GFECPARAM n/a
+ETHTOOL_SFECPARAM n/a
+
diff --git a/include/linux/ethtool_netlink.h b/include/linux/ethtool_netlink.h
new file mode 100644
index 000000000000..0412adb4f42f
--- /dev/null
+++ b/include/linux/ethtool_netlink.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+#ifndef _LINUX_ETHTOOL_NETLINK_H_
+#define _LINUX_ETHTOOL_NETLINK_H_
+
+#include <uapi/linux/ethtool_netlink.h>
+#include <linux/ethtool.h>
+
+#endif /* _LINUX_ETHTOOL_NETLINK_H_ */
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
new file mode 100644
index 000000000000..06cff2b52dfe
--- /dev/null
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+#ifndef _UAPI_LINUX_ETHTOOL_NETLINK_H_
+#define _UAPI_LINUX_ETHTOOL_NETLINK_H_
+
+#include <linux/ethtool.h>
+
+/* identifies the device to query/set
+ * - use either ifindex or ifname, not both
+ * - for dumps and messages not related to a particular devices, fill neither
+ * - info_mask is a bitfield, interpretation depends on the command
+ */
+struct ethnlmsghdr {
+ __u32 ifindex; /* device ifindex */
+ __u16 flags; /* request/response flags */
+ __u16 info_mask; /* request/response info mask */
+ char ifname[IFNAMSIZ]; /* device name */
+};
+
+#define ETHNL_HDRLEN NLMSG_ALIGN(sizeof(struct ethnlmsghdr))
+
+enum {
+ ETHTOOL_CMD_NOOP,
+
+ __ETHTOOL_CMD_MAX,
+ ETHTOOL_CMD_MAX = (__ETHTOOL_CMD_MAX - 1),
+};
+
+/* generic netlink info */
+#define ETHTOOL_GENL_NAME "ethtool"
+#define ETHTOOL_GENL_VERSION 1
+
+#endif /* _UAPI_LINUX_ETHTOOL_NETLINK_H_ */
diff --git a/net/Kconfig b/net/Kconfig
index 9dba2715919d..a5e3c89a2495 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -440,6 +440,13 @@ config MAY_USE_DEVLINK
on MAY_USE_DEVLINK to ensure they do not cause link errors when
devlink is a loadable module and the driver using it is built-in.

+config ETHTOOL_NETLINK
+ tristate "Netlink interface for ethtool"
+ default m
+ help
+ New netlink based interface for ethtool which is going to obsolete
+ the old ioctl based one once it provides all features.
+
endif # if NET

# Used by archs to tell that they support BPF JIT compiler plus which flavour.
diff --git a/net/core/Makefile b/net/core/Makefile
index 1fd0a9c88b1b..617ab2abecdf 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_DST_CACHE) += dst_cache.o
obj-$(CONFIG_HWBM) += hwbm.o
obj-$(CONFIG_NET_DEVLINK) += devlink.o
obj-$(CONFIG_GRO_CELLS) += gro_cells.o
+obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_netlink.o
diff --git a/net/core/ethtool_netlink.c b/net/core/ethtool_netlink.c
new file mode 100644
index 000000000000..46a226bb9a2c
--- /dev/null
+++ b/net/core/ethtool_netlink.c
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+#include <linux/module.h>
+#include <linux/ethtool_netlink.h>
+#include <linux/netdevice.h>
+#include <net/genetlink.h>
+#include "ethtool_common.h"
+
+static struct genl_family ethtool_genl_family;
+
+/* genetlink paperwork */
+
+static const struct genl_ops ethtool_genl_ops[] = {
+};
+
+static struct genl_family ethtool_genl_family = {
+ .hdrsize = ETHNL_HDRLEN,
+ .name = ETHTOOL_GENL_NAME,
+ .version = ETHTOOL_GENL_VERSION,
+ .netnsok = true,
+ .ops = ethtool_genl_ops,
+ .n_ops = ARRAY_SIZE(ethtool_genl_ops),
+};
+
+/* module paperwork */
+
+static int __init ethtool_nl_init(void)
+{
+ return genl_register_family(&ethtool_genl_family);
+}
+
+static void __exit ethtool_nl_exit(void)
+{
+ genl_unregister_family(&ethtool_genl_family);
+}
+
+module_init(ethtool_nl_init);
+module_exit(ethtool_nl_exit);
+
+/* this alias is for autoload */
+MODULE_ALIAS("net-pf-" __stringify(PF_NETLINK)
+ "-proto-" __stringify(NETLINK_GENERIC)
+ "-family-" ETHTOOL_GENL_NAME);
+MODULE_AUTHOR("Michal Kubecek <[email protected]>");
+MODULE_DESCRIPTION("Netlink interface for ethtool");
+MODULE_LICENSE("GPL");
--
2.15.1

2017-12-11 13:53:53

by Michal Kubecek

[permalink] [raw]
Subject: [RFC PATCH 3/9] ethtool: helper functions for netlink interface

Misc helpers used by ethtool netlink code.

Signed-off-by: Michal Kubecek <[email protected]>
---
net/core/ethtool_netlink.c | 177 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 177 insertions(+)

diff --git a/net/core/ethtool_netlink.c b/net/core/ethtool_netlink.c
index 46a226bb9a2c..22d23d057623 100644
--- a/net/core/ethtool_netlink.c
+++ b/net/core/ethtool_netlink.c
@@ -8,6 +8,183 @@

static struct genl_family ethtool_genl_family;

+/* misc helper functions */
+
+static int ethnl_str_size(const char *s)
+{
+ return nla_total_size(strlen(s) + 1);
+}
+
+static int ethnl_str_ifne_size(const char *s)
+{
+ return s[0] ? ethnl_str_size(s) : 0;
+}
+
+static int ethnl_put_str_ifne(struct sk_buff *skb, int attrtype, const char *s)
+{
+ if (!s[0])
+ return 0;
+ return nla_put_string(skb, attrtype, s);
+}
+
+static struct nlattr *ethnl_nest_start(struct sk_buff *skb, int attrtype)
+{
+ return nla_nest_start(skb, attrtype | NLA_F_NESTED);
+}
+
+/* ethnl_update_* return true if the value is changed */
+static bool ethnl_update_u32(u32 *dst, struct nlattr *attr)
+{
+ u32 val;
+
+ if (!attr)
+ return false;
+ val = nla_get_u32(attr);
+ if (*dst == val)
+ return false;
+
+ *dst = val;
+ return true;
+}
+
+static bool ethnl_update_u8(u8 *dst, struct nlattr *attr)
+{
+ u8 val;
+
+ if (!attr)
+ return false;
+ val = nla_get_u8(attr);
+ if (*dst == val)
+ return false;
+
+ *dst = val;
+ return true;
+}
+
+/* update u32 value used as bool from NLA_U8 */
+static bool ethnl_update_bool32(u32 *dst, struct nlattr *attr)
+{
+ u8 val;
+
+ if (!attr)
+ return false;
+ val = !!nla_get_u8(attr);
+ if (!!*dst == val)
+ return false;
+
+ *dst = val;
+ return true;
+}
+
+static bool ethnl_update_binary(u8 *dst, unsigned int len, struct nlattr *attr)
+{
+ if (!attr)
+ return false;
+ if (nla_len(attr) < len)
+ len = nla_len(attr);
+ if (!memcmp(dst, nla_data(attr), len))
+ return false;
+
+ memcpy(dst, nla_data(attr), len);
+ return true;
+}
+
+static bool ethnl_update_bitfield32(u32 *dst, struct nlattr *attr)
+{
+ struct nla_bitfield32 change;
+ u32 newval;
+
+ if (!attr)
+ return false;
+ change = nla_get_bitfield32(attr);
+ newval = (*dst & ~change.selector) | (change.value & change.selector);
+ if (*dst == newval)
+ return false;
+
+ *dst = newval;
+ return true;
+}
+
+static struct net_device *ethnl_dev_get(struct genl_info *info)
+{
+ const struct ethnlmsghdr *hdr = info->userhdr;
+ struct net *net = genl_info_net(info);
+ struct net_device *dev;
+
+ if (hdr->ifindex) {
+ dev = dev_get_by_index(net, hdr->ifindex);
+ if (!dev)
+ return ERR_PTR(-ENODEV);
+ /* if both ifindex and ifname are passed, both must match */
+ if (hdr->ifname && strncmp(dev->name, hdr->ifname, IFNAMSIZ)) {
+ GENL_SET_ERR_MSG(info,
+ "ifindex and ifname do not match");
+ return ERR_PTR(-ENODEV);
+ }
+ return dev;
+ } else if (hdr->ifname[0]) {
+ dev = dev_get_by_name(net, hdr->ifname);
+ return dev ?: ERR_PTR(-ENODEV);
+ }
+
+ return ERR_PTR(-EINVAL);
+}
+
+static void warn_partial_info(struct genl_info *info)
+{
+ GENL_SET_ERR_MSG(info, "not all requested data could be retrieved");
+}
+
+/* Check user privileges explicitly to allow finer access control based on
+ * context of the request or hiding part of the information from unprivileged
+ * users
+ */
+static bool ethnl_is_privileged(struct sk_buff *skb, struct genl_info *info)
+{
+ struct net *net = genl_info_net(info);
+
+ return netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN);
+}
+
+/* create skb for a reply
+ * payload: payload length (without netlink, genetlink and ethnl headers)
+ * dev: device the reply is about
+ * cmd: ETHTOOL_CMD_* command for reply
+ * info: info for the received packet we respond to
+ * ehdrp: place to store pointer to ethtool specific header (may be null)
+ * returns: skb or null on error
+ */
+static struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev,
+ u8 cmd, struct genl_info *info,
+ struct ethnlmsghdr **ehdrp)
+{
+ struct ethnlmsghdr *ehdr;
+ struct sk_buff *rskb;
+
+ rskb = genlmsg_new(ETHNL_HDRLEN + payload, GFP_KERNEL);
+ if (!rskb) {
+ GENL_SET_ERR_MSG(info,
+ "failed to allocate reply message");
+ return NULL;
+ }
+
+ ehdr = genlmsg_put_reply(rskb, info, &ethtool_genl_family, 0, cmd);
+ if (!ehdr) {
+ nlmsg_free(rskb);
+ return NULL;
+ }
+ if (ehdrp)
+ *ehdrp = ehdr;
+
+ ehdr->ifindex = dev->ifindex;
+ ehdr->flags = 0;
+ ehdr->info_mask = 0;
+ strncpy(ehdr->ifname, dev->name, sizeof(ehdr->ifname));
+ ehdr->ifname[sizeof(ehdr->ifname) - 1] = '\0';
+
+ return rskb;
+}
+
/* genetlink paperwork */

static const struct genl_ops ethtool_genl_ops[] = {
--
2.15.1

2017-12-11 13:53:56

by Michal Kubecek

[permalink] [raw]
Subject: [RFC PATCH 4/9] ethtool: netlink bitset handling

Declare attribute type constants and add helper functions to handle
arbitrary length bit sets.

Signed-off-by: Michal Kubecek <[email protected]>
---
Documentation/networking/ethtool-netlink.txt | 56 +++++
include/uapi/linux/ethtool_netlink.h | 31 +++
net/core/ethtool_netlink.c | 346 +++++++++++++++++++++++++++
3 files changed, 433 insertions(+)

diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
index c94da66cb5fb..893e5156f6a7 100644
--- a/Documentation/networking/ethtool-netlink.txt
+++ b/Documentation/networking/ethtool-netlink.txt
@@ -48,6 +48,62 @@ in "set" requests). For these attributes, the "true" value should be passed as
number 1 but any non-zero value should be understood as "true" by recipient.


+Bit sets
+--------
+
+For short bitmaps of (reasonably) fixed length, standard NLA_BITFIELD32 type
+is used. For arbitrary length bitmaps, ethtool netlink uses a nested attribute
+with contents of one of two forms: compact (two binary bitmaps representing
+bit values and mask of affected bits) and bit-by-bit (list of bits identified
+by either index or name).
+
+Compact form: nested (bitset) atrribute contents:
+
+ ETHA_BITSET_SIZE (u32) number of significant bits
+ ETHA_BITSET_VALUE (binary) bitmap of bit values
+ ETHA_BITSET_MASK (binary) bitmap of valid bits
+
+Value and mask must have length at least ETHA_BITSET_SIZE bits rounded up to
+a multiple of 32 bits. They consist of 32-bit words in host byt order, words
+ordered from least significant to most significant (i.e. the same way as
+bitmaps are passed with ioctl interface).
+
+For compact form, ETHA_BITSET_SIZE and ETHA_BITSET_VALUE are mandatory.
+Similar to BITFIELD32, a compact form bit set requests to set bits in the mask
+to 1 (if set in value) or 0 (if not) and preserve the rest. If the mask is
+omitted, it is supposed to be "all ones", i.e. set all bits according to
+value.
+
+Kernel bit set length may differ from userspace length if older application is
+used on newer kernel or vice versa. If userspace bitmaps are longer, error is
+only issued if request actually tries to set bits not implemented in kernel.
+
+Bit-by-bit form: nested (bitset) attribute contents:
+
+ ETHA_BITSET_SIZE (u32) number of significant bits (optional)
+ ETHA_BITSET_BITS (nested) array of bits
+ ETHA_BITSET_BIT
+ ETHA_BIT_INDEX (u32) bit index (0 for LSB)
+ ETHA_BIT_NAME (string) bit name
+ ETHA_BIT_VALUE (flag) present if bit is set
+ ETHA_BITSET_BIT
+ ...
+
+Bit size is optional for bit-by-bit form. ETHA_BITSET_BITS nest can only
+contain ETHA_BITS_BIT attributes but there can be an arbitrary number of them.
+A bit may be identified by its index or by its name. When used in requests,
+listed bits are set to 0 or 1 according to ETHA_BIT_VALUE, the rest is
+preserved. A request fails if index exceeds kernel bit length or if name is
+not recognized.
+
+In requests, application can use either form. Form used by kernel in reply is
+determined by a flag in flags field of request header. Semantics of value and
+mask depends on the attribute. General idea is that flags control request
+processing, info_mask control which parts of the information are returned in
+"get" request and index identifies a particular subcommand or an object to
+which the request applies.
+
+
List of message types
---------------------

diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 06cff2b52dfe..6db35f00ea32 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -26,6 +26,37 @@ enum {
ETHTOOL_CMD_MAX = (__ETHTOOL_CMD_MAX - 1),
};

+/* bit sets */
+
+enum {
+ ETHA_BIT_UNSPEC,
+ ETHA_BIT_INDEX, /* u32 */
+ ETHA_BIT_NAME, /* string */
+ ETHA_BIT_VALUE, /* flag */
+
+ __ETHA_BIT_MAX,
+ ETHA_BIT_MAX = (__ETHA_BIT_MAX - 1),
+};
+
+enum {
+ ETHA_BITS_UNSPEC,
+ ETHA_BITS_BIT,
+
+ __ETHA_BITS_MAX,
+ ETHA_BITS_MAX = (__ETHA_BITS_MAX - 1),
+};
+
+enum {
+ ETHA_BITSET_UNSPEC,
+ ETHA_BITSET_SIZE, /* u32 */
+ ETHA_BITSET_BITS, /* nest - ETHA_BITS_* */
+ ETHA_BITSET_VALUE, /* binary */
+ ETHA_BITSET_MASK, /* binary */
+
+ __ETHA_BITSET_MAX,
+ ETHA_BITSET_MAX = (__ETHA_BITSET_MAX - 1),
+};
+
/* generic netlink info */
#define ETHTOOL_GENL_NAME "ethtool"
#define ETHTOOL_GENL_VERSION 1
diff --git a/net/core/ethtool_netlink.c b/net/core/ethtool_netlink.c
index 22d23d057623..9f287e67f30b 100644
--- a/net/core/ethtool_netlink.c
+++ b/net/core/ethtool_netlink.c
@@ -3,6 +3,7 @@
#include <linux/module.h>
#include <linux/ethtool_netlink.h>
#include <linux/netdevice.h>
+#include <linux/bitmap.h>
#include <net/genetlink.h>
#include "ethtool_common.h"

@@ -130,6 +131,351 @@ static struct net_device *ethnl_dev_get(struct genl_info *info)
return ERR_PTR(-EINVAL);
}

+/* bitset helper functions */
+
+static bool ethnl_test_bit(u32 *val, unsigned int index)
+{
+ if (val)
+ return val[index / 32] & (1 << (index % 32));
+ else
+ return true;
+}
+
+static void ethnl_copy_bitmap(u32 *dst, u32 *src, unsigned int size)
+{
+ unsigned int full_words = size / 32;
+
+ memcpy(dst, src, full_words * sizeof(u32));
+ if (size % 32 != 0)
+ dst[full_words] = src[full_words] & ((1U << (size % 32)) - 1);
+}
+
+static void ethnl_fill_bitmap(u32 *dst, unsigned int size)
+{
+ unsigned int full_words = size / 32;
+
+ memset(dst, 0xff, full_words * sizeof(u32));
+ if (size % 32 != 0)
+ dst[full_words] = (1U << (size % 32)) - 1;
+}
+
+/* convert standard kernel bitmap (long sized words) to ethtool one (u32 words)
+ * bitmap_to_u32array() can in fact do an "in place" conversion but it's not
+ * documented so we cannot rely on it; moreover, we can use the fact that this
+ * conversion is no-op except for 64-bit big endian architectures
+ */
+static void ethnl_bitmap_to_u32(unsigned long *bitmap, unsigned int nwords)
+{
+#if BITS_PER_LONG == 64 && defined(__BIG_ENDIAN)
+ u32 *dst = (u32 *)bitmap;
+ unsigned int i;
+
+ for (i = 0; i < nwords; i++) {
+ unsigned long tmp = READ_ONCE(bitmap[i]);
+
+ dst[2 * i] = tmp & 0xffffffff;
+ dst[2 * i + 1] = tmp >> 32;
+ }
+#endif
+}
+
+/* calculate size for a bitset attribute
+ * see ethnl_put_bitset() for arguments
+ */
+static int ethnl_bitset_size(bool compact, unsigned int size, u32 *val,
+ u32 *mask, const char *const *names)
+{
+ unsigned int nwords = (size + 31) / 32;
+ unsigned int len;
+
+ if (WARN_ON(!compact && !names))
+ return -EINVAL;
+ /* size */
+ len = nla_total_size(sizeof(u32));
+
+ if (compact) {
+ /* values, mask */
+ len += 2 * nla_total_size(nwords * sizeof(u32));
+ } else {
+ unsigned int bits_len = 0;
+ unsigned int bit_len, i;
+
+ for (i = 0; i < size; i++) {
+ if (!ethnl_test_bit(mask, i))
+ continue;
+ /* index */
+ bit_len = nla_total_size(sizeof(u32));
+ /* name */
+ bit_len += ethnl_str_size(names[i]);
+ /* value */
+ if (ethnl_test_bit(val, i))
+ bit_len += nla_total_size(0);
+
+ /* bit nest */
+ bits_len += nla_total_size(bit_len);
+ }
+ len += nla_total_size(bits_len);
+ }
+
+ /* outermost nest */
+ return nla_total_size(len);
+}
+
+/* put bitset into a message
+ * skb: skb with the message
+ * attrtype: attribute type for the bitset
+ * compact: compact (bitmaps) or verbose (bit-by-bit with names) format
+ * size: size of the set in bits
+ * val: bitset values
+ * mask: mask of valid bits
+ * names: bit names (only used for verbose format)
+ */
+static int ethnl_put_bitset(struct sk_buff *skb, int attrtype, bool compact,
+ unsigned int size, u32 *val, u32 *mask,
+ const char *const *names)
+{
+ struct nlattr *nest;
+ struct nlattr *attr;
+ int ret;
+
+ if (WARN_ON(!compact && !names))
+ return -EINVAL;
+ nest = ethnl_nest_start(skb, attrtype);
+ if (!nest)
+ return -EMSGSIZE;
+
+ ret = -EMSGSIZE;
+ if (nla_put_u32(skb, ETHA_BITSET_SIZE, size))
+ goto err;
+ if (compact) {
+ unsigned int bytesize = ((size + 31) / 32) * sizeof(u32);
+
+ ret = -EMSGSIZE;
+ attr = nla_reserve(skb, ETHA_BITSET_VALUE, bytesize);
+ if (!attr)
+ goto err;
+ ethnl_copy_bitmap(nla_data(attr), val, size);
+ attr = nla_reserve(skb, ETHA_BITSET_MASK, bytesize);
+ if (!attr)
+ goto err;
+ if (mask)
+ ethnl_copy_bitmap(nla_data(attr), mask, size);
+ else
+ ethnl_fill_bitmap(nla_data(attr), size);
+ } else {
+ struct nlattr *bits;
+ unsigned int i;
+
+ bits = ethnl_nest_start(skb, ETHA_BITSET_BITS);
+ if (!bits)
+ goto err;
+ for (i = 0; i < size; i++) {
+ if (!ethnl_test_bit(mask, i))
+ continue;
+ attr = ethnl_nest_start(skb, ETHA_BITS_BIT);
+ if (!attr ||
+ nla_put_u32(skb, ETHA_BIT_INDEX, i) ||
+ nla_put_string(skb, ETHA_BIT_NAME, names[i]))
+ goto err;
+ if (ethnl_test_bit(val, i))
+ if (nla_put_flag(skb, ETHA_BIT_VALUE))
+ goto err;
+ nla_nest_end(skb, attr);
+ }
+ nla_nest_end(skb, bits);
+ }
+
+ nla_nest_end(skb, nest);
+ return 0;
+err:
+ nla_nest_cancel(skb, nest);
+ return ret;
+}
+
+static const struct nla_policy bitset_policy[ETHA_BITSET_MAX + 1] = {
+ [ETHA_BITSET_UNSPEC] = { .type = NLA_UNSPEC },
+ [ETHA_BITSET_SIZE] = { .type = NLA_U32 },
+ [ETHA_BITSET_BITS] = { .type = NLA_NESTED },
+ [ETHA_BITSET_VALUE] = { .type = NLA_BINARY },
+ [ETHA_BITSET_MASK] = { .type = NLA_BINARY },
+};
+
+static const struct nla_policy bit_policy[ETHA_BIT_MAX + 1] = {
+ [ETHA_BIT_UNSPEC] = { .type = NLA_UNSPEC },
+ [ETHA_BIT_INDEX] = { .type = NLA_U32 },
+ [ETHA_BIT_NAME] = { .type = NLA_STRING },
+ [ETHA_BIT_VALUE] = { .type = NLA_FLAG },
+};
+
+static int ethnl_name_to_idx(const char *const *names, unsigned int n_names,
+ const char *name, unsigned int name_len)
+{
+ unsigned int i;
+
+ for (i = 0; i < n_names; i++)
+ if (!strncmp(names[i], name, name_len))
+ return i;
+
+ return n_names;
+}
+
+static int ethnl_update_bit(unsigned long *bitmap, unsigned int nbits,
+ struct nlattr *bit_attr, const char *const *names,
+ struct genl_info *info)
+{
+ struct nlattr *tb[ETHA_BIT_MAX + 1];
+ int ret, idx;
+
+ if (nla_type(bit_attr) != ETHA_BITS_BIT) {
+ GENL_SET_ERR_MSG(info,
+ "ETHA_BITSET_BITS can contain only ETHA_BITS_BIT");
+ return genl_err_attr(info, -EINVAL, bit_attr);
+ }
+ ret = nla_parse_nested(tb, ETHA_BIT_MAX, bit_attr, bit_policy,
+ info->extack);
+ if (ret < 0)
+ return ret;
+
+ if (tb[ETHA_BIT_INDEX]) {
+ idx = nla_get_u32(tb[ETHA_BIT_INDEX]);
+ if (idx >= nbits) {
+ GENL_SET_ERR_MSG(info, "bit index too high");
+ return genl_err_attr(info, -EOPNOTSUPP,
+ tb[ETHA_BIT_INDEX]);
+ }
+ if (tb[ETHA_BIT_NAME] &&
+ strncmp(nla_data(tb[ETHA_BIT_NAME]), names[idx],
+ nla_len(tb[ETHA_BIT_NAME]))) {
+ GENL_SET_ERR_MSG(info, "bit index and name mismatch");
+ return genl_err_attr(info, -EINVAL, bit_attr);
+ }
+ } else if (tb[ETHA_BIT_NAME]) {
+ idx = ethnl_name_to_idx(names, nbits,
+ nla_data(tb[ETHA_BIT_NAME]),
+ nla_len(tb[ETHA_BIT_NAME]));
+ if (idx >= nbits) {
+ GENL_SET_ERR_MSG(info, "bit index too high");
+ return genl_err_attr(info, -EOPNOTSUPP,
+ tb[ETHA_BIT_NAME]);
+ }
+ } else {
+ GENL_SET_ERR_MSG(info, "neither bit index nor name specified");
+ return genl_err_attr(info, -EINVAL, bit_attr);
+ }
+
+ if (tb[ETHA_BIT_VALUE])
+ set_bit(idx, bitmap);
+ else
+ clear_bit(idx, bitmap);
+ return 0;
+}
+
+static bool ethnl_update_bitset(unsigned long *bitmap, unsigned int nbits,
+ struct nlattr *attr, int *err,
+ const char *const *names,
+ struct genl_info *info)
+{
+ struct nlattr *tb[ETHA_BITSET_MAX + 1];
+ unsigned int change_bits = 0;
+ bool mod = false;
+
+ if (!attr)
+ return false;
+ *err = nla_parse_nested(tb, ETHA_BITSET_MAX, attr, bitset_policy,
+ info->extack);
+ if (*err < 0)
+ return mod;
+
+ /* To let new userspace to work with old kernel, we allow bitmaps
+ * from userspace to be longer than kernel ones and only issue an
+ * error if userspace actually tries to change a bit not existing
+ * in kernel.
+ */
+ if (tb[ETHA_BITSET_SIZE])
+ change_bits = nla_get_u32(tb[ETHA_BITSET_SIZE]);
+
+ if (tb[ETHA_BITSET_BITS]) {
+ DECLARE_BITMAP(val, nbits);
+ struct nlattr *bit_attr;
+ int rem;
+
+ *err = -EINVAL;
+ if (tb[ETHA_BITSET_VALUE] || tb[ETHA_BITSET_MASK])
+ return mod;
+ bitmap_copy(val, bitmap, __ETHTOOL_LINK_MODE_MASK_NBITS);
+ nla_for_each_nested(bit_attr, tb[ETHA_BITSET_BITS], rem) {
+ *err = ethnl_update_bit(val, nbits, bit_attr, names,
+ info);
+ if (*err < 0)
+ return mod;
+ }
+ mod = !bitmap_equal(val, bitmap, nbits);
+ if (mod)
+ bitmap_copy(bitmap, val, nbits);
+ } else {
+ const unsigned int max_bits =
+ max_t(unsigned int, nbits, change_bits);
+ unsigned int nwords = (change_bits + 31) / 32;
+ DECLARE_BITMAP(mask, max_bits);
+ DECLARE_BITMAP(val, max_bits);
+
+ *err = -EINVAL;
+ if (!change_bits || !tb[ETHA_BITSET_VALUE])
+ return mod;
+ if (nla_len(tb[ETHA_BITSET_VALUE]) < nwords * sizeof(u32))
+ return mod;
+ if (tb[ETHA_BITSET_MASK] &&
+ nla_len(tb[ETHA_BITSET_VALUE]) < nwords * sizeof(u32))
+ return mod;
+
+ bitmap_zero(val, max_bits);
+ bitmap_from_u32array(val, change_bits,
+ nla_data(tb[ETHA_BITSET_VALUE]), nwords);
+ bitmap_zero(mask, max_bits);
+ if (tb[ETHA_BITSET_MASK])
+ bitmap_from_u32array(mask, change_bits,
+ nla_data(tb[ETHA_BITSET_MASK]),
+ nwords);
+ else
+ bitmap_fill(mask, change_bits);
+
+ if (nbits < change_bits) {
+ unsigned int idx = find_next_bit(mask, max_bits, nbits);
+
+ *err = -EINVAL;
+ if (idx >= nbits)
+ return mod;
+ }
+
+ bitmap_and(val, val, mask, nbits);
+ bitmap_complement(mask, mask, nbits);
+ bitmap_and(mask, mask, bitmap, nbits);
+ bitmap_or(val, val, mask, nbits);
+ mod = !bitmap_equal(val, bitmap, nbits);
+ if (mod)
+ bitmap_copy(bitmap, val, nbits);
+ }
+
+ *err = 0;
+ return mod;
+}
+
+static bool ethnl_update_bitset32(u32 *bitmap, unsigned int nbits,
+ struct nlattr *attr, int *err,
+ const char *const *names,
+ struct genl_info *info)
+{
+ unsigned int nwords = (nbits + 31) / 32;
+ DECLARE_BITMAP(tmp, nbits);
+ bool mod;
+
+ bitmap_from_u32array(tmp, nbits, bitmap, nwords);
+ mod = ethnl_update_bitset(tmp, nbits, attr, err, names, info);
+ if (!*err && mod)
+ bitmap_to_u32array(bitmap, nwords, tmp, nbits);
+ return (!*err && mod);
+}
+
static void warn_partial_info(struct genl_info *info)
{
GENL_SET_ERR_MSG(info, "not all requested data could be retrieved");
--
2.15.1

2017-12-11 13:54:10

by Michal Kubecek

[permalink] [raw]
Subject: [RFC PATCH 5/9] ethtool: implement GET_DRVINFO message

Request the same information as ETHTOOL_GDRVINFO. This is read-only so that
corresponding SET_DRVINFO exists but is only used in kernel replies. Rip
the code to query the driver out of the legacy interface and move it to
a new file ethtool_common.c so that both interfaces can use it.

Signed-off-by: Michal Kubecek <[email protected]>
---
Documentation/networking/ethtool-netlink.txt | 30 ++++++++++-
include/uapi/linux/ethtool_netlink.h | 21 ++++++++
net/core/Makefile | 2 +-
net/core/ethtool.c | 42 +++-------------
net/core/ethtool_common.c | 46 +++++++++++++++++
net/core/ethtool_common.h | 11 ++++
net/core/ethtool_netlink.c | 75 ++++++++++++++++++++++++++++
7 files changed, 189 insertions(+), 38 deletions(-)
create mode 100644 net/core/ethtool_common.c
create mode 100644 net/core/ethtool_common.h

diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
index 893e5156f6a7..cb992180b211 100644
--- a/Documentation/networking/ethtool-netlink.txt
+++ b/Documentation/networking/ethtool-netlink.txt
@@ -107,6 +107,9 @@ which the request applies.
List of message types
---------------------

+ ETHTOOL_CMD_GET_DRVINFO
+ ETHTOOL_CMD_SET_DRVINFO response only
+
All constants use ETHTOOL_CMD_ prefix followed by "GET", "SET" or "ACT" to
indicate the type.

@@ -119,6 +122,31 @@ messages marked as "response only" in the table above.
Later sections describe the format and semantics of these request messages.


+GET_DRVINFO
+-----------
+
+GET_DRVINFO request corresponds to ETHTOOL_GDRVINFO ioctl command and provides
+basic driver information. The request doesn't use any attributes and flags,
+info_mask and index field in request header are ignored. Kernel response
+contents:
+
+ ETHA_DRVINFO_DRIVER (string) driver name
+ ETHA_DRVINFO_VERSION (string) driver version
+ ETHA_DRVINFO_FWVERSION (string) firmware version
+ ETHA_DRVINFO_BUSINFO (string) device bus address
+ ETHA_DRVINFO_EROM_VER (string) expansion ROM version
+ ETHA_DRVINFO_N_PRIV_FLAGS (u32) number of private flags
+ ETHA_DRVINFO_N_STATS (u32) number of device stats
+ ETHA_DRVINFO_TESTINFO_LEN (u32) number of test results
+ ETHA_DRVINFO_EEDUMP_LEN (u32) EEPROM dump size
+ ETHA_DRVINFO_REGDUMP_LEN (u32) register dump size
+
+The meaning of these follows the corresponding fields of ETHTOOL_GDRVINFO
+response.
+
+All information is read only, SET_DRVINFO request is not implemented.
+
+
Request translation
-------------------

@@ -130,7 +158,7 @@ ioctl command netlink command
---------------------------------------------------------------------
ETHTOOL_GSET n/a
ETHTOOL_SSET n/a
-ETHTOOL_GDRVINFO n/a
+ETHTOOL_GDRVINFO ETHTOOL_CMD_GET_DRVINFO
ETHTOOL_GREGS n/a
ETHTOOL_GWOL n/a
ETHTOOL_SWOL n/a
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 6db35f00ea32..d6ab1d73d494 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -21,6 +21,8 @@ struct ethnlmsghdr {

enum {
ETHTOOL_CMD_NOOP,
+ ETHTOOL_CMD_GET_DRVINFO,
+ ETHTOOL_CMD_SET_DRVINFO, /* only for reply */

__ETHTOOL_CMD_MAX,
ETHTOOL_CMD_MAX = (__ETHTOOL_CMD_MAX - 1),
@@ -57,6 +59,25 @@ enum {
ETHA_BITSET_MAX = (__ETHA_BITSET_MAX - 1),
};

+/* GET_DRVINFO / SET_DRVINFO */
+
+enum {
+ ETHA_DRVINFO_UNSPEC,
+ ETHA_DRVINFO_DRIVER, /* string */
+ ETHA_DRVINFO_VERSION, /* string */
+ ETHA_DRVINFO_FWVERSION, /* string */
+ ETHA_DRVINFO_BUSINFO, /* string */
+ ETHA_DRVINFO_EROM_VER, /* string */
+ ETHA_DRVINFO_N_PRIV_FLAGS, /* u32 */
+ ETHA_DRVINFO_N_STATS, /* u32 */
+ ETHA_DRVINFO_TESTINFO_LEN, /* u32 */
+ ETHA_DRVINFO_EEDUMP_LEN, /* u32 */
+ ETHA_DRVINFO_REGDUMP_LEN, /* u32 */
+
+ __ETHA_DRVINFO_MAX,
+ ETHA_DRVINFO_MAX = (__ETHA_DRVINFO_MAX - 1),
+};
+
/* generic netlink info */
#define ETHTOOL_GENL_NAME "ethtool"
#define ETHTOOL_GENL_VERSION 1
diff --git a/net/core/Makefile b/net/core/Makefile
index 617ab2abecdf..3196641c0e70 100644
--- a/net/core/Makefile
+++ b/net/core/Makefile
@@ -11,7 +11,7 @@ obj-$(CONFIG_SYSCTL) += sysctl_net_core.o
obj-y += dev.o ethtool.o dev_addr_lists.o dst.o netevent.o \
neighbour.o rtnetlink.o utils.o link_watch.o filter.o \
sock_diag.o dev_ioctl.o tso.o sock_reuseport.o \
- fib_notifier.o
+ fib_notifier.o ethtool_common.o

obj-y += net-sysfs.o
obj-$(CONFIG_PROC_FS) += net-procfs.o
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f8fcf450a36e..09e780a748f9 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -26,6 +26,7 @@
#include <linux/rtnetlink.h>
#include <linux/sched/signal.h>
#include <linux/net.h>
+#include "ethtool_common.h"

/*
* Some useful ethtool_ops methods that're device independent.
@@ -885,45 +886,14 @@ static noinline_for_stack int ethtool_get_drvinfo(struct net_device *dev,
void __user *useraddr)
{
struct ethtool_drvinfo info;
- const struct ethtool_ops *ops = dev->ethtool_ops;
-
- memset(&info, 0, sizeof(info));
- info.cmd = ETHTOOL_GDRVINFO;
- if (ops->get_drvinfo) {
- ops->get_drvinfo(dev, &info);
- } else if (dev->dev.parent && dev->dev.parent->driver) {
- strlcpy(info.bus_info, dev_name(dev->dev.parent),
- sizeof(info.bus_info));
- strlcpy(info.driver, dev->dev.parent->driver->name,
- sizeof(info.driver));
- } else {
- return -EOPNOTSUPP;
- }
-
- /*
- * this method of obtaining string set info is deprecated;
- * Use ETHTOOL_GSSET_INFO instead.
- */
- if (ops->get_sset_count) {
- int rc;
-
- rc = ops->get_sset_count(dev, ETH_SS_TEST);
- if (rc >= 0)
- info.testinfo_len = rc;
- rc = ops->get_sset_count(dev, ETH_SS_STATS);
- if (rc >= 0)
- info.n_stats = rc;
- rc = ops->get_sset_count(dev, ETH_SS_PRIV_FLAGS);
- if (rc >= 0)
- info.n_priv_flags = rc;
- }
- if (ops->get_regs_len)
- info.regdump_len = ops->get_regs_len(dev);
- if (ops->get_eeprom_len)
- info.eedump_len = ops->get_eeprom_len(dev);
+ int rc;

+ rc = __ethtool_get_drvinfo(dev, &info);
+ if (rc < 0)
+ return rc;
if (copy_to_user(useraddr, &info, sizeof(info)))
return -EFAULT;
+
return 0;
}

diff --git a/net/core/ethtool_common.c b/net/core/ethtool_common.c
new file mode 100644
index 000000000000..2c0abab0e43c
--- /dev/null
+++ b/net/core/ethtool_common.c
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+#include <linux/rtnetlink.h>
+#include "ethtool_common.h"
+
+int __ethtool_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
+{
+ const struct ethtool_ops *ops = dev->ethtool_ops;
+
+ memset(info, 0, sizeof(*info));
+ info->cmd = ETHTOOL_GDRVINFO;
+ if (ops->get_drvinfo) {
+ ops->get_drvinfo(dev, info);
+ } else if (dev->dev.parent && dev->dev.parent->driver) {
+ strlcpy(info->bus_info, dev_name(dev->dev.parent),
+ sizeof(info->bus_info));
+ strlcpy(info->driver, dev->dev.parent->driver->name,
+ sizeof(info->driver));
+ } else {
+ return -EOPNOTSUPP;
+ }
+
+ /* this method of obtaining string set info is deprecated;
+ * Use ETHTOOL_GSSET_INFO instead.
+ */
+ if (ops->get_sset_count) {
+ int rc;
+
+ rc = ops->get_sset_count(dev, ETH_SS_TEST);
+ if (rc >= 0)
+ info->testinfo_len = rc;
+ rc = ops->get_sset_count(dev, ETH_SS_STATS);
+ if (rc >= 0)
+ info->n_stats = rc;
+ rc = ops->get_sset_count(dev, ETH_SS_PRIV_FLAGS);
+ if (rc >= 0)
+ info->n_priv_flags = rc;
+ }
+ if (ops->get_regs_len)
+ info->regdump_len = ops->get_regs_len(dev);
+ if (ops->get_eeprom_len)
+ info->eedump_len = ops->get_eeprom_len(dev);
+
+ return 0;
+}
+EXPORT_SYMBOL(__ethtool_get_drvinfo);
diff --git a/net/core/ethtool_common.h b/net/core/ethtool_common.h
new file mode 100644
index 000000000000..1f031c1d943a
--- /dev/null
+++ b/net/core/ethtool_common.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+
+#ifndef _ETHTOOL_COMMON_H
+#define _ETHTOOL_COMMON_H
+
+#include <linux/netdevice.h>
+#include <linux/ethtool.h>
+
+int __ethtool_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info);
+
+#endif /* _ETHTOOL_COMMON_H */
diff --git a/net/core/ethtool_netlink.c b/net/core/ethtool_netlink.c
index 9f287e67f30b..077814fd36bd 100644
--- a/net/core/ethtool_netlink.c
+++ b/net/core/ethtool_netlink.c
@@ -531,9 +531,84 @@ static struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev,
return rskb;
}

+/* GET_DRVINFO */
+
+static int ethnl_drvinfo_size(struct ethtool_drvinfo *drvinfo)
+{
+ int len = 0;
+
+ len += ethnl_str_ifne_size(drvinfo->driver);
+ len += ethnl_str_ifne_size(drvinfo->version);
+ len += ethnl_str_ifne_size(drvinfo->fw_version);
+ len += ethnl_str_ifne_size(drvinfo->bus_info);
+ len += ethnl_str_ifne_size(drvinfo->erom_version);
+ /* n_priv_flags, n_stats, testinfo_len, eedump_len, regdump_len */
+ len += 5 * nla_total_size(sizeof(u32));
+
+ return len;
+}
+
+static int ethnl_get_drvinfo(struct sk_buff *skb, struct genl_info *info)
+{
+ struct ethnlmsghdr *ehdr;
+ struct ethtool_drvinfo drvinfo = {};
+ struct net_device *dev;
+ struct sk_buff *rskb;
+ int reply_len;
+ int rc = 0;
+
+ dev = ethnl_dev_get(info);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+ rc = __ethtool_get_drvinfo(dev, &drvinfo);
+ if (rc < 0) {
+ dev_put(dev);
+ GENL_SET_ERR_MSG(info,
+ "failed to retrieve driver info from driver");
+ return rc;
+ }
+
+ reply_len = ethnl_drvinfo_size(&drvinfo);
+ rskb = ethnl_reply_init(reply_len, dev, ETHTOOL_CMD_SET_DRVINFO, info,
+ &ehdr);
+ dev_put(dev);
+ if (!rskb)
+ return -ENOMEM;
+
+ if (ethnl_put_str_ifne(rskb, ETHA_DRVINFO_DRIVER, drvinfo.driver) ||
+ ethnl_put_str_ifne(rskb, ETHA_DRVINFO_VERSION, drvinfo.version) ||
+ ethnl_put_str_ifne(rskb, ETHA_DRVINFO_FWVERSION,
+ drvinfo.fw_version) ||
+ ethnl_put_str_ifne(rskb, ETHA_DRVINFO_BUSINFO, drvinfo.bus_info) ||
+ ethnl_put_str_ifne(rskb, ETHA_DRVINFO_EROM_VER,
+ drvinfo.erom_version) ||
+ nla_put_u32(rskb, ETHA_DRVINFO_N_PRIV_FLAGS,
+ drvinfo.n_priv_flags) ||
+ nla_put_u32(rskb, ETHA_DRVINFO_N_STATS, drvinfo.n_stats) ||
+ nla_put_u32(rskb, ETHA_DRVINFO_TESTINFO_LEN,
+ drvinfo.testinfo_len) ||
+ nla_put_u32(rskb, ETHA_DRVINFO_EEDUMP_LEN, drvinfo.eedump_len) ||
+ nla_put_u32(rskb, ETHA_DRVINFO_REGDUMP_LEN, drvinfo.regdump_len))
+ goto err;
+
+ genlmsg_end(rskb, ehdr);
+ return genlmsg_reply(rskb, info);
+
+err:
+ nlmsg_free(rskb);
+ GENL_SET_ERR_MSG(info, "kernel error, see kernel log for details");
+ WARN_ONCE(1, "calculated message payload length (%d) not sufficient\n",
+ reply_len);
+ return -EMSGSIZE;
+}
+
/* genetlink paperwork */

static const struct genl_ops ethtool_genl_ops[] = {
+ {
+ .cmd = ETHTOOL_CMD_GET_DRVINFO,
+ .doit = ethnl_get_drvinfo,
+ },
};

static struct genl_family ethtool_genl_family = {
--
2.15.1

2017-12-11 13:54:19

by Michal Kubecek

[permalink] [raw]
Subject: [RFC PATCH 6/9] ethtool: implement GET_SETTINGS message

Requests the information provided by ETHTOOL_GLINKSETTINGS, ETHTOOL_GWOL
and ETHTOOL_GMSGLVL. The info_mask header field can be used to request only
part of the information. Flag ETH_SETTINGS_RF_COMPACT_BITSETS switches
between flag-by-flag list and compact bitmaps for link modes in the reply.

Signed-off-by: Michal Kubecek <[email protected]>
---
Documentation/networking/ethtool-netlink.txt | 62 +++++-
include/linux/ethtool_netlink.h | 3 +
include/linux/netdevice.h | 2 +
include/uapi/linux/ethtool.h | 3 +
include/uapi/linux/ethtool_netlink.h | 36 ++++
net/core/ethtool.c | 108 +---------
net/core/ethtool_common.c | 112 ++++++++++
net/core/ethtool_common.h | 8 +
net/core/ethtool_netlink.c | 299 +++++++++++++++++++++++++++
9 files changed, 528 insertions(+), 105 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
index cb992180b211..7aabc87c9f09 100644
--- a/Documentation/networking/ethtool-netlink.txt
+++ b/Documentation/networking/ethtool-netlink.txt
@@ -109,6 +109,8 @@ List of message types

ETHTOOL_CMD_GET_DRVINFO
ETHTOOL_CMD_SET_DRVINFO response only
+ ETHTOOL_CMD_GET_SETTINGS
+ ETHTOOL_CMD_SET_SETTINGS response only (for now)

All constants use ETHTOOL_CMD_ prefix followed by "GET", "SET" or "ACT" to
indicate the type.
@@ -147,6 +149,56 @@ response.
All information is read only, SET_DRVINFO request is not implemented.


+GET_SETTINGS
+------------
+
+GET_SETTINGS request retrieves information provided by ETHTOOL_GLINKSETTINGS,
+ETHTOOL_GWOL, ETHTOOL_GMSGLVL and ETHTOOL_GLINK ioctl requests. The request
+doesn't use any attributes.
+
+Header flags:
+ ETH_SETTINGS_RF_COMPACT_BITSETS bitset form in response (1 is compact)
+
+Header info_mask bits:
+ ETH_SETTINGS_IM_LINKINFO link_ksettings except link modes
+ ETH_SETTINGS_IM_LINKMODES link modes from link_ksettings
+ ETH_SETTINGS_IM_MSGLEVEL msglevel
+ ETH_SETTINGS_IM_WOLINFO struct ethtool_wolinfo
+ ETH_SETTINGS_IM_LINK link state
+
+Zero info_mask
+
+Response contents:
+
+ ETHA_SETTINGS_SPEED (u32) link speed (Mb/s)
+ ETHA_SETTINGS_DUPLEX (u8) duplex mode
+ ETHA_SETTINGS_PORT (u8) physical port
+ ETHA_SETTINGS_PHYADDR (u8) MDIO address of phy
+ ETHA_SETTINGS_AUTONEG (u8) autoneotiation status
+ ETHA_SETTINGS_MDIO_SUPPORT (bitfield32) MDIO support flags
+ ETHA_SETTINGS_TP_MDIX (u8) MDI(-X) status
+ ETHA_SETTINGS_TP_MDIX_CTRL (u8) MDI(-X) control
+ ETHA_SETTINGS_TRANSCEIVER (u8) transceiver
+ ETHA_SETTINGS_WOL_MODES (bitfield32) wake-on-lan modes
+ ETHA_SETTINGS_SOPASS (binary) SecureOn(tm) password
+ ETHA_SETTINGS_MSGLVL (bitfield32) debug level
+ ETHA_SETTINGS_LINK_MODES (bitset) device link modes
+ ETHA_SETTINGS_PEER_MODES (bitset) link partner link modes
+ ETHA_SETTINGS_LINK (u32) link state
+
+Most of the attributes have the same meaning (including values) as
+corresponding members of ioctl structures. For ETHA_SETTINGS_MDIO_SUPPORT and
+ETHA_SETTINGS_MSGLVL, selector reports flags supported by kernel. For
+ETHA_SETTINGS_WOL_MODES it reports flags supported by the device. For
+ETHA_SETTINGS_LINK_MODES, value represent advertised modes and mask represents
+supported modes. For ETHA_SETTINGS_PEER_MODES, both value and mask represent
+partner advertised link modes.
+
+GET_SETTINGS request is allowed for unprivileged user but ETHA_SETTINGS_SOPASS
+is only provided by kernel in response to privileged (netns CAP_NET_ADMIN)
+requests.
+
+
Request translation
-------------------

@@ -156,16 +208,16 @@ have their netlink replacement yet.

ioctl command netlink command
---------------------------------------------------------------------
-ETHTOOL_GSET n/a
+ETHTOOL_GSET ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_SSET n/a
ETHTOOL_GDRVINFO ETHTOOL_CMD_GET_DRVINFO
ETHTOOL_GREGS n/a
-ETHTOOL_GWOL n/a
+ETHTOOL_GWOL ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_SWOL n/a
-ETHTOOL_GMSGLVL n/a
+ETHTOOL_GMSGLVL ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_SMSGLVL n/a
ETHTOOL_NWAY_RST n/a
-ETHTOOL_GLINK n/a
+ETHTOOL_GLINK ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_GEEPROM n/a
ETHTOOL_SEEPROM n/a
ETHTOOL_GCOALESCE n/a
@@ -230,7 +282,7 @@ ETHTOOL_GTUNABLE n/a
ETHTOOL_STUNABLE n/a
ETHTOOL_GPHYSTATS n/a
ETHTOOL_PERQUEUE n/a
-ETHTOOL_GLINKSETTINGS n/a
+ETHTOOL_GLINKSETTINGS ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_SLINKSETTINGS n/a
ETHTOOL_PHY_GTUNABLE n/a
ETHTOOL_PHY_STUNABLE n/a
diff --git a/include/linux/ethtool_netlink.h b/include/linux/ethtool_netlink.h
index 0412adb4f42f..1787359f9e5d 100644
--- a/include/linux/ethtool_netlink.h
+++ b/include/linux/ethtool_netlink.h
@@ -6,4 +6,7 @@
#include <uapi/linux/ethtool_netlink.h>
#include <linux/ethtool.h>

+#define __ETHTOOL_LINK_MODE_MASK_NWORDS \
+ ((__ETHTOOL_LINK_MODE_MASK_NBITS + 31) / 32)
+
#endif /* _LINUX_ETHTOOL_NETLINK_H_ */
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index cc4ce7456e38..0e1d0a04c3cc 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3506,6 +3506,8 @@ enum {
NETIF_MSG_PKTDATA = 0x1000,
NETIF_MSG_HW = 0x2000,
NETIF_MSG_WOL = 0x4000,
+
+ NETIF_MSG_ALL = 0x7fff,
};

#define netif_msg_drv(p) ((p)->msg_enable & NETIF_MSG_DRV)
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index 44a0b675a6bc..a9076a76cdb4 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -143,6 +143,9 @@ static inline __u32 ethtool_cmd_speed(const struct ethtool_cmd *ep)
*/
#define ETH_MDIO_SUPPORTS_C45 2

+/* All defined ETH_MDIO_SUPPORTS_* flags */
+#define ETH_MDIO_SUPPORTS_ALL (ETH_MDIO_SUPPORTS_C22 | ETH_MDIO_SUPPORTS_C45)
+
#define ETHTOOL_FWVERS_LEN 32
#define ETHTOOL_BUSINFO_LEN 32
#define ETHTOOL_EROMVERS_LEN 32
diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index d6ab1d73d494..9520d13fc9ab 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -23,6 +23,8 @@ enum {
ETHTOOL_CMD_NOOP,
ETHTOOL_CMD_GET_DRVINFO,
ETHTOOL_CMD_SET_DRVINFO, /* only for reply */
+ ETHTOOL_CMD_GET_SETTINGS,
+ ETHTOOL_CMD_SET_SETTINGS,

__ETHTOOL_CMD_MAX,
ETHTOOL_CMD_MAX = (__ETHTOOL_CMD_MAX - 1),
@@ -78,6 +80,40 @@ enum {
ETHA_DRVINFO_MAX = (__ETHA_DRVINFO_MAX - 1),
};

+/* GET_SETTINGS / SET_SETTINGS */
+
+enum {
+ ETHA_SETTINGS_UNSPEC,
+ ETHA_SETTINGS_SPEED, /* u32 */
+ ETHA_SETTINGS_DUPLEX, /* u8 */
+ ETHA_SETTINGS_PORT, /* u8 */
+ ETHA_SETTINGS_PHYADDR, /* u8 */
+ ETHA_SETTINGS_AUTONEG, /* u8 */
+ ETHA_SETTINGS_MDIO_SUPPORT, /* bitfield32 */
+ ETHA_SETTINGS_TP_MDIX, /* u8 */
+ ETHA_SETTINGS_TP_MDIX_CTRL, /* u8 */
+ ETHA_SETTINGS_TRANSCEIVER, /* u8 */
+ ETHA_SETTINGS_WOL_MODES, /* bitfield32 */
+ ETHA_SETTINGS_SOPASS, /* binary */
+ ETHA_SETTINGS_MSGLVL, /* bitfield32 */
+ ETHA_SETTINGS_LINK_MODES, /* bitset */
+ ETHA_SETTINGS_PEER_MODES, /* bitset */
+ ETHA_SETTINGS_LINK, /* u32 */
+
+ __ETHA_SETTINGS_MAX,
+ ETHA_SETTINGS_MAX = (__ETHA_SETTINGS_MAX - 1),
+};
+
+#define ETH_SETTINGS_RF_COMPACT_BITSETS 0x1
+
+#define ETH_SETTINGS_IM_LINKINFO 0x01
+#define ETH_SETTINGS_IM_LINKMODES 0x02
+#define ETH_SETTINGS_IM_MSGLEVEL 0x04
+#define ETH_SETTINGS_IM_WOLINFO 0x08
+#define ETH_SETTINGS_IM_LINK 0x10
+
+#define ETH_SETTINGS_IM_DEFAULT 0x1f
+
/* generic netlink info */
#define ETHTOOL_GENL_NAME "ethtool"
#define ETHTOOL_GENL_VERSION 1
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 09e780a748f9..b08a2efa2e89 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -452,54 +452,6 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
}
EXPORT_SYMBOL(ethtool_convert_link_mode_to_legacy_u32);

-/* return false if legacy contained non-0 deprecated fields
- * maxtxpkt/maxrxpkt. rest of ksettings always updated
- */
-static bool
-convert_legacy_settings_to_link_ksettings(
- struct ethtool_link_ksettings *link_ksettings,
- const struct ethtool_cmd *legacy_settings)
-{
- bool retval = true;
-
- memset(link_ksettings, 0, sizeof(*link_ksettings));
-
- /* This is used to tell users that driver is still using these
- * deprecated legacy fields, and they should not use
- * %ETHTOOL_GLINKSETTINGS/%ETHTOOL_SLINKSETTINGS
- */
- if (legacy_settings->maxtxpkt ||
- legacy_settings->maxrxpkt)
- retval = false;
-
- ethtool_convert_legacy_u32_to_link_mode(
- link_ksettings->link_modes.supported,
- legacy_settings->supported);
- ethtool_convert_legacy_u32_to_link_mode(
- link_ksettings->link_modes.advertising,
- legacy_settings->advertising);
- ethtool_convert_legacy_u32_to_link_mode(
- link_ksettings->link_modes.lp_advertising,
- legacy_settings->lp_advertising);
- link_ksettings->base.speed
- = ethtool_cmd_speed(legacy_settings);
- link_ksettings->base.duplex
- = legacy_settings->duplex;
- link_ksettings->base.port
- = legacy_settings->port;
- link_ksettings->base.phy_address
- = legacy_settings->phy_address;
- link_ksettings->base.autoneg
- = legacy_settings->autoneg;
- link_ksettings->base.mdio_support
- = legacy_settings->mdio_support;
- link_ksettings->base.eth_tp_mdix
- = legacy_settings->eth_tp_mdix;
- link_ksettings->base.eth_tp_mdix_ctrl
- = legacy_settings->eth_tp_mdix_ctrl;
- return retval;
-}
-
/* return false if ksettings link modes had higher bits
* set. legacy_settings always updated (best effort)
*/
@@ -560,50 +512,6 @@ struct ethtool_link_usettings {
} link_modes;
};

-/* Internal kernel helper to query a device ethtool_link_settings.
- *
- * Backward compatibility note: for compatibility with legacy drivers
- * that implement only the ethtool_cmd API, this has to work with both
- * drivers implementing get_link_ksettings API and drivers
- * implementing get_settings API. When drivers implement get_settings
- * and report ethtool_cmd deprecated fields
- * (transceiver/maxrxpkt/maxtxpkt), these fields are silently ignored
- * because the resulting struct ethtool_link_settings does not report them.
- */
-int __ethtool_get_link_ksettings(struct net_device *dev,
- struct ethtool_link_ksettings *link_ksettings)
-{
- int err;
- struct ethtool_cmd cmd;
-
- ASSERT_RTNL();
-
- if (dev->ethtool_ops->get_link_ksettings) {
- memset(link_ksettings, 0, sizeof(*link_ksettings));
- return dev->ethtool_ops->get_link_ksettings(dev,
- link_ksettings);
- }
-
- /* driver doesn't support %ethtool_link_ksettings API. revert to
- * legacy %ethtool_cmd API, unless it's not supported either.
- * TODO: remove when ethtool_ops::get_settings disappears internally
- */
- if (!dev->ethtool_ops->get_settings)
- return -EOPNOTSUPP;
-
- memset(&cmd, 0, sizeof(cmd));
- cmd.cmd = ETHTOOL_GSET;
- err = dev->ethtool_ops->get_settings(dev, &cmd);
- if (err < 0)
- return err;
-
- /* we ignore deprecated fields transceiver/maxrxpkt/maxtxpkt
- */
- convert_legacy_settings_to_link_ksettings(link_ksettings, &cmd);
- return err;
-}
-EXPORT_SYMBOL(__ethtool_get_link_ksettings);
-
/* convert ethtool_link_usettings in user space to a kernel internal
* ethtool_link_ksettings. return 0 on success, errno on error.
*/
@@ -1437,11 +1345,11 @@ static int ethtool_reset(struct net_device *dev, char __user *useraddr)
static int ethtool_get_wol(struct net_device *dev, char __user *useraddr)
{
struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
+ int rc;

- if (!dev->ethtool_ops->get_wol)
- return -EOPNOTSUPP;
-
- dev->ethtool_ops->get_wol(dev, &wol);
+ rc = __ethtool_get_wol(dev, &wol);
+ if (rc < 0)
+ return rc;

if (copy_to_user(useraddr, &wol, sizeof(wol)))
return -EFAULT;
@@ -1506,12 +1414,12 @@ static int ethtool_nway_reset(struct net_device *dev)
static int ethtool_get_link(struct net_device *dev, char __user *useraddr)
{
struct ethtool_value edata = { .cmd = ETHTOOL_GLINK };
+ int link = __ethtool_get_link(dev);

- if (!dev->ethtool_ops->get_link)
- return -EOPNOTSUPP;
-
- edata.data = netif_running(dev) && dev->ethtool_ops->get_link(dev);
+ if (link < 0)
+ return link;

+ edata.data = link;
if (copy_to_user(useraddr, &edata, sizeof(edata)))
return -EFAULT;
return 0;
diff --git a/net/core/ethtool_common.c b/net/core/ethtool_common.c
index 2c0abab0e43c..30bc2b14cf2a 100644
--- a/net/core/ethtool_common.c
+++ b/net/core/ethtool_common.c
@@ -44,3 +44,115 @@ int __ethtool_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
return 0;
}
EXPORT_SYMBOL(__ethtool_get_drvinfo);
+
+/* return false if legacy contained non-0 deprecated fields
+ * maxtxpkt/maxrxpkt. rest of ksettings always updated
+ */
+bool
+convert_legacy_settings_to_link_ksettings(
+ struct ethtool_link_ksettings *link_ksettings,
+ const struct ethtool_cmd *legacy_settings)
+{
+ bool retval = true;
+
+ memset(link_ksettings, 0, sizeof(*link_ksettings));
+
+ /* This is used to tell users that driver is still using these
+ * deprecated legacy fields, and they should not use
+ * %ETHTOOL_GLINKSETTINGS/%ETHTOOL_SLINKSETTINGS
+ */
+ if (legacy_settings->maxtxpkt ||
+ legacy_settings->maxrxpkt)
+ retval = false;
+
+ ethtool_convert_legacy_u32_to_link_mode(
+ link_ksettings->link_modes.supported,
+ legacy_settings->supported);
+ ethtool_convert_legacy_u32_to_link_mode(
+ link_ksettings->link_modes.advertising,
+ legacy_settings->advertising);
+ ethtool_convert_legacy_u32_to_link_mode(
+ link_ksettings->link_modes.lp_advertising,
+ legacy_settings->lp_advertising);
+ link_ksettings->base.speed
+ = ethtool_cmd_speed(legacy_settings);
+ link_ksettings->base.duplex
+ = legacy_settings->duplex;
+ link_ksettings->base.port
+ = legacy_settings->port;
+ link_ksettings->base.phy_address
+ = legacy_settings->phy_address;
+ link_ksettings->base.autoneg
+ = legacy_settings->autoneg;
+ link_ksettings->base.mdio_support
+ = legacy_settings->mdio_support;
+ link_ksettings->base.eth_tp_mdix
+ = legacy_settings->eth_tp_mdix;
+ link_ksettings->base.eth_tp_mdix_ctrl
+ = legacy_settings->eth_tp_mdix_ctrl;
+ return retval;
+}
+
+/* Internal kernel helper to query a device ethtool_link_settings.
+ *
+ * Backward compatibility note: for compatibility with legacy drivers
+ * that implement only the ethtool_cmd API, this has to work with both
+ * drivers implementing get_link_ksettings API and drivers
+ * implementing get_settings API. When drivers implement get_settings
+ * and report ethtool_cmd deprecated fields
+ * (transceiver/maxrxpkt/maxtxpkt), these fields are silently ignored
+ * because the resulting struct ethtool_link_settings does not report them.
+ */
+int __ethtool_get_link_ksettings(struct net_device *dev,
+ struct ethtool_link_ksettings *link_ksettings)
+{
+ int err;
+ struct ethtool_cmd cmd;
+
+ ASSERT_RTNL();
+
+ if (dev->ethtool_ops->get_link_ksettings) {
+ memset(link_ksettings, 0, sizeof(*link_ksettings));
+ return dev->ethtool_ops->get_link_ksettings(dev,
+ link_ksettings);
+ }
+
+ /* driver doesn't support %ethtool_link_ksettings API. revert to
+ * legacy %ethtool_cmd API, unless it's not supported either.
+ * TODO: remove when ethtool_ops::get_settings disappears internally
+ */
+ if (!dev->ethtool_ops->get_settings)
+ return -EOPNOTSUPP;
+
+ memset(&cmd, 0, sizeof(cmd));
+ cmd.cmd = ETHTOOL_GSET;
+ err = dev->ethtool_ops->get_settings(dev, &cmd);
+ if (err < 0)
+ return err;
+
+ /* we ignore deprecated fields transceiver/maxrxpkt/maxtxpkt
+ */
+ convert_legacy_settings_to_link_ksettings(link_ksettings, &cmd);
+ return err;
+}
+EXPORT_SYMBOL(__ethtool_get_link_ksettings);
+
+int __ethtool_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol)
+{
+ if (!dev->ethtool_ops->get_wol)
+ return -EOPNOTSUPP;
+
+ dev->ethtool_ops->get_wol(dev, wol);
+
+ return 0;
+}
+EXPORT_SYMBOL(__ethtool_get_wol);
+
+int __ethtool_get_link(struct net_device *dev)
+{
+ if (!dev->ethtool_ops->get_link)
+ return -EOPNOTSUPP;
+
+ return netif_running(dev) && dev->ethtool_ops->get_link(dev);
+}
+EXPORT_SYMBOL(__ethtool_get_link);
diff --git a/net/core/ethtool_common.h b/net/core/ethtool_common.h
index 1f031c1d943a..92e236952f18 100644
--- a/net/core/ethtool_common.h
+++ b/net/core/ethtool_common.h
@@ -7,5 +7,13 @@
#include <linux/ethtool.h>

int __ethtool_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info);
+int __ethtool_get_wol(struct net_device *dev, struct ethtool_wolinfo *wol);
+int __ethtool_get_link_ksettings(struct net_device *dev,
+ struct ethtool_link_ksettings *link_ksettings);
+int __ethtool_get_link(struct net_device *dev);
+
+bool convert_legacy_settings_to_link_ksettings(
+ struct ethtool_link_ksettings *link_ksettings,
+ const struct ethtool_cmd *legacy_settings);

#endif /* _ETHTOOL_COMMON_H */
diff --git a/net/core/ethtool_netlink.c b/net/core/ethtool_netlink.c
index 077814fd36bd..0c2bed7850bc 100644
--- a/net/core/ethtool_netlink.c
+++ b/net/core/ethtool_netlink.c
@@ -4,11 +4,69 @@
#include <linux/ethtool_netlink.h>
#include <linux/netdevice.h>
#include <linux/bitmap.h>
+#include <linux/rtnetlink.h>
#include <net/genetlink.h>
#include "ethtool_common.h"

static struct genl_family ethtool_genl_family;

+/* dictionary */
+
+static const char *const link_mode_names[] = {
+ [ETHTOOL_LINK_MODE_10baseT_Half_BIT] = "10baseT/Half",
+ [ETHTOOL_LINK_MODE_10baseT_Full_BIT] = "10baseT/Full",
+ [ETHTOOL_LINK_MODE_100baseT_Half_BIT] = "100baseT/Half",
+ [ETHTOOL_LINK_MODE_100baseT_Full_BIT] = "100baseT/Full",
+ [ETHTOOL_LINK_MODE_1000baseT_Half_BIT] = "1000baseT/Half",
+ [ETHTOOL_LINK_MODE_1000baseT_Full_BIT] = "1000baseT/Full",
+ [ETHTOOL_LINK_MODE_Autoneg_BIT] = "Autoneg",
+ [ETHTOOL_LINK_MODE_TP_BIT] = "TP",
+ [ETHTOOL_LINK_MODE_AUI_BIT] = "AUI",
+ [ETHTOOL_LINK_MODE_MII_BIT] = "MII",
+ [ETHTOOL_LINK_MODE_FIBRE_BIT] = "FIBRE",
+ [ETHTOOL_LINK_MODE_BNC_BIT] = "BNC",
+ [ETHTOOL_LINK_MODE_10000baseT_Full_BIT] = "10000baseT/Full",
+ [ETHTOOL_LINK_MODE_Pause_BIT] = "Pause",
+ [ETHTOOL_LINK_MODE_Asym_Pause_BIT] = "Asym_Pause",
+ [ETHTOOL_LINK_MODE_2500baseX_Full_BIT] = "2500baseX/Full",
+ [ETHTOOL_LINK_MODE_Backplane_BIT] = "Backplane",
+ [ETHTOOL_LINK_MODE_1000baseKX_Full_BIT] = "1000baseKX/Full",
+ [ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT] = "10000baseKX4/Full",
+ [ETHTOOL_LINK_MODE_10000baseKR_Full_BIT] = "10000baseKR/Full",
+ [ETHTOOL_LINK_MODE_10000baseR_FEC_BIT] = "10000baseR/FEC",
+ [ETHTOOL_LINK_MODE_20000baseMLD2_Full_BIT] = "20000baseMLD2/Full",
+ [ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT] = "20000baseKR2/Full",
+ [ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT] = "40000baseKR4/Full",
+ [ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT] = "40000baseCR4/Full",
+ [ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT] = "40000baseSR4/Full",
+ [ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT] = "40000baseLR4/Full",
+ [ETHTOOL_LINK_MODE_56000baseKR4_Full_BIT] = "56000baseKR4/Full",
+ [ETHTOOL_LINK_MODE_56000baseCR4_Full_BIT] = "56000baseCR4/Full",
+ [ETHTOOL_LINK_MODE_56000baseSR4_Full_BIT] = "56000baseSR4/Full",
+ [ETHTOOL_LINK_MODE_56000baseLR4_Full_BIT] = "56000baseLR4/Full",
+ [ETHTOOL_LINK_MODE_25000baseCR_Full_BIT] = "25000baseCR/Full",
+ [ETHTOOL_LINK_MODE_25000baseKR_Full_BIT] = "25000baseKR/Full",
+ [ETHTOOL_LINK_MODE_25000baseSR_Full_BIT] = "25000baseSR/Full",
+ [ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT] = "50000baseCR2/Full",
+ [ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT] = "50000baseKR2/Full",
+ [ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT] = "100000baseKR4/Full",
+ [ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT] = "100000baseSR4/Full",
+ [ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT] = "100000baseCR4/Full",
+ [ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT] = "100000baseLR4/ER4_Full",
+ [ETHTOOL_LINK_MODE_50000baseSR2_Full_BIT] = "50000baseSR2/Full",
+ [ETHTOOL_LINK_MODE_1000baseX_Full_BIT] = "1000baseX/Full",
+ [ETHTOOL_LINK_MODE_10000baseCR_Full_BIT] = "10000baseCR/Full",
+ [ETHTOOL_LINK_MODE_10000baseSR_Full_BIT] = "10000baseSR/Full",
+ [ETHTOOL_LINK_MODE_10000baseLR_Full_BIT] = "10000baseLR/Full",
+ [ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT] = "10000baseLRM/Full",
+ [ETHTOOL_LINK_MODE_10000baseER_Full_BIT] = "10000baseER/Full",
+ [ETHTOOL_LINK_MODE_2500baseT_Full_BIT] = "2500baseT/Full",
+ [ETHTOOL_LINK_MODE_5000baseT_Full_BIT] = "5000baseT/Full",
+ [ETHTOOL_LINK_MODE_FEC_NONE_BIT] = "None",
+ [ETHTOOL_LINK_MODE_FEC_RS_BIT] = "RS",
+ [ETHTOOL_LINK_MODE_FEC_BASER_BIT] = "BASER",
+};
+
/* misc helper functions */

static int ethnl_str_size(const char *s)
@@ -602,6 +660,243 @@ static int ethnl_get_drvinfo(struct sk_buff *skb, struct genl_info *info)
return -EMSGSIZE;
}

+/* GET_SETTINGS */
+
+/* To keep things simple, reserve space for some attributes which may not
+ * be added to the message (e.g. ETHA_SETTINGS_SOPASS); therefore the length
+ * returned may be bigger than the actual length of the message sent
+ */
+static int ethnl_settings_size(struct ethnlmsghdr *ehdr,
+ struct ethtool_link_ksettings *ksettings,
+ struct net_device *dev, u16 req_mask)
+{
+ size_t len = 0;
+ int rc = 0;
+
+ if (req_mask & ETH_SETTINGS_IM_LINKINFO) {
+ /* speed */
+ len += nla_total_size(sizeof(u32));
+ /* duplex, autoneg, port, phyaddr, mdix, mdixctrl, transcvr */
+ len += 7 * nla_total_size(sizeof(u8));
+ /* mdio_support */
+ len += nla_total_size(sizeof(struct nla_bitfield32));
+ }
+ if (req_mask & ETH_SETTINGS_IM_LINKMODES) {
+ u32 *supported = (u32 *)ksettings->link_modes.supported;
+ u32 *advertising = (u32 *)ksettings->link_modes.advertising;
+ u32 *lp_advertising =
+ (u32 *)ksettings->link_modes.lp_advertising;
+ bool compact = ehdr->flags & ETH_SETTINGS_RF_COMPACT_BITSETS;
+
+ rc = ethnl_bitset_size(compact, __ETHTOOL_LINK_MODE_MASK_NBITS,
+ advertising, supported, link_mode_names);
+ if (rc < 0)
+ return rc;
+ len += rc;
+ rc = ethnl_bitset_size(compact, __ETHTOOL_LINK_MODE_MASK_NBITS,
+ lp_advertising, lp_advertising,
+ link_mode_names);
+ if (rc < 0)
+ return rc;
+ len += rc;
+ }
+ if (req_mask & ETH_SETTINGS_IM_MSGLEVEL)
+ len += nla_total_size(sizeof(struct nla_bitfield32));
+ if (req_mask & ETH_SETTINGS_IM_WOLINFO) {
+ /* wolopts / wol_supported */
+ len += nla_total_size(sizeof(struct nla_bitfield32));
+ /* sopass */
+ len += nla_total_size(SOPASS_MAX);
+ }
+ if (req_mask & ETH_SETTINGS_IM_LINK)
+ len += nla_total_size(sizeof(u32));
+
+ return len;
+}
+
+static int ethnl_get_link_ksettings(struct genl_info *info,
+ struct net_device *dev,
+ struct ethtool_link_ksettings *ksettings)
+{
+ int ret;
+
+ ret = __ethtool_get_link_ksettings(dev, ksettings);
+
+ if (ret < 0)
+ GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
+ return ret;
+}
+
+static int ethnl_get_wol(struct genl_info *info, struct net_device *dev,
+ struct ethtool_wolinfo *wolinfo)
+{
+ int ret = __ethtool_get_wol(dev, wolinfo);
+
+ if (ret < 0)
+ GENL_SET_ERR_MSG(info, "failed to retrieve wol info");
+ return ret;
+}
+
+static int ethnl_get_settings(struct sk_buff *skb, struct genl_info *info)
+{
+ struct ethtool_link_ksettings ksettings = {};
+ struct ethtool_link_settings *lsettings;
+ struct ethnlmsghdr *ehdr;
+ struct ethtool_wolinfo wolinfo = {};
+ struct net_device *dev;
+ struct sk_buff *rskb;
+ unsigned int reply_len;
+ bool lpm_empty = true;
+ u16 req_flags;
+ u16 req_mask;
+ u32 msglevel;
+ int ret = 0;
+ int link = -EOPNOTSUPP;
+
+ lsettings = &ksettings.base;
+ ehdr = info->userhdr;
+ if (!ehdr->info_mask)
+ ehdr->info_mask = ETH_SETTINGS_IM_DEFAULT;
+ req_mask = ehdr->info_mask;
+ req_flags = ehdr->flags;
+
+ dev = ethnl_dev_get(info);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+ if (req_mask & (ETH_SETTINGS_IM_LINKINFO | ETH_SETTINGS_IM_LINKMODES)) {
+ rtnl_lock();
+ ret = ethnl_get_link_ksettings(info, dev, &ksettings);
+ rtnl_unlock();
+ if (ret < 0) {
+ warn_partial_info(info);
+ req_mask &= ~(ETH_SETTINGS_IM_LINKINFO |
+ ETH_SETTINGS_IM_LINKMODES);
+ }
+ }
+ if (req_mask & ETH_SETTINGS_IM_LINKMODES) {
+ lpm_empty = bitmap_empty(ksettings.link_modes.lp_advertising,
+ __ETHTOOL_LINK_MODE_MASK_NBITS);
+ ethnl_bitmap_to_u32(ksettings.link_modes.supported,
+ __ETHTOOL_LINK_MODE_MASK_NWORDS);
+ ethnl_bitmap_to_u32(ksettings.link_modes.advertising,
+ __ETHTOOL_LINK_MODE_MASK_NWORDS);
+ ethnl_bitmap_to_u32(ksettings.link_modes.lp_advertising,
+ __ETHTOOL_LINK_MODE_MASK_NWORDS);
+ }
+ if (req_mask & ETH_SETTINGS_IM_MSGLEVEL) {
+ if (dev->ethtool_ops->get_msglevel) {
+ msglevel = dev->ethtool_ops->get_msglevel(dev);
+ } else {
+ warn_partial_info(info);
+ req_mask &= ~ETH_SETTINGS_IM_MSGLEVEL;
+ }
+ }
+ if (req_mask & ETH_SETTINGS_IM_WOLINFO) {
+ ret = ethnl_get_wol(info, dev, &wolinfo);
+ if (ret < 0) {
+ warn_partial_info(info);
+ req_mask &= ~ETH_SETTINGS_IM_WOLINFO;
+ }
+ }
+ if (req_mask & ETH_SETTINGS_IM_LINK)
+ link = __ethtool_get_link(dev);
+
+ ret = ethnl_settings_size(ehdr, &ksettings, dev, req_mask);
+ if (ret < 0)
+ goto err_putdev;
+ else
+ reply_len = ret;
+ rskb = ethnl_reply_init(reply_len, dev, ETHTOOL_CMD_SET_SETTINGS, info,
+ &ehdr);
+ ret = -ENOMEM;
+ if (!rskb)
+ goto err_putdev;
+ if (req_mask != ETH_SETTINGS_IM_DEFAULT)
+ ehdr->info_mask = req_mask;
+
+ ret = -EMSGSIZE;
+ if (req_mask & ETH_SETTINGS_IM_LINKINFO) {
+ if (nla_put_u32(rskb, ETHA_SETTINGS_SPEED, lsettings->speed) ||
+ nla_put_u8(rskb, ETHA_SETTINGS_DUPLEX, lsettings->duplex) ||
+ nla_put_u8(rskb, ETHA_SETTINGS_PORT, lsettings->port) ||
+ nla_put_u8(rskb, ETHA_SETTINGS_PHYADDR,
+ lsettings->phy_address) ||
+ nla_put_u8(rskb, ETHA_SETTINGS_AUTONEG,
+ lsettings->autoneg) ||
+ nla_put_bitfield32(rskb, ETHA_SETTINGS_MDIO_SUPPORT,
+ lsettings->mdio_support,
+ ETH_MDIO_SUPPORTS_ALL) ||
+ nla_put_u8(rskb, ETHA_SETTINGS_TP_MDIX,
+ lsettings->eth_tp_mdix) ||
+ nla_put_u8(rskb, ETHA_SETTINGS_TP_MDIX_CTRL,
+ lsettings->eth_tp_mdix_ctrl) ||
+ nla_put_u8(rskb, ETHA_SETTINGS_TRANSCEIVER,
+ lsettings->transceiver))
+ goto err;
+ }
+ if (req_mask & ETH_SETTINGS_IM_LINKMODES) {
+ u32 *supported = (u32 *)ksettings.link_modes.supported;
+ u32 *advertising = (u32 *)ksettings.link_modes.advertising;
+ u32 *lp_advertising =
+ (u32 *)ksettings.link_modes.lp_advertising;
+ bool compact = req_flags & ETH_SETTINGS_RF_COMPACT_BITSETS;
+
+ ret = ethnl_put_bitset(rskb, ETHA_SETTINGS_LINK_MODES, compact,
+ __ETHTOOL_LINK_MODE_MASK_NBITS,
+ advertising, supported, link_mode_names);
+ if (ret < 0)
+ goto err;
+ if (!lpm_empty) {
+ ret = ethnl_put_bitset(rskb, ETHA_SETTINGS_PEER_MODES,
+ compact,
+ __ETHTOOL_LINK_MODE_MASK_NBITS,
+ lp_advertising, lp_advertising,
+ link_mode_names);
+ if (ret < 0)
+ goto err;
+ }
+ ret = -EMSGSIZE;
+ }
+ if (req_mask & ETH_SETTINGS_IM_MSGLEVEL) {
+ if (nla_put_bitfield32(rskb, ETHA_SETTINGS_MSGLVL, msglevel,
+ NETIF_MSG_ALL))
+ goto err;
+ }
+ if (req_mask & ETH_SETTINGS_IM_WOLINFO) {
+ /* ioctl() restricts read access to wolinfo but the actual
+ * reason is to hide sopass from unprivileged users; netlink
+ * can show wol modes without sopass
+ */
+ if (nla_put_bitfield32(rskb, ETHA_SETTINGS_WOL_MODES,
+ wolinfo.wolopts, wolinfo.supported))
+ goto err;
+ if (ethnl_is_privileged(skb, info) &&
+ nla_put(rskb, ETHA_SETTINGS_SOPASS, sizeof(wolinfo.sopass),
+ wolinfo.sopass))
+ goto err;
+ }
+ if (req_mask & ETH_SETTINGS_IM_LINK && link >= 0) {
+ if (nla_put_u32(rskb, ETHA_SETTINGS_LINK, link))
+ goto err;
+ }
+
+ dev_put(dev);
+ genlmsg_end(rskb, ehdr);
+ return genlmsg_reply(rskb, info);
+
+err:
+ nlmsg_free(rskb);
+err_putdev:
+ dev_put(dev);
+ if (ret == -EMSGSIZE)
+ GENL_SET_ERR_MSG(info,
+ "kernel error, see kernel log for details");
+ WARN_ONCE(ret == -EMSGSIZE,
+ "calculated message payload length (%d) not sufficient\n",
+ reply_len);
+ return ret;
+}
+
/* genetlink paperwork */

static const struct genl_ops ethtool_genl_ops[] = {
@@ -609,6 +904,10 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_CMD_GET_DRVINFO,
.doit = ethnl_get_drvinfo,
},
+ {
+ .cmd = ETHTOOL_CMD_GET_SETTINGS,
+ .doit = ethnl_get_settings,
+ },
};

static struct genl_family ethtool_genl_family = {
--
2.15.1

2017-12-11 13:54:32

by Michal Kubecek

[permalink] [raw]
Subject: [RFC PATCH 7/9] ethtool: implement SET_SETTINGS message

Sets the information provided by ETHTOOL_SLINKSETTINGS, ETHTOOL_SWOL and
ETHTOOL_SMSGLVL. Unlike with ioctl(), userspace can send only some
attributes so that we only need to call ethtool_ops callbacks which we
really need (and the "set" callback is only called when we actually changed
some setting).

Signed-off-by: Michal Kubecek <[email protected]>
---
Documentation/networking/ethtool-netlink.txt | 42 +-
net/core/ethtool_netlink.c | 547 +++++++++++++++++++++++++++
2 files changed, 584 insertions(+), 5 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
index 7aabc87c9f09..187fab33f721 100644
--- a/Documentation/networking/ethtool-netlink.txt
+++ b/Documentation/networking/ethtool-netlink.txt
@@ -110,7 +110,7 @@ List of message types
ETHTOOL_CMD_GET_DRVINFO
ETHTOOL_CMD_SET_DRVINFO response only
ETHTOOL_CMD_GET_SETTINGS
- ETHTOOL_CMD_SET_SETTINGS response only (for now)
+ ETHTOOL_CMD_SET_SETTINGS

All constants use ETHTOOL_CMD_ prefix followed by "GET", "SET" or "ACT" to
indicate the type.
@@ -199,6 +199,38 @@ is only provided by kernel in response to privileged (netns CAP_NET_ADMIN)
requests.


+SET_SETTINGS
+------------
+
+SET_SETTINGS request allows setting some of the data reported by GET_SETTINGS.
+Request flags, info_mask and index are ignored. These attributes are allowed
+to be passed with SET_SETTINGS request:
+
+ ETHA_SETTINGS_SPEED (u32) link speed (Mb/s)
+ ETHA_SETTINGS_DUPLEX (u8) duplex mode
+ ETHA_SETTINGS_PORT (u8) physical port
+ ETHA_SETTINGS_PHYADDR (u8) MDIO address of phy
+ ETHA_SETTINGS_AUTONEG (u8) autoneotiation status
+ ETHA_SETTINGS_TP_MDIX_CTRL (u8) MDI(-X) control
+ ETHA_SETTINGS_WOL_MODES (bitfield32) wake-on-lan modes
+ ETHA_SETTINGS_SOPASS (binary) SecureOn(tm) password
+ ETHA_SETTINGS_MSGLVL (bitfield32) debug level
+ ETHA_SETTINGS_LINK_MODES (bitset) device link modes
+
+For both bitfield32 types, value and selector work the usual way, i.e. bits
+set in selector are set to corresponding bits from value and the rest is
+preserved. In a similar fashion, ETHA_SETTINGS_LINK_MODES allows setting
+advertised link modes.
+
+If autonegotiation is on (either set now or kept from before), advertised
+modes are not changed (no ETHA_SETTINGS_LINK_MODES attribute) and at least one
+of speed and duplex is specified, kernel adjusts advertised modes to all
+supported modes matching speed, duplex or both (whatever is specified). This
+autoselection is done on ethtool side with ioctl interface, netlink interface
+is supposed to allow requesting changes without knowing what exactly kernel
+supports.
+
+
Request translation
-------------------

@@ -209,13 +241,13 @@ have their netlink replacement yet.
ioctl command netlink command
---------------------------------------------------------------------
ETHTOOL_GSET ETHTOOL_CMD_GET_SETTINGS
-ETHTOOL_SSET n/a
+ETHTOOL_SSET ETHTOOL_CMD_SET_SETTINGS
ETHTOOL_GDRVINFO ETHTOOL_CMD_GET_DRVINFO
ETHTOOL_GREGS n/a
ETHTOOL_GWOL ETHTOOL_CMD_GET_SETTINGS
-ETHTOOL_SWOL n/a
+ETHTOOL_SWOL ETHTOOL_CMD_SET_SETTINGS
ETHTOOL_GMSGLVL ETHTOOL_CMD_GET_SETTINGS
-ETHTOOL_SMSGLVL n/a
+ETHTOOL_SMSGLVL ETHTOOL_CMD_SET_SETTINGS
ETHTOOL_NWAY_RST n/a
ETHTOOL_GLINK ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_GEEPROM n/a
@@ -283,7 +315,7 @@ ETHTOOL_STUNABLE n/a
ETHTOOL_GPHYSTATS n/a
ETHTOOL_PERQUEUE n/a
ETHTOOL_GLINKSETTINGS ETHTOOL_CMD_GET_SETTINGS
-ETHTOOL_SLINKSETTINGS n/a
+ETHTOOL_SLINKSETTINGS ETHTOOL_CMD_SET_SETTINGS
ETHTOOL_PHY_GTUNABLE n/a
ETHTOOL_PHY_STUNABLE n/a
ETHTOOL_GFECPARAM n/a
diff --git a/net/core/ethtool_netlink.c b/net/core/ethtool_netlink.c
index 0c2bed7850bc..4b14a02be12a 100644
--- a/net/core/ethtool_netlink.c
+++ b/net/core/ethtool_netlink.c
@@ -67,6 +67,222 @@ static const char *const link_mode_names[] = {
[ETHTOOL_LINK_MODE_FEC_BASER_BIT] = "BASER",
};

+struct link_mode_info {
+ int speed;
+ u8 duplex;
+};
+
+static const struct link_mode_info link_mode_params[] = {
+ [ETHTOOL_LINK_MODE_10baseT_Half_BIT] = {
+ .speed = 10,
+ .duplex = DUPLEX_HALF,
+ },
+ [ETHTOOL_LINK_MODE_10baseT_Full_BIT] = {
+ .speed = 10,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_100baseT_Half_BIT] = {
+ .speed = 100,
+ .duplex = DUPLEX_HALF,
+ },
+ [ETHTOOL_LINK_MODE_100baseT_Full_BIT] = {
+ .speed = 100,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_1000baseT_Half_BIT] = {
+ .speed = 1000,
+ .duplex = DUPLEX_HALF,
+ },
+ [ETHTOOL_LINK_MODE_1000baseT_Full_BIT] = {
+ .speed = 1000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_Autoneg_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_TP_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_AUI_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_MII_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_FIBRE_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_BNC_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_10000baseT_Full_BIT] = {
+ .speed = 10000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_Pause_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_Asym_Pause_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_2500baseX_Full_BIT] = {
+ .speed = 2500,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_Backplane_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_1000baseKX_Full_BIT] = {
+ .speed = 1000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT] = {
+ .speed = 10000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_10000baseKR_Full_BIT] = {
+ .speed = 10000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_10000baseR_FEC_BIT] = {
+ .speed = 10000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_20000baseMLD2_Full_BIT] = {
+ .speed = 20000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT] = {
+ .speed = 20000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT] = {
+ .speed = 40000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT] = {
+ .speed = 40000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_40000baseSR4_Full_BIT] = {
+ .speed = 40000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_40000baseLR4_Full_BIT] = {
+ .speed = 40000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_56000baseKR4_Full_BIT] = {
+ .speed = 56000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_56000baseCR4_Full_BIT] = {
+ .speed = 56000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_56000baseSR4_Full_BIT] = {
+ .speed = 56000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_56000baseLR4_Full_BIT] = {
+ .speed = 56000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_25000baseCR_Full_BIT] = {
+ .speed = 25000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_25000baseKR_Full_BIT] = {
+ .speed = 25000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_25000baseSR_Full_BIT] = {
+ .speed = 25000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_50000baseCR2_Full_BIT] = {
+ .speed = 50000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_50000baseKR2_Full_BIT] = {
+ .speed = 50000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT] = {
+ .speed = 100000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_100000baseSR4_Full_BIT] = {
+ .speed = 100000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT] = {
+ .speed = 100000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_100000baseLR4_ER4_Full_BIT] = {
+ .speed = 100000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_50000baseSR2_Full_BIT] = {
+ .speed = 50000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_1000baseX_Full_BIT] = {
+ .speed = 1000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_10000baseCR_Full_BIT] = {
+ .speed = 10000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_10000baseSR_Full_BIT] = {
+ .speed = 10000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_10000baseLR_Full_BIT] = {
+ .speed = 10000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_10000baseLRM_Full_BIT] = {
+ .speed = 10000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_10000baseER_Full_BIT] = {
+ .speed = 10000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_2500baseT_Full_BIT] = {
+ .speed = 2500,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_5000baseT_Full_BIT] = {
+ .speed = 5000,
+ .duplex = DUPLEX_FULL,
+ },
+ [ETHTOOL_LINK_MODE_FEC_NONE_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_FEC_RS_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+ [ETHTOOL_LINK_MODE_FEC_BASER_BIT] = {
+ .speed = SPEED_UNKNOWN,
+ .duplex = DUPLEX_UNKNOWN,
+ },
+};
+
/* misc helper functions */

static int ethnl_str_size(const char *s)
@@ -662,6 +878,34 @@ static int ethnl_get_drvinfo(struct sk_buff *skb, struct genl_info *info)

/* GET_SETTINGS */

+/* We want to allow ~0 as selector for backward compatibility (to just set
+ * given set of modes, whatever kernel supports) so that we allow all bits
+ * on validation and do our own sanity check later.
+ */
+static u32 all_bits = ~(u32)0;
+
+static const struct nla_policy settings_policy[ETHA_SETTINGS_MAX + 1] = {
+ [ETHA_SETTINGS_UNSPEC] = { .type = NLA_UNSPEC },
+ [ETHA_SETTINGS_SPEED] = { .type = NLA_U32 },
+ [ETHA_SETTINGS_DUPLEX] = { .type = NLA_U8 },
+ [ETHA_SETTINGS_PORT] = { .type = NLA_U8 },
+ [ETHA_SETTINGS_PHYADDR] = { .type = NLA_U8 },
+ [ETHA_SETTINGS_AUTONEG] = { .type = NLA_U8 },
+ [ETHA_SETTINGS_MDIO_SUPPORT] = { .type = NLA_BITFIELD32 },
+ [ETHA_SETTINGS_TP_MDIX] = { .type = NLA_U8 },
+ [ETHA_SETTINGS_TP_MDIX_CTRL] = { .type = NLA_U8 },
+ [ETHA_SETTINGS_TRANSCEIVER] = { .type = NLA_U8 },
+ [ETHA_SETTINGS_WOL_MODES] = { .type = NLA_BITFIELD32,
+ .validation_data = &all_bits },
+ [ETHA_SETTINGS_SOPASS] = { .type = NLA_BINARY,
+ .len = SOPASS_MAX },
+ [ETHA_SETTINGS_MSGLVL] = { .type = NLA_BITFIELD32,
+ .validation_data = &all_bits },
+ [ETHA_SETTINGS_LINK_MODES] = { .type = NLA_NESTED },
+ [ETHA_SETTINGS_PEER_MODES] = { .type = NLA_NESTED },
+ [ETHA_SETTINGS_LINK] = { .type = NLA_FLAG },
+};
+
/* To keep things simple, reserve space for some attributes which may not
* be added to the message (e.g. ETHA_SETTINGS_SOPASS); therefore the length
* returned may be bigger than the actual length of the message sent
@@ -727,6 +971,24 @@ static int ethnl_get_link_ksettings(struct genl_info *info,
return ret;
}

+static int ethnl_get_legacy_settings(struct genl_info *info,
+ struct net_device *dev,
+ struct ethtool_cmd *cmd)
+{
+ int ret;
+
+ if (!dev->ethtool_ops->get_settings) {
+ /* we already tried ->get_link_ksettings */
+ GENL_SET_ERR_MSG(info, "link settings retrieval unsupported");
+ return -EOPNOTSUPP;
+ }
+ ret = dev->ethtool_ops->get_settings(dev, cmd);
+ if (ret < 0)
+ GENL_SET_ERR_MSG(info, "failed to retrieve link settings");
+
+ return ret;
+}
+
static int ethnl_get_wol(struct genl_info *info, struct net_device *dev,
struct ethtool_wolinfo *wolinfo)
{
@@ -737,6 +999,79 @@ static int ethnl_get_wol(struct genl_info *info, struct net_device *dev,
return ret;
}

+static int ethnl_set_link_ksettings(struct genl_info *info,
+ struct net_device *dev,
+ struct ethtool_link_ksettings *ksettings)
+{
+ int ret = dev->ethtool_ops->set_link_ksettings(dev, ksettings);
+
+ if (ret < 0)
+ GENL_SET_ERR_MSG(info, "link settings update failed");
+ return ret;
+}
+
+static int ethnl_set_legacy_settings(struct genl_info *info,
+ struct net_device *dev,
+ struct ethtool_cmd *cmd)
+{
+ int ret;
+
+ if (!dev->ethtool_ops->set_settings) {
+ /* we already tried ->set_link_ksettings */
+ GENL_SET_ERR_MSG(info, "link settings update unsupported");
+ return -EOPNOTSUPP;
+ }
+ ret = dev->ethtool_ops->set_settings(dev, cmd);
+ if (ret < 0)
+ GENL_SET_ERR_MSG(info, "link settings update failed");
+
+ return ret;
+}
+
+static int ethnl_set_wol(struct genl_info *info, struct net_device *dev,
+ struct ethtool_wolinfo *wolinfo)
+{
+ int ret = dev->ethtool_ops->set_wol(dev, wolinfo);
+
+ if (ret < 0)
+ GENL_SET_ERR_MSG(info, "wol info update failed");
+ return ret;
+}
+
+/* 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.
+ * Returns true if advertised modes bitmap was modified.
+ */
+static bool auto_link_modes(unsigned long *supported,
+ unsigned long *advertising, unsigned int nbits,
+ struct nlattr *speed_attr,
+ struct nlattr *duplex_attr)
+{
+ u8 duplex = duplex_attr ? nla_get_u8(duplex_attr) : DUPLEX_UNKNOWN;
+ u32 speed = speed_attr ? nla_get_u32(speed_attr) : SPEED_UNKNOWN;
+ DECLARE_BITMAP(old_adv, nbits);
+ unsigned int i;
+
+ bitmap_copy(old_adv, advertising, nbits);
+
+ for (i = 0; i < nbits; i++) {
+ const struct link_mode_info *info = &link_mode_params[i];
+
+ if (info->speed == SPEED_UNKNOWN)
+ continue;
+ if (test_bit(i, supported) &&
+ (!speed_attr || info->speed == speed) &&
+ (!duplex_attr || info->duplex == duplex))
+ set_bit(i, advertising);
+ else
+ clear_bit(i, advertising);
+ }
+
+ return !bitmap_equal(old_adv, advertising, nbits);
+}
+
static int ethnl_get_settings(struct sk_buff *skb, struct genl_info *info)
{
struct ethtool_link_ksettings ksettings = {};
@@ -897,6 +1232,212 @@ static int ethnl_get_settings(struct sk_buff *skb, struct genl_info *info)
return ret;
}

+/* Update device settings using ->set_link_ksettings() callback */
+static int ethnl_update_ksettings(struct genl_info *info, struct nlattr **tb,
+ struct net_device *dev)
+{
+ struct ethtool_link_ksettings ksettings = {};
+ struct ethtool_link_settings *lsettings;
+ bool mod = false;
+ int ret;
+
+ rtnl_lock();
+ ret = ethnl_get_link_ksettings(info, dev, &ksettings);
+ if (ret < 0)
+ goto out_unlock;
+ lsettings = &ksettings.base;
+
+ mod = false;
+ if (ethnl_update_u32(&lsettings->speed, tb[ETHA_SETTINGS_SPEED]))
+ mod = true;
+ if (ethnl_update_u8(&lsettings->duplex, tb[ETHA_SETTINGS_DUPLEX]))
+ mod = true;
+ if (ethnl_update_u8(&lsettings->port, tb[ETHA_SETTINGS_PORT]))
+ mod = true;
+ if (ethnl_update_u8(&lsettings->phy_address, tb[ETHA_SETTINGS_PHYADDR]))
+ mod = true;
+ if (ethnl_update_u8(&lsettings->autoneg, tb[ETHA_SETTINGS_AUTONEG]))
+ mod = true;
+ if (ethnl_update_u8(&lsettings->eth_tp_mdix_ctrl,
+ tb[ETHA_SETTINGS_TP_MDIX_CTRL]))
+ mod = true;
+ if (ethnl_update_bitset(ksettings.link_modes.advertising,
+ __ETHTOOL_LINK_MODE_MASK_NBITS,
+ tb[ETHA_SETTINGS_LINK_MODES],
+ &ret, link_mode_names, info))
+ mod = true;
+ if (ret < 0)
+ goto out_unlock;
+
+ if (!tb[ETHA_SETTINGS_LINK_MODES] && lsettings->autoneg &&
+ (tb[ETHA_SETTINGS_SPEED] || tb[ETHA_SETTINGS_DUPLEX])) {
+ if (auto_link_modes(ksettings.link_modes.supported,
+ ksettings.link_modes.advertising,
+ __ETHTOOL_LINK_MODE_MASK_NBITS,
+ tb[ETHA_SETTINGS_SPEED],
+ tb[ETHA_SETTINGS_DUPLEX]))
+ mod = true;
+ }
+
+ if (mod) {
+ ret = ethnl_set_link_ksettings(info, dev, &ksettings);
+ if (ret < 0)
+ goto out_unlock;
+ }
+
+ ret = 0;
+out_unlock:
+ rtnl_unlock();
+ return ret;
+}
+
+/* Update legacy settings using ->set_settings() callback. */
+static int ethnl_update_lsettings(struct genl_info *info, struct nlattr **tb,
+ struct net_device *dev)
+{
+ struct ethtool_cmd cmd = {};
+ DECLARE_BITMAP(advertising, sizeof(u32));
+ DECLARE_BITMAP(supported, sizeof(u32));
+ bool mod = false;
+ u32 speed;
+ int ret;
+
+ rtnl_lock();
+ ret = ethnl_get_legacy_settings(info, dev, &cmd);
+ if (ret < 0)
+ goto out_unlock;
+ bitmap_from_u32array(supported, sizeof(u32), &cmd.supported, 1);
+ bitmap_from_u32array(advertising, sizeof(u32), &cmd.advertising, 1);
+
+ mod = false;
+ speed = ethtool_cmd_speed(&cmd);
+ if (ethnl_update_u32(&speed, tb[ETHA_SETTINGS_SPEED])) {
+ ethtool_cmd_speed_set(&cmd, speed);
+ mod = true;
+ }
+ if (ethnl_update_u8(&cmd.duplex, tb[ETHA_SETTINGS_DUPLEX]))
+ mod = true;
+ if (ethnl_update_u8(&cmd.port, tb[ETHA_SETTINGS_PORT]))
+ mod = true;
+ if (ethnl_update_u8(&cmd.phy_address, tb[ETHA_SETTINGS_PHYADDR]))
+ mod = true;
+ if (ethnl_update_u8(&cmd.autoneg, tb[ETHA_SETTINGS_AUTONEG]))
+ mod = true;
+ if (ethnl_update_u8(&cmd.eth_tp_mdix_ctrl,
+ tb[ETHA_SETTINGS_TP_MDIX_CTRL]))
+ mod = true;
+ if (ethnl_update_bitset(advertising, sizeof(cmd.advertising),
+ tb[ETHA_SETTINGS_LINK_MODES],
+ &ret, link_mode_names, info)) {
+ bitmap_to_u32array(&cmd.advertising, 1, advertising,
+ sizeof(cmd.advertising));
+ mod = true;
+ }
+ if (ret < 0)
+ goto out_unlock;
+
+ if (!tb[ETHA_SETTINGS_LINK_MODES] && cmd.autoneg &&
+ (tb[ETHA_SETTINGS_SPEED] || tb[ETHA_SETTINGS_DUPLEX])) {
+ if (auto_link_modes(supported, advertising,
+ sizeof(cmd.advertising),
+ tb[ETHA_SETTINGS_SPEED],
+ tb[ETHA_SETTINGS_DUPLEX])) {
+ bitmap_to_u32array(&cmd.advertising, 1, advertising,
+ sizeof(cmd.advertising));
+ mod = true;
+ }
+ }
+
+ if (mod) {
+ ret = ethnl_set_legacy_settings(info, dev, &cmd);
+ if (ret < 0)
+ goto out_unlock;
+ }
+
+ ret = 0;
+out_unlock:
+ rtnl_unlock();
+ return ret;
+}
+
+static int ethnl_set_settings(struct sk_buff *skb, struct genl_info *info)
+{
+ struct nlattr *tb[ETHA_SETTINGS_MAX + 1];
+ struct ethnlmsghdr *ehdr;
+ struct ethtool_wolinfo wolinfo = {};
+ struct net_device *dev;
+ bool mod;
+ int ret;
+
+ ehdr = info->userhdr;
+ dev = ethnl_dev_get(info);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+
+ ret = genlmsg_parse(info->nlhdr, &ethtool_genl_family, tb,
+ ETHA_SETTINGS_MAX, settings_policy, info->extack);
+ if (ret < 0)
+ goto err_putdev;
+
+ /* read only attributes */
+ ret = -EINVAL;
+ if (tb[ETHA_SETTINGS_MDIO_SUPPORT] || tb[ETHA_SETTINGS_TP_MDIX] ||
+ tb[ETHA_SETTINGS_TRANSCEIVER] || tb[ETHA_SETTINGS_PEER_MODES] ||
+ tb[ETHA_SETTINGS_LINK]) {
+ GENL_SET_ERR_MSG(info, "attempt to set a read only attribute");
+ goto err_putdev;
+ }
+
+ if (tb[ETHA_SETTINGS_SPEED] || tb[ETHA_SETTINGS_DUPLEX] ||
+ tb[ETHA_SETTINGS_PORT] || tb[ETHA_SETTINGS_PHYADDR] ||
+ tb[ETHA_SETTINGS_AUTONEG] || tb[ETHA_SETTINGS_TP_MDIX_CTRL] ||
+ tb[ETHA_SETTINGS_LINK_MODES]) {
+ if (dev->ethtool_ops->get_link_ksettings)
+ ret = ethnl_update_ksettings(info, tb, dev);
+ else
+ ret = ethnl_update_lsettings(info, tb, dev);
+ if (ret < 0)
+ goto err_putdev;
+ }
+ if (tb[ETHA_SETTINGS_WOL_MODES] || tb[ETHA_SETTINGS_SOPASS]) {
+ ret = ethnl_get_wol(info, dev, &wolinfo);
+ if (ret < 0)
+ goto err_putdev;
+
+ mod = false;
+ if (ethnl_update_bitfield32(&wolinfo.wolopts,
+ tb[ETHA_SETTINGS_WOL_MODES]))
+ mod = true;
+ if (ethnl_update_binary(wolinfo.sopass, SOPASS_MAX,
+ tb[ETHA_SETTINGS_SOPASS]))
+ mod = true;
+ if (mod) {
+ ret = ethnl_set_wol(info, dev, &wolinfo);
+ if (ret < 0)
+ goto err_putdev;
+ }
+ }
+ if (tb[ETHA_SETTINGS_MSGLVL]) {
+ u32 msglvl;
+
+ ret = -EOPNOTSUPP;
+ if (!dev->ethtool_ops->get_msglevel ||
+ !dev->ethtool_ops->set_msglevel) {
+ GENL_SET_ERR_MSG(info,
+ "device does not provide msglvl access");
+ goto err_putdev;
+ }
+ msglvl = dev->ethtool_ops->get_msglevel(dev);
+ if (ethnl_update_bitfield32(&msglvl, tb[ETHA_SETTINGS_MSGLVL]))
+ dev->ethtool_ops->set_msglevel(dev, msglvl);
+ }
+
+ ret = 0;
+err_putdev:
+ dev_put(dev);
+ return ret;
+}
+
/* genetlink paperwork */

static const struct genl_ops ethtool_genl_ops[] = {
@@ -908,6 +1449,12 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_CMD_GET_SETTINGS,
.doit = ethnl_get_settings,
},
+ {
+ .cmd = ETHTOOL_CMD_SET_SETTINGS,
+ .flags = GENL_UNS_ADMIN_PERM,
+ .policy = settings_policy,
+ .doit = ethnl_set_settings,
+ },
};

static struct genl_family ethtool_genl_family = {
--
2.15.1

2017-12-11 13:54:39

by Michal Kubecek

[permalink] [raw]
Subject: [RFC PATCH 8/9] ethtool: implement GET_PARAMS message

Requests the information provide by ETHTOOL_GCOALESCE, ETHTOOL_GRINGPARAM,
ETHTOOL_GPAUSEPARAM, ETHTOOL_GCHANNELS, ETHTOOL_GEEE and ETHTOOL_GFECPARAM.
Flags in info_mask allow selecting only some (or on) of these categories.

Signed-off-by: Michal Kubecek <[email protected]>
---
Documentation/networking/ethtool-netlink.txt | 98 ++++++-
include/uapi/linux/ethtool_netlink.h | 118 ++++++++
net/core/ethtool_netlink.c | 411 +++++++++++++++++++++++++++
3 files changed, 621 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
index 187fab33f721..45ed1801ab50 100644
--- a/Documentation/networking/ethtool-netlink.txt
+++ b/Documentation/networking/ethtool-netlink.txt
@@ -111,6 +111,8 @@ List of message types
ETHTOOL_CMD_SET_DRVINFO response only
ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_CMD_SET_SETTINGS
+ ETHTOOL_CMD_GET_PARAMS
+ ETHTOOL_CMD_SET_PARAMS response only (for now)

All constants use ETHTOOL_CMD_ prefix followed by "GET", "SET" or "ACT" to
indicate the type.
@@ -231,6 +233,90 @@ is supposed to allow requesting changes without knowing what exactly kernel
supports.


+GET_PARAMS
+----------
+
+GET_PARAMS request retrieves information provided by legacy comands
+ETHTOOL_GCOALESCE (coalescing setting), ETHTOOL_GRINGPARAM (ring parameters),
+ETHTOOL_GPAUSEPARAM (pause parameters), ETHTOOL_GCHANNELS (channel settings),
+ETHTOOL_GEEE (EEE settings) and ETHTOOL_GFECPARAM (FEC parameters). For each
+of these, there is a bit in header info_mask so that only one type of
+information can be requested.
+
+Header flags:
+ ETH_PARAMS_RF_COMPACT_BITSETS bitset form in response (1 is compact)
+
+Header info_mask bits:
+ ETH_PARAMS_IM_COALESCE coalescing settings
+ ETH_PARAMS_IM_RING ring parameters
+ ETH_PARAMS_IM_PAUSE pause parameters
+ ETH_PARAMS_IM_CHANNELS channel settings
+ ETH_PARAMS_IM_EEE EEE settings
+ ETH_PARAMS_IM_FEC FEC parameters
+
+On top level, there is one attribute for each of the information categories,
+the information is nested in it.
+
+ ETHA_PARAMS_COALESCE (nest) coalescing
+ ETHA_COALESCE_RX_USECS (u32)
+ ETHA_COALESCE_RX_MAXFRM (u32)
+ ETHA_COALESCE_RX_USECS_IRQ (u32)
+ ETHA_COALESCE_RX_MAXFRM_IRQ (u32)
+ ETHA_COALESCE_RX_USECS_LOW (u32)
+ ETHA_COALESCE_RX_MAXFRM_LOW (u32)
+ ETHA_COALESCE_RX_USECS_HIGH (u32)
+ ETHA_COALESCE_RX_MAXFRM_HIGH (u32)
+ ETHA_COALESCE_TX_USECS (u32)
+ ETHA_COALESCE_TX_MAXFRM (u32)
+ ETHA_COALESCE_TX_USECS_IRQ (u32)
+ ETHA_COALESCE_TX_MAXFRM_IRQ (u32)
+ ETHA_COALESCE_TX_USECS_LOW (u32)
+ ETHA_COALESCE_TX_MAXFRM_LOW (u32)
+ ETHA_COALESCE_TX_USECS_HIGH (u32)
+ ETHA_COALESCE_TX_MAXFRM_HIGH (u32)
+ ETHA_COALESCE_PKT_RATE_LOW (u32)
+ ETHA_COALESCE_PKT_RATE_HIGH (u32)
+ ETHA_COALESCE_RX_USE_ADAPTIVE (bool)
+ ETHA_COALESCE_TX_USE_ADAPTIVE (bool)
+ ETHA_COALESCE_RATE_SAMPLE_INTERVAL (u32)
+ ETHA_COALESCE_STATS_BLOCK_USECS (u32)
+ ETHA_PARAMS_RING (nest) ring parameters
+ ETHA_RING_RX_MAX_PENDING (u32)
+ ETHA_RING_RX_MINI_MAX_PENDING (u32)
+ ETHA_RING_RX_JUMBO_MAX_PENDING (u32)
+ ETHA_RING_TX_MAX_PENDING (u32)
+ ETHA_RING_RX_PENDING (u32)
+ ETHA_RING_RX_MINI_PENDING (u32)
+ ETHA_RING_RX_JUMBO_PENDING (u32)
+ ETHA_RING_TX_PENDING (u32)
+ ETHA_PARAMS_PAUSE (nest) pause parameters
+ ETHA_PAUSE_AUTONEG (bool)
+ ETHA_PAUSE_RX (bool)
+ ETHA_PAUSE_TX (bool)
+ ETHA_PARAMS_CHANNELS (nest) channel settings
+ ETHA_CHANNELS_MAX_RX (u32)
+ ETHA_CHANNELS_MAX_TX (u32)
+ ETHA_CHANNELS_MAX_OTHER (u32)
+ ETHA_CHANNELS_MAX_COMBINED (u32)
+ ETHA_CHANNELS_RX_COUNT (u32)
+ ETHA_CHANNELS_TX_COUNT (u32)
+ ETHA_CHANNELS_OTHER_COUNT (u32)
+ ETHA_CHANNELS_COMBINED_COUNT (u32)
+ ETHA_PARAMS_EEE (nest) EEE settings
+ ETHA_EEE_LINK_MODES (bitset)
+ - modes for which EEE is advertised (value) or supported (mask)
+ ETHA_EEE_PEER_MODES (bitset)
+ - modes for which link partner advertises EEE
+ ETHA_EEE_ACTIVE (bool)
+ ETHA_EEE_ENABLED (bool)
+ ETHA_EEE_TX_LPI_ENABLED (bool)
+ ETHA_EEE_TX_LPI_TIMER (u32)
+ ETHA_PARAMS_FEC (nest) FEC parameters
+ ETHA_FEC_MODES (bitfield32)
+ - active (value) and configured (selector) FEC encodings
+
+
+
Request translation
-------------------

@@ -252,11 +338,11 @@ ETHTOOL_NWAY_RST n/a
ETHTOOL_GLINK ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_GEEPROM n/a
ETHTOOL_SEEPROM n/a
-ETHTOOL_GCOALESCE n/a
+ETHTOOL_GCOALESCE ETHTOOL_CMD_GET_PARAMS
ETHTOOL_SCOALESCE n/a
-ETHTOOL_GRINGPARAM n/a
+ETHTOOL_GRINGPARAM ETHTOOL_CMD_GET_PARAMS
ETHTOOL_SRINGPARAM n/a
-ETHTOOL_GPAUSEPARAM n/a
+ETHTOOL_GPAUSEPARAM ETHTOOL_CMD_GET_PARAMS
ETHTOOL_SPAUSEPARAM n/a
ETHTOOL_GRXCSUM n/a
ETHTOOL_SRXCSUM n/a
@@ -298,7 +384,7 @@ ETHTOOL_GRXFHINDIR n/a
ETHTOOL_SRXFHINDIR n/a
ETHTOOL_GFEATURES n/a
ETHTOOL_SFEATURES n/a
-ETHTOOL_GCHANNELS n/a
+ETHTOOL_GCHANNELS ETHTOOL_CMD_GET_PARAMS
ETHTOOL_SCHANNELS n/a
ETHTOOL_SET_DUMP n/a
ETHTOOL_GET_DUMP_FLAG n/a
@@ -306,7 +392,7 @@ ETHTOOL_GET_DUMP_DATA n/a
ETHTOOL_GET_TS_INFO n/a
ETHTOOL_GMODULEINFO n/a
ETHTOOL_GMODULEEEPROM n/a
-ETHTOOL_GEEE n/a
+ETHTOOL_GEEE ETHTOOL_CMD_GET_PARAMS
ETHTOOL_SEEE n/a
ETHTOOL_GRSSH n/a
ETHTOOL_SRSSH n/a
@@ -318,6 +404,6 @@ ETHTOOL_GLINKSETTINGS ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_SLINKSETTINGS ETHTOOL_CMD_SET_SETTINGS
ETHTOOL_PHY_GTUNABLE n/a
ETHTOOL_PHY_STUNABLE n/a
-ETHTOOL_GFECPARAM n/a
+ETHTOOL_GFECPARAM ETHTOOL_CMD_GET_PARAMS
ETHTOOL_SFECPARAM n/a

diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
index 9520d13fc9ab..8621e0a9d481 100644
--- a/include/uapi/linux/ethtool_netlink.h
+++ b/include/uapi/linux/ethtool_netlink.h
@@ -25,6 +25,8 @@ enum {
ETHTOOL_CMD_SET_DRVINFO, /* only for reply */
ETHTOOL_CMD_GET_SETTINGS,
ETHTOOL_CMD_SET_SETTINGS,
+ ETHTOOL_CMD_GET_PARAMS,
+ ETHTOOL_CMD_SET_PARAMS,

__ETHTOOL_CMD_MAX,
ETHTOOL_CMD_MAX = (__ETHTOOL_CMD_MAX - 1),
@@ -114,6 +116,122 @@ enum {

#define ETH_SETTINGS_IM_DEFAULT 0x1f

+/* GET_PARAMS / SET_PARAMS */
+
+enum {
+ ETHA_PARAMS_UNSPEC,
+ ETHA_PARAMS_COALESCE, /* nest - ETHA_COALESCE_* */
+ ETHA_PARAMS_RING, /* nest - ETHA_RING_* */
+ ETHA_PARAMS_PAUSE, /* nest - ETHA_PAUSE_* */
+ ETHA_PARAMS_CHANNELS, /* nest - ETHA_CHANNELS_* */
+ ETHA_PARAMS_EEE, /* nest - ETHA_EEE_* */
+ ETHA_PARAMS_FEC, /* nest - ETHA_FEC_* */
+
+ __ETHA_PARAMS_MAX,
+ ETHA_PARAMS_MAX = (__ETHA_PARAMS_MAX - 1),
+};
+
+#define ETH_PARAMS_RF_COMPACT_BITSETS 0x1
+
+#define ETH_PARAMS_IM_COALESCE 0x01
+#define ETH_PARAMS_IM_RING 0x02
+#define ETH_PARAMS_IM_PAUSE 0x04
+#define ETH_PARAMS_IM_CHANNELS 0x08
+#define ETH_PARAMS_IM_EEE 0x10
+#define ETH_PARAMS_IM_FEC 0x20
+
+#define ETH_PARAMS_IM_DEFAULT 0x3f
+
+enum {
+ ETHA_COALESCE_UNSPEC,
+ ETHA_COALESCE_RX_USECS, /* u32 */
+ ETHA_COALESCE_RX_MAXFRM, /* u32 */
+ ETHA_COALESCE_RX_USECS_IRQ, /* u32 */
+ ETHA_COALESCE_RX_MAXFRM_IRQ, /* u32 */
+ ETHA_COALESCE_RX_USECS_LOW, /* u32 */
+ ETHA_COALESCE_RX_MAXFRM_LOW, /* u32 */
+ ETHA_COALESCE_RX_USECS_HIGH, /* u32 */
+ ETHA_COALESCE_RX_MAXFRM_HIGH, /* u32 */
+ ETHA_COALESCE_TX_USECS, /* u32 */
+ ETHA_COALESCE_TX_MAXFRM, /* u32 */
+ ETHA_COALESCE_TX_USECS_IRQ, /* u32 */
+ ETHA_COALESCE_TX_MAXFRM_IRQ, /* u32 */
+ ETHA_COALESCE_TX_USECS_LOW, /* u32 */
+ ETHA_COALESCE_TX_MAXFRM_LOW, /* u32 */
+ ETHA_COALESCE_TX_USECS_HIGH, /* u32 */
+ ETHA_COALESCE_TX_MAXFRM_HIGH, /* u32 */
+ ETHA_COALESCE_PKT_RATE_LOW, /* u32 */
+ ETHA_COALESCE_PKT_RATE_HIGH, /* u32 */
+ ETHA_COALESCE_RX_USE_ADAPTIVE, /* u8 */
+ ETHA_COALESCE_TX_USE_ADAPTIVE, /* u8 */
+ ETHA_COALESCE_RATE_SAMPLE_INTERVAL, /* u32 */
+ ETHA_COALESCE_STATS_BLOCK_USECS, /* u32 */
+
+ __ETHA_COALESCE_MAX,
+ ETHA_COALESCE_MAX = (__ETHA_COALESCE_MAX - 1),
+};
+
+enum {
+ ETHA_RING_UNSPEC,
+ ETHA_RING_RX_MAX_PENDING, /* u32 */
+ ETHA_RING_RX_MINI_MAX_PENDING, /* u32 */
+ ETHA_RING_RX_JUMBO_MAX_PENDING, /* u32 */
+ ETHA_RING_TX_MAX_PENDING, /* u32 */
+ ETHA_RING_RX_PENDING, /* u32 */
+ ETHA_RING_RX_MINI_PENDING, /* u32 */
+ ETHA_RING_RX_JUMBO_PENDING, /* u32 */
+ ETHA_RING_TX_PENDING, /* u32 */
+
+ __ETHA_RING_MAX,
+ ETHA_RING_MAX = (__ETHA_RING_MAX - 1),
+};
+
+enum {
+ ETHA_PAUSE_UNSPEC,
+ ETHA_PAUSE_AUTONEG, /* u8 */
+ ETHA_PAUSE_RX, /* u8 */
+ ETHA_PAUSE_TX, /* u8 */
+
+ __ETHA_PAUSE_MAX,
+ ETHA_PAUSE_MAX = (__ETHA_PAUSE_MAX - 1),
+};
+
+enum {
+ ETHA_CHANNELS_UNSPEC,
+ ETHA_CHANNELS_MAX_RX, /* u32 */
+ ETHA_CHANNELS_MAX_TX, /* u32 */
+ ETHA_CHANNELS_MAX_OTHER, /* u32 */
+ ETHA_CHANNELS_MAX_COMBINED, /* u32 */
+ ETHA_CHANNELS_RX_COUNT, /* u32 */
+ ETHA_CHANNELS_TX_COUNT, /* u32 */
+ ETHA_CHANNELS_OTHER_COUNT, /* u32 */
+ ETHA_CHANNELS_COMBINED_COUNT, /* u32 */
+
+ __ETHA_CHANNELS_MAX,
+ ETHA_CHANNELS_MAX = (__ETHA_CHANNELS_MAX - 1),
+};
+
+enum {
+ ETHA_EEE_UNSPEC,
+ ETHA_EEE_LINK_MODES, /* bitset */
+ ETHA_EEE_PEER_MODES, /* bitset */
+ ETHA_EEE_ACTIVE, /* u8 */
+ ETHA_EEE_ENABLED, /* u8 */
+ ETHA_EEE_TX_LPI_ENABLED, /* u8 */
+ ETHA_EEE_TX_LPI_TIMER, /* u32 */
+
+ __ETHA_EEE_MAX,
+ ETHA_EEE_MAX = (__ETHA_EEE_MAX - 1),
+};
+
+enum {
+ ETHA_FEC_UNSPEC,
+ ETHA_FEC_MODES, /* bitfield32 */
+
+ __ETHA_FEC_MAX,
+ ETHA_FEC_MAX = (__ETHA_FEC_MAX - 1),
+};
+
/* generic netlink info */
#define ETHTOOL_GENL_NAME "ethtool"
#define ETHTOOL_GENL_VERSION 1
diff --git a/net/core/ethtool_netlink.c b/net/core/ethtool_netlink.c
index 4b14a02be12a..3fb49427f211 100644
--- a/net/core/ethtool_netlink.c
+++ b/net/core/ethtool_netlink.c
@@ -1438,6 +1438,413 @@ static int ethnl_set_settings(struct sk_buff *skb, struct genl_info *info)
return ret;
}

+/* GET_PARAMS */
+
+static int ethnl_get_coalesce(struct net_device *dev,
+ struct ethtool_coalesce *data)
+{
+ if (!dev->ethtool_ops->get_coalesce)
+ return -EOPNOTSUPP;
+ return dev->ethtool_ops->get_coalesce(dev, data);
+}
+
+static int ethnl_get_ring(struct net_device *dev,
+ struct ethtool_ringparam *data)
+{
+ if (!dev->ethtool_ops->get_ringparam)
+ return -EOPNOTSUPP;
+ dev->ethtool_ops->get_ringparam(dev, data);
+ return 0;
+}
+
+static int ethnl_get_pause(struct net_device *dev,
+ struct ethtool_pauseparam *data)
+{
+ if (!dev->ethtool_ops->get_pauseparam)
+ return -EOPNOTSUPP;
+ dev->ethtool_ops->get_pauseparam(dev, data);
+ return 0;
+}
+
+static int ethnl_get_channels(struct net_device *dev,
+ struct ethtool_channels *data)
+{
+ if (!dev->ethtool_ops->get_channels)
+ return -EOPNOTSUPP;
+ dev->ethtool_ops->get_channels(dev, data);
+ return 0;
+}
+
+static int ethnl_get_eee(struct net_device *dev, struct ethtool_eee *data)
+{
+ if (!dev->ethtool_ops->get_eee)
+ return -EOPNOTSUPP;
+ return dev->ethtool_ops->get_eee(dev, data);
+}
+
+static int ethnl_get_fec(struct net_device *dev, struct ethtool_fecparam *data)
+{
+ if (!dev->ethtool_ops->get_fecparam)
+ return -EOPNOTSUPP;
+ return dev->ethtool_ops->get_fecparam(dev, data);
+}
+
+static int ethnl_params_size(struct ethtool_eee *eee, u16 flags, u16 mask)
+{
+ int len = 0;
+
+ if (mask & ETH_PARAMS_IM_COALESCE)
+ len += nla_total_size(20 * nla_total_size(sizeof(u32)) +
+ 2 * nla_total_size(sizeof(u8)));
+ if (mask & ETH_PARAMS_IM_RING)
+ len += nla_total_size(8 * nla_total_size(sizeof(u32)));
+ if (mask & ETH_PARAMS_IM_PAUSE)
+ len += nla_total_size(3 * nla_total_size(sizeof(u8)));
+ if (mask & ETH_PARAMS_IM_CHANNELS)
+ len += nla_total_size(8 * nla_total_size(sizeof(u32)));
+ if (mask & ETH_PARAMS_IM_EEE) {
+ int nlen = 0;
+ int ret;
+
+ /* link_modes */
+ ret = ethnl_bitset_size(flags & ETH_PARAMS_RF_COMPACT_BITSETS,
+ sizeof(u32) * 8, &eee->advertised,
+ &eee->supported, link_mode_names);
+ if (ret < 0)
+ return ret;
+ nlen += ret;
+ /* peer_modes */
+ ret = ethnl_bitset_size(flags & ETH_PARAMS_RF_COMPACT_BITSETS,
+ sizeof(u32) * 8, &eee->lp_advertised,
+ &eee->lp_advertised, link_mode_names);
+ if (ret < 0)
+ return ret;
+ nlen += ret;
+ /* active, enabled, tx_lpi_enabled */
+ nlen += 3 * nla_total_size(sizeof(u8));
+ /* tx_lpi_timer */
+ nlen += nla_total_size(sizeof(u32));
+ /* nest */
+ len += nla_total_size(nlen);
+ }
+ if (mask & ETH_PARAMS_IM_FEC) {
+ int nlen = nla_total_size(sizeof(struct nla_bitfield32));
+
+ len += nla_total_size(nlen);
+ }
+
+ return len;
+}
+
+static int fill_coalesce(struct sk_buff *skb, struct ethtool_coalesce *data)
+{
+ struct nlattr *nest = ethnl_nest_start(skb, ETHA_PARAMS_COALESCE);
+
+ if (!nest)
+ return -EMSGSIZE;
+ if (nla_put_u32(skb, ETHA_COALESCE_RX_USECS,
+ data->rx_coalesce_usecs) ||
+ nla_put_u32(skb, ETHA_COALESCE_RX_MAXFRM,
+ data->rx_max_coalesced_frames) ||
+ nla_put_u32(skb, ETHA_COALESCE_RX_USECS_IRQ,
+ data->rx_coalesce_usecs_irq) ||
+ nla_put_u32(skb, ETHA_COALESCE_RX_MAXFRM_IRQ,
+ data->rx_max_coalesced_frames_irq) ||
+ nla_put_u32(skb, ETHA_COALESCE_RX_USECS_LOW,
+ data->rx_coalesce_usecs_low) ||
+ nla_put_u32(skb, ETHA_COALESCE_RX_MAXFRM_LOW,
+ data->rx_max_coalesced_frames_low) ||
+ nla_put_u32(skb, ETHA_COALESCE_RX_USECS_HIGH,
+ data->rx_coalesce_usecs_high) ||
+ nla_put_u32(skb, ETHA_COALESCE_RX_MAXFRM_HIGH,
+ data->rx_max_coalesced_frames_high) ||
+ nla_put_u32(skb, ETHA_COALESCE_TX_USECS,
+ data->tx_coalesce_usecs) ||
+ nla_put_u32(skb, ETHA_COALESCE_TX_MAXFRM,
+ data->tx_max_coalesced_frames) ||
+ nla_put_u32(skb, ETHA_COALESCE_TX_USECS_IRQ,
+ data->tx_coalesce_usecs_irq) ||
+ nla_put_u32(skb, ETHA_COALESCE_TX_MAXFRM_IRQ,
+ data->tx_max_coalesced_frames_irq) ||
+ nla_put_u32(skb, ETHA_COALESCE_TX_USECS_LOW,
+ data->tx_coalesce_usecs_low) ||
+ nla_put_u32(skb, ETHA_COALESCE_TX_MAXFRM_LOW,
+ data->tx_max_coalesced_frames_low) ||
+ nla_put_u32(skb, ETHA_COALESCE_TX_USECS_HIGH,
+ data->tx_coalesce_usecs_high) ||
+ nla_put_u32(skb, ETHA_COALESCE_TX_MAXFRM_HIGH,
+ data->tx_max_coalesced_frames_high) ||
+ nla_put_u32(skb, ETHA_COALESCE_PKT_RATE_LOW,
+ data->pkt_rate_low) ||
+ nla_put_u32(skb, ETHA_COALESCE_PKT_RATE_HIGH,
+ data->pkt_rate_high) ||
+ nla_put_u8(skb, ETHA_COALESCE_RX_USE_ADAPTIVE,
+ !!data->use_adaptive_rx_coalesce) ||
+ nla_put_u8(skb, ETHA_COALESCE_TX_USE_ADAPTIVE,
+ !!data->use_adaptive_tx_coalesce) ||
+ nla_put_u32(skb, ETHA_COALESCE_RATE_SAMPLE_INTERVAL,
+ data->rate_sample_interval) ||
+ nla_put_u32(skb, ETHA_COALESCE_STATS_BLOCK_USECS,
+ data->stats_block_coalesce_usecs)) {
+ nla_nest_cancel(skb, nest);
+ return -EMSGSIZE;
+ }
+
+ nla_nest_end(skb, nest);
+ return 0;
+}
+
+static int fill_ring(struct sk_buff *skb, struct ethtool_ringparam *data)
+{
+ struct nlattr *nest = ethnl_nest_start(skb, ETHA_PARAMS_RING);
+
+ if (!nest)
+ return -EMSGSIZE;
+ if (nla_put_u32(skb, ETHA_RING_RX_MAX_PENDING,
+ data->rx_max_pending) ||
+ nla_put_u32(skb, ETHA_RING_RX_MINI_MAX_PENDING,
+ data->rx_mini_max_pending) ||
+ nla_put_u32(skb, ETHA_RING_RX_JUMBO_MAX_PENDING,
+ data->rx_jumbo_max_pending) ||
+ nla_put_u32(skb, ETHA_RING_TX_MAX_PENDING,
+ data->tx_max_pending) ||
+ nla_put_u32(skb, ETHA_RING_RX_PENDING,
+ data->rx_pending) ||
+ nla_put_u32(skb, ETHA_RING_RX_MINI_PENDING,
+ data->rx_mini_pending) ||
+ nla_put_u32(skb, ETHA_RING_RX_JUMBO_PENDING,
+ data->rx_jumbo_pending) ||
+ nla_put_u32(skb, ETHA_RING_TX_PENDING,
+ data->tx_pending)) {
+ nla_nest_cancel(skb, nest);
+ return -EMSGSIZE;
+ }
+
+ nla_nest_end(skb, nest);
+ return 0;
+}
+
+static int fill_pause(struct sk_buff *skb, struct ethtool_pauseparam *data)
+{
+ struct nlattr *nest = ethnl_nest_start(skb, ETHA_PARAMS_PAUSE);
+
+ if (!nest)
+ return -EMSGSIZE;
+ if (nla_put_u8(skb, ETHA_PAUSE_AUTONEG, !!data->autoneg) ||
+ nla_put_u8(skb, ETHA_PAUSE_RX, !!data->rx_pause) ||
+ nla_put_u8(skb, ETHA_PAUSE_TX, !!data->tx_pause)) {
+ nla_nest_cancel(skb, nest);
+ return -EMSGSIZE;
+ }
+
+ nla_nest_end(skb, nest);
+ return 0;
+}
+
+static int fill_channels(struct sk_buff *skb, struct ethtool_channels *data)
+{
+ struct nlattr *nest = ethnl_nest_start(skb, ETHA_PARAMS_CHANNELS);
+
+ if (!nest)
+ return -EMSGSIZE;
+ if (nla_put_u32(skb, ETHA_CHANNELS_MAX_RX, data->max_rx) ||
+ nla_put_u32(skb, ETHA_CHANNELS_MAX_TX, data->max_tx) ||
+ nla_put_u32(skb, ETHA_CHANNELS_MAX_OTHER, data->max_other) ||
+ nla_put_u32(skb, ETHA_CHANNELS_MAX_COMBINED, data->max_combined) ||
+ nla_put_u32(skb, ETHA_CHANNELS_RX_COUNT, data->rx_count) ||
+ nla_put_u32(skb, ETHA_CHANNELS_TX_COUNT, data->tx_count) ||
+ nla_put_u32(skb, ETHA_CHANNELS_OTHER_COUNT, data->other_count) ||
+ nla_put_u32(skb, ETHA_CHANNELS_COMBINED_COUNT,
+ data->combined_count)) {
+ return -EMSGSIZE;
+ }
+
+ nla_nest_end(skb, nest);
+ return 0;
+}
+
+static int fill_eee(struct sk_buff *skb, struct ethtool_eee *data, u16 flags)
+{
+ struct nlattr *nest = ethnl_nest_start(skb, ETHA_PARAMS_EEE);
+ int ret;
+
+ if (!nest)
+ return -EMSGSIZE;
+ ret = ethnl_put_bitset(skb, ETHA_EEE_LINK_MODES,
+ flags & ETH_PARAMS_RF_COMPACT_BITSETS,
+ sizeof(data->advertised) * 8, &data->advertised,
+ &data->supported, link_mode_names);
+ if (ret < 0)
+ goto err;
+ ret = ethnl_put_bitset(skb, ETHA_EEE_PEER_MODES,
+ flags & ETH_PARAMS_RF_COMPACT_BITSETS,
+ sizeof(data->advertised) * 8,
+ &data->lp_advertised, &data->lp_advertised,
+ link_mode_names);
+ if (ret < 0)
+ goto err;
+
+ if (nla_put_u8(skb, ETHA_EEE_ACTIVE, !!data->eee_active) ||
+ nla_put_u8(skb, ETHA_EEE_ENABLED, !!data->eee_enabled) ||
+ nla_put_u8(skb, ETHA_EEE_TX_LPI_ENABLED, !!data->tx_lpi_enabled) ||
+ nla_put_u32(skb, ETHA_EEE_TX_LPI_TIMER, data->tx_lpi_timer)) {
+ ret = -EMSGSIZE;
+ goto err;
+ }
+
+ nla_nest_end(skb, nest);
+ return 0;
+err:
+ nla_nest_cancel(skb, nest);
+ return ret;
+}
+
+static int fill_fec(struct sk_buff *skb, struct ethtool_fecparam *data)
+{
+ struct nlattr *nest = ethnl_nest_start(skb, ETHA_PARAMS_FEC);
+
+ if (!nest)
+ return -EMSGSIZE;
+ if (nla_put_bitfield32(skb, ETHA_FEC_MODES, data->active_fec,
+ data->fec)) {
+ nla_nest_cancel(skb, nest);
+ return -EMSGSIZE;
+ }
+
+ nla_nest_end(skb, nest);
+ return 0;
+}
+
+static int ethnl_get_params(struct sk_buff *skb, struct genl_info *info)
+{
+ struct ethtool_coalesce coalesce = {};
+ struct ethtool_channels channels = {};
+ struct ethtool_pauseparam pause = {};
+ struct ethtool_ringparam ring = {};
+ struct ethtool_fecparam fec = {};
+ struct ethtool_eee eee = {};
+ unsigned int reply_len = 0;
+ struct ethnlmsghdr *ehdr;
+ struct net_device *dev;
+ struct sk_buff *rskb;
+ u16 req_flags;
+ u16 req_mask;
+ int ret = 0;
+
+ ehdr = info->userhdr;
+ if (!ehdr->info_mask)
+ ehdr->info_mask = ETH_PARAMS_IM_DEFAULT;
+ req_mask = ehdr->info_mask;
+ req_flags = ehdr->flags;
+
+ dev = ethnl_dev_get(info);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+
+ if (req_mask & ETH_PARAMS_IM_COALESCE) {
+ ret = ethnl_get_coalesce(dev, &coalesce);
+ if (ret < 0) {
+ warn_partial_info(info);
+ req_mask &= ~ETH_PARAMS_IM_COALESCE;
+ }
+ }
+ if (req_mask & ETH_PARAMS_IM_RING) {
+ ret = ethnl_get_ring(dev, &ring);
+ if (ret < 0) {
+ warn_partial_info(info);
+ req_mask &= ~ETH_PARAMS_IM_RING;
+ }
+ }
+ if (req_mask & ETH_PARAMS_IM_PAUSE) {
+ ret = ethnl_get_pause(dev, &pause);
+ if (ret < 0) {
+ warn_partial_info(info);
+ req_mask &= ~ETH_PARAMS_IM_PAUSE;
+ }
+ }
+ if (req_mask & ETH_PARAMS_IM_CHANNELS) {
+ ret = ethnl_get_channels(dev, &channels);
+ if (ret < 0) {
+ warn_partial_info(info);
+ req_mask &= ~ETH_PARAMS_IM_CHANNELS;
+ }
+ }
+ if (req_mask & ETH_PARAMS_IM_EEE) {
+ ret = ethnl_get_eee(dev, &eee);
+ if (ret < 0) {
+ warn_partial_info(info);
+ req_mask &= ~ETH_PARAMS_IM_EEE;
+ }
+ }
+ if (req_mask & ETH_PARAMS_IM_FEC) {
+ ret = ethnl_get_fec(dev, &fec);
+ if (ret < 0) {
+ warn_partial_info(info);
+ req_mask &= ~ETH_PARAMS_IM_FEC;
+ }
+ }
+
+ ret = ethnl_params_size(&eee, req_flags, req_mask);
+ if (ret < 0)
+ goto err_putdev;
+ else
+ reply_len = ret;
+ rskb = ethnl_reply_init(reply_len, dev, ETHTOOL_CMD_SET_PARAMS, info,
+ &ehdr);
+ ret = -ENOMEM;
+ if (!rskb)
+ goto err_putdev;
+ if (req_mask != ETH_SETTINGS_IM_DEFAULT)
+ ehdr->info_mask = req_mask;
+
+ if (req_mask & ETH_PARAMS_IM_COALESCE) {
+ ret = fill_coalesce(rskb, &coalesce);
+ if (ret < 0)
+ goto err;
+ }
+ if (req_mask & ETH_PARAMS_IM_RING) {
+ ret = fill_ring(rskb, &ring);
+ if (ret < 0)
+ goto err;
+ }
+ if (req_mask & ETH_PARAMS_IM_PAUSE) {
+ ret = fill_pause(rskb, &pause);
+ if (ret < 0)
+ goto err;
+ }
+ if (req_mask & ETH_PARAMS_IM_CHANNELS) {
+ ret = fill_channels(rskb, &channels);
+ if (ret < 0)
+ goto err;
+ }
+ if (req_mask & ETH_PARAMS_IM_EEE) {
+ ret = fill_eee(rskb, &eee, req_flags);
+ if (ret < 0)
+ goto err;
+ }
+ if (req_mask & ETH_PARAMS_IM_FEC) {
+ ret = fill_fec(rskb, &fec);
+ if (ret < 0)
+ goto err;
+ }
+
+ dev_put(dev);
+ genlmsg_end(rskb, ehdr);
+ return genlmsg_reply(rskb, info);
+
+err:
+ nlmsg_free(rskb);
+err_putdev:
+ dev_put(dev);
+ if (ret == -EMSGSIZE)
+ GENL_SET_ERR_MSG(info,
+ "kernel error, see kernel log for details");
+ WARN_ONCE(ret == -EMSGSIZE,
+ "calculated message payload length (%d) not sufficient\n",
+ reply_len);
+ return ret;
+}
+
/* genetlink paperwork */

static const struct genl_ops ethtool_genl_ops[] = {
@@ -1455,6 +1862,10 @@ static const struct genl_ops ethtool_genl_ops[] = {
.policy = settings_policy,
.doit = ethnl_set_settings,
},
+ {
+ .cmd = ETHTOOL_CMD_GET_PARAMS,
+ .doit = ethnl_get_params,
+ },
};

static struct genl_family ethtool_genl_family = {
--
2.15.1

2017-12-11 13:54:52

by Michal Kubecek

[permalink] [raw]
Subject: [RFC PATCH 9/9] ethtool: implement SET_PARAMS message

Sets the information provided by ETHTOOL_SCOALESCE, ETHTOOL_SRINGPARAM,
ETHTOOL_SPAUSEPARAM, ETHTOOL_SCHANNELS, ETHTOOL_SEEE and ETHTOOL_SFECPARAM.
Each of these has corresponding nesting attribute containing attributes for
its settings. This way, userspace can request changes equivalent to one or
more of the legacy requests (and provide only part of the data to update).

Signed-off-by: Michal Kubecek <[email protected]>
---
Documentation/networking/ethtool-netlink.txt | 71 ++++-
net/core/ethtool_netlink.c | 422 +++++++++++++++++++++++++++
2 files changed, 486 insertions(+), 7 deletions(-)

diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
index 45ed1801ab50..e96c18f52356 100644
--- a/Documentation/networking/ethtool-netlink.txt
+++ b/Documentation/networking/ethtool-netlink.txt
@@ -112,7 +112,7 @@ List of message types
ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_CMD_SET_SETTINGS
ETHTOOL_CMD_GET_PARAMS
- ETHTOOL_CMD_SET_PARAMS response only (for now)
+ ETHTOOL_CMD_SET_PARAMS

All constants use ETHTOOL_CMD_ prefix followed by "GET", "SET" or "ACT" to
indicate the type.
@@ -316,6 +316,63 @@ the information is nested in it.
- active (value) and configured (selector) FEC encodings


+SET_PARAMS
+----------
+
+SET_PARAMS request modifies the settings retrieved by GET_PARAMS, i.e. it
+replaces ETHTOOL_SCOALESCE, ETHTOOL_SRINGPARAM, ETHTOOL_SPAUSEPARAM,
+ETHTOOL_SCHANNELS, ETHTOOL_SEEE and ETHTOOL_SFECPARAM legacy commands. For
+each of these, relevant data attributes are contained in a corresponding nest
+attribute. Some of the attributes provided by GET_SETPARAMS are read only and
+cannot be set by SET_PARAMS request.
+
+ ETHA_PARAMS_COALESCE (nest) coalescing
+ ETHA_COALESCE_RX_USECS (u32)
+ ETHA_COALESCE_RX_MAXFRM (u32)
+ ETHA_COALESCE_RX_USECS_IRQ (u32)
+ ETHA_COALESCE_RX_MAXFRM_IRQ (u32)
+ ETHA_COALESCE_RX_USECS_LOW (u32)
+ ETHA_COALESCE_RX_MAXFRM_LOW (u32)
+ ETHA_COALESCE_RX_USECS_HIGH (u32)
+ ETHA_COALESCE_RX_MAXFRM_HIGH (u32)
+ ETHA_COALESCE_TX_USECS (u32)
+ ETHA_COALESCE_TX_MAXFRM (u32)
+ ETHA_COALESCE_TX_USECS_IRQ (u32)
+ ETHA_COALESCE_TX_MAXFRM_IRQ (u32)
+ ETHA_COALESCE_TX_USECS_LOW (u32)
+ ETHA_COALESCE_TX_MAXFRM_LOW (u32)
+ ETHA_COALESCE_TX_USECS_HIGH (u32)
+ ETHA_COALESCE_TX_MAXFRM_HIGH (u32)
+ ETHA_COALESCE_PKT_RATE_LOW (u32)
+ ETHA_COALESCE_PKT_RATE_HIGH (u32)
+ ETHA_COALESCE_RX_USE_ADAPTIVE (bool)
+ ETHA_COALESCE_TX_USE_ADAPTIVE (bool)
+ ETHA_COALESCE_RATE_SAMPLE_INTERVAL (u32)
+ ETHA_COALESCE_STATS_BLOCK_USECS (u32)
+ ETHA_PARAMS_RING (nest) ring parameters
+ ETHA_RING_RX_PENDING (u32)
+ ETHA_RING_RX_MINI_PENDING (u32)
+ ETHA_RING_RX_JUMBO_PENDING (u32)
+ ETHA_RING_TX_PENDING (u32)
+ ETHA_PARAMS_PAUSE (nest) pause parameters
+ ETHA_PAUSE_AUTONEG (bool)
+ ETHA_PAUSE_RX (bool)
+ ETHA_PAUSE_TX (bool)
+ ETHA_PARAMS_CHANNELS (nest) channel settings
+ ETHA_CHANNELS_RX_COUNT (u32)
+ ETHA_CHANNELS_TX_COUNT (u32)
+ ETHA_CHANNELS_OTHER_COUNT (u32)
+ ETHA_CHANNELS_COMBINED_COUNT (u32)
+ ETHA_PARAMS_EEE (nest) EEE settings
+ ETHA_EEE_LINK_MODES (bitset)
+ - change modes for which EEE is advertised
+ ETHA_EEE_ENABLED (bool)
+ ETHA_EEE_TX_LPI_ENABLED (bool)
+ ETHA_EEE_TX_LPI_TIMER (u32)
+ ETHA_PARAMS_FEC (nest) FEC parameters
+ ETHA_FEC_MODES (bitfield32)
+ - change configured FEC encodings
+

Request translation
-------------------
@@ -339,11 +396,11 @@ ETHTOOL_GLINK ETHTOOL_CMD_GET_SETTINGS
ETHTOOL_GEEPROM n/a
ETHTOOL_SEEPROM n/a
ETHTOOL_GCOALESCE ETHTOOL_CMD_GET_PARAMS
-ETHTOOL_SCOALESCE n/a
+ETHTOOL_SCOALESCE ETHTOOL_CMD_SET_PARAMS
ETHTOOL_GRINGPARAM ETHTOOL_CMD_GET_PARAMS
-ETHTOOL_SRINGPARAM n/a
+ETHTOOL_SRINGPARAM ETHTOOL_CMD_SET_PARAMS
ETHTOOL_GPAUSEPARAM ETHTOOL_CMD_GET_PARAMS
-ETHTOOL_SPAUSEPARAM n/a
+ETHTOOL_SPAUSEPARAM ETHTOOL_CMD_SET_PARAMS
ETHTOOL_GRXCSUM n/a
ETHTOOL_SRXCSUM n/a
ETHTOOL_GTXCSUM n/a
@@ -385,7 +442,7 @@ ETHTOOL_SRXFHINDIR n/a
ETHTOOL_GFEATURES n/a
ETHTOOL_SFEATURES n/a
ETHTOOL_GCHANNELS ETHTOOL_CMD_GET_PARAMS
-ETHTOOL_SCHANNELS n/a
+ETHTOOL_SCHANNELS ETHTOOL_CMD_SET_PARAMS
ETHTOOL_SET_DUMP n/a
ETHTOOL_GET_DUMP_FLAG n/a
ETHTOOL_GET_DUMP_DATA n/a
@@ -393,7 +450,7 @@ ETHTOOL_GET_TS_INFO n/a
ETHTOOL_GMODULEINFO n/a
ETHTOOL_GMODULEEEPROM n/a
ETHTOOL_GEEE ETHTOOL_CMD_GET_PARAMS
-ETHTOOL_SEEE n/a
+ETHTOOL_SEEE ETHTOOL_CMD_SET_PARAMS
ETHTOOL_GRSSH n/a
ETHTOOL_SRSSH n/a
ETHTOOL_GTUNABLE n/a
@@ -405,5 +462,5 @@ ETHTOOL_SLINKSETTINGS ETHTOOL_CMD_SET_SETTINGS
ETHTOOL_PHY_GTUNABLE n/a
ETHTOOL_PHY_STUNABLE n/a
ETHTOOL_GFECPARAM ETHTOOL_CMD_GET_PARAMS
-ETHTOOL_SFECPARAM n/a
+ETHTOOL_SFECPARAM ETHTOOL_CMD_SET_PARAMS

diff --git a/net/core/ethtool_netlink.c b/net/core/ethtool_netlink.c
index 3fb49427f211..874716baaec3 100644
--- a/net/core/ethtool_netlink.c
+++ b/net/core/ethtool_netlink.c
@@ -1845,6 +1845,422 @@ static int ethnl_get_params(struct sk_buff *skb, struct genl_info *info)
return ret;
}

+/* SET_PARAMS */
+
+static const struct nla_policy params_policy[ETHA_SETTINGS_MAX + 1] = {
+ [ETHA_PARAMS_UNSPEC] = { .type = NLA_UNSPEC },
+ [ETHA_PARAMS_COALESCE] = { .type = NLA_NESTED },
+ [ETHA_PARAMS_RING] = { .type = NLA_NESTED },
+ [ETHA_PARAMS_PAUSE] = { .type = NLA_NESTED },
+ [ETHA_PARAMS_CHANNELS] = { .type = NLA_NESTED },
+ [ETHA_PARAMS_EEE] = { .type = NLA_NESTED },
+ [ETHA_PARAMS_FEC] = { .type = NLA_NESTED },
+};
+
+static const struct nla_policy coalesce_policy[ETHA_COALESCE_MAX + 1] = {
+ [ETHA_COALESCE_UNSPEC] = { .type = NLA_UNSPEC },
+ [ETHA_COALESCE_RX_USECS] = { .type = NLA_U32 },
+ [ETHA_COALESCE_RX_MAXFRM] = { .type = NLA_U32 },
+ [ETHA_COALESCE_RX_USECS_IRQ] = { .type = NLA_U32 },
+ [ETHA_COALESCE_RX_MAXFRM_IRQ] = { .type = NLA_U32 },
+ [ETHA_COALESCE_RX_USECS_LOW] = { .type = NLA_U32 },
+ [ETHA_COALESCE_RX_MAXFRM_LOW] = { .type = NLA_U32 },
+ [ETHA_COALESCE_RX_USECS_HIGH] = { .type = NLA_U32 },
+ [ETHA_COALESCE_RX_MAXFRM_HIGH] = { .type = NLA_U32 },
+ [ETHA_COALESCE_TX_USECS] = { .type = NLA_U32 },
+ [ETHA_COALESCE_TX_MAXFRM] = { .type = NLA_U32 },
+ [ETHA_COALESCE_TX_USECS_IRQ] = { .type = NLA_U32 },
+ [ETHA_COALESCE_TX_MAXFRM_IRQ] = { .type = NLA_U32 },
+ [ETHA_COALESCE_TX_USECS_LOW] = { .type = NLA_U32 },
+ [ETHA_COALESCE_TX_MAXFRM_LOW] = { .type = NLA_U32 },
+ [ETHA_COALESCE_TX_USECS_HIGH] = { .type = NLA_U32 },
+ [ETHA_COALESCE_TX_MAXFRM_HIGH] = { .type = NLA_U32 },
+ [ETHA_COALESCE_PKT_RATE_LOW] = { .type = NLA_U32 },
+ [ETHA_COALESCE_PKT_RATE_HIGH] = { .type = NLA_U32 },
+ [ETHA_COALESCE_RX_USE_ADAPTIVE] = { .type = NLA_U8 },
+ [ETHA_COALESCE_TX_USE_ADAPTIVE] = { .type = NLA_U8 },
+ [ETHA_COALESCE_RATE_SAMPLE_INTERVAL] = { .type = NLA_U32 },
+ [ETHA_COALESCE_STATS_BLOCK_USECS] = { .type = NLA_U32 },
+};
+
+static int update_coalesce(struct genl_info *info, struct net_device *dev,
+ struct nlattr *nest)
+{
+ struct nlattr *tb[ETHA_COALESCE_MAX + 1];
+ struct ethtool_coalesce data = {};
+ bool mod = false;
+ int ret;
+
+ if (!nest)
+ return 0;
+ if (!dev->ethtool_ops->get_coalesce || !dev->ethtool_ops->set_coalesce)
+ return -EOPNOTSUPP;
+ ret = dev->ethtool_ops->get_coalesce(dev, &data);
+ if (ret < 0)
+ return ret;
+
+ ret = nla_parse_nested(tb, ETHA_COALESCE_MAX, nest, coalesce_policy,
+ info->extack);
+ if (ret < 0)
+ return ret;
+
+ if (ethnl_update_u32(&data.rx_coalesce_usecs,
+ tb[ETHA_COALESCE_RX_USECS]))
+ mod = true;
+ if (ethnl_update_u32(&data.rx_max_coalesced_frames,
+ tb[ETHA_COALESCE_RX_MAXFRM]))
+ mod = true;
+ if (ethnl_update_u32(&data.rx_coalesce_usecs_irq,
+ tb[ETHA_COALESCE_RX_USECS_IRQ]))
+ mod = true;
+ if (ethnl_update_u32(&data.rx_max_coalesced_frames_irq,
+ tb[ETHA_COALESCE_RX_MAXFRM_IRQ]))
+ mod = true;
+ if (ethnl_update_u32(&data.rx_coalesce_usecs_low,
+ tb[ETHA_COALESCE_RX_USECS_LOW]))
+ mod = true;
+ if (ethnl_update_u32(&data.rx_max_coalesced_frames_low,
+ tb[ETHA_COALESCE_RX_MAXFRM_LOW]))
+ mod = true;
+ if (ethnl_update_u32(&data.rx_coalesce_usecs_high,
+ tb[ETHA_COALESCE_RX_USECS_HIGH]))
+ mod = true;
+ if (ethnl_update_u32(&data.rx_max_coalesced_frames_high,
+ tb[ETHA_COALESCE_RX_MAXFRM_HIGH]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_coalesce_usecs,
+ tb[ETHA_COALESCE_TX_USECS]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_max_coalesced_frames,
+ tb[ETHA_COALESCE_TX_MAXFRM]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_coalesce_usecs_irq,
+ tb[ETHA_COALESCE_TX_USECS_IRQ]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_max_coalesced_frames_irq,
+ tb[ETHA_COALESCE_TX_MAXFRM_IRQ]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_coalesce_usecs_low,
+ tb[ETHA_COALESCE_TX_USECS_LOW]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_max_coalesced_frames_low,
+ tb[ETHA_COALESCE_TX_MAXFRM_LOW]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_coalesce_usecs_high,
+ tb[ETHA_COALESCE_TX_USECS_HIGH]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_max_coalesced_frames_high,
+ tb[ETHA_COALESCE_TX_MAXFRM_HIGH]))
+ mod = true;
+ if (ethnl_update_u32(&data.pkt_rate_low,
+ tb[ETHA_COALESCE_PKT_RATE_LOW]))
+ mod = true;
+ if (ethnl_update_u32(&data.pkt_rate_high,
+ tb[ETHA_COALESCE_PKT_RATE_HIGH]))
+ mod = true;
+ if (ethnl_update_bool32(&data.use_adaptive_rx_coalesce,
+ tb[ETHA_COALESCE_RX_USE_ADAPTIVE]))
+ mod = true;
+ if (ethnl_update_bool32(&data.use_adaptive_tx_coalesce,
+ tb[ETHA_COALESCE_TX_USE_ADAPTIVE]))
+ mod = true;
+ if (ethnl_update_u32(&data.rate_sample_interval,
+ tb[ETHA_COALESCE_RATE_SAMPLE_INTERVAL]))
+ mod = true;
+ if (ethnl_update_u32(&data.stats_block_coalesce_usecs,
+ tb[ETHA_COALESCE_STATS_BLOCK_USECS]))
+ mod = true;
+
+ if (mod)
+ return dev->ethtool_ops->set_coalesce(dev, &data);
+ else
+ return 0;
+}
+
+static const struct nla_policy ring_policy[ETHA_RING_MAX + 1] = {
+ [ETHA_RING_UNSPEC] = { .type = NLA_UNSPEC },
+ [ETHA_RING_RX_MAX_PENDING] = { .type = NLA_U32 },
+ [ETHA_RING_RX_MINI_MAX_PENDING] = { .type = NLA_U32 },
+ [ETHA_RING_RX_JUMBO_MAX_PENDING] = { .type = NLA_U32 },
+ [ETHA_RING_TX_MAX_PENDING] = { .type = NLA_U32 },
+ [ETHA_RING_RX_PENDING] = { .type = NLA_U32 },
+ [ETHA_RING_RX_MINI_PENDING] = { .type = NLA_U32 },
+ [ETHA_RING_RX_JUMBO_PENDING] = { .type = NLA_U32 },
+ [ETHA_RING_TX_PENDING] = { .type = NLA_U32 },
+};
+
+static int update_ring(struct genl_info *info, struct net_device *dev,
+ struct nlattr *nest)
+{
+ struct nlattr *tb[ETHA_RING_MAX + 1];
+ struct ethtool_ringparam data = {};
+ bool mod = false;
+ int ret;
+
+ if (!nest)
+ return 0;
+ if (!dev->ethtool_ops->get_ringparam ||
+ !dev->ethtool_ops->set_ringparam)
+ return -EOPNOTSUPP;
+ dev->ethtool_ops->get_ringparam(dev, &data);
+
+ ret = nla_parse_nested(tb, ETHA_RING_MAX, nest, ring_policy,
+ info->extack);
+ if (ret < 0)
+ return ret;
+ /* read only attributes */
+ if (tb[ETHA_RING_RX_MAX_PENDING] || tb[ETHA_RING_RX_MINI_MAX_PENDING] ||
+ tb[ETHA_RING_RX_JUMBO_MAX_PENDING] ||
+ tb[ETHA_RING_TX_MAX_PENDING]) {
+ GENL_SET_ERR_MSG(info, "attempt to set a read only attribute");
+ return -EINVAL;
+ }
+
+ if (ethnl_update_u32(&data.rx_pending, tb[ETHA_RING_RX_PENDING]))
+ mod = true;
+ if (ethnl_update_u32(&data.rx_mini_pending,
+ tb[ETHA_RING_RX_MINI_PENDING]))
+ mod = true;
+ if (ethnl_update_u32(&data.rx_jumbo_pending,
+ tb[ETHA_RING_RX_JUMBO_PENDING]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_pending, tb[ETHA_RING_TX_PENDING]))
+ mod = true;
+
+ if (mod)
+ return dev->ethtool_ops->set_ringparam(dev, &data);
+ else
+ return 0;
+}
+
+static const struct nla_policy pause_policy[ETHA_PAUSE_MAX + 1] = {
+ [ETHA_PAUSE_UNSPEC] = { .type = NLA_UNSPEC },
+ [ETHA_PAUSE_AUTONEG] = { .type = NLA_U8 },
+ [ETHA_PAUSE_RX] = { .type = NLA_U8 },
+ [ETHA_PAUSE_TX] = { .type = NLA_U8 },
+};
+
+static int update_pause(struct genl_info *info, struct net_device *dev,
+ struct nlattr *nest)
+{
+ struct nlattr *tb[ETHA_RING_MAX + 1];
+ struct ethtool_pauseparam data = {};
+ bool mod = false;
+ int ret;
+
+ if (!nest)
+ return 0;
+ if (!dev->ethtool_ops->get_pauseparam ||
+ !dev->ethtool_ops->set_pauseparam)
+ return -EOPNOTSUPP;
+ dev->ethtool_ops->get_pauseparam(dev, &data);
+
+ ret = nla_parse_nested(tb, ETHA_PAUSE_MAX, nest, pause_policy,
+ info->extack);
+ if (ret < 0)
+ return ret;
+
+ if (ethnl_update_u32(&data.autoneg, tb[ETHA_PAUSE_AUTONEG]))
+ mod = true;
+ if (ethnl_update_u32(&data.rx_pause, tb[ETHA_PAUSE_RX]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_pause, tb[ETHA_PAUSE_TX]))
+ mod = true;
+
+ if (mod)
+ return dev->ethtool_ops->set_pauseparam(dev, &data);
+ else
+ return 0;
+}
+
+static const struct nla_policy channels_policy[ETHA_CHANNELS_MAX + 1] = {
+ [ETHA_CHANNELS_UNSPEC] = { .type = NLA_UNSPEC },
+ [ETHA_CHANNELS_MAX_RX] = { .type = NLA_U32 },
+ [ETHA_CHANNELS_MAX_TX] = { .type = NLA_U32 },
+ [ETHA_CHANNELS_MAX_OTHER] = { .type = NLA_U32 },
+ [ETHA_CHANNELS_MAX_COMBINED] = { .type = NLA_U32 },
+ [ETHA_CHANNELS_RX_COUNT] = { .type = NLA_U32 },
+ [ETHA_CHANNELS_TX_COUNT] = { .type = NLA_U32 },
+ [ETHA_CHANNELS_OTHER_COUNT] = { .type = NLA_U32 },
+ [ETHA_CHANNELS_COMBINED_COUNT] = { .type = NLA_U32 },
+};
+
+static int update_channels(struct genl_info *info, struct net_device *dev,
+ struct nlattr *nest)
+{
+ struct nlattr *tb[ETHA_CHANNELS_MAX + 1];
+ struct ethtool_channels data = {};
+ bool mod = false;
+ int ret;
+
+ if (!nest)
+ return 0;
+ if (!dev->ethtool_ops->get_channels ||
+ !dev->ethtool_ops->set_channels)
+ return -EOPNOTSUPP;
+ dev->ethtool_ops->get_channels(dev, &data);
+
+ ret = nla_parse_nested(tb, ETHA_CHANNELS_MAX, nest, channels_policy,
+ info->extack);
+ if (ret < 0)
+ return ret;
+ /* read only attributes */
+ if (tb[ETHA_CHANNELS_MAX_RX] || tb[ETHA_CHANNELS_MAX_TX] ||
+ tb[ETHA_CHANNELS_MAX_OTHER] || tb[ETHA_CHANNELS_MAX_COMBINED]) {
+ GENL_SET_ERR_MSG(info, "attempt to set a read only attribute");
+ return -EINVAL;
+ }
+
+ if (ethnl_update_u32(&data.rx_count, tb[ETHA_CHANNELS_RX_COUNT]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_count, tb[ETHA_CHANNELS_TX_COUNT]))
+ mod = true;
+ if (ethnl_update_u32(&data.other_count, tb[ETHA_CHANNELS_OTHER_COUNT]))
+ mod = true;
+ if (ethnl_update_u32(&data.combined_count,
+ tb[ETHA_CHANNELS_COMBINED_COUNT]))
+ mod = true;
+
+ if (mod)
+ return dev->ethtool_ops->set_channels(dev, &data);
+ else
+ return 0;
+}
+
+static const struct nla_policy eee_policy[ETHA_EEE_MAX + 1] = {
+ [ETHA_EEE_UNSPEC] = { .type = NLA_UNSPEC },
+ [ETHA_EEE_LINK_MODES] = { .type = NLA_NESTED },
+ [ETHA_EEE_PEER_MODES] = { .type = NLA_NESTED },
+ [ETHA_EEE_ACTIVE] = { .type = NLA_U8 },
+ [ETHA_EEE_ENABLED] = { .type = NLA_U8 },
+ [ETHA_EEE_TX_LPI_ENABLED] = { .type = NLA_U8 },
+ [ETHA_EEE_TX_LPI_TIMER] = { .type = NLA_U32 },
+};
+
+static int update_eee(struct genl_info *info, struct net_device *dev,
+ struct nlattr *nest)
+{
+ struct nlattr *tb[ETHA_EEE_MAX + 1];
+ struct ethtool_eee data = {};
+ bool mod = false;
+ int ret;
+
+ if (!nest)
+ return 0;
+ if (!dev->ethtool_ops->get_eee ||
+ !dev->ethtool_ops->set_eee)
+ return -EOPNOTSUPP;
+ ret = dev->ethtool_ops->get_eee(dev, &data);
+ if (ret < 0)
+ return ret;
+
+ ret = nla_parse_nested(tb, ETHA_EEE_MAX, nest, eee_policy,
+ info->extack);
+ if (ret < 0)
+ return ret;
+ /* read only attributes */
+ if (tb[ETHA_EEE_PEER_MODES] || tb[ETHA_EEE_ACTIVE]) {
+ GENL_SET_ERR_MSG(info, "attempt to set a read only attribute");
+ return -EINVAL;
+ }
+
+ if (ethnl_update_bitset32(&data.advertised, 32, tb[ETHA_EEE_LINK_MODES],
+ &ret, link_mode_names, info))
+ mod = true;
+ if (ret < 0)
+ return ret;
+ if (ethnl_update_bool32(&data.eee_enabled, tb[ETHA_EEE_ENABLED]))
+ mod = true;
+ if (ethnl_update_bool32(&data.tx_lpi_enabled,
+ tb[ETHA_EEE_TX_LPI_ENABLED]))
+ mod = true;
+ if (ethnl_update_u32(&data.tx_lpi_timer, tb[ETHA_EEE_TX_LPI_TIMER]))
+ mod = true;
+
+ if (mod)
+ return dev->ethtool_ops->set_eee(dev, &data);
+ else
+ return 0;
+}
+
+static const struct nla_policy fec_policy[ETHA_FEC_MAX + 1] = {
+ [ETHA_FEC_UNSPEC] = { .type = NLA_UNSPEC },
+ [ETHA_FEC_MODES] = { .type = NLA_U32 },
+};
+
+static int update_fec(struct genl_info *info, struct net_device *dev,
+ struct nlattr *nest)
+{
+ struct nlattr *tb[ETHA_FEC_MAX + 1];
+ struct ethtool_fecparam data = {};
+ bool mod = false;
+ int ret;
+
+ if (!nest)
+ return 0;
+ if (!dev->ethtool_ops->get_fecparam ||
+ !dev->ethtool_ops->set_fecparam)
+ return -EOPNOTSUPP;
+ ret = dev->ethtool_ops->get_fecparam(dev, &data);
+ if (ret < 0)
+ return ret;
+
+ ret = nla_parse_nested(tb, ETHA_FEC_MAX, nest, fec_policy,
+ info->extack);
+ if (ret < 0)
+ return ret;
+
+ if (ethnl_update_bitfield32(&data.fec, tb[ETHA_FEC_MODES]))
+ mod = true;
+
+ if (mod)
+ return dev->ethtool_ops->set_fecparam(dev, &data);
+ else
+ return 0;
+}
+
+static int ethnl_set_params(struct sk_buff *skb, struct genl_info *info)
+{
+ struct nlattr *tb[ETHA_PARAMS_MAX + 1];
+ struct ethnlmsghdr *ehdr;
+ struct net_device *dev;
+ int ret;
+
+ ehdr = info->userhdr;
+ dev = ethnl_dev_get(info);
+ if (IS_ERR(dev))
+ return PTR_ERR(dev);
+
+ ret = genlmsg_parse(info->nlhdr, &ethtool_genl_family, tb,
+ ETHA_PARAMS_MAX, params_policy, info->extack);
+ if (ret < 0)
+ goto err_putdev;
+
+ ret = update_coalesce(info, dev, tb[ETHA_PARAMS_COALESCE]);
+ if (ret < 0)
+ goto err_putdev;
+ ret = update_ring(info, dev, tb[ETHA_PARAMS_RING]);
+ if (ret < 0)
+ goto err_putdev;
+ ret = update_pause(info, dev, tb[ETHA_PARAMS_PAUSE]);
+ if (ret < 0)
+ goto err_putdev;
+ ret = update_channels(info, dev, tb[ETHA_PARAMS_CHANNELS]);
+ if (ret < 0)
+ goto err_putdev;
+ ret = update_eee(info, dev, tb[ETHA_PARAMS_EEE]);
+ if (ret < 0)
+ goto err_putdev;
+ ret = update_fec(info, dev, tb[ETHA_PARAMS_FEC]);
+ if (ret < 0)
+ goto err_putdev;
+
+ ret = 0;
+err_putdev:
+ dev_put(dev);
+ return ret;
+}
+
/* genetlink paperwork */

static const struct genl_ops ethtool_genl_ops[] = {
@@ -1866,6 +2282,12 @@ static const struct genl_ops ethtool_genl_ops[] = {
.cmd = ETHTOOL_CMD_GET_PARAMS,
.doit = ethnl_get_params,
},
+ {
+ .cmd = ETHTOOL_CMD_SET_PARAMS,
+ .flags = GENL_UNS_ADMIN_PERM,
+ .policy = params_policy,
+ .doit = ethnl_set_params,
+ },
};

static struct genl_family ethtool_genl_family = {
--
2.15.1

2017-12-11 16:02:29

by Jiri Pirko

[permalink] [raw]
Subject: Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

Mon, Dec 11, 2017 at 02:53:31PM CET, [email protected] wrote:
>No function implemented yet, only genetlink and module infrastructure.
>Register/unregister genetlink family "ethtool" and allow the module to be
>autoloaded by genetlink code (if built as a module, distributions would
>probably prefer "y").
>
>Signed-off-by: Michal Kubecek <[email protected]>

[...]


>+
>+/* identifies the device to query/set
>+ * - use either ifindex or ifname, not both
>+ * - for dumps and messages not related to a particular devices, fill neither
>+ * - info_mask is a bitfield, interpretation depends on the command
>+ */
>+struct ethnlmsghdr {
>+ __u32 ifindex; /* device ifindex */
>+ __u16 flags; /* request/response flags */
>+ __u16 info_mask; /* request/response info mask */
>+ char ifname[IFNAMSIZ]; /* device name */

Why do you need this header? You can have 2 attrs:
ETHTOOL_ATTR_IFINDEX and
ETHTOOL_ATTR_IFNAME

Why do you need per-command flags and info_mask? Could be bitfield
attr if needed by specific command.



>+};
>+
>+#define ETHNL_HDRLEN NLMSG_ALIGN(sizeof(struct ethnlmsghdr))
>+
>+enum {
>+ ETHTOOL_CMD_NOOP,
>+
>+ __ETHTOOL_CMD_MAX,
>+ ETHTOOL_CMD_MAX = (__ETHTOOL_CMD_MAX - 1),
>+};
>+
>+/* generic netlink info */
>+#define ETHTOOL_GENL_NAME "ethtool"
>+#define ETHTOOL_GENL_VERSION 1
>+
>+#endif /* _UAPI_LINUX_ETHTOOL_NETLINK_H_ */
>diff --git a/net/Kconfig b/net/Kconfig
>index 9dba2715919d..a5e3c89a2495 100644
>--- a/net/Kconfig
>+++ b/net/Kconfig
>@@ -440,6 +440,13 @@ config MAY_USE_DEVLINK
> on MAY_USE_DEVLINK to ensure they do not cause link errors when
> devlink is a loadable module and the driver using it is built-in.
>
>+config ETHTOOL_NETLINK
>+ tristate "Netlink interface for ethtool"
>+ default m
>+ help
>+ New netlink based interface for ethtool which is going to obsolete
>+ the old ioctl based one once it provides all features.
>+
> endif # if NET
>
> # Used by archs to tell that they support BPF JIT compiler plus which flavour.
>diff --git a/net/core/Makefile b/net/core/Makefile
>index 1fd0a9c88b1b..617ab2abecdf 100644
>--- a/net/core/Makefile
>+++ b/net/core/Makefile
>@@ -30,3 +30,4 @@ obj-$(CONFIG_DST_CACHE) += dst_cache.o
> obj-$(CONFIG_HWBM) += hwbm.o
> obj-$(CONFIG_NET_DEVLINK) += devlink.o
> obj-$(CONFIG_GRO_CELLS) += gro_cells.o
>+obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_netlink.o
>diff --git a/net/core/ethtool_netlink.c b/net/core/ethtool_netlink.c
>new file mode 100644
>index 000000000000..46a226bb9a2c
>--- /dev/null
>+++ b/net/core/ethtool_netlink.c
>@@ -0,0 +1,46 @@
>+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>+
>+#include <linux/module.h>
>+#include <linux/ethtool_netlink.h>
>+#include <linux/netdevice.h>
>+#include <net/genetlink.h>
>+#include "ethtool_common.h"
>+
>+static struct genl_family ethtool_genl_family;
>+
>+/* genetlink paperwork */

What "paperwork"? :)


>+
>+static const struct genl_ops ethtool_genl_ops[] = {
>+};
>+
>+static struct genl_family ethtool_genl_family = {
>+ .hdrsize = ETHNL_HDRLEN,
>+ .name = ETHTOOL_GENL_NAME,
>+ .version = ETHTOOL_GENL_VERSION,
>+ .netnsok = true,
>+ .ops = ethtool_genl_ops,
>+ .n_ops = ARRAY_SIZE(ethtool_genl_ops),
>+};
>+
>+/* module paperwork */
>+
>+static int __init ethtool_nl_init(void)
>+{
>+ return genl_register_family(&ethtool_genl_family);
>+}
>+
>+static void __exit ethtool_nl_exit(void)
>+{
>+ genl_unregister_family(&ethtool_genl_family);
>+}
>+
>+module_init(ethtool_nl_init);
>+module_exit(ethtool_nl_exit);
>+
>+/* this alias is for autoload */
>+MODULE_ALIAS("net-pf-" __stringify(PF_NETLINK)
>+ "-proto-" __stringify(NETLINK_GENERIC)
>+ "-family-" ETHTOOL_GENL_NAME);

You can use MODULE_ALIAS_GENL_FAMILY instead.


>+MODULE_AUTHOR("Michal Kubecek <[email protected]>");
>+MODULE_DESCRIPTION("Netlink interface for ethtool");
>+MODULE_LICENSE("GPL");

"GPL v2"?


>--
>2.15.1
>

2017-12-11 16:16:14

by Jiri Pirko

[permalink] [raw]
Subject: Re: [RFC PATCH 5/9] ethtool: implement GET_DRVINFO message

Mon, Dec 11, 2017 at 02:54:01PM CET, [email protected] wrote:
>Request the same information as ETHTOOL_GDRVINFO. This is read-only so that
>corresponding SET_DRVINFO exists but is only used in kernel replies. Rip
>the code to query the driver out of the legacy interface and move it to
>a new file ethtool_common.c so that both interfaces can use it.
>
>Signed-off-by: Michal Kubecek <[email protected]>
>---
> Documentation/networking/ethtool-netlink.txt | 30 ++++++++++-
> include/uapi/linux/ethtool_netlink.h | 21 ++++++++
> net/core/Makefile | 2 +-
> net/core/ethtool.c | 42 +++-------------
> net/core/ethtool_common.c | 46 +++++++++++++++++
> net/core/ethtool_common.h | 11 ++++
> net/core/ethtool_netlink.c | 75 ++++++++++++++++++++++++++++
> 7 files changed, 189 insertions(+), 38 deletions(-)
> create mode 100644 net/core/ethtool_common.c
> create mode 100644 net/core/ethtool_common.h
>
>diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt
>index 893e5156f6a7..cb992180b211 100644
>--- a/Documentation/networking/ethtool-netlink.txt
>+++ b/Documentation/networking/ethtool-netlink.txt
>@@ -107,6 +107,9 @@ which the request applies.
> List of message types
> ---------------------
>
>+ ETHTOOL_CMD_GET_DRVINFO
>+ ETHTOOL_CMD_SET_DRVINFO response only
>+
> All constants use ETHTOOL_CMD_ prefix followed by "GET", "SET" or "ACT" to
> indicate the type.
>
>@@ -119,6 +122,31 @@ messages marked as "response only" in the table above.
> Later sections describe the format and semantics of these request messages.
>
>
>+GET_DRVINFO
>+-----------
>+
>+GET_DRVINFO request corresponds to ETHTOOL_GDRVINFO ioctl command and provides
>+basic driver information. The request doesn't use any attributes and flags,
>+info_mask and index field in request header are ignored. Kernel response
>+contents:
>+
>+ ETHA_DRVINFO_DRIVER (string) driver name
>+ ETHA_DRVINFO_VERSION (string) driver version

You use 3 prefixes:
ETHTOOL_ for cmd
ETHA_ for attrs
ethnl_ for function

I suggest to sync this, perhaps to:
ETHNL_CMD_*
ETHNL_ATTR_*
ethnl_*
?


>+ ETHA_DRVINFO_FWVERSION (string) firmware version
>+ ETHA_DRVINFO_BUSINFO (string) device bus address
>+ ETHA_DRVINFO_EROM_VER (string) expansion ROM version
>+ ETHA_DRVINFO_N_PRIV_FLAGS (u32) number of private flags
>+ ETHA_DRVINFO_N_STATS (u32) number of device stats
>+ ETHA_DRVINFO_TESTINFO_LEN (u32) number of test results
>+ ETHA_DRVINFO_EEDUMP_LEN (u32) EEPROM dump size
>+ ETHA_DRVINFO_REGDUMP_LEN (u32) register dump size

We are now working on providing various fw memory regions dump in
devlink. It makes sense to have it in devlink for couple of reasons:
1) per-asic, not netdev specific, therefore does not really make sense
to have netdev as handle, but rather devlink handle.
2) snapshot support - we need to provide support for getting snapshot
(for example on failure), transferring to user and deleting it
(remove from driver memory).

Also, driver name, version, fwversion, etc is per-asic. Would make sense
to have it in devlink as well.

I think this is great opprotunity to move things where they should be to
be alligned with the current world and kernel infrastructure.

2017-12-11 16:33:11

by Jiri Pirko

[permalink] [raw]
Subject: Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)

Mon, Dec 11, 2017 at 02:53:11PM CET, [email protected] wrote:
>This is still work in progress and only a very small part of the ioctl
>interface is reimplemented but I would like to get some comments before
>the patchset becomes too big and changing things becomes too tedious.
>
>The interface used for communication between ethtool and kernel is based on
>ioctl() and suffers from many problems. The most pressing seems the be the
>lack of extensibility. While some of the newer commands use structures
>designed to allow future extensions (e.g. GFEATURES or TEST), most either
>allow no extension at all (GPAUSEPARAM, GCOALESCE) or only limited set of
>reserved fields (GDRVINFO, GEEE). Even most of those which support future
>extensions limit the data types that can be used.
>
>This series aims to provide an alternative interface based on netlink which
>is what other network configuration utilities use. In particular, it uses
>generic netlink (family "ethtool"). The goal is to provide an interface
>which would be extensible, flexible and practical both for ethtool and for
>other network configuration tools (e.g. wicked or systemd-networkd).
>
>The interface is documented in Documentation/networking/ethtool-netlink.txt
>
>I'll post RFC patch series for ethtool in a few days, it still needs some
>more polishing.

First of all, thank you Michale for taking a stab at this!

I think that it does not make sense to convert ethtool->netlink_ethtool
1:1 feature wise. Now we have devlink, ritch switch representation
model, tc offload and many others. Lot of things that are in
ethtool, should be done in devlink. Also, there are couple of things
that should just die - nice example is ethtool --config-ntuple - we
should use tc for that.

So I believe that it would be better to introduce new iterface called
differently, to avoid confusion, like "ethlink" and start to migrate
things there, without existing baggage. In kernel the existing ethtool
would just call compat layer inside ethlink code. Similarly with devlink.
ethtool api would freeze. Similar scenario to rtnetlink and legacy ioctl.

Also, this new netlink iface should have userspace notification
support from day 1.

2017-12-11 16:56:55

by David Miller

[permalink] [raw]
Subject: Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

From: Jiri Pirko <[email protected]>
Date: Mon, 11 Dec 2017 17:02:21 +0100

> Mon, Dec 11, 2017 at 02:53:31PM CET, [email protected] wrote:
>>No function implemented yet, only genetlink and module infrastructure.
>>Register/unregister genetlink family "ethtool" and allow the module to be
>>autoloaded by genetlink code (if built as a module, distributions would
>>probably prefer "y").
>>
>>Signed-off-by: Michal Kubecek <[email protected]>
>
> [...]
>
>
>>+
>>+/* identifies the device to query/set
>>+ * - use either ifindex or ifname, not both
>>+ * - for dumps and messages not related to a particular devices, fill neither
>>+ * - info_mask is a bitfield, interpretation depends on the command
>>+ */
>>+struct ethnlmsghdr {
>>+ __u32 ifindex; /* device ifindex */
>>+ __u16 flags; /* request/response flags */
>>+ __u16 info_mask; /* request/response info mask */
>>+ char ifname[IFNAMSIZ]; /* device name */
>
> Why do you need this header? You can have 2 attrs:
> ETHTOOL_ATTR_IFINDEX and
> ETHTOOL_ATTR_IFNAME
>
> Why do you need per-command flags and info_mask? Could be bitfield
> attr if needed by specific command.

Jiri, we've had this discussion before :-)

For elements which are common to most, if not all, requests it makes
sense to define a base netlink message.

My opinion on this matter has not changed at all since the last time
we discussed this.

So unless you have new information to provide to me on this issue
which might change my mind, please accept the result of the previous
discussion which is that a base netlink message is not only
appropriate but desirable.

Thank you.

2017-12-11 17:01:52

by David Miller

[permalink] [raw]
Subject: Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)

From: Jiri Pirko <[email protected]>
Date: Mon, 11 Dec 2017 17:32:46 +0100

> I think that it does not make sense to convert ethtool->netlink_ethtool
> 1:1 feature wise. Now we have devlink, ritch switch representation
> model, tc offload and many others. Lot of things that are in
> ethtool, should be done in devlink. Also, there are couple of things
> that should just die - nice example is ethtool --config-ntuple - we
> should use tc for that.

Whilst I do agree that devlink is probably a good place for this stuff
(we want to be able to do ethetool things on objects that lack a netdev)
I do not agree with the tc angle.

It is entirely appropriate to set the ntuple settings of a driver
without being required to use TC or similar.

All you are going to do with your suggestion is make people keep using
the existing ethtool ioctl, because they'll say "screw this, I'm not
using TC I have something which works just fine already". And that's
not the goal of putting this stuff into netlink, we want people to
use the new facilities and move off of the ioctl.

2017-12-11 17:59:32

by Jiri Pirko

[permalink] [raw]
Subject: Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)

Mon, Dec 11, 2017 at 06:01:44PM CET, [email protected] wrote:
>From: Jiri Pirko <[email protected]>
>Date: Mon, 11 Dec 2017 17:32:46 +0100
>
>> I think that it does not make sense to convert ethtool->netlink_ethtool
>> 1:1 feature wise. Now we have devlink, ritch switch representation
>> model, tc offload and many others. Lot of things that are in
>> ethtool, should be done in devlink. Also, there are couple of things
>> that should just die - nice example is ethtool --config-ntuple - we
>> should use tc for that.
>
>Whilst I do agree that devlink is probably a good place for this stuff
>(we want to be able to do ethetool things on objects that lack a netdev)
>I do not agree with the tc angle.
>
>It is entirely appropriate to set the ntuple settings of a driver
>without being required to use TC or similar.
>
>All you are going to do with your suggestion is make people keep using
>the existing ethtool ioctl, because they'll say "screw this, I'm not
>using TC I have something which works just fine already". And that's
>not the goal of putting this stuff into netlink, we want people to
>use the new facilities and move off of the ioctl.

Sure, but this is a great opportunity to avoid copying old mistakes.
That is why I suggested to do it not 1:1 but rather introduce brand new
netlink-based interface that would not carry old baggage.

2017-12-11 18:02:23

by Jiri Pirko

[permalink] [raw]
Subject: Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

Mon, Dec 11, 2017 at 05:56:51PM CET, [email protected] wrote:
>From: Jiri Pirko <[email protected]>
>Date: Mon, 11 Dec 2017 17:02:21 +0100
>
>> Mon, Dec 11, 2017 at 02:53:31PM CET, [email protected] wrote:
>>>No function implemented yet, only genetlink and module infrastructure.
>>>Register/unregister genetlink family "ethtool" and allow the module to be
>>>autoloaded by genetlink code (if built as a module, distributions would
>>>probably prefer "y").
>>>
>>>Signed-off-by: Michal Kubecek <[email protected]>
>>
>> [...]
>>
>>
>>>+
>>>+/* identifies the device to query/set
>>>+ * - use either ifindex or ifname, not both
>>>+ * - for dumps and messages not related to a particular devices, fill neither
>>>+ * - info_mask is a bitfield, interpretation depends on the command
>>>+ */
>>>+struct ethnlmsghdr {
>>>+ __u32 ifindex; /* device ifindex */
>>>+ __u16 flags; /* request/response flags */
>>>+ __u16 info_mask; /* request/response info mask */
>>>+ char ifname[IFNAMSIZ]; /* device name */
>>
>> Why do you need this header? You can have 2 attrs:
>> ETHTOOL_ATTR_IFINDEX and
>> ETHTOOL_ATTR_IFNAME
>>
>> Why do you need per-command flags and info_mask? Could be bitfield
>> attr if needed by specific command.
>
>Jiri, we've had this discussion before :-)
>
>For elements which are common to most, if not all, requests it makes
>sense to define a base netlink message.
>
>My opinion on this matter has not changed at all since the last time
>we discussed this.

The discussion we had before was about flag bitfield that was there
*always*. In this case, that is not true. It is either ifindex or
ifname. Even rtnetlink has ifname as attribute.

The flags and info_mask is just big mystery. If it is per-command,
seems natural to have it as attributes.


>
>So unless you have new information to provide to me on this issue
>which might change my mind, please accept the result of the previous
>discussion which is that a base netlink message is not only
>appropriate but desirable.
>
>Thank you.

2017-12-11 18:45:52

by David Miller

[permalink] [raw]
Subject: Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

From: Jiri Pirko <[email protected]>
Date: Mon, 11 Dec 2017 19:02:19 +0100

> The discussion we had before was about flag bitfield that was there
> *always*. In this case, that is not true. It is either ifindex or
> ifname. Even rtnetlink has ifname as attribute.
>
> The flags and info_mask is just big mystery. If it is per-command,
> seems natural to have it as attributes.

I think flags and info_mask indeed can be moved out of this struct.

I guess, in this case, I can see your point of view especially if we
allow ethtool operations on non-netdev entities.

So, ok, let's move forward without a base command struct and just
use attributes.

Thanks :)

2017-12-11 19:03:51

by Florian Fainelli

[permalink] [raw]
Subject: Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)

On 12/11/2017 09:01 AM, David Miller wrote:
> From: Jiri Pirko <[email protected]>
> Date: Mon, 11 Dec 2017 17:32:46 +0100
>
>> I think that it does not make sense to convert ethtool->netlink_ethtool
>> 1:1 feature wise. Now we have devlink, ritch switch representation
>> model, tc offload and many others. Lot of things that are in
>> ethtool, should be done in devlink. Also, there are couple of things
>> that should just die - nice example is ethtool --config-ntuple - we
>> should use tc for that.
>
> Whilst I do agree that devlink is probably a good place for this stuff
> (we want to be able to do ethetool things on objects that lack a netdev)
> I do not agree with the tc angle.
>
> It is entirely appropriate to set the ntuple settings of a driver
> without being required to use TC or similar.
>
> All you are going to do with your suggestion is make people keep using
> the existing ethtool ioctl, because they'll say "screw this, I'm not
> using TC I have something which works just fine already". And that's
> not the goal of putting this stuff into netlink, we want people to
> use the new facilities and move off of the ioctl.

I agree, we can't walk away from that feature today, there are many more
drivers implementing ethtool::ntuple (counting 22) than there are
implementing cls_flower (counting 6), also they are not strictly
equivalent feature wise, one thing critically missing in cls_flower
(last I looked) is the ability to either auto-place (RX_CLS_LOC_ANY) a
matching rule, or select a particular location. For specifying what to
match in a packet and how, cls_flower is superior.

We can probably advise people not to use that feature and request their
driver provider to switch over to cls_flower, but considering how many
more LOCs are needed in the driver to implement that (as opposed to
ethtool ntuple), I just don't see it being something people will be
thrilled to do.

Writing a shim that converts from ethtool ntuple to the equivalent in
kernel space representation of the same call through cls_flower would be
reasonably easy, but same thing, that still requires migrating your
kernel driver over to the cls_flower interface, which is the harder part.

That being said, we should probably discourage implementing both for new
drivers, since they are likely to use the same centralized HW resources
towards the same goals: match + actions, with no visibility into one
another (can't see rules set-up in ethtool via cls_flower and vice
versa) and that just sucks.
--
Florian

2017-12-12 15:32:50

by Roopa Prabhu

[permalink] [raw]
Subject: Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)

On Mon, Dec 11, 2017 at 5:53 AM, Michal Kubecek <[email protected]> wrote:
> This is still work in progress and only a very small part of the ioctl
> interface is reimplemented but I would like to get some comments before
> the patchset becomes too big and changing things becomes too tedious.
>
> The interface used for communication between ethtool and kernel is based on
> ioctl() and suffers from many problems. The most pressing seems the be the
> lack of extensibility. While some of the newer commands use structures
> designed to allow future extensions (e.g. GFEATURES or TEST), most either
> allow no extension at all (GPAUSEPARAM, GCOALESCE) or only limited set of
> reserved fields (GDRVINFO, GEEE). Even most of those which support future
> extensions limit the data types that can be used.
>
> This series aims to provide an alternative interface based on netlink which
> is what other network configuration utilities use. In particular, it uses
> generic netlink (family "ethtool"). The goal is to provide an interface
> which would be extensible, flexible and practical both for ethtool and for
> other network configuration tools (e.g. wicked or systemd-networkd).
>
> The interface is documented in Documentation/networking/ethtool-netlink.txt
>
> I'll post RFC patch series for ethtool in a few days, it still needs some
> more polishing.
>
> Basic concepts:
>
> - the interface is based on generic netlink (family name "ethtool")
>
> - provide everything ioctl can do but allow easier future extensions
>
> - inextensibility of ioctl interface resulted in way too many commands,
> many of them obsoleted by newer ones; reduce the number by ignoring the
> obsolete commands and grouping some together
>
> - for "set" type commands, netlink allows providing only the attributes to
> be changed; therefore we don't need a get-modify-set cycle, userspace can
> simply say what it wants to change and how
>
> - be less dependent on ethtool and kernel being in sync; allow e.g. saying
> "ethtool -s eth0 advertise xyz off" without knowing what "xyz" means;
> it's kernel's job to know what mode "xyz" is and if it exists and is
> supported
>
> Unresolved questions/tasks:
>
> - allow dumps for "get" type requests, e.g. listing EEE settings for all
> interfaces in current netns
>
> - find reasonable format for data transfers (e.g. eeprom dump or flash)
>
> - while the netlink interface allows easy future extensions, ethtool_ops
> interface does not; some settings could be implemented using tunables and
> accessed via relevant netlink messages (as well as tunables) from
> userspace but in the long term, something better will be needed
>
> - it would be nice if driver could provide useful error/warning messages to
> be passed to userspace via extended ACK; example: while testing, I found
> a driver which only allows values 0, 1, 3 and 10000 for certain parameter
> but the only way poor user can find out is either by trying all values or
> by checking driver source
>
> Michal Kubecek (9):
> netlink: introduce nla_put_bitfield32()
> ethtool: introduce ethtool netlink interface
> ethtool: helper functions for netlink interface
> ethtool: netlink bitset handling
> ethtool: implement GET_DRVINFO message
> ethtool: implement GET_SETTINGS message
> ethtool: implement SET_SETTINGS message
> ethtool: implement GET_PARAMS message
> ethtool: implement SET_PARAMS message
>

Thanks for working on this!. Agree with most comments already
discussed on this thread.
I would prefer if we fold ethtool netlink into devlink since there is
already an overlap.
many reasons:
- have just one driver api for device global and per port config
(devlink already provides that)
- some of the devlink commands like port split/unsplit can already be
applied per netdev (and since you bring up network interface managers,
we are looking at getting these in network managers for switch ports)
- if we keep them separate, we will soon see that drivers will need
handlers for both devlink and ethtool
- and the overlap is going to be confusing for both drivers and
user-space

2017-12-12 23:47:54

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)

On Tue, 12 Dec 2017 07:32:44 -0800, Roopa Prabhu wrote:
> Thanks for working on this!. Agree with most comments already
> discussed on this thread.
> I would prefer if we fold ethtool netlink into devlink since there is
> already an overlap.
> many reasons:
> - have just one driver api for device global and per port config
> (devlink already provides that)
> - some of the devlink commands like port split/unsplit can already be
> applied per netdev (and since you bring up network interface managers,
> we are looking at getting these in network managers for switch ports)
> - if we keep them separate, we will soon see that drivers will need
> handlers for both devlink and ethtool
> - and the overlap is going to be confusing for both drivers and user-space

+1

FWIW my gut feeling is that extending devlink-port makes more sense
than creating another command. Although things like RSS tables don't
feel very natural in port context.

The other issue that springs to mind is that ethtool ops take RTNL lock
today, and devlink doesn't. It would be cool if we could make ethtool
not require RTNL lock (and drivers didn't have to drop locks half way
through the .flash_device callback, sigh...) but OTOH that would make
the driver conversion more brittle and potentially block the much
needed netlink conversion...

That's my $0.02 ;)

2017-12-12 23:54:46

by Michal Kubecek

[permalink] [raw]
Subject: Re: [RFC PATCH 5/9] ethtool: implement GET_DRVINFO message

On Mon, Dec 11, 2017 at 05:16:01PM +0100, Jiri Pirko wrote:
> Mon, Dec 11, 2017 at 02:54:01PM CET, [email protected] wrote:
> >+
> >+ ETHA_DRVINFO_DRIVER (string) driver name
> >+ ETHA_DRVINFO_VERSION (string) driver version
>
> You use 3 prefixes:
> ETHTOOL_ for cmd
> ETHA_ for attrs
> ethnl_ for function
>
> I suggest to sync this, perhaps to:
> ETHNL_CMD_*
> ETHNL_ATTR_*
> ethnl_*
> ?

Switching to ETHNL_CMD_* is certainly a good idea. For attributes, I'm a
bit worried that ETHNL_ATTR_ prefix might make it even harder to fit
into the 80 characters per line limit. I'll try and see what it would
look like.

> >+ ETHA_DRVINFO_FWVERSION (string) firmware version
> >+ ETHA_DRVINFO_BUSINFO (string) device bus address
> >+ ETHA_DRVINFO_EROM_VER (string) expansion ROM version
> >+ ETHA_DRVINFO_N_PRIV_FLAGS (u32) number of private flags
> >+ ETHA_DRVINFO_N_STATS (u32) number of device stats
> >+ ETHA_DRVINFO_TESTINFO_LEN (u32) number of test results
> >+ ETHA_DRVINFO_EEDUMP_LEN (u32) EEPROM dump size
> >+ ETHA_DRVINFO_REGDUMP_LEN (u32) register dump size
>
> We are now working on providing various fw memory regions dump in
> devlink. It makes sense to have it in devlink for couple of reasons:
> 1) per-asic, not netdev specific, therefore does not really make sense
> to have netdev as handle, but rather devlink handle.
> 2) snapshot support - we need to provide support for getting snapshot
> (for example on failure), transferring to user and deleting it
> (remove from driver memory).
>
> Also, driver name, version, fwversion, etc is per-asic. Would make sense
> to have it in devlink as well.
>
> I think this is great opprotunity to move things where they should be to
> be alligned with the current world and kernel infrastructure.

IMHO this depends on the question whether we are going to rework also
the interface between kernel and NIC drivers (currently ethtool_ops).
If we are, it would make good sense to split the information in both
interfaces. If not, it doesn't seem to make userspace do two queries,
each getting one part of the same information provided by NIC.

Also, I must admit that one thing I dislike about the iw tool is that it
pushes me to distinguish between operations on "phy" and "dev". While
I understand that it makes perfect sense for someone familiar with the
internals, it's quite annoying for an occasional "consumer". It would be
sad if we created a set of perfectly logical and clean tools and
(almost) 20 years later, people still used old ioctl based ethtool
because "it's what they are used to and it works just fine for them"
(like they do with ifconfig, netstat or brctl).

Michal

2017-12-12 23:57:03

by Michal Kubecek

[permalink] [raw]
Subject: Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface

On Mon, Dec 11, 2017 at 01:45:47PM -0500, David Miller wrote:
> From: Jiri Pirko <[email protected]>
> Date: Mon, 11 Dec 2017 19:02:19 +0100
>
> > The discussion we had before was about flag bitfield that was there
> > *always*. In this case, that is not true. It is either ifindex or
> > ifname. Even rtnetlink has ifname as attribute.
> >
> > The flags and info_mask is just big mystery. If it is per-command,
> > seems natural to have it as attributes.
>
> I think flags and info_mask indeed can be moved out of this struct.
>
> I guess, in this case, I can see your point of view especially if we
> allow ethtool operations on non-netdev entities.
>
> So, ok, let's move forward without a base command struct and just
> use attributes.

OK, I'll rework the interface to use attributes for all data.

Michal Kubecek

2017-12-13 06:57:09

by Jiri Pirko

[permalink] [raw]
Subject: Re: [RFC PATCH 5/9] ethtool: implement GET_DRVINFO message

Wed, Dec 13, 2017 at 12:54:39AM CET, [email protected] wrote:
>On Mon, Dec 11, 2017 at 05:16:01PM +0100, Jiri Pirko wrote:
>> Mon, Dec 11, 2017 at 02:54:01PM CET, [email protected] wrote:
>> >+
>> >+ ETHA_DRVINFO_DRIVER (string) driver name
>> >+ ETHA_DRVINFO_VERSION (string) driver version
>>
>> You use 3 prefixes:
>> ETHTOOL_ for cmd
>> ETHA_ for attrs
>> ethnl_ for function
>>
>> I suggest to sync this, perhaps to:
>> ETHNL_CMD_*
>> ETHNL_ATTR_*
>> ethnl_*
>> ?
>
>Switching to ETHNL_CMD_* is certainly a good idea. For attributes, I'm a
>bit worried that ETHNL_ATTR_ prefix might make it even harder to fit
>into the 80 characters per line limit. I'll try and see what it would
>look like.
>
>> >+ ETHA_DRVINFO_FWVERSION (string) firmware version
>> >+ ETHA_DRVINFO_BUSINFO (string) device bus address
>> >+ ETHA_DRVINFO_EROM_VER (string) expansion ROM version
>> >+ ETHA_DRVINFO_N_PRIV_FLAGS (u32) number of private flags
>> >+ ETHA_DRVINFO_N_STATS (u32) number of device stats
>> >+ ETHA_DRVINFO_TESTINFO_LEN (u32) number of test results
>> >+ ETHA_DRVINFO_EEDUMP_LEN (u32) EEPROM dump size
>> >+ ETHA_DRVINFO_REGDUMP_LEN (u32) register dump size
>>
>> We are now working on providing various fw memory regions dump in
>> devlink. It makes sense to have it in devlink for couple of reasons:
>> 1) per-asic, not netdev specific, therefore does not really make sense
>> to have netdev as handle, but rather devlink handle.
>> 2) snapshot support - we need to provide support for getting snapshot
>> (for example on failure), transferring to user and deleting it
>> (remove from driver memory).
>>
>> Also, driver name, version, fwversion, etc is per-asic. Would make sense
>> to have it in devlink as well.
>>
>> I think this is great opprotunity to move things where they should be to
>> be alligned with the current world and kernel infrastructure.
>
>IMHO this depends on the question whether we are going to rework also
>the interface between kernel and NIC drivers (currently ethtool_ops).
>If we are, it would make good sense to split the information in both
>interfaces. If not, it doesn't seem to make userspace do two queries,
>each getting one part of the same information provided by NIC.

Yeah, agreed. I believe we should.


>
>Also, I must admit that one thing I dislike about the iw tool is that it
>pushes me to distinguish between operations on "phy" and "dev". While
>I understand that it makes perfect sense for someone familiar with the
>internals, it's quite annoying for an occasional "consumer". It would be
>sad if we created a set of perfectly logical and clean tools and
>(almost) 20 years later, people still used old ioctl based ethtool
>because "it's what they are used to and it works just fine for them"
>(like they do with ifconfig, netstat or brctl).

Good documentation should take care of that.



>
>Michal

2017-12-14 21:15:19

by John W. Linville

[permalink] [raw]
Subject: Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)

Michal,

Thanks for looking at this issue.

While the kernel and userland bits here are at least somewhat
decoupled, as the current maintainer for the userland ethtool I
feel compelled to comment. FWIW I had started down a similar road,
but I became dissatisfied with my own results. The main problem was
that the only obvious API-conversion technique essentially amounted
to re-implementing the ioctl-based API, but on top of the netlink
transport. Having been burned by something similar in the pre-nl80211
days with wireless extensions, I really didn't want to repeat that.

Even without considering the ioctl problesms, the current ethtool
API seems a bit crufty. It has been a catch-all, "where else would it
go?" dumping ground for a long time, and it has accrued a number of
not-entirely-related bits of functionality. In my mind, what needs
to happen is that these various bits of functionality need to be
reorganized into a handful of groupings. Then, each group needs an
API designed around semantics that are natural to the functionality
being addressed. I believe this is essentially the idea that others
have expressed with the "move some of the ethtool bits to devlink"
comments. I think that probably makes sense, although trying to shove
everything into devlink probably makes no more sense than keeping
the entire ethtool API intact on top of a netlink transport. Anyway,
I think that with a reasonable set of groupings, the semantics would
fall-out naturally and implementing them on netlink or any other
suitable transport would be reasonably trivial.

Unfortunately, I have yet to formultate a useful set of abstractions
for grouping the various bits of the ethtool API. I have reached-out
to a few community folks seeking their wisdom, but since no one has
been forthcoming with such a set of abstractions, I'll presume that
either I failed to convey my message, my idea isn't too good, or none
of them are any smarter than me -- I'll avoid identifying any of them
here in order to save us all some embarassment! :-)

In short, what I would like to see is a true rethink of what APIs need
to be provided to NICs, outside of the basic netdev abstraction. I
wish I had the answer to what those APIs should be, but I don't. I do
believe that with a well-conceived group of APIs, the proper semantics
will fall-out naturally. Any takers? :-)

Michal, once again -- thanks for attempting to address this
issue. Please do not take any of the above as discouragement. It is
clear that ethtool needs replacement, and we all know that 'perfect'
is the enemy of 'good'. I just would hate to miss the opportunity for a
'better' API just because ethtool's ioctly API has lived too long.

John

On Mon, Dec 11, 2017 at 02:53:11PM +0100, Michal Kubecek wrote:
> This is still work in progress and only a very small part of the ioctl
> interface is reimplemented but I would like to get some comments before
> the patchset becomes too big and changing things becomes too tedious.
>
> The interface used for communication between ethtool and kernel is based on
> ioctl() and suffers from many problems. The most pressing seems the be the
> lack of extensibility. While some of the newer commands use structures
> designed to allow future extensions (e.g. GFEATURES or TEST), most either
> allow no extension at all (GPAUSEPARAM, GCOALESCE) or only limited set of
> reserved fields (GDRVINFO, GEEE). Even most of those which support future
> extensions limit the data types that can be used.
>
> This series aims to provide an alternative interface based on netlink which
> is what other network configuration utilities use. In particular, it uses
> generic netlink (family "ethtool"). The goal is to provide an interface
> which would be extensible, flexible and practical both for ethtool and for
> other network configuration tools (e.g. wicked or systemd-networkd).
>
> The interface is documented in Documentation/networking/ethtool-netlink.txt
>
> I'll post RFC patch series for ethtool in a few days, it still needs some
> more polishing.
>
> Basic concepts:
>
> - the interface is based on generic netlink (family name "ethtool")
>
> - provide everything ioctl can do but allow easier future extensions
>
> - inextensibility of ioctl interface resulted in way too many commands,
> many of them obsoleted by newer ones; reduce the number by ignoring the
> obsolete commands and grouping some together
>
> - for "set" type commands, netlink allows providing only the attributes to
> be changed; therefore we don't need a get-modify-set cycle, userspace can
> simply say what it wants to change and how
>
> - be less dependent on ethtool and kernel being in sync; allow e.g. saying
> "ethtool -s eth0 advertise xyz off" without knowing what "xyz" means;
> it's kernel's job to know what mode "xyz" is and if it exists and is
> supported
>
> Unresolved questions/tasks:
>
> - allow dumps for "get" type requests, e.g. listing EEE settings for all
> interfaces in current netns
>
> - find reasonable format for data transfers (e.g. eeprom dump or flash)
>
> - while the netlink interface allows easy future extensions, ethtool_ops
> interface does not; some settings could be implemented using tunables and
> accessed via relevant netlink messages (as well as tunables) from
> userspace but in the long term, something better will be needed
>
> - it would be nice if driver could provide useful error/warning messages to
> be passed to userspace via extended ACK; example: while testing, I found
> a driver which only allows values 0, 1, 3 and 10000 for certain parameter
> but the only way poor user can find out is either by trying all values or
> by checking driver source
>
> Michal Kubecek (9):
> netlink: introduce nla_put_bitfield32()
> ethtool: introduce ethtool netlink interface
> ethtool: helper functions for netlink interface
> ethtool: netlink bitset handling
> ethtool: implement GET_DRVINFO message
> ethtool: implement GET_SETTINGS message
> ethtool: implement SET_SETTINGS message
> ethtool: implement GET_PARAMS message
> ethtool: implement SET_PARAMS message
>
> Documentation/networking/ethtool-netlink.txt | 466 ++++++
> include/linux/ethtool_netlink.h | 12 +
> include/linux/netdevice.h | 2 +
> include/net/netlink.h | 15 +
> include/uapi/linux/ethtool.h | 3 +
> include/uapi/linux/ethtool_netlink.h | 239 +++
> net/Kconfig | 7 +
> net/core/Makefile | 3 +-
> net/core/ethtool.c | 150 +-
> net/core/ethtool_common.c | 158 ++
> net/core/ethtool_common.h | 19 +
> net/core/ethtool_netlink.c | 2323 ++++++++++++++++++++++++++
> 12 files changed, 3260 insertions(+), 137 deletions(-)
> create mode 100644 Documentation/networking/ethtool-netlink.txt
> create mode 100644 include/linux/ethtool_netlink.h
> create mode 100644 include/uapi/linux/ethtool_netlink.h
> create mode 100644 net/core/ethtool_common.c
> create mode 100644 net/core/ethtool_common.h
> create mode 100644 net/core/ethtool_netlink.c
>
> --
> 2.15.1
>
>

--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.

2017-12-18 19:39:30

by David Miller

[permalink] [raw]
Subject: Re: [RFC PATCH 0/9] ethtool netlink interface (WiP)

From: "John W. Linville" <[email protected]>
Date: Thu, 14 Dec 2017 16:07:56 -0500

> Even without considering the ioctl problesms, the current ethtool
> API seems a bit crufty. It has been a catch-all, "where else would it
> go?" dumping ground for a long time, and it has accrued a number of
> not-entirely-related bits of functionality. In my mind, what needs
> to happen is that these various bits of functionality need to be
> reorganized into a handful of groupings. Then, each group needs an
> API designed around semantics that are natural to the functionality
> being addressed. I believe this is essentially the idea that others
> have expressed with the "move some of the ethtool bits to devlink"
> comments. I think that probably makes sense, although trying to shove
> everything into devlink probably makes no more sense than keeping
> the entire ethtool API intact on top of a netlink transport. Anyway,
> I think that with a reasonable set of groupings, the semantics would
> fall-out naturally and implementing them on netlink or any other
> suitable transport would be reasonably trivial.

Thanks for your valueable feedback John.

Let's keep in mind that really the core impetus to move ethtool stuff
to netlink is visibility.

Someone trying to monitor network config events in the system can't
see anything that happens with ethtool currently. It's completely
invisible.

Even ancient ifconfig ioctls generate proper netlink events.

Ethtool is one of the few, if not the only, network config mechanism
that elides netlink event visibility.

And I think fixing that core issue is what is driving the focus onto a
pure 1-to-1 conversion, be it to a separate netlink/genetlink family
or to devlink.