Re: [PATCH net v4] net: dsa: mt7530: fix .get_stats64 sleeping in atomic context

Daniel Golle
2026-04-29T23:55:15Z

On Wed, Apr 29, 2026 at 04:46:13PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: dsa: mt7530: fix .get_stats64 sleeping in atomic context
>
> This commit introduces a delayed workqueue to asynchronously poll MIB counters
> for MDIO-connected switches. This caches the results so they can be returned
> safely without sleeping in the atomic .get_stats64 callback.
>
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index b9423389c2ef..66bff861a921 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
>
> [ ... ]
>
> > @@ -3137,9 +3187,25 @@ mt753x_setup(struct dsa_switch *ds)
> > if (ret && priv->irq_domain)
> > mt7530_free_mdio_irq(priv);
> >
> > + if (!ret && priv->bus) {
> > + spin_lock_init(&priv->stats_lock);
> > + INIT_DELAYED_WORK(&priv->stats_work, mt7530_stats_poll);
> > + mt7530_stats_refresh(priv);
> > + schedule_delayed_work(&priv->stats_work,
> > + MT7530_STATS_POLL_INTERVAL);
> > + }
> > +
> > return ret;
> > }
>
> [ ... ]
>
> > @@ -3409,6 +3476,9 @@ mt7530_remove_common(struct mt7530_priv *priv)
> >
> > dsa_unregister_switch(priv->ds);
> >
> > + if (priv->bus)
> > + cancel_delayed_work_sync(&priv->stats_work);
>
> Could this call cancel_delayed_work_sync() on an uninitialized work struct
> if the driver is unbound before the DSA switch tree completes?
>
> The priv->bus pointer is initialized during probe. However,
> INIT_DELAYED_WORK() is only called in mt753x_setup(), which might never
> execute if the DSA tree remains incomplete.
>
> If the driver is unbound in this state, mt7530_remove_common() will evaluate
> priv->bus as true and pass a zero-initialized work struct to
> cancel_delayed_work_sync(). This usually triggers lockdep ("trying to
> register non-static key") and debugobjects ("assert_init not available")
> warnings.
>
> Would it be better to move the INIT_DELAYED_WORK() call into the probe
> path or track whether the setup function was actually completed?

Not sure if this is real. The fix could be simply

if (priv->bus && priv->ds->setup)
cancel_delayed_work_sync(&priv->stats_work);

Anyone?