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

[Jacob Keller <jacob.e.keller@intel.com>]

On 6/4/2026 5:29 PM, Jakub Kicinski wrote:
> 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.
> 

Agreed. It might be nicer to opt-out first, and then opt-in the drivers
that don't do the update_features.. That would make it easier to prevent
buggy drivers slipping in as easily... But that would result in the
following messy situation:

ethtool ops for ops-locked drivers don't hold RTNL lock, *except* some
which do because they might call update_features.. *except* those which
opt-out of RTNL because they know their driver implementation doesn't
call update_features...

Yea. I think that is too messy. This approach requires slightly more
vigilance on the part of reviewers to ensure that ops-locked drivers
don't make a mistake here. However, the checks to see if features
changed while the RTNL wasn't held should be enough to help catch most
cases. Ok.

> 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.
> 

Makes sense. Taking some steps towards the end goal is better than
trying to wait until every piece here is done.

Whole series looks good to me. I had one minor nit about one patch
adding a bit flag at BIT(1) and then later changing that to BIT(5) but
its ultimately harmless and should not force a respin of this important
work.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

> Tested on mlx5, bnxt and fbnic.
> 

I think currently only iAVF is ops-locked for Intel. Tony, would it make
sense to see if our virtualization validation folks have cycles to give
this a quick pass on iAVF? (Regardless of whether it merges before or
after, it would be beneficial to make sure we don't have any lingering
issues there).

I know there was work also being done to migrate ice to ops-locked but I
don't think that has managed to land yet.

> 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(-)
>