Re: [PATCH net-next v3 4/6] net: dsa: motorcomm: Use u64_stats_t for MIB stats

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1 (1)

- [Low] In "net: dsa: motorcomm: Use u64_stats_t for MIB stats" the MIB
  counters are converted from u64 to u64_stats_t while no `struct…

Pre-existing issues:
- [Medium] For the 64-bit MIB counters (desc->size == 2: RX_GOOD_BYTES,
  RX_BAD_BYTES, TX_GOOD_BYTES) yt921x_mib_read() performs two…
- [Low] yt921x_dsa_get_stats64() (drivers/net/dsa/motorcomm/mib.c) reads
  the shared MIB counters with no lock and, at the reviewed commit, no…

--

net: dsa: motorcomm: Use u64_stats_t for MIB stats

This converts every MIB counter in struct yt921x_mib_stats and the derived
rx_frames/tx_frames fields from plain u64 to u64_stats_t, and updates all
readers in mib.c to use u64_stats_read().  The changelog notes that
u64_stats_sync is added in the next commit.

> diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c
> index b1296f224cf69..f5d39d22215d4 100644
> --- a/drivers/net/dsa/motorcomm/mib.c
> +++ b/drivers/net/dsa/motorcomm/mib.c
> @@ -92,22 +92,16 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
>  	struct device *dev = to_device(priv);
>  	struct yt921x_mib *pm = pp->mib;
>  	struct yt921x_mib_stats *mib;
> +	u64 rx_frames;
> +	u64 tx_frames;
>  	int res = 0;
>  
>  	mib = &pm->stats;
>  
> -	/* Reading of yt921x_port::mib is not protected by a lock and it's vain
> -	 * to keep its consistency, since we have to read registers one by one
> -	 * and there is no way to make a snapshot of MIB stats.
> -	 *
> -	 * Writing (by this function only) is and should be protected by
> -	 * reg_lock.
> -	 */
> -
>  	for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) {
>  		const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i];
>  		u32 reg = YT921X_MIBn_DATA0(port) + desc->offset;
> -		u64 *valp = &((u64 *)mib)[i];
> +		u64_stats_t *valp = &((u64_stats_t *)mib)[i];
>  		u32 val0;
>  		u64 val;
>  
> @@ -116,7 +110,7 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
>  			break;
>  
>  		if (desc->size <= 1) {
> -			u64 old_val = *valp;
> +			u64 old_val = u64_stats_read(valp);
>  
>  			val = (old_val & ~(u64)U32_MAX) | val0;
>  			if (val < old_val)

[Severity: Low]
The changelog says the change addresses the case where "64-bit variables
might not be atomic on 32-bit architectures, and could lead to load/store
tearing".  Does this commit on its own change anything on 32-bit?

With BITS_PER_LONG == 32, u64_stats_read() and u64_stats_set() are plain
accesses:

	static inline u64 u64_stats_read(const u64_stats_t *p)
	{
		return p->v;
	}

	static inline void u64_stats_set(u64_stats_t *p, u64 val)
	{
		p->v = val;
	}

struct yt921x_mib gains no struct u64_stats_sync here, yt921x_mib_read()
has no u64_stats_update_begin()/u64_stats_update_end() region, and
yt921x_dsa_get_stats64() has no u64_stats_fetch_begin()/
u64_stats_fetch_retry() loop, so the tearing named in the changelog is
still there after this patch.

Also, the writer store goes from WRITE_ONCE(*valp, val) to
u64_stats_set(valp, val), which on 32-bit is a plain store, and the comment
documenting the "written only here under reg_lock, read locklessly" rule is
deleted with nothing replacing it.  Would it be preferable to fold this
patch together with the follow-up, or at least keep the WRITE_ONCE()
equivalent and the locking comment until the follow-up lands?

For the record, the next patch in this series, "net: dsa: motorcomm: Fix MIB
synchronization", does add struct u64_stats_sync syncp to struct yt921x_mib,
wraps the write-back in u64_stats_update_begin()/u64_stats_update_end(),
turns yt921x_dsa_get_stats64() into a fetch_begin()/fetch_retry() loop,
moves mutex_unlock(&priv->reg_lock) to after the counter reads in
yt921x_dsa_get_eth_mac_stats(), yt921x_dsa_get_eth_ctrl_stats(),
yt921x_dsa_get_rmon_stats(), yt921x_dsa_get_pause_stats() and
yt921x_dsa_get_ethtool_stats(), and restores the access documentation as
/* protected by syncp OR priv->reg_lock */ in mib.h.

> @@ -130,17 +124,25 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
>  			val = ((u64)val1 << 32) | val0;
        ^^^^

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but while
yt921x_mib_read() is being touched: for the desc->size == 2 counters
(RX_GOOD_BYTES, RX_BAD_BYTES, TX_GOOD_BYTES) the low word at
YT921X_MIBn_DATA0(port) + desc->offset and the high word at + 4 are fetched
in two separate SMI transactions with nothing latching the pair.  If the low
word wraps between the two reads, does the composed value end up about 4 GiB
too high, and does that then get cached as the base for the 32-bit
wraparound arithmetic above and exported as a non-monotonic rx_bytes/
tx_bytes?

The last patch of this series, "net: dsa: motorcomm: Use safe 64-bit counter
reader", replaces this with yt921x_counter_read(), which re-reads hi and lo
and accepts the pair only when lo >= old_lo, so the window is closed there.

>  		}
>  
> -		WRITE_ONCE(*valp, val);
> +		u64_stats_set(valp, val);
>  	}
>  
> -	pm->rx_frames = mib->rx_64byte + mib->rx_65_127byte +
> -			mib->rx_128_255byte + mib->rx_256_511byte +
> -			mib->rx_512_1023byte + mib->rx_1024_1518byte +
> -			mib->rx_jumbo;
> -	pm->tx_frames = mib->tx_64byte + mib->tx_65_127byte +
> -			mib->tx_128_255byte + mib->tx_256_511byte +
> -			mib->tx_512_1023byte + mib->tx_1024_1518byte +
> -			mib->tx_jumbo;
> +	rx_frames = u64_stats_read(&mib->rx_64byte) +
> +		    u64_stats_read(&mib->rx_65_127byte) +
> +		    u64_stats_read(&mib->rx_128_255byte) +
> +		    u64_stats_read(&mib->rx_256_511byte) +
> +		    u64_stats_read(&mib->rx_512_1023byte) +
> +		    u64_stats_read(&mib->rx_1024_1518byte) +
> +		    u64_stats_read(&mib->rx_jumbo);
> +	tx_frames = u64_stats_read(&mib->tx_64byte) +
> +		    u64_stats_read(&mib->tx_65_127byte) +
> +		    u64_stats_read(&mib->tx_128_255byte) +
> +		    u64_stats_read(&mib->tx_256_511byte) +
> +		    u64_stats_read(&mib->tx_512_1023byte) +
> +		    u64_stats_read(&mib->tx_1024_1518byte) +
> +		    u64_stats_read(&mib->tx_jumbo);
> +	u64_stats_set(&pm->rx_frames, rx_frames);
> +	u64_stats_set(&pm->tx_frames, tx_frames);
>  
>  	if (res)
>  		dev_err(dev, "Failed to %s port %d: %i\n", "read stats for",

[ ... ]

> @@ -246,28 +248,28 @@ yt921x_dsa_get_eth_mac_stats(struct dsa_switch *ds, int port,
>  	yt921x_mib_read(priv, port);
>  	mutex_unlock(&priv->reg_lock);
>  
> -	mac_stats->FramesTransmittedOK = pm->tx_frames;
> +	mac_stats->FramesTransmittedOK = u64_stats_read(&pm->tx_frames);

[ ... ]

> @@ -358,32 +360,34 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
>  		return;
>  	mib = &pm->stats;
>  

[ ... ]

> +	stats->rx_packets = u64_stats_read(&pm->rx_frames);
> +	stats->tx_packets = u64_stats_read(&pm->tx_frames);
> +	stats->rx_bytes = u64_stats_read(&mib->rx_good_bytes) -
> +			  ETH_FCS_LEN * stats->rx_packets;
> +	stats->tx_bytes = u64_stats_read(&mib->tx_good_bytes) -
> +			  ETH_FCS_LEN * stats->tx_packets;

[Severity: Low]
This is a pre-existing issue rather than something this patch introduces,
but the derivation here mixes two independently loaded counters while
yt921x_dsa_get_stats64() holds no lock:

	CPU0 yt921x_dsa_get_stats64()
	    stats->rx_packets = u64_stats_read(&pm->rx_frames);
	    /* CPU1: yt921x_mib_poll() -> yt921x_mib_read() rewrites
	     * all counters under priv->reg_lock */
	    stats->rx_bytes = u64_stats_read(&mib->rx_good_bytes) -
			      ETH_FCS_LEN * stats->rx_packets;

If rx_packets is observed fresh and rx_good_bytes stale, can this unsigned
subtraction wrap and report an rx_bytes value close to 2^64 through
rtnl_link_stats64?

The follow-up patch "net: dsa: motorcomm: Fix MIB synchronization" wraps
this whole block in a u64_stats_fetch_begin()/u64_stats_fetch_retry() loop
and publishes all counters plus rx_frames/tx_frames in one writer section,
which gives a consistent snapshot on 32-bit.

> diff --git a/drivers/net/dsa/motorcomm/mib.h b/drivers/net/dsa/motorcomm/mib.h
> index cfad0665baaea..d84ae2f449ac8 100644
> --- a/drivers/net/dsa/motorcomm/mib.h
> +++ b/drivers/net/dsa/motorcomm/mib.h

[ ... ]

> @@ -128,8 +130,8 @@ struct yt921x_mib {
>  
>  	struct delayed_work work;
>  	struct yt921x_mib_stats stats;
> -	u64 rx_frames;
> -	u64 tx_frames;
> +	u64_stats_t rx_frames;
> +	u64_stats_t tx_frames;
>  };
>  
>  void yt921x_mib_poll(struct work_struct *work);

Would it make sense to add struct u64_stats_sync syncp to struct yt921x_mib
in this same patch, so the u64_stats_t fields are never present without
their companion seqcount?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906131414.2683362-1-mmyangfl%40gmail.com