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

[Jakub Kicinski <kuba@kernel.org>]

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 change converts MDIO-connected mt7530 switches to poll MIB counters
via a delayed workqueue and have .get_stats64 return cached values under
a spinlock, avoiding a sleeping-in-atomic-context issue.  MMIO-connected
variants continue to read directly.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index b9423389c2ef..9733ff9fc322 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c

[ ... ]

> +static void mt7530_stats_poll(struct work_struct *work)
> +{
> +	struct mt7530_priv *priv = container_of(work, struct mt7530_priv,
> +						stats_work.work);
> +	struct rtnl_link_stats64 stats = {};
> +	struct dsa_port *dp;
> +	int port;
> +
> +	dsa_switch_for_each_user_port(dp, priv->ds) {
> +		port = dp->index;
> +
> +		mt7530_read_port_stats64(priv, port, &stats);
> +
> +		spin_lock_bh(&priv->stats_lock);
> +		priv->ports[port].stats = stats;
> +		priv->stats_last = jiffies;
> +		spin_unlock_bh(&priv->stats_lock);
> +	}
> +
> +	schedule_delayed_work(&priv->stats_work,
> +			      MT7530_STATS_POLL_INTERVAL);
> +}
> +
> +static void mt7530_get_stats64(struct dsa_switch *ds, int port,
> +			       struct rtnl_link_stats64 *storage)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	bool refresh;
> +
> +	if (priv->bus) {
> +		spin_lock_bh(&priv->stats_lock);
> +		*storage = priv->ports[port].stats;
> +		refresh = time_after(jiffies, priv->stats_last +
> +					      MT7530_STATS_RATE_LIMIT);
> +		spin_unlock_bh(&priv->stats_lock);
> +		if (refresh)
> +			mod_delayed_work(system_wq, &priv->stats_work, 0);
             ^^^^^^^^^

Is the target workqueue here intentional?  The re-arm path at the end of
mt7530_stats_poll() and the initial arming in mt753x_setup() both go
through schedule_delayed_work(), which expands to:

    queue_delayed_work(system_percpu_wq, dwork, delay)

whereas this site passes system_wq.  Those are two distinct workqueue
allocations in kernel/workqueue.c, so each on-demand refresh moves the
delayed_work between the two queues.

include/linux/workqueue.h also marks system_wq as deprecated:

    /* Please use system_percpu_wq, this will be removed */
    extern struct workqueue_struct *system_wq;

Should this call use system_percpu_wq (or mod_delayed_work on the same
queue that schedule_delayed_work() targets) to keep the work on a single
workqueue and avoid the deprecated symbol?

> +	} else {
> +		mt7530_read_port_stats64(priv, port, storage);
> +	}
> +}
> +

[ ... ]

> @@ -3137,9 +3181,24 @@ 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);
> +		schedule_delayed_work(&priv->stats_work,
> +				      MT7530_STATS_POLL_INTERVAL);
> +	}
> +
>  	return ret;
>  }

How does .get_stats64 behave during the first MT7530_STATS_POLL_INTERVAL
(1 * HZ) after setup?

priv->ports[port].stats and priv->stats_last come from the devm_kzalloc
allocation in mt7530_probe_common(), so both are zero.  The first poll
is scheduled with a delay of MT7530_STATS_POLL_INTERVAL, so any reader
during that first second sees:

    spin_lock_bh(&priv->stats_lock);
    *storage = priv->ports[port].stats;   /* all zero */
    refresh = time_after(jiffies, priv->stats_last +
                                  MT7530_STATS_RATE_LIMIT);
    spin_unlock_bh(&priv->stats_lock);

Can the on-demand refresh path help here?  time_after() is defined as:

    #define time_after(a,b)         \
        (typecheck(unsigned long, a) && \
         typecheck(unsigned long, b) && \
         ((long)((b) - (a)) < 0))

On 32-bit, jiffies starts at INITIAL_JIFFIES = (unsigned long)(unsigned
int)(-300*HZ), which is close to ULONG_MAX.  With stats_last == 0, the
signed difference check can evaluate to false for a long time, so the
mod_delayed_work() refresh is suppressed until the first scheduled poll
updates stats_last.

Would seeding priv->stats_last = jiffies at init, and either populating
the cache synchronously in mt753x_setup() or scheduling the first poll
with delay 0, avoid returning zeroed counters and the suppressed
on-demand refresh during the initial window?
-- 
pw-bot: cr