[PATCH net-next 06/14] net: ethtool: optionally skip rtnl_lock on Netlink path for GET ops

[Jakub Kicinski <kuba@kernel.org>]

ethnl_default_doit() and ethnl_default_dump_one() are both used
exclusively for GET callbacks (former to get info for a single
device or get global strings). ops-locked devices don't need
rtnl_lock for GET callbacks, stop taking it.

Introduce an opt-out mechanism for devices which use phylink (fbnic)
since phylink currently depends on rtnl_lock protection. Subsequent
patches will add more exceptions, anyway. Practically the new helpers
for judging if command needs rtnl_lock could also call
netdev_need_ops_lock() but I find that it makes the code in the callers
slightly less obvious.

Add a helper for IOCTLs already, even tho its unused so that
we can keep them in sync as the series progresses.

This is the first user-visible step of moving ethtool ops out
from under rtnl. Subsequent patches do the same for SET ops,
as well as the ioctl path.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/ethtool.h                       | 18 +++++++-
 net/ethtool/common.h                          | 46 +++++++++++++++++++
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |  2 +
 net/ethtool/mm.c                              |  5 +-
 net/ethtool/netlink.c                         | 19 ++++++--
 5 files changed, 83 insertions(+), 7 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 0caaeb91b094..f4f933b8be26 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -930,6 +930,13 @@ struct kernel_ethtool_ts_info {
 	u32 rx_filters;
 };
 
