Re: [net-next,v4,1/2] net: dsa: yt921x: Fix MIB overflow wraparound routine

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: yt921x: Use u64_stats_t for MIB stats

This patch converts MIB statistics from plain u64 to u64_stats_t and adds
u64_stats_sync protection for atomic statistics access on 32-bit systems.

> diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> index 7b8c1549a0fb..3d87944830f6 100644

[ ... ]

> @@ -762,22 +779,27 @@ yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data)
>  	struct yt921x_priv *priv = to_yt921x_priv(ds);
>  	struct yt921x_port *pp = &priv->ports[port];
>  	struct yt921x_mib *mib = &pp->mib;
> +	unsigned int start;
>  	size_t j;
>
>  	mutex_lock(&priv->reg_lock);
>  	yt921x_read_mib(priv, port);
>  	mutex_unlock(&priv->reg_lock);
>
> -	j = 0;
> -	for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
> -		const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
> +	do {
> +		start = u64_stats_fetch_begin(&pp->syncp);

This uses pp->syncp via u64_stats_fetch_begin(), but the syncp field does
not appear to be initialized with u64_stats_init() anywhere.

Looking at yt921x_mdio_probe(), the port initialization loop initializes
pp->index and pp->mib_read but not pp->syncp:

    for (size_t i = 0; i < ARRAY_SIZE(priv->ports); i++) {
        struct yt921x_port *pp = &priv->ports[i];
        pp->index = i;
        INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib);
    }

On 32-bit systems, u64_stats_init() calls seqcount_init() which sets up
the lockdep class. Without this initialization, CONFIG_DEBUG_LOCK_ALLOC
builds may produce lockdep warnings.

For comparison, the xrs700x driver at drivers/net/dsa/xrs700x/xrs700x.c
initializes its syncp field with u64_stats_init(&p->syncp).

Should u64_stats_init(&pp->syncp) be added to the port initialization loop
in yt921x_mdio_probe()?

[ ... ]

> diff --git a/drivers/net/dsa/yt921x.h b/drivers/net/dsa/yt921x.h
> index 61bb0ab3b09a..17fee83952c2 100644
> --- a/drivers/net/dsa/yt921x.h
> +++ b/drivers/net/dsa/yt921x.h

[ ... ]

> @@ -533,9 +535,13 @@ struct yt921x_port {
>  	bool isolated;
>
>  	struct delayed_work mib_read;
> +	struct u64_stats_sync syncp;
>  	struct yt921x_mib mib;
> -	u64 rx_frames;
> -	u64 tx_frames;
> +	u64_stats_t rx_frames;
> +	u64_stats_t tx_frames;
> +
> +	/* only used by read routine to avoid huge allocations on the stack */
> +	struct yt921x_mib mib_new;
>  };

-- 
pw-bot: cr