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

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.

[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.

        ^^^^

[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",

[ ... ]

[ ... ]

[ ... ]

+	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.

[ ... ]

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