+/* Bits for ethtool_ops::op_needs_rtnl
+ * LINKSETTINGS cover a number of commands, but in most cases we want to keep
+ * these bits separate, per GET and SET. GET is much easier to "unlock".
+ */
+#define ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS	BIT(0)
+#define ETHTOOL_OP_NEEDS_RTNL_GPAUSEPARAM	BIT(1)
+
 /**
  * struct ethtool_ops - optional netdev operations
  * @supported_input_xfrm: supported types of input xfrm from %RXH_XFRM_*.
@@ -956,6 +963,14 @@ struct kernel_ethtool_ts_info {
  * @supported_coalesce_params: supported types of interrupt coalescing.
  * @supported_ring_params: supported ring params.
  * @supported_hwtstamp_qualifiers: bitfield of supported hwtstamp qualifier.
+ * @op_needs_rtnl: mask of %ETHTOOL_OP_NEEDS_RTNL_* bits.
+ *	For use with ops-locked drivers (ignored otherwise). Selects which
+ *	ethtool callbacks driver needs to still be executed under rtnl_lock
+ *	(in addition to the netdev instance lock).
+ *	The following commonly used core APIs currently require rtnl_lock
+ *	(this list may not be exhaustive):
+ *	 - phylink helpers (note that phydev is currently unsupported!)
+ *
  * @get_drvinfo: Report driver/device information. Modern drivers no
  *	longer have to implement this callback. Most fields are
  *	correctly filled in by the core using system information, or
@@ -1155,7 +1170,7 @@ struct kernel_ethtool_ts_info {
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
- * hold the RTNL lock.
+ * hold the RTNL lock or netdev instance lock (see @op_needs_rtnl).
  *
  * See the structures used by these operations for further documentation.
  * Note that for all operations using a structure ending with a zero-
@@ -1178,6 +1193,7 @@ struct ethtool_ops {
 	u32	supported_coalesce_params;
 	u32	supported_ring_params;
 	u32	supported_hwtstamp_qualifiers;
+	u32	op_needs_rtnl;
 	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
 	int	(*get_regs_len)(struct net_device *);
 	void	(*get_regs)(struct net_device *, struct ethtool_regs *, void *);
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index 1609cf4e53eb..391c41ca56be 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -80,6 +80,52 @@ int ethtool_get_module_eeprom_call(struct net_device *dev,
 
 bool __ethtool_dev_mm_supported(struct net_device *dev);
 
+/**
+ * ethtool_nl_msg_needs_rtnl() - does this Netlink cmd need rtnl_lock?
+ * @dev: target device
+ * @cmd: ETHTOOL_MSG_* Netlink command value
+ *
+ * Return: true if @cmd is a command for which @dev has opted-in to
+ * keeping rtnl_lock held across the call (via op_needs_rtnl).
+ */
+static inline bool
+ethtool_nl_msg_needs_rtnl(const struct net_device *dev, u8 cmd)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	switch (cmd) {
+	case ETHTOOL_MSG_LINKINFO_GET:
+	case ETHTOOL_MSG_LINKMODES_GET:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS;
+	case ETHTOOL_MSG_PAUSE_GET:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_GPAUSEPARAM;
+	}
+	return false;
+}
+
+/**
+ * ethtool_ioctl_needs_rtnl() - does this legacy ioctl cmd need rtnl_lock?
+ * @dev: target device
+ * @ethcmd: ETHTOOL_* ioctl command value
+ *
+ * Return: true if @ethcmd is a command for which @dev has opted-in to
+ * keeping rtnl_lock held across the call (via op_needs_rtnl).
+ */
+static inline bool
+ethtool_ioctl_needs_rtnl(const struct net_device *dev, u32 ethcmd)
+{
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+
+	switch (ethcmd) {
+	case ETHTOOL_GLINKSETTINGS:
+	case ETHTOOL_GSET:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS;
+	case ETHTOOL_GPAUSEPARAM:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_GPAUSEPARAM;
+	}
+	return false;
+}
+
 #if IS_ENABLED(CONFIG_ETHTOOL_NETLINK)
 void ethtool_rss_notify(struct net_device *dev, u32 type, u32 rss_context);
 #else
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index f14de2366854..a2c16d599389 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -2020,6 +2020,8 @@ static const struct ethtool_ops fbnic_ethtool_ops = {
 	.supported_ring_params		= ETHTOOL_RING_USE_TCP_DATA_SPLIT |
 					  ETHTOOL_RING_USE_HDS_THRS,
 	.rxfh_max_num_contexts		= FBNIC_RPC_RSS_TBL_COUNT,
+	.op_needs_rtnl			= ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS |
+					  ETHTOOL_OP_NEEDS_RTNL_GPAUSEPARAM,
 	.get_drvinfo			= fbnic_get_drvinfo,
 	.get_regs_len			= fbnic_get_regs_len,
 	.get_regs			= fbnic_get_regs,
diff --git a/net/ethtool/mm.c b/net/ethtool/mm.c
index 29bbbc149375..7092f1ce8612 100644
--- a/net/ethtool/mm.c
+++ b/net/ethtool/mm.c
@@ -246,8 +246,9 @@ const struct ethnl_request_ops ethnl_mm_request_ops = {
 };
 
 /* Returns whether a given device supports the MAC merge layer
- * (has an eMAC and a pMAC). Must be called under rtnl_lock() and
- * ethnl_ops_begin().
+ * (has an eMAC and a pMAC). Must be called under whichever lock
+ * netdev_ops_assert_locked() accepts (rtnl for traditional drivers,
+ * the netdev instance lock for ops-locked ones) and ethnl_ops_begin().
  */
 bool __ethtool_dev_mm_supported(struct net_device *dev)
 {
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index a91cc4bc964f..ddb35790063d 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -7,6 +7,7 @@
 #include <linux/phy_link_topology.h>
 #include <linux/pm_runtime.h>
 
+#include "common.h"
 #include "module_fw.h"
 #include "netlink.h"
 
@@ -509,6 +510,7 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
 	struct ethnl_req_info *req_info = NULL;
 	const u8 cmd = info->genlhdr->cmd;
 	const struct ethnl_request_ops *ops;
+	bool need_rtnl = false;
 	int hdr_len, reply_len;
 	struct sk_buff *rskb;
 	void *reply_payload;
@@ -535,13 +537,17 @@ static int ethnl_default_doit(struct sk_buff *skb, struct genl_info *info)
 	ethnl_init_reply_data(reply_data, ops, req_info->dev);
 
 	if (req_info->dev) {
-		rtnl_lock();
+		need_rtnl = !netdev_need_ops_lock(req_info->dev) ||
+			    ethtool_nl_msg_needs_rtnl(req_info->dev, cmd);
+		if (need_rtnl)
+			rtnl_lock();
 		netdev_lock_ops(req_info->dev);
 	}
 	ret = ops->prepare_data(req_info, reply_data, info);
 	if (req_info->dev) {
 		netdev_unlock_ops(req_info->dev);
-		rtnl_unlock();
+		if (need_rtnl)
+			rtnl_unlock();
 	}
 	if (ret < 0)
 		goto err_dev;
@@ -589,6 +595,7 @@ static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
 				  const struct ethnl_dump_ctx *ctx,
 				  const struct genl_info *info)
 {
+	bool need_rtnl;
 	void *ehdr;
 	int ret;
 
@@ -599,11 +606,15 @@ static int ethnl_default_dump_one(struct sk_buff *skb, struct net_device *dev,
 		return -EMSGSIZE;
 
 	ethnl_init_reply_data(ctx->reply_data, ctx->ops, dev);
-	rtnl_lock();
+	need_rtnl = !netdev_need_ops_lock(dev) ||
+		    ethtool_nl_msg_needs_rtnl(dev, ctx->ops->request_cmd);
+	if (need_rtnl)
+		rtnl_lock();
 	netdev_lock_ops(dev);
 	ret = ctx->ops->prepare_data(ctx->req_info, ctx->reply_data, info);
 	netdev_unlock_ops(dev);
-	rtnl_unlock();
+	if (need_rtnl)
+		rtnl_unlock();
 	if (ret < 0)
 		goto out_cancel;
 	ret = ethnl_fill_reply_header(skb, dev, ctx->ops->hdr_attr);
-- 
2.54.0