Date: Mon, 11 Dec 2017 17:02:21 +0100
From: Jiri Pirko <jiri@resnulli.us>
To: Michal Kubecek <mkubecek@suse.cz>
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 2/9] ethtool: introduce ethtool netlink interface
Message-ID: <20171211160221.GA1885@nanopsycho>
References: <cover.1513000306.git.mkubecek@suse.cz>
 <37174026f818dd1a61ca1b37bef2f1b114198de2.1513000306.git.mkubecek@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <37174026f818dd1a61ca1b37bef2f1b114198de2.1513000306.git.mkubecek@suse.cz>
User-Agent: Mutt/1.8.3 (2017-05-23)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3879
Lines: 142

Mon, Dec 11, 2017 at 02:53:31PM CET, mkubecek@suse.cz 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 <mkubecek@suse.cz>

[...]


>+
>+/* 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 <mkubecek@suse.cz>");
>+MODULE_DESCRIPTION("Netlink interface for ethtool");
>+MODULE_LICENSE("GPL");

"GPL v2"?


>-- 
>2.15.1
>