[PATCH net-next v2 00/12] net: ethtool: let ops locked drivers run without rtnl_lock

[PATCH net-next v2 00/12] net: ethtool: let ops locked drivers run without rtnl_lock

[Jakub Kicinski]

With the ethtool_get_link_ksettings() situation hopefully ironed out
the previous series (commit 6a5d837f0ce2) let's return to the main
part of the series.

We have been slowly moving towards removing the rtnl_lock dependency
in driver ops since the concept of "ops-locked" drivers have been
introduced last year. Since last year will take the netdev instance
lock before invoking any ndo or ethtool op of "ops-locked" drivers.

We dipped our toes into rtnl_lock-less ops with the queue binding API.
Queue stats, NAPI, and other netdev-netlink objects are also queried
without holding rtnl_lock already. It's time to take the next logical
step and lift the requirement from ethtool ops.

The direct motivation for this patchset is that ethtool ops often
involve communicating with device FW, and may take a long time
to complete. Aggressive polling of device state on machines
with 10+ NICs have been shown to significantly increase rtnl_lock
pressure.

There's a handful of areas which still need rtnl_lock (see below).
I decided to convert everything to rtnl_lock-less by default, and
add a set of flags which let the drivers request rtnl_lock to still
be taken. I don't love this, but I'm worried that opt-in would be
even more confusing.

Known issues / exclusions:
 - qdiscs - qdisc configuration currently assumes rtnl_lock, this
   is mostly impacting set_channels callback. qdisc config is probably
   the easiest one of the exclusions to tackle, it's fairly self-contained.
 - features - even tho feature changes are (correctly) plumbed to
   the driver thru ndos they are part of ethtool uAPI. ethtool itself
   calls netdev_features_change() if it has spotted device feature change
   before vs after to the callback. Some drivers also call
   netdev_features_change() directly in response to various changes,
   e.g. setting priv flags.
   Since features have to propagate to upper and lower devices anything
   that touches features is quite hard to move from under rtnl_lock.
 - phylink - phylink and SFP depend on rtnl_lock today, I suspect
   that this is purely for historic reasons. I started poking at
   it and don't really see a need for a global lock. But accessing
   the netdev instance lock from the SFP entry points will require
   some attention from the phylink folks.
 - phydev - similar to phylink, looks quite doable. But no ops-locked
   driver currently has a phydev (fbnic only uses phylink) so phydev
   related paths retain a ASSERT_RTNL() for now.

Tested on mlx5, bnxt and fbnic.

Jakub Kicinski (12):
  net: ethtool: serialize broadcast notification sequence allocation
  net: ethtool: relax ethnl_req_get_phydev() locking assertion
  net: ethtool: make dev->hwprov ops-protected
  net: ethtool: optionally skip rtnl_lock on Netlink path for GET ops
  net: ethtool: optionally skip rtnl_lock on Netlink path for SET ops
  net: ethtool: optionally skip rtnl_lock in cable test handlers
  net: ethtool: optionally skip rtnl_lock in ethnl_tsinfo_dumpit()
  net: ethtool: optionally skip rtnl_lock in ethnl_act_module_fw_flash()
  net: ethtool: optionally skip rtnl_lock in RSS context handlers
  net: ethtool: ioctl: concentrate the locking
  net: ethtool: optionally skip rtnl_lock on IOCTL path
  docs: net: ethtool: document ops-locked drivers and op_needs_rtnl

 Documentation/networking/netdev-features.rst  |   7 +
 Documentation/networking/netdevices.rst       |  17 ++-
 include/linux/ethtool.h                       |  36 ++++-
 include/linux/netdevice.h                     |   3 +
 include/linux/phy_link_topology.h             |   5 +
 include/net/netdev_lock.h                     |  11 ++
 net/ethtool/common.h                          |  76 +++++++++++
 net/ethtool/netlink.h                         |   8 +-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |   4 +
 drivers/net/ethernet/google/gve/gve_ethtool.c |   6 +-
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |   3 +
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |   2 +
 .../mellanox/mlx5/core/ipoib/ethtool.c        |   2 +
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |   5 +
 .../ethernet/microsoft/mana/mana_ethtool.c    |   2 +
 drivers/net/netdevsim/ethtool.c               |   1 +
 drivers/net/phy/phy_device.c                  |   3 +
 drivers/net/phy/phy_link_topology.c           |  10 ++
 net/core/dev_ioctl.c                          |   4 +-
 net/ethtool/cabletest.c                       |  12 +-
 net/ethtool/ioctl.c                           | 123 ++++++++++++++----
 net/ethtool/mm.c                              |   5 +-
 net/ethtool/module.c                          |   6 +-
 net/ethtool/netlink.c                         |  62 ++++++---
 net/ethtool/phy.c                             |   1 -
 net/ethtool/rss.c                             |  21 +--
 net/ethtool/tsconfig.c                        |  10 +-
 net/ethtool/tsinfo.c                          |  32 +++--
 28 files changed, 363 insertions(+), 114 deletions(-)

-- 
2.54.0

[PATCH net-next v2 01/12] net: ethtool: serialize broadcast notification sequence allocation

[Jakub Kicinski]

ethnl_bcast_seq is a global counter stamped into the nlmsg_seq field
of every multicast notification, allowing userspace to detect dropped
messages. Today the ordering is achieved by using rtnl_lock().

Moving forward we will want ethtool ops to run under just the netdev
instance lock so to establish ordering we need a separate lock
for notifications. With the netdev instance locks operations on
different devices may bypass each other but the expectation is
that it should not matter. What we need to prevent is:
 - notification IDs getting out of order
 - operations on one device getting out of order

For simplicity defer allocating the ID of the notification right
before the notification is delivered. This removes the need for
special handling in ethnl_rss_create_send_ntf().

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/netlink.h |  1 -
 net/ethtool/netlink.c | 28 +++++++++++++++++-----------
 net/ethtool/rss.c     |  1 -
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index 674c9c19529b..f94aaa66379c 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -10,7 +10,6 @@
 
 struct ethnl_req_info;
 
-u32 ethnl_bcast_seq_next(void);
 int ethnl_parse_header_dev_get(struct ethnl_req_info *req_info,
 			       const struct nlattr *nest, struct net *net,
 			       struct netlink_ext_ack *extack,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 25e22c48060a..c4054a9795ff 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -13,6 +13,12 @@
 static struct genl_family ethtool_genl_family;
 
 static bool ethnl_ok __read_mostly;
+
+/* Serializes broadcast notification sequence allocation with the multicast
+ * send, so that userspace observes nlmsg_seq monotonic in receive order
+ * regardless of which lock the caller holds (rtnl or instance lock).
+ */
+static DEFINE_MUTEX(ethnl_bcast_lock);
 static u32 ethnl_bcast_seq;
 
 #define ETHTOOL_FLAGS_BASIC (ETHTOOL_FLAG_COMPACT_BITSETS |	\
@@ -82,12 +88,6 @@ static void ethnl_sock_priv_destroy(void *priv)
 	}
 }
 
