Messages in this thread Patch in this message | | | Date | Tue, 26 Mar 2019 13:09:09 +0100 | From | Jiri Pirko <> | Subject | Re: [PATCH net-next v5 05/22] ethtool: introduce ethtool netlink interface |
| |
Mon, Mar 25, 2019 at 06:08:09PM CET, mkubecek@suse.cz wrote: >Basic genetlink and init infrastructure for the netlink interface, register >genetlink family "ethtool". Introduce CONFIG_ETHTOOL_NETLINK Kconfig >option. Add interface description into Documentation/networking. > >Signed-off-by: Michal Kubecek <mkubecek@suse.cz> >--- > Documentation/networking/ethtool-netlink.txt | 168 +++++++++++++++++++ > include/linux/ethtool_netlink.h | 9 + > include/uapi/linux/ethtool_netlink.h | 25 +++ > net/Kconfig | 8 + > net/ethtool/Makefile | 6 +- > net/ethtool/netlink.c | 34 ++++ > net/ethtool/netlink.h | 12 ++ > 7 files changed, 261 insertions(+), 1 deletion(-) > 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/ethtool/netlink.c > create mode 100644 net/ethtool/netlink.h > >diff --git a/Documentation/networking/ethtool-netlink.txt b/Documentation/networking/ethtool-netlink.txt >new file mode 100644 >index 000000000000..377d64c9b7fa >--- /dev/null >+++ b/Documentation/networking/ethtool-netlink.txt >@@ -0,0 +1,168 @@ >+ 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 does not use >+a specific header, all information in requests and replies is passed using >+netlink attributes. >+ >+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
s/request/requests/
>+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. >+ >+Some request types allow passing an attribute named ETHA_*_INFOMASK with >+a bitmask telling kernel that we are only interested in some parts of the >+information. If info mask is omitted, all available information is returned. >+Meaning of info mask bits depends on request type and is listed below. >+ >+ >+Device identification >+--------------------- >+ >+When appropriate, network device is identified by a nested attribute named >+ETHA_*_DEV. This attribute can contain
Isn't it ETHA_DEV_*? I must admit I'm a bit confused.
>+ >+ ETHA_DEV_INDEX (u32) device ifindex >+ ETHA_DEV_NAME (string) device name >+ >+In device related requests, one of these is sufficient; if both are used, they >+must match (i.e. identify the same device). In device related replies both are
You say this now for the second time. First time this was said in second para.
>+provided by kernel. In dump requests, device is not specified and kernel >+replies with one message per network device (only those for which the request >+is supported). >+ >+ >+List of message types >+--------------------- >+ >+All constants use ETHNL_CMD_ prefix, usually followed by "GET", "SET" or "ACT"
Why "usually"? Why not "always"?
>+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 corresponding "set" message;
Okay. Do we want reply to "*_cmd_something_get" command to be "*_cmd_something_set". That sounds odd. Why reply has to be "cmd"? Why not something like "reply" or "response"? This should work for both "doit/dumpit" and notifications.
>+the same message can be also used to set (some of) the parameters, except for >+messages marked as "response only" in the table above. "Get" messages with >+NLM_F_DUMP flags and no device identification dump the information for all >+devices supporting the request. >+ >+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 rtnetlink RTM_GETLINK >+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..6aa267451542 >--- /dev/null >+++ b/include/uapi/linux/ethtool_netlink.h >@@ -0,0 +1,25 @@ >+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ >+/* >+ * include/uapi/linux/ethtool_netlink.h - netlink interface for ethtool >+ * >+ * See Documentation/networking/ethtool-netlink.txt in kernel source tree for >+ * doucumentation of the interface. >+ */ >+ >+#ifndef _UAPI_LINUX_ETHTOOL_NETLINK_H_ >+#define _UAPI_LINUX_ETHTOOL_NETLINK_H_ >+ >+#include <linux/ethtool.h> >+ >+enum { >+ ETHNL_CMD_NOOP, >+
Usually headers have something like: /* add new commands above here */ here.
>+ __ETHNL_CMD_CNT, >+ ETHNL_CMD_MAX = (__ETHNL_CMD_CNT - 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 3e8fdd688329..75c600b45775 100644 >--- a/net/Kconfig >+++ b/net/Kconfig >@@ -448,6 +448,14 @@ config FAILOVER > migration of VMs with direct attached VFs by failing over to the > paravirtual datapath when the VF is unplugged. > >+config ETHTOOL_NETLINK >+ bool "Netlink interface for ethtool" >+ default y >+ help >+ An alternative userspace interface for ethtool based on generic >+ netlink. It provides better extensibility and some new features, >+ e.g. notification messages. >+ > endif # if NET > > # Used by archs to tell that they support BPF JIT compiler plus which flavour. >diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile >index 3ebfab2bca66..f30e0da88be5 100644 >--- a/net/ethtool/Makefile >+++ b/net/ethtool/Makefile >@@ -1,3 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 > >-obj-y += ioctl.o >+obj-y += ioctl.o >+ >+obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
Why? I believe this should be always build-in same as ioctl.
>+ >+ethtool_nl-y := netlink.o >diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c >new file mode 100644 >index 000000000000..85dd6dac71a2 >--- /dev/null >+++ b/net/ethtool/netlink.c >@@ -0,0 +1,34 @@ >+// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note >+ >+#include <linux/ethtool_netlink.h> >+#include "netlink.h" >+ >+/* genetlink setup */ >+ >+static const struct genl_ops ethtool_genl_ops[] = { >+}; >+ >+struct genl_family ethtool_genl_family = { >+ .hdrsize = 0, No need to set 0.
>+ .name = ETHTOOL_GENL_NAME, >+ .version = ETHTOOL_GENL_VERSION, >+ .netnsok = true, >+ .parallel_ops = true, >+ .ops = ethtool_genl_ops, >+ .n_ops = ARRAY_SIZE(ethtool_genl_ops), >+}; >+ >+/* module setup */ >+ >+static int __init ethnl_init(void) >+{ >+ int ret; >+ >+ ret = genl_register_family(ðtool_genl_family); >+ if (WARN(ret < 0, "ethtool: genetlink family registration failed")) >+ return ret; >+ >+ return 0; >+} >+ >+subsys_initcall(ethnl_init); >diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h >new file mode 100644 >index 000000000000..63063b582ca2 >--- /dev/null >+++ b/net/ethtool/netlink.h >@@ -0,0 +1,12 @@ >+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ >+ >+#ifndef _NET_ETHTOOL_NETLINK_H >+#define _NET_ETHTOOL_NETLINK_H >+ >+#include <linux/ethtool_netlink.h> >+#include <linux/netdevice.h> >+#include <net/genetlink.h> >+ >+extern struct genl_family ethtool_genl_family;
Why? You need this just within "netlink.c", don't you?
>+ >+#endif /* _NET_ETHTOOL_NETLINK_H */ >-- >2.21.0 >
| |