From: Jakub Kicinski Date: Fri, 5 Jun 2026 00:29:04 +0000 (-0700) Subject: net: ethtool: optionally skip rtnl_lock on Netlink path for GET ops X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=45079e00133ee78fd216ccc4285534044ea69173;p=thirdparty%2Flinux.git net: ethtool: optionally skip rtnl_lock on Netlink path for GET ops 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. Reviewed-by: Eric Dumazet Acked-by: Stanislav Fomichev Reviewed-by: Jacob Keller Link: https://patch.msgid.link/20260605002912.3456868-5-kuba@kernel.org Signed-off-by: Jakub Kicinski --- 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 +#include #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/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/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 #include +#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);