-u32 ethnl_bcast_seq_next(void)
-{
-	ASSERT_RTNL();
-	return ++ethnl_bcast_seq;
-}
-
 int ethnl_ops_begin(struct net_device *dev)
 {
 	int ret;
@@ -329,8 +329,7 @@ void *ethnl_dump_put(struct sk_buff *skb, struct netlink_callback *cb, u8 cmd)
 
 void *ethnl_bcastmsg_put(struct sk_buff *skb, u8 cmd)
 {
-	return genlmsg_put(skb, 0, ++ethnl_bcast_seq, &ethtool_genl_family, 0,
-			   cmd);
+	return genlmsg_put(skb, 0, 0, &ethtool_genl_family, 0, cmd);
 }
 
 void *ethnl_unicast_put(struct sk_buff *skb, u32 portid, u32 seq, u8 cmd)
@@ -340,8 +339,15 @@ void *ethnl_unicast_put(struct sk_buff *skb, u32 portid, u32 seq, u8 cmd)
 
 int ethnl_multicast(struct sk_buff *skb, struct net_device *dev)
 {
-	return genlmsg_multicast_netns(&ethtool_genl_family, dev_net(dev), skb,
-				       0, ETHNL_MCGRP_MONITOR, GFP_KERNEL);
+	struct nlmsghdr *nlh = nlmsg_hdr(skb);
+	int ret;
+
+	mutex_lock(&ethnl_bcast_lock);
+	nlh->nlmsg_seq = ++ethnl_bcast_seq;
+	ret = genlmsg_multicast_netns(&ethtool_genl_family, dev_net(dev), skb,
+				      0, ETHNL_MCGRP_MONITOR, GFP_KERNEL);
+	mutex_unlock(&ethnl_bcast_lock);
+	return ret;
 }
 
 /* GET request helpers */
@@ -1081,7 +1087,7 @@ void ethnl_notify(struct net_device *dev, unsigned int cmd,
 {
 	if (unlikely(!ethnl_ok))
 		return;
-	ASSERT_RTNL();
+	netdev_assert_locked_ops_compat(dev);
 
 	if (likely(cmd < ARRAY_SIZE(ethnl_notify_handlers) &&
 		   ethnl_notify_handlers[cmd]))
diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index 53792f53f922..65bad23d5c59 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -995,7 +995,6 @@ ethnl_rss_create_send_ntf(const struct sk_buff *rsp, struct net_device *dev)
 	nlh = nlmsg_hdr(ntf);
 	/* Convert the reply into a notification */
 	nlh->nlmsg_pid = 0;
-	nlh->nlmsg_seq = ethnl_bcast_seq_next();
 
 	genl_hdr = nlmsg_data(nlh);
 	genl_hdr->cmd =	ETHTOOL_MSG_RSS_CREATE_NTF;
-- 
2.54.0

[PATCH net-next v2 02/12] net: ethtool: relax ethnl_req_get_phydev() locking assertion

[Jakub Kicinski]

phydev <> netdev linking and lifecycle depends on rtnl_lock.
We want to switch to instance locks for most ethtool ops.
Let's add an assert that ops locked devices don't use phydev
today. If one does we can either opt the phy ops out of
being purely ops locked, or do deeper surgery to make phy
locking ops-compatible. I don't think there's any fundamental
challenge to make that work.

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/phy_link_topology.h   | 5 +++++
 net/ethtool/netlink.h               | 7 ++++---
 drivers/net/phy/phy_link_topology.c | 8 ++++++++
 net/ethtool/netlink.c               | 6 ++++--
 net/ethtool/phy.c                   | 1 -
 5 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/include/linux/phy_link_topology.h b/include/linux/phy_link_topology.h
index 68a59e25821c..95575f68d5bc 100644
--- a/include/linux/phy_link_topology.h
+++ b/include/linux/phy_link_topology.h
@@ -36,6 +36,11 @@ struct phy_device_node {
 	struct phy_device *phy;
 };
 
+static inline bool phy_link_topo_empty(struct net_device *dev)
+{
+	return !dev->link_topo;
+}
+
 #if IS_ENABLED(CONFIG_PHYLIB)
 int phy_link_topo_add_phy(struct net_device *dev,
 			  struct phy_device *phy,
diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h
index f94aaa66379c..4ca2eca2e94b 100644
--- a/net/ethtool/netlink.h
+++ b/net/ethtool/netlink.h
@@ -275,14 +275,15 @@ static inline void ethnl_parse_header_dev_put(struct ethnl_req_info *req_info)
 
 /**
  * ethnl_req_get_phydev() - Gets the phy_device targeted by this request,
- *			    if any. Must be called under rntl_lock().
+ *			    if any.
  * @req_info:	The ethnl request to get the phy from.
  * @tb:		The netlink attributes array, for error reporting.
  * @header:	The netlink header index, used for error reporting.
  * @extack:	The netlink extended ACK, for error reporting.
  *
- * The caller must hold RTNL, until it's done interacting with the returned
- * phy_device.
+ * If a phy_device is returned the caller must hold rtnl_lock when calling
+ * this function, and until it's done interacting with the returned phy_device.
+ * IOW caller must hold rtnl_lock unless they know netdev has no phy_device.
  *
  * Return: A phy_device pointer corresponding either to the passed phy_index
  *	   if one is provided. If not, the phy_device attached to the
diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
index 1f1eb5d59b38..aed3b26c1674 100644
--- a/drivers/net/phy/phy_link_topology.c
+++ b/drivers/net/phy/phy_link_topology.c
@@ -10,6 +10,7 @@
 #include <linux/phy.h>
 #include <linux/rtnetlink.h>
 #include <linux/xarray.h>
+#include <net/netdev_lock.h>
 
 static int netdev_alloc_phy_link_topology(struct net_device *dev)
 {
@@ -35,6 +36,13 @@ int phy_link_topo_add_phy(struct net_device *dev,
 	struct phy_device_node *pdn;
 	int ret;
 
+	/* ethtool ops may run without rtnl_lock, and rtnl_lock is what
+	 * currently protects the PHY topology. No driver currently mixes
+	 * the two, flag if someone tries. See also ethnl_req_get_phydev().
+	 */
+	if (WARN_ON_ONCE(netdev_need_ops_lock(dev)))
+		return -EOPNOTSUPP;
+
 	if (!topo) {
 		ret = netdev_alloc_phy_link_topology(dev);
 		if (ret)
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index c4054a9795ff..afafed738584 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -226,11 +226,13 @@ struct phy_device *ethnl_req_get_phydev(const struct ethnl_req_info *req_info,
 {
 	struct phy_device *phydev;
 
-	ASSERT_RTNL();
-
 	if (!req_info->dev)
 		return NULL;
 
+	/* If there is no PHY in sight there's no need for assert locking */
+	if (!phy_link_topo_empty(req_info->dev))
+		ASSERT_RTNL();
+
 	if (!req_info->phy_index)
 		return req_info->dev->phydev;
 
diff --git a/net/ethtool/phy.c b/net/ethtool/phy.c
index ddc6eab701ed..018b0412be86 100644
--- a/net/ethtool/phy.c
+++ b/net/ethtool/phy.c
@@ -78,7 +78,6 @@ static int phy_prepare_data(const struct ethnl_req_info *req_info,
 	struct phy_device *phydev;
 	int ret;
 
-	/* RTNL is held by the caller */
 	phydev = ethnl_req_get_phydev(req_info, tb, ETHTOOL_A_PHY_HEADER,
 				      info->extack);
 	if (IS_ERR_OR_NULL(phydev))
-- 
2.54.0

[PATCH net-next v2 03/12] net: ethtool: make dev->hwprov ops-protected

[Jakub Kicinski]

dev->hwprov tracks the active hwtstamp provider for the device.
Make it ops protected (instance lock if the netdev driver opts
into holding instance lock around callbacks, otherwise rtnl_lock).

hwprov is written and read in:
 - drivers/net/phy/phy_device.c
    phydev and ops protection don't currently mix, add a comment
 - net/ethtool/
    as of now holds both rtnl lock and ops lock, this one will
    soon only hold one lock or the other

read in:
 - net/core/dev_ioctl.c
    holds both rtnl lock and ops lock
 - net/core/timestamping.c
    RCU reader

The new netdev_ops_lock_dereference() helper does not have
"compat" in the name. The name would be quite long and I think
in this case it should be obvious that we need _a_ lock.
netdev_lock_dereference() already exists and means dev->lock
is always expected.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/netdevice.h           |  3 +++
 include/net/netdev_lock.h           | 11 +++++++++++
 drivers/net/phy/phy_device.c        |  3 +++
 drivers/net/phy/phy_link_topology.c |  4 +++-
 net/core/dev_ioctl.c                |  4 ++--
 net/ethtool/tsconfig.c              | 10 ++++++----
 6 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 74507c006490..a8709d0cc8d4 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2583,6 +2583,9 @@ struct net_device {
 	 * Double protects:
 	 *	@up, @moving_ns, @nd_net, @xdp_features
 	 *
+	 * Ops protects:
+	 *	@hwprov
+	 *
 	 * Double ops protects:
 	 *	@real_num_rx_queues, @real_num_tx_queues
 	 *
diff --git a/include/net/netdev_lock.h b/include/net/netdev_lock.h
index d3daec4e93f3..9fb3e93857c3 100644
--- a/include/net/netdev_lock.h
+++ b/include/net/netdev_lock.h
@@ -102,6 +102,14 @@ static inline void netdev_unlock_ops_compat(struct net_device *dev)
 		rtnl_unlock();
 }
 
+/* Matching "ops protected" category from netdevice.h */
+static inline int netdev_is_locked_ops_compat(const struct net_device *dev)
+{
+	if (netdev_need_ops_lock(dev))
+		return lockdep_is_held(&dev->lock);
+	return lockdep_rtnl_is_held();
+}
+
 static inline int netdev_lock_cmp_fn(const struct lockdep_map *a,
 				     const struct lockdep_map *b)
 {
@@ -138,6 +146,9 @@ static inline int netdev_lock_cmp_fn(const struct lockdep_map *a,
 #define netdev_lock_dereference(p, dev)				\
 	rcu_dereference_protected(p, lockdep_is_held(&(dev)->lock))
 
+#define netdev_ops_lock_dereference(p, dev)				\
+	rcu_dereference_protected(p, netdev_is_locked_ops_compat(dev))
+
 int netdev_debug_event(struct notifier_block *nb, unsigned long event,
 		       void *ptr);
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3370eb822017..ea53e477465d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1935,6 +1935,9 @@ void phy_detach(struct phy_device *phydev)
 	if (dev) {
 		struct hwtstamp_provider *hwprov;
 
+		/* hwprov may technically be protected by ops lock but
+		 * not for devices with a phydev, see phy_link_topo_add_phy()
+		 */
 		hwprov = rtnl_dereference(dev->hwprov);
 		/* Disable timestamp if it is the one selected */
 		if (hwprov && hwprov->phydev == phydev) {
diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
index aed3b26c1674..4134de7ae313 100644
--- a/drivers/net/phy/phy_link_topology.c
+++ b/drivers/net/phy/phy_link_topology.c
@@ -38,7 +38,9 @@ int phy_link_topo_add_phy(struct net_device *dev,
 
 	/* ethtool ops may run without rtnl_lock, and rtnl_lock is what
 	 * currently protects the PHY topology. No driver currently mixes
-	 * the two, flag if someone tries. See also ethnl_req_get_phydev().
+	 * the two, flag if someone tries. See also:
+	 *  - ethnl_req_get_phydev()
+	 *  - phy_detach()
 	 */
 	if (WARN_ON_ONCE(netdev_need_ops_lock(dev)))
 		return -EOPNOTSUPP;
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index f3979b276090..a320e264eaaf 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -260,7 +260,7 @@ int dev_get_hwtstamp_phylib(struct net_device *dev,
 {
 	struct hwtstamp_provider *hwprov;
 
-	hwprov = rtnl_dereference(dev->hwprov);
+	hwprov = netdev_ops_lock_dereference(dev->hwprov, dev);
 	if (hwprov) {
 		cfg->qualifier = hwprov->desc.qualifier;
 		if (hwprov->source == HWTSTAMP_SOURCE_PHYLIB &&
@@ -337,7 +337,7 @@ int dev_set_hwtstamp_phylib(struct net_device *dev,
 	bool phy_ts;
 	int err;
 
-	hwprov = rtnl_dereference(dev->hwprov);
+	hwprov = netdev_ops_lock_dereference(dev->hwprov, dev);
 	if (hwprov) {
 		if (hwprov->source == HWTSTAMP_SOURCE_PHYLIB &&
 		    hwprov->phydev) {
diff --git a/net/ethtool/tsconfig.c b/net/ethtool/tsconfig.c
index 664c3fe49b5b..24b64862011f 100644
--- a/net/ethtool/tsconfig.c
+++ b/net/ethtool/tsconfig.c
@@ -2,6 +2,7 @@
 
 #include <linux/net_tstamp.h>
 #include <linux/ptp_clock_kernel.h>
+#include <net/netdev_lock.h>
 
 #include "bitset.h"
 #include "common.h"
@@ -57,7 +58,7 @@ static int tsconfig_prepare_data(const struct ethnl_req_info *req_base,
 	data->hwtst_config.flags = cfg.flags;
 
 	data->hwprov_desc.index = -1;
-	hwprov = rtnl_dereference(dev->hwprov);
+	hwprov = netdev_ops_lock_dereference(dev->hwprov, dev);
 	if (hwprov) {
 		data->hwprov_desc.index = hwprov->desc.index;
 		data->hwprov_desc.qualifier = hwprov->desc.qualifier;
@@ -213,7 +214,7 @@ static int tsconfig_send_reply(struct net_device *dev, struct genl_info *info)
 		return -ENOMEM;
 	}
 
-	ASSERT_RTNL();
+	netdev_assert_locked_ops_compat(dev);
 	reply_data->base.dev = dev;
 	ret = tsconfig_prepare_data(&req_info->base, &reply_data->base, info);
 	if (ret < 0)
@@ -316,7 +317,7 @@ static int ethnl_set_tsconfig(struct ethnl_req_info *req_base,
 		struct hwtstamp_provider_desc __hwprov_desc = {.index = -1};
 		struct hwtstamp_provider *__hwprov;
 
-		__hwprov = rtnl_dereference(dev->hwprov);
+		__hwprov = netdev_ops_lock_dereference(dev->hwprov, dev);
 		if (__hwprov) {
 			__hwprov_desc.index = __hwprov->desc.index;
 			__hwprov_desc.qualifier = __hwprov->desc.qualifier;
@@ -414,7 +415,8 @@ static int ethnl_set_tsconfig(struct ethnl_req_info *req_base,
 			goto err_free_hwprov;
 
 		/* Change the selected hwtstamp source */
-		__hwprov = rcu_replace_pointer_rtnl(dev->hwprov, hwprov);
+		__hwprov = rcu_replace_pointer(dev->hwprov, hwprov,
+					       netdev_is_locked_ops_compat(dev));
 		if (__hwprov)
 			kfree_rcu(__hwprov, rcu_head);
 	}
-- 
2.54.0

[PATCH net-next v2 04/12] net: ethtool: optionally skip rtnl_lock on Netlink path for GET ops

[Jakub Kicinski]

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 it's 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>
---
v2:
 - replace ASSERT_RTNL() with netdev_assert_locked() in gve (Stan)
   I said I will remove it but I see that there are reset paths
   which explicitly take rtnl_lock, which I suspect may be the
   reason for the assert?
---
 include/linux/ethtool.h                       | 18 +++++++-
 net/ethtool/common.h                          | 46 +++++++++++++++++++
 drivers/net/ethernet/google/gve/gve_ethtool.c |  4 +-
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |  2 +
 net/ethtool/mm.c                              |  5 +-
 net/ethtool/netlink.c                         | 19 ++++++--
 6 files changed, 85 insertions(+), 9 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index f51346a6a686..1da49161d36f 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/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index dc2213b5ce24..7cd43e082b2a 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -4,7 +4,7 @@
  * Copyright (C) 2015-2024 Google LLC
  */
 
-#include <linux/rtnetlink.h>
+#include <net/netdev_lock.h>
 #include "gve.h"
 #include "gve_adminq.h"
 #include "gve_dqo.h"
@@ -171,7 +171,7 @@ gve_get_ethtool_stats(struct net_device *netdev,
 	int ring;
 	int i, j;
 
-	ASSERT_RTNL();
+	netdev_assert_locked(netdev);
 
 	priv = netdev_priv(netdev);
 	num_tx_queues = gve_num_tx_queues(priv);
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..2d10e2a1393f 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_assert_locked_ops_compat() 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 afafed738584..2c30eb1f4666 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

[PATCH net-next v2 05/12] net: ethtool: optionally skip rtnl_lock on Netlink path for SET ops

[Jakub Kicinski]

Make ethtool not take rtnl_lock for SET commands when operation
is performed on an ops-locked driver. cfg/cfg_pending are now
ops-locked, since only ethtool modifies them.

Some SET driver callbacks will still need rtnl_lock, most notably
those which may end up calling netdev_update_features() or the qdisc
layer (via netif_set_real_num_tx_queues()). Let drivers selectively
opt back into the rtnl_lock with a new bitfield in ops.

We need two helpers since Netlink and ioctl cmds have different
values. Keep the helpers side by side in common.h to make sure
they get updated together, even tho they will only get called
from ioctl.c and netlink.c.

SET commands which don't use ethnl_default_set_doit() are converted
by subsequent commits.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/ethtool.h                       | 10 ++++++-
 include/linux/netdevice.h                     |  2 +-
 net/ethtool/common.h                          | 30 +++++++++++++++++++
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  4 +++
 drivers/net/ethernet/google/gve/gve_ethtool.c |  2 ++
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  3 ++
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |  2 ++
 .../mellanox/mlx5/core/ipoib/ethtool.c        |  2 ++
 .../net/ethernet/meta/fbnic/fbnic_ethtool.c   |  5 +++-
 .../ethernet/microsoft/mana/mana_ethtool.c    |  2 ++
 drivers/net/netdevsim/ethtool.c               |  1 +
 net/ethtool/netlink.c                         |  9 ++++--
 12 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 1da49161d36f..74c8109b0cf3 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -935,7 +935,13 @@ struct kernel_ethtool_ts_info {
  * 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)
+#define ETHTOOL_OP_NEEDS_RTNL_SPFLAGS		BIT(1)
+#define ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM	BIT(2)
+#define ETHTOOL_OP_NEEDS_RTNL_SCHANNELS		BIT(3)
+#define ETHTOOL_OP_NEEDS_RTNL_SCOALESCE		BIT(4)
+#define ETHTOOL_OP_NEEDS_RTNL_GPAUSEPARAM	BIT(5)
+#define ETHTOOL_OP_NEEDS_RTNL_SPAUSEPARAM	BIT(6)
+#define ETHTOOL_OP_NEEDS_RTNL_RSS		BIT(7)
 
 /**
  * struct ethtool_ops - optional netdev operations
@@ -970,6 +976,8 @@ struct kernel_ethtool_ts_info {
  *	The following commonly used core APIs currently require rtnl_lock
  *	(this list may not be exhaustive):
  *	 - phylink helpers (note that phydev is currently unsupported!)
+ *	 - netdev_update_features()
+ *	 - netif_set_real_num_tx_queues()
  *
  * @get_drvinfo: Report driver/device information. Modern drivers no
  *	longer have to implement this callback. Most fields are
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index a8709d0cc8d4..403b6d1c67f8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2584,7 +2584,7 @@ struct net_device {
 	 *	@up, @moving_ns, @nd_net, @xdp_features
 	 *
 	 * Ops protects:
-	 *	@hwprov
+	 *	@cfg, @cfg_pending, @hwprov
 	 *
 	 * Double ops protects:
 	 *	@real_num_rx_queues, @real_num_tx_queues
diff --git a/net/ethtool/common.h b/net/ethtool/common.h
index 391c41ca56be..e3052972f953 100644
--- a/net/ethtool/common.h
+++ b/net/ethtool/common.h
@@ -95,10 +95,24 @@ ethtool_nl_msg_needs_rtnl(const struct net_device *dev, u8 cmd)
 
 	switch (cmd) {
 	case ETHTOOL_MSG_LINKINFO_GET:
+	case ETHTOOL_MSG_LINKINFO_SET:
 	case ETHTOOL_MSG_LINKMODES_GET:
+	case ETHTOOL_MSG_LINKMODES_SET:
 		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS;
+	case ETHTOOL_MSG_PRIVFLAGS_SET:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_SPFLAGS;
+	case ETHTOOL_MSG_RINGS_SET:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM;
+	case ETHTOOL_MSG_CHANNELS_SET:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_SCHANNELS;
+	case ETHTOOL_MSG_COALESCE_SET:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_SCOALESCE;
 	case ETHTOOL_MSG_PAUSE_GET:
 		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_GPAUSEPARAM;
+	case ETHTOOL_MSG_PAUSE_SET:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_SPAUSEPARAM;
+	case ETHTOOL_MSG_RSS_SET:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_RSS;
 	}
 	return false;
 }
@@ -119,9 +133,25 @@ ethtool_ioctl_needs_rtnl(const struct net_device *dev, u32 ethcmd)
 	switch (ethcmd) {
 	case ETHTOOL_GLINKSETTINGS:
 	case ETHTOOL_GSET:
+	case ETHTOOL_SLINKSETTINGS:
+	case ETHTOOL_SSET:
 		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_LINKSETTINGS;
+	case ETHTOOL_SPFLAGS:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_SPFLAGS;
+	case ETHTOOL_SRINGPARAM:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM;
+	case ETHTOOL_SCHANNELS:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_SCHANNELS;
+	case ETHTOOL_SCOALESCE:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_SCOALESCE;
 	case ETHTOOL_GPAUSEPARAM:
 		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_GPAUSEPARAM;
+	case ETHTOOL_SPAUSEPARAM:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_SPAUSEPARAM;
+	case ETHTOOL_SRSSH:
+	case ETHTOOL_SRXFH:
+	case ETHTOOL_SRXFHINDIR:
+		return ops->op_needs_rtnl & ETHTOOL_OP_NEEDS_RTNL_RSS;
 	}
 	return false;
 }
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index edafa79f636c..56d74a3c24b7 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -5729,6 +5729,10 @@ const struct ethtool_ops bnxt_ethtool_ops = {
 	.rxfh_max_num_contexts		= BNXT_MAX_ETH_RSS_CTX + 1,
 	.rxfh_indir_space		= BNXT_MAX_RSS_TABLE_ENTRIES_P5,
 	.rxfh_priv_size			= sizeof(struct bnxt_rss_ctx),
+	.op_needs_rtnl			= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
+					  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
+					  ETHTOOL_OP_NEEDS_RTNL_SCOALESCE |
+					  ETHTOOL_OP_NEEDS_RTNL_RSS,
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES |
 				     ETHTOOL_COALESCE_USECS_IRQ |
diff --git a/drivers/net/ethernet/google/gve/gve_ethtool.c b/drivers/net/ethernet/google/gve/gve_ethtool.c
index 7cd43e082b2a..7cc22916852f 100644
--- a/drivers/net/ethernet/google/gve/gve_ethtool.c
+++ b/drivers/net/ethernet/google/gve/gve_ethtool.c
@@ -983,6 +983,8 @@ const struct ethtool_ops gve_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS,
 	.supported_ring_params = ETHTOOL_RING_USE_TCP_DATA_SPLIT |
 				 ETHTOOL_RING_USE_RX_BUF_LEN,
+	.op_needs_rtnl = ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
+			 ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM,
 	.get_drvinfo = gve_get_drvinfo,
 	.get_strings = gve_get_strings,
 	.get_sset_count = gve_get_sset_count,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 61993485e451..2f5b626ba33f 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -2719,6 +2719,9 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
 	.rxfh_per_ctx_fields	= true,
 	.rxfh_per_ctx_key	= true,
 	.rxfh_max_num_contexts	= MLX5E_MAX_NUM_RSS,
+	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
+				  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM |
+				  ETHTOOL_OP_NEEDS_RTNL_SPFLAGS,
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE |
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
index ba6c0f38cc73..1a8a19f980d3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
@@ -418,6 +418,8 @@ static const struct ethtool_ops mlx5e_rep_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE,
+	.op_needs_rtnl	   = ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
+			     ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM,
 	.get_drvinfo	   = mlx5e_rep_get_drvinfo,
 	.get_link	   = ethtool_op_get_link,
 	.get_strings       = mlx5e_rep_get_strings,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c
index 3b2f54ca30a8..9b3b32408c64 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ethtool.c
@@ -285,6 +285,8 @@ const struct ethtool_ops mlx5i_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_USECS |
 				     ETHTOOL_COALESCE_MAX_FRAMES |
 				     ETHTOOL_COALESCE_USE_ADAPTIVE,
+	.op_needs_rtnl	    = ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
+			      ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM,
 	.get_drvinfo        = mlx5i_get_drvinfo,
 	.get_strings        = mlx5i_get_strings,
 	.get_sset_count     = mlx5i_get_sset_count,
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
index a2c16d599389..cb34fc166ef9 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_ethtool.c
@@ -2021,7 +2021,10 @@ static const struct ethtool_ops fbnic_ethtool_ops = {
 					  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,
+					  ETHTOOL_OP_NEEDS_RTNL_GPAUSEPARAM |
+					  ETHTOOL_OP_NEEDS_RTNL_SPAUSEPARAM |
+					  ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
+					  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM,
 	.get_drvinfo			= fbnic_get_drvinfo,
 	.get_regs_len			= fbnic_get_regs_len,
 	.get_regs			= fbnic_get_regs,
diff --git a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
index 04350973e19e..55df44d728a0 100644
--- a/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
+++ b/drivers/net/ethernet/microsoft/mana/mana_ethtool.c
@@ -575,6 +575,8 @@ static int mana_get_link_ksettings(struct net_device *ndev,
 
 const struct ethtool_ops mana_ethtool_ops = {
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_CQE_FRAMES,
+	.op_needs_rtnl		= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS |
+				  ETHTOOL_OP_NEEDS_RTNL_SRINGPARAM,
 	.get_ethtool_stats	= mana_get_ethtool_stats,
 	.get_sset_count		= mana_get_sset_count,
 	.get_strings		= mana_get_strings,
diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index 36a201533aae..9350ba48eb81 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c
@@ -209,6 +209,7 @@ static const struct ethtool_ops nsim_ethtool_ops = {
 	.supported_coalesce_params	= ETHTOOL_COALESCE_ALL_PARAMS,
 	.supported_ring_params		= ETHTOOL_RING_USE_TCP_DATA_SPLIT |
 					  ETHTOOL_RING_USE_HDS_THRS,
+	.op_needs_rtnl			= ETHTOOL_OP_NEEDS_RTNL_SCHANNELS,
 	.get_pause_stats	        = nsim_get_pause_stats,
 	.get_pauseparam		        = nsim_get_pauseparam,
 	.set_pauseparam		        = nsim_set_pauseparam,
diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
index 2c30eb1f4666..1af395b54330 100644
--- a/net/ethtool/netlink.c
+++ b/net/ethtool/netlink.c
@@ -903,6 +903,7 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	const u8 cmd = info->genlhdr->cmd;
 	struct ethnl_req_info *req_info;
 	struct net_device *dev;
+	bool need_rtnl;
 	int ret;
 
 	ops = ethnl_default_requests[cmd];
@@ -927,8 +928,11 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	dev = req_info->dev;
+	need_rtnl = !netdev_need_ops_lock(dev) ||
+		    ethtool_nl_msg_needs_rtnl(dev, cmd);
 
-	rtnl_lock();
+	if (need_rtnl)
+		rtnl_lock();
 	netdev_lock_ops(dev);
 	dev->cfg_pending = kmemdup(dev->cfg, sizeof(*dev->cfg),
 				   GFP_KERNEL_ACCOUNT);
@@ -958,7 +962,8 @@ static int ethnl_default_set_doit(struct sk_buff *skb, struct genl_info *info)
 out_tie_cfg:
 	dev->cfg_pending = dev->cfg;
 	netdev_unlock_ops(dev);
-	rtnl_unlock();
+	if (need_rtnl)
+		rtnl_unlock();
 out_dev:
 	ethnl_parse_header_dev_put(req_info);
 out_free_req:
-- 
2.54.0

[PATCH net-next v2 06/12] net: ethtool: optionally skip rtnl_lock in cable test handlers

[Jakub Kicinski]

Skip rtnl_lock in cable test handlers. This is really a noop since
no ops locked device supports these.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/cabletest.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/ethtool/cabletest.c b/net/ethtool/cabletest.c
index 8d375dac2a40..9c22d4c767c6 100644
--- a/net/ethtool/cabletest.c
+++ b/net/ethtool/cabletest.c
@@ -73,8 +73,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 
 	dev = req_info.dev;
 
-	rtnl_lock();
-	netdev_lock_ops(dev);
+	netdev_lock_ops_compat(dev);
 	phydev = ethnl_req_get_phydev(&req_info, tb,
 				      ETHTOOL_A_CABLE_TEST_HEADER,
 				      info->extack);
@@ -101,8 +100,7 @@ int ethnl_act_cable_test(struct sk_buff *skb, struct genl_info *info)
 		ethnl_cable_test_started(phydev, ETHTOOL_MSG_CABLE_TEST_NTF);
 
 out_unlock:
-	netdev_unlock_ops(dev);
-	rtnl_unlock();
+	netdev_unlock_ops_compat(dev);
 	ethnl_parse_header_dev_put(&req_info);
 	return ret;
 }
@@ -342,8 +340,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 	if (ret)
 		goto out_dev_put;
 
-	rtnl_lock();
-	netdev_lock_ops(dev);
+	netdev_lock_ops_compat(dev);
 	phydev = ethnl_req_get_phydev(&req_info, tb,
 				      ETHTOOL_A_CABLE_TEST_TDR_HEADER,
 				      info->extack);
@@ -371,8 +368,7 @@ int ethnl_act_cable_test_tdr(struct sk_buff *skb, struct genl_info *info)
 					 ETHTOOL_MSG_CABLE_TEST_TDR_NTF);
 
 out_unlock:
-	netdev_unlock_ops(dev);
-	rtnl_unlock();
+	netdev_unlock_ops_compat(dev);
 out_dev_put:
 	ethnl_parse_header_dev_put(&req_info);
 	return ret;
-- 
2.54.0

[PATCH net-next v2 07/12] net: ethtool: optionally skip rtnl_lock in ethnl_tsinfo_dumpit()

[Jakub Kicinski]

ethnl_tsinfo_dumpit() iterates netdevs and per-netdev PHY topology
calling ops->get_ts_info(). Switch to the "ops compat locking"
helpers which take either rtnl_lock or instance lock, depending
on what the device needs.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/tsinfo.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/net/ethtool/tsinfo.c b/net/ethtool/tsinfo.c
index 14bf01e3b55c..c9b680a9cc3f 100644
--- a/net/ethtool/tsinfo.c
+++ b/net/ethtool/tsinfo.c
@@ -6,6 +6,7 @@
 #include <linux/ptp_clock_kernel.h>
 #include <net/netdev_lock.h>
 
+#include "../core/dev.h"
 #include "bitset.h"
 #include "common.h"
 #include "netlink.h"
@@ -473,28 +474,25 @@ int ethnl_tsinfo_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct ethnl_tsinfo_dump_ctx *ctx = (void *)cb->ctx;
 	struct net *net = sock_net(skb->sk);
-	struct net_device *dev;
 	int ret = 0;
 
-	rtnl_lock();
 	if (ctx->req_info->base.dev) {
-		dev = ctx->req_info->base.dev;
-		netdev_lock_ops(dev);
+		struct net_device *dev = ctx->req_info->base.dev;
+
+		netdev_lock_ops_compat(dev);
 		ret = ethnl_tsinfo_dump_one_net_topo(skb, dev, cb);
-		netdev_unlock_ops(dev);
-	} else {
-		for_each_netdev_dump(net, dev, ctx->pos_ifindex) {
-			netdev_lock_ops(dev);
-			ret = ethnl_tsinfo_dump_one_net_topo(skb, dev, cb);
-			netdev_unlock_ops(dev);
-			if (ret < 0 && ret != -EOPNOTSUPP)
-				break;
-			ctx->pos_phyindex = 0;
-			ctx->netdev_dump_done = false;
-			ctx->pos_phcqualifier = HWTSTAMP_PROVIDER_QUALIFIER_PRECISE;
-		}
+		netdev_unlock_ops_compat(dev);
+		return ret;
+	}
+
+	for_each_netdev_lock_ops_compat_scoped(net, dev, ctx->pos_ifindex) {
+		ret = ethnl_tsinfo_dump_one_net_topo(skb, dev, cb);
+		if (ret < 0 && ret != -EOPNOTSUPP)
+			break;
+		ctx->pos_phyindex = 0;
+		ctx->netdev_dump_done = false;
+		ctx->pos_phcqualifier = HWTSTAMP_PROVIDER_QUALIFIER_PRECISE;
 	}
-	rtnl_unlock();
 
 	return ret;
 }
-- 
2.54.0

[PATCH net-next v2 08/12] net: ethtool: optionally skip rtnl_lock in ethnl_act_module_fw_flash()

[Jakub Kicinski]

Module firmware flashing reads SFF-8024 identifier bytes via
.get_module_eeprom_by_page(). Other than that it modifies
a bit in the netdev->ethtool struct. Both should be ops-locked
at this point.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/netdevice.h | 2 +-
 net/ethtool/module.c      | 6 ++----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 403b6d1c67f8..9b876cd930d7 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2584,7 +2584,7 @@ struct net_device {
 	 *	@up, @moving_ns, @nd_net, @xdp_features
 	 *
 	 * Ops protects:
-	 *	@cfg, @cfg_pending, @hwprov
+	 *	@cfg, @cfg_pending, @ethtool, @hwprov
 	 *
 	 * Double ops protects:
 	 *	@real_num_rx_queues, @real_num_tx_queues
diff --git a/net/ethtool/module.c b/net/ethtool/module.c
index c3388e6d7ec8..9cf670e089f2 100644
--- a/net/ethtool/module.c
+++ b/net/ethtool/module.c
@@ -429,8 +429,7 @@ int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info)
 		return ret;
 	dev = req_info.dev;
 
-	rtnl_lock();
-	netdev_lock_ops(dev);
+	netdev_lock_ops_compat(dev);
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
 		goto out_unlock;
@@ -445,8 +444,7 @@ int ethnl_act_module_fw_flash(struct sk_buff *skb, struct genl_info *info)
 	ethnl_ops_complete(dev);
 
 out_unlock:
-	netdev_unlock_ops(dev);
-	rtnl_unlock();
+	netdev_unlock_ops_compat(dev);
 	ethnl_parse_header_dev_put(&req_info);
 	return ret;
 }
-- 
2.54.0

[PATCH net-next v2 09/12] net: ethtool: optionally skip rtnl_lock in RSS context handlers

[Jakub Kicinski]

Skip rtnl_lock in RSS context handlers if device is ops-locked.
Fairly trivial conversion. bnxt needed rtnl_lock for changing
the main context but looks like additional contexts are fine
without it.

Note (for review bots?) that ethnl_ops_begin() checks whether
the device is still registered.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/rss.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/net/ethtool/rss.c b/net/ethtool/rss.c
index 65bad23d5c59..d8adc78e3775 100644
--- a/net/ethtool/rss.c
+++ b/net/ethtool/rss.c
@@ -2,6 +2,7 @@
 
 #include <net/netdev_lock.h>
 
+#include "../core/dev.h"
 #include "common.h"
 #include "netlink.h"
 
@@ -468,21 +469,16 @@ int ethnl_rss_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
 {
 	struct rss_nl_dump_ctx *ctx = rss_dump_ctx(cb);
 	struct net *net = sock_net(skb->sk);
-	struct net_device *dev;
 	int ret = 0;
 
-	rtnl_lock();
-	for_each_netdev_dump(net, dev, ctx->ifindex) {
+	for_each_netdev_lock_ops_compat_scoped(net, dev, ctx->ifindex) {
 		if (ctx->match_ifindex && ctx->match_ifindex != ctx->ifindex)
 			break;
 
-		netdev_lock_ops(dev);
 		ret = rss_dump_one_dev(skb, cb, dev);
-		netdev_unlock_ops(dev);
 		if (ret)
 			break;
 	}
-	rtnl_unlock();
 
 	return ret;
 }
@@ -1037,8 +1033,7 @@ int ethnl_rss_create_doit(struct sk_buff *skb, struct genl_info *info)
 	if (ret)
 		goto exit_free_dev;
 
-	rtnl_lock();
-	netdev_lock_ops(dev);
+	netdev_lock_ops_compat(dev);
 
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
@@ -1125,8 +1120,7 @@ int ethnl_rss_create_doit(struct sk_buff *skb, struct genl_info *info)
 exit_ops:
 	ethnl_ops_complete(dev);
 exit_dev_unlock:
-	netdev_unlock_ops(dev);
-	rtnl_unlock();
+	netdev_unlock_ops_compat(dev);
 exit_free_dev:
 	ethnl_parse_header_dev_put(&req.base);
 exit_free_rsp:
@@ -1179,8 +1173,7 @@ int ethnl_rss_delete_doit(struct sk_buff *skb, struct genl_info *info)
 		goto exit_free_dev;
 	}
 
-	rtnl_lock();
-	netdev_lock_ops(dev);
+	netdev_lock_ops_compat(dev);
 
 	ret = ethnl_ops_begin(dev);
 	if (ret < 0)
@@ -1210,8 +1203,7 @@ int ethnl_rss_delete_doit(struct sk_buff *skb, struct genl_info *info)
 	mutex_unlock(&dev->ethtool->rss_lock);
 	ethnl_ops_complete(dev);
 exit_dev_unlock:
-	netdev_unlock_ops(dev);
-	rtnl_unlock();
+	netdev_unlock_ops_compat(dev);
 exit_free_dev:
 	ethnl_parse_header_dev_put(&req);
 	return ret;
-- 
2.54.0

[PATCH net-next v2 10/12] net: ethtool: ioctl: concentrate the locking

[Jakub Kicinski]

Add another layer of helper functions to make upcoming locking
changes easier. Otherwise we'd need a pretty complex goto
structure. netdev instance lock is now taken slightly sooner
but that should not be an issue since rtnl_lock is already held,
anyway.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/ethtool/ioctl.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index a4b0cbae4063..c32c099845e3 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -3258,18 +3258,14 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 /* The main entry point in this file.  Called from net/core/dev_ioctl.c */
 
 static int
-__dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
-	      u32 ethcmd, struct ethtool_devlink_compat *devlink_state)
+dev_ethtool_locked(struct net *net, struct net_device *dev,
+		   void __user *useraddr,
+		   u32 ethcmd, struct ethtool_devlink_compat *devlink_state)
 {
-	struct net_device *dev;
 	u32 sub_cmd;
 	int rc;
 	netdev_features_t old_features;
 
-	dev = __dev_get_by_name(net, ifr->ifr_name);
-	if (!dev)
-		return -ENODEV;
-
 	if (ethcmd == ETHTOOL_PERQUEUE) {
 		if (copy_from_user(&sub_cmd, useraddr + sizeof(ethcmd), sizeof(sub_cmd)))
 			return -EFAULT;
@@ -3320,7 +3316,6 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
 			return -EPERM;
 	}
 
-	netdev_lock_ops(dev);
 	if (dev->dev.parent)
 		pm_runtime_get_sync(dev->dev.parent);
 
@@ -3560,7 +3555,29 @@ __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
 out:
 	if (dev->dev.parent)
 		pm_runtime_put(dev->dev.parent);
+
+	return rc;
+}
+
+static int
+__dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
+	      u32 ethcmd, struct ethtool_devlink_compat *devlink_state)
+{
+	struct net_device *dev;
+	int rc;
+
+	rtnl_lock();
+	dev = __dev_get_by_name(net, ifr->ifr_name);
+	if (!dev) {
+		rc = -ENODEV;
+		goto exit_rtnl_unlock;
+	}
+
+	netdev_lock_ops(dev);
+	rc = dev_ethtool_locked(net, dev, useraddr, ethcmd, devlink_state);
 	netdev_unlock_ops(dev);
+exit_rtnl_unlock:
+	rtnl_unlock();
 
 	return rc;
 }
@@ -3588,9 +3605,7 @@ int dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr)
 		break;
 	}
 
-	rtnl_lock();
 	rc = __dev_ethtool(net, ifr, useraddr, ethcmd, state);
-	rtnl_unlock();
 	if (rc)
 		goto exit_free;
 
-- 
2.54.0

[PATCH net-next v2 11/12] net: ethtool: optionally skip rtnl_lock on IOCTL path

[Jakub Kicinski]

Convert the IOCTL path similarly to how we converted Netlink.
The device lookup gets a little hairy. We could take rtnl_lock
unconditionally and drop it before calling the driver (this would
avoid the reference + liveness check). But I think being able
to make progress even if rtnl is dead-locked is quite useful.

First extra concern is handling features. List all the cmds which
modify features and always take rtnl_lock. We could fold this list
into ethtool_ioctl_needs_rtnl() but seems cleaner to keep
ethtool_ioctl_needs_rtnl() driver-related. If a driver changed
features and we were not holding rtnl_lock - warn about it.
It can only happen on buggy ops locked drivers (buggy because
they should have set appropriate "I need rtnl for op X" bit).

Second wrinkle is the PHY ID hack which drops the locks while
sleeping. Convert its static "busy" variable which used to
be protected by rtnl_lock to a field in struct ethtool_netdev_state.
This feature is about identifying an adapter or a port within
a system, so being able to blink multiple LEDs at the same
time is likely not very useful in practice. But it's the simplest
fix, we can add a mutex if someone thinks a system should only
be ID'ing one port at a time.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/linux/ethtool.h |   2 +
 net/ethtool/ioctl.c     | 106 ++++++++++++++++++++++++++++++----------
 2 files changed, 82 insertions(+), 26 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 74c8109b0cf3..da29017c757f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1375,6 +1375,7 @@ int ethtool_virtdev_set_link_ksettings(struct net_device *dev,
  *			within RTNL.
  * @rss_indir_user_size: Number of user provided entries for the default
  *			 (context 0) indirection table.
+ * @phys_id_busy:	Loop blinking the device LED is running.
  * @wol_enabled:	Wake-on-LAN is enabled
  * @module_fw_flash_in_progress: Module firmware flashing is in progress.
  */
@@ -1382,6 +1383,7 @@ struct ethtool_netdev_state {
 	struct xarray		rss_ctx;
 	struct mutex		rss_lock;
 	u32			rss_indir_user_size;
+	unsigned		phys_id_busy:1;
 	unsigned		wol_enabled:1;
 	unsigned		module_fw_flash_in_progress:1;
 };
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index c32c099845e3..a7bff829b758 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -544,7 +544,7 @@ static int ethtool_get_link_ksettings(struct net_device *dev,
 	int err = 0;
 	struct ethtool_link_ksettings link_ksettings;
 
-	ASSERT_RTNL();
+	netdev_assert_locked_ops_compat(dev);
 	if (!dev->ethtool_ops->get_link_ksettings)
 		return -EOPNOTSUPP;
 
@@ -601,7 +601,7 @@ static int ethtool_set_link_ksettings(struct net_device *dev,
 	struct ethtool_link_ksettings link_ksettings = {};
 	int err;
 
-	ASSERT_RTNL();
+	netdev_assert_locked_ops_compat(dev);
 
 	if (!dev->ethtool_ops->set_link_ksettings)
 		return -EOPNOTSUPP;
@@ -675,7 +675,7 @@ static int ethtool_get_settings(struct net_device *dev, void __user *useraddr)
 	struct ethtool_cmd cmd;
 	int err;
 
-	ASSERT_RTNL();
+	netdev_assert_locked_ops_compat(dev);
 	if (!dev->ethtool_ops->get_link_ksettings)
 		return -EOPNOTSUPP;
 
@@ -711,7 +711,7 @@ static int ethtool_set_settings(struct net_device *dev, void __user *useraddr)
 	struct ethtool_cmd cmd;
 	int ret;
 
-	ASSERT_RTNL();
+	netdev_assert_locked_ops_compat(dev);
 
 	if (copy_from_user(&cmd, useraddr, sizeof(cmd)))
 		return -EFAULT;
@@ -2452,10 +2452,10 @@ void ethtool_puts(u8 **data, const char *str)
 }
 EXPORT_SYMBOL(ethtool_puts);
 
-static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
+static int ethtool_phys_id(struct net_device *dev, void __user *useraddr,
+			   bool has_rtnl_lock)
 {
 	struct ethtool_value id;
-	static bool busy;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 	netdevice_tracker dev_tracker;
 	int rc;
@@ -2463,7 +2463,7 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
 	if (!ops->set_phys_id)
 		return -EOPNOTSUPP;
 
-	if (busy)
+	if (dev->ethtool->phys_id_busy)
 		return -EBUSY;
 
 	if (copy_from_user(&id, useraddr, sizeof(id)))
@@ -2473,13 +2473,14 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
 	if (rc < 0)
 		return rc;
 
-	/* Drop the RTNL lock while waiting, but prevent reentry or
+	/* Drop the locks while waiting, but prevent reentry or
 	 * removal of the device.
 	 */
-	busy = true;
+	dev->ethtool->phys_id_busy = true;
 	netdev_hold(dev, &dev_tracker, GFP_KERNEL);
 	netdev_unlock_ops(dev);
-	rtnl_unlock();
+	if (has_rtnl_lock)
+		rtnl_unlock();
 
 	if (rc == 0) {
 		/* Driver will handle this itself */
@@ -2492,22 +2493,25 @@ static int ethtool_phys_id(struct net_device *dev, void __user *useraddr)
 		u64 i = 0;
 
 		do {
-			rtnl_lock();
+			if (has_rtnl_lock)
+				rtnl_lock();
 			netdev_lock_ops(dev);
 			rc = ops->set_phys_id(dev,
 				    (i++ & 1) ? ETHTOOL_ID_OFF : ETHTOOL_ID_ON);
 			netdev_unlock_ops(dev);
-			rtnl_unlock();
+			if (has_rtnl_lock)
+				rtnl_unlock();
 			if (rc)
 				break;
 			schedule_timeout_interruptible(interval);
 		} while (!signal_pending(current) && (!id.data || i < count));
 	}
 
-	rtnl_lock();
+	if (has_rtnl_lock)
+		rtnl_lock();
 	netdev_lock_ops(dev);
 	netdev_put(dev, &dev_tracker);
-	busy = false;
+	dev->ethtool->phys_id_busy = false;
 
 	(void) ops->set_phys_id(dev, ETHTOOL_ID_INACTIVE);
 	return rc;
@@ -3260,7 +3264,8 @@ static int ethtool_set_fecparam(struct net_device *dev, void __user *useraddr)
 static int
 dev_ethtool_locked(struct net *net, struct net_device *dev,
 		   void __user *useraddr,
-		   u32 ethcmd, struct ethtool_devlink_compat *devlink_state)
+		   u32 ethcmd, struct ethtool_devlink_compat *devlink_state,
+		   bool has_rtnl_lock)
 {
 	u32 sub_cmd;
 	int rc;
@@ -3316,6 +3321,8 @@ dev_ethtool_locked(struct net *net, struct net_device *dev,
 			return -EPERM;
 	}
 
+	netdev_assert_locked_ops_compat(dev);
+
 	if (dev->dev.parent)
 		pm_runtime_get_sync(dev->dev.parent);
 
@@ -3403,7 +3410,7 @@ dev_ethtool_locked(struct net *net, struct net_device *dev,
 		rc = ethtool_get_strings(dev, useraddr);
 		break;
 	case ETHTOOL_PHYS_ID:
-		rc = ethtool_phys_id(dev, useraddr);
+		rc = ethtool_phys_id(dev, useraddr, has_rtnl_lock);
 		break;
 	case ETHTOOL_GSTATS:
 		rc = ethtool_get_stats(dev, useraddr);
@@ -3550,8 +3557,20 @@ dev_ethtool_locked(struct net *net, struct net_device *dev,
 	if (dev->ethtool_ops->complete)
 		dev->ethtool_ops->complete(dev);
 
-	if (old_features != dev->features)
-		netdev_features_change(dev);
+	switch (ethcmd) {
+	case ETHTOOL_PHYS_ID:
+		/* Don't check features if operation drops the locks.
+		 * Someone else may have changed features in parallel.
+		 */
+		break;
+	default:
+		if (old_features != dev->features) {
+			if (has_rtnl_lock)
+				netdev_features_change(dev);
+			else
+				netdev_WARN(dev, "ethtool cmd %u changed features without rtnl_lock", ethcmd);
+		}
+	}
 out:
 	if (dev->dev.parent)
 		pm_runtime_put(dev->dev.parent);
@@ -3559,25 +3578,60 @@ dev_ethtool_locked(struct net *net, struct net_device *dev,
 	return rc;
 }
 
+/* Commands that may toggle dev->features in net/ethtool/ioctl.c and so
+ * call into __netdev_update_features(), which still requires rtnl_lock.
+ * Driver-decided SET commands that may chain into rtnl-only helpers are
+ * covered by ethtool_ioctl_needs_rtnl()/ETHTOOL_OP_NEEDS_RTNL_*.
+ */
+static bool ethtool_cmd_changes_features(u32 ethcmd)
+{
+	switch (ethcmd) {
+	case ETHTOOL_SFEATURES:
+	case ETHTOOL_SFLAGS:
+	case ETHTOOL_STXCSUM:
+	case ETHTOOL_SRXCSUM:
+	case ETHTOOL_SSG:
+	case ETHTOOL_STSO:
+	case ETHTOOL_SGSO:
+	case ETHTOOL_SGRO:
+		return true;
+	}
+	return false;
+}
+
 static int
 __dev_ethtool(struct net *net, struct ifreq *ifr, void __user *useraddr,
 	      u32 ethcmd, struct ethtool_devlink_compat *devlink_state)
 {
+	netdevice_tracker dev_tracker;
 	struct net_device *dev;
+	bool need_rtnl;
 	int rc;
 
-	rtnl_lock();
-	dev = __dev_get_by_name(net, ifr->ifr_name);
-	if (!dev) {
+	dev = netdev_get_by_name(net, ifr->ifr_name, &dev_tracker, GFP_KERNEL);
+	if (!dev)
+		return -ENODEV;
+
+	need_rtnl = !netdev_need_ops_lock(dev) ||
+		    ethtool_cmd_changes_features(ethcmd) ||
+		    ethtool_ioctl_needs_rtnl(dev, ethcmd);
+	if (need_rtnl)
+		rtnl_lock();
+	netdev_lock_ops(dev);
+	if (dev->reg_state > NETREG_REGISTERED ||
+	    dev->moving_ns || !net_eq(dev_net(dev), net)) {
 		rc = -ENODEV;
-		goto exit_rtnl_unlock;
+		goto exit_ops_unlock;
 	}
 
-	netdev_lock_ops(dev);
-	rc = dev_ethtool_locked(net, dev, useraddr, ethcmd, devlink_state);
+	rc = dev_ethtool_locked(net, dev, useraddr, ethcmd, devlink_state,
+				need_rtnl);
+
+exit_ops_unlock:
 	netdev_unlock_ops(dev);
-exit_rtnl_unlock:
-	rtnl_unlock();
+	if (need_rtnl)
+		rtnl_unlock();
+	netdev_put(dev, &dev_tracker);
 
 	return rc;
 }
-- 
2.54.0

[PATCH net-next v2 12/12] docs: net: ethtool: document ops-locked drivers and op_needs_rtnl

[Jakub Kicinski]

Catch up various bits of documentation after the locking changes.

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 Documentation/networking/netdev-features.rst |  7 +++++++
 Documentation/networking/netdevices.rst      | 17 ++++++++++-------
 include/linux/ethtool.h                      | 10 ++++++++--
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/Documentation/networking/netdev-features.rst b/Documentation/networking/netdev-features.rst
index 6293d47e5b09..f9e1f3921f53 100644
--- a/Documentation/networking/netdev-features.rst
+++ b/Documentation/networking/netdev-features.rst
@@ -76,6 +76,13 @@ netdev instance lock, that lock must be held as well. This should not be
 done from ndo_*_features callbacks. netdev->features should not be modified
 by driver except by means of ndo_fix_features callback.
 
+For "ops locked" drivers (see Documentation/networking/netdevices.rst),
+ethtool callbacks that may end up invoking netdev_update_features() must
+opt back into rtnl_lock by setting the matching ETHTOOL_OP_NEEDS_RTNL_*
+bit in ``ethtool_ops::op_needs_rtnl``. The ethtool core then keeps
+rtnl_lock held across those SET callbacks so the contract above still
+holds.
+
 ndo_features_check is called for each skb before that skb is passed to
 ndo_start_xmit. Driver may perform any non-trivial checks (e.g. exact
 header geometry / length) and withdraw features like HW_CSUM or TSO,
diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
index 8fc96975b3bd..fde601acd1d2 100644
--- a/Documentation/networking/netdevices.rst
+++ b/Documentation/networking/netdevices.rst
@@ -351,10 +351,6 @@ virtual and the physical device. To prevent deadlocks, the virtual device's
 lock must always be acquired before the physical device's (see
 ``netdev_nl_queue_create_doit``).
 
-In the future, there will be an option for individual
-drivers to opt out of using ``rtnl_lock`` and instead perform their control
-operations directly under the netdev instance lock.
-
 Device drivers are encouraged to rely on the instance lock where possible.
 
 For the (mostly software) drivers that need to interact with the core stack,
@@ -375,9 +371,16 @@ the instance lock.
 struct ethtool_ops
 ------------------
 
-Similarly to ``ndos`` the instance lock is only held for select drivers.
-For "ops locked" drivers all ethtool ops without exceptions should
-be called under the instance lock.
+For non-"ops locked" drivers ethtool_ops are executed under ``rtnl_lock``.
+
+For "ops locked" drivers, ``ethtool_ops``, unlike ``ndos``, run under
+the instance lock **only**. Drivers may request that ``rtnl_lock``
+is held around specific operations (both SET and GET) by setting
+appropriate bits in ``ethtool_ops::op_needs_rtnl`` (if the necessary
+``ETHTOOL_OP_NEEDS_RTNL_*`` bit doesn't exist, just add it).
+Commonly used core helpers which force drivers to selectively opt-in to
+``rtnl_lock`` protection include ``netdev_update_features()``,
+``netif_set_real_num_tx_queues()``, and phylink helpers.
 
 struct netdev_stat_ops
 ----------------------
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index da29017c757f..1b834e2a522e 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1177,8 +1177,14 @@ struct kernel_ethtool_ts_info {
  * @get_mm_stats: Query the 802.3 MAC Merge layer statistics.
  *
  * 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 or netdev instance lock (see @op_needs_rtnl).
+ * to %NULL) and callers must take this into account.
+ *
+ * For traditional drivers callers hold ``rtnl_lock`` across the call.
+ * For "ops locked" drivers (see Documentation/networking/netdevices.rst)
+ * callers instead hold the netdev instance lock (``netdev_lock_ops``);
+ * ``rtnl_lock`` is additionally held only for callbacks for which
+ * the driver opts in via the matching ``ETHTOOL_OP_NEEDS_RTNL_*`` bit
+ * in @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-
-- 
2.54.0