Re: [PATCH net-next 02/14] net: ethtool: make sure __ethtool_get_link_ksettings() is ops-locked

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 02 Jun 2026 13:35:33 +0200 Jakub Sitnicki wrote:
> On Tue, Jun 02, 2026 at 01:20 PM +02, Nicolai Buchwitz wrote:
> >> Not sure why we're using the "compat" assertion here, which falls back
> >> to check if RTNL is held, instead of the newly introduced
> >> netdev_assert_locked_if_ops().
> >> The contract here is that all callers are expected to hold the
> >> netdev lock (if needed), IIUC.
> >> [...]  
> >
> > AFAIU that's on purpose: the original helper had ASSERT_RTNL(), and
> > legacy callers still enter with rtnl_lock via the wrapper. _if_ops
> > would silently pass for them if rtnl ever went missing.  
> 
> But the legacy callers continue using __ethtool_get_link_ksettings(),
> which has an explicit ASSERT_RTNL(), so I still don't get it:
> 
> | +/* Convenience helper for callers that hold only rtnl_lock(). */
> | +int __ethtool_get_link_ksettings(struct net_device *dev,
> | +				 struct ethtool_link_ksettings *link_ksettings)
> | +{
> | +	int ret;
> | +
> | +	ASSERT_RTNL();
> | +
> | +	netdev_lock_ops(dev);
> | +	ret = ethtool_get_link_ksettings_locked(dev, link_ksettings);
> | +	netdev_unlock_ops(dev);
> | +	return ret;
> | +}
> |  EXPORT_SYMBOL(__ethtool_get_link_ksettings);
> 
> Anyway, no sense speculating. Let's wait for Jakub.

You're right, I probably didn't use the .._if_ops.. because the
spelling was kinda unusual. Looking at my local tree now I seem
to have switched to the _compat helper like you are suggesting
when renaming things.

I also renamed the new helper to netif_get_link_ksettings()
since that's the naming scheme we followed elsewhere (the netif_ 
prefix for helpers called with ops lock already held).

I've been digging thru the drivers calling
__ethtool_get_link_ksettings() since yesterday. Turns out I was quite
optimistic on none of them holding the ops lock :( Hopefully I'm
getting close to posting the first chunk of v2 (patches 1+2 from here
plus all the driver adjustments needed).