Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 26 Mar 2019 17:34:00 +0100
From:   Jiri Pirko <jiri@resnulli.us>
To:     Michal Kubecek <mkubecek@suse.cz>
Cc:     David Miller <davem@davemloft.net>, netdev@vger.kernel.org,
        Jakub Kicinski <jakub.kicinski@netronome.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        John Linville <linville@tuxdriver.com>,
        Stephen Hemminger <stephen@networkplumber.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v5 08/22] ethtool: support for netlink
 notifications
Message-ID: <20190326163400.GC4958@nanopsycho.orion>
References: <cover.1553532199.git.mkubecek@suse.cz>
 <3578e5f334acf11b84e75d0ee41c072340a7b085.1553532199.git.mkubecek@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <3578e5f334acf11b84e75d0ee41c072340a7b085.1553532199.git.mkubecek@suse.cz>
User-Agent: Mutt/1.11.3 (2019-02-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Mon, Mar 25, 2019 at 06:08:18PM CET, mkubecek@suse.cz wrote:
>Add infrastructure for ethtool netlink notifications. There is only one
>multicast group "monitor" which is used to notify userspace about changes.
>Notifications are supposed to be broadcasted on every configuration change,
>whether it is done using the netlink interface or legacy ioctl one.
>
>To trigger an ethtool notification, both ethtool netlink and external code
>use ethtool_notify() helper. This helper requires RTNL to be held and may
>sleep.
>
>Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>---
> include/linux/ethtool_netlink.h      |  5 ++++
> include/linux/netdevice.h            | 12 +++++++++
> include/uapi/linux/ethtool_netlink.h |  2 ++
> net/ethtool/netlink.c                | 37 ++++++++++++++++++++++++++++
> net/ethtool/netlink.h                |  2 ++
> 5 files changed, 58 insertions(+)
>
>diff --git a/include/linux/ethtool_netlink.h b/include/linux/ethtool_netlink.h
>index 0412adb4f42f..2a15e64a16f3 100644
>--- a/include/linux/ethtool_netlink.h
>+++ b/include/linux/ethtool_netlink.h
>@@ -5,5 +5,10 @@
> 
> #include <uapi/linux/ethtool_netlink.h>
> #include <linux/ethtool.h>
>+#include <linux/netdevice.h>
>+
>+enum ethtool_multicast_groups {
>+	ETHNL_MCGRP_MONITOR,
>+};
> 
> #endif /* _LINUX_ETHTOOL_NETLINK_H_ */
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index 166fdc0a78b4..bc761511edb4 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -4353,6 +4353,18 @@ struct netdev_notifier_bonding_info {
> void netdev_bonding_info_change(struct net_device *dev,
> 				struct netdev_bonding_info *bonding_info);
> 
>+#if IS_ENABLED(CONFIG_ETHTOOL_NETLINK)
>+void ethtool_notify(struct net_device *dev, struct netlink_ext_ack *extack,
>+		    unsigned int cmd, u32 req_mask, const void *data);
>+#else
>+static inline void ethtool_notify(struct net_device *dev,
>+				  struct netlink_ext_ack *extack,
>+				  unsigned int cmd, u32 req_mask,
>+				  const void *data)
>+{
>+}
>+#endif
>+
> static inline
> struct sk_buff *skb_gso_segment(struct sk_buff *skb, netdev_features_t features)
> {
>diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
>index de18e076ed69..91e4d117957b 100644
>--- a/include/uapi/linux/ethtool_netlink.h
>+++ b/include/uapi/linux/ethtool_netlink.h
>@@ -65,4 +65,6 @@ enum {
> #define ETHTOOL_GENL_NAME "ethtool"
> #define ETHTOOL_GENL_VERSION 1
> 
>+#define ETHTOOL_MCGRP_MONITOR_NAME "monitor"
>+
> #endif /* _UAPI_LINUX_ETHTOOL_NETLINK_H_ */
>diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
>index cc6829ba4331..4a31765165ea 100644
>--- a/net/ethtool/netlink.c
>+++ b/net/ethtool/netlink.c
>@@ -4,6 +4,10 @@
> #include <linux/ethtool_netlink.h>
> #include "netlink.h"
> 
>+u32 ethnl_bcast_seq;
>+
>+static bool ethnl_ok __read_mostly;
>+
> static const struct nla_policy dev_policy[ETHA_DEV_MAX + 1] = {
> 	[ETHA_DEV_UNSPEC]	= { .type = NLA_REJECT },
> 	[ETHA_DEV_INDEX]	= { .type = NLA_U32 },
>@@ -147,11 +151,41 @@ struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd,
> 	return NULL;
> }
> 
>+/* notifications */
>+
>+typedef void (*ethnl_notify_handler_t)(struct net_device *dev,
>+				       struct netlink_ext_ack *extack,
>+				       unsigned int cmd, u32 req_mask,
>+				       const void *data);
>+
>+ethnl_notify_handler_t ethnl_notify_handlers[] = {
>+};
>+
>+void ethtool_notify(struct net_device *dev, struct netlink_ext_ack *extack,
>+		    unsigned int cmd, u32 req_mask, const void *data)
>+{
>+	if (unlikely(!ethnl_ok))

Why do you need this?


>+		return;
>+	ASSERT_RTNL();
>+
>+	if (likely(cmd < ARRAY_SIZE(ethnl_notify_handlers) &&
>+		   ethnl_notify_handlers[cmd]))
>+		ethnl_notify_handlers[cmd](dev, extack, cmd, req_mask, data);
>+	else
>+		WARN_ONCE(1, "notification %u not implemented (dev=%s, req_mask=0x%x)\n",
>+			  cmd, netdev_name(dev), req_mask);
>+}
>+EXPORT_SYMBOL(ethtool_notify);

Please be consistent with prefixes. Rest of the code (most of it) has
prefix "ethnl".


>+
> /* genetlink setup */
> 
> static const struct genl_ops ethtool_genl_ops[] = {
> };
> 
>+static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
>+	[ETHNL_MCGRP_MONITOR] = { .name = ETHTOOL_MCGRP_MONITOR_NAME },
>+};
>+
> struct genl_family ethtool_genl_family = {
> 	.hdrsize	= 0,
> 	.name		= ETHTOOL_GENL_NAME,
>@@ -160,6 +194,8 @@ struct genl_family ethtool_genl_family = {
> 	.parallel_ops	= true,
> 	.ops		= ethtool_genl_ops,
> 	.n_ops		= ARRAY_SIZE(ethtool_genl_ops),
>+	.mcgrps		= ethtool_nl_mcgrps,
>+	.n_mcgrps	= ARRAY_SIZE(ethtool_nl_mcgrps),
> };
> 
> /* module setup */
>@@ -171,6 +207,7 @@ static int __init ethnl_init(void)
> 	ret = genl_register_family(&ethtool_genl_family);
> 	if (WARN(ret < 0, "ethtool: genetlink family registration failed"))
> 		return ret;
>+	ethnl_ok = true;
> 
> 	return 0;
> }
>diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
>index b8a6cd3dc3e3..5f2299548915 100644
>--- a/net/ethtool/netlink.h
>+++ b/net/ethtool/netlink.h
>@@ -11,6 +11,8 @@
> #define ETHNL_SET_ERRMSG(info, msg) \
> 	do { if (info) GENL_SET_ERR_MSG(info, msg); } while (0)
> 
>+extern u32 ethnl_bcast_seq;

Why do you need to have this in header? Second, it is not used by
anything. Please don't introduce variables that are not used. Introduce
them only in patch where you use it.


>+
> extern struct genl_family ethtool_genl_family;
> 
> struct net_device *ethnl_dev_get(struct genl_info *info, struct nlattr *nest);
>-- 
>2.21.0
>