Re: [PATCH net-next v5 03/12] net: hold netdev instance lock during queue operations

[Stanislav Fomichev <stfomichev@gmail.com>]

On 02/20, Sabrina Dubroca wrote:
> 2025-02-19, 12:27:10 -0800, Stanislav Fomichev wrote:
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index 533e659b15b3..cf9bd08d04b2 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
> > @@ -1886,7 +1886,7 @@ static void gve_turndown(struct gve_priv *priv)
> >  			netif_queue_set_napi(priv->dev, idx,
> >  					     NETDEV_QUEUE_TYPE_TX, NULL);
> >  
> > -		napi_disable(&block->napi);
> > +		napi_disable_locked(&block->napi);
> 
> I don't think all the codepaths that can lead to gve_turndown have the
> required netdev_lock():
> 
> gve_resume -> gve_reset_recovery -> gve_turndown
Good catch, looks like suspend is missing the netdev lock as well, will
add.

> gve_user_reset -> gve_reset -> gve_reset_recovery
I believe this should be covered by patch "net: ethtool: try to protect
all callback with netdev instance lock", no?

__dev_ethtool
  netdev_lock_ops
  ethtool_reset
    gve_user_reset

Or is there some other reset path I'm missing?

> (and nit:) There's also a few places in the series (bnxt, ethtool
> calling __netdev_update_features) where the lockdep
> annotation/_locked() variant gets introduced before the patch adding
> the corresponding lock.

This is mostly about ethtool patch and queue ops patch? The latter
converts most of the napi/netif calls to _locked variant leaving
a small window where some of the paths might be not properly locked.
Not sure what to do about it, but probably nothing since everything
is still rtnl_lock-protected and the issue is mostly about (temporary)
wrong lockdep annotations? Any other suggestions?

Thanks for the review!