[PATCH net-next 0/4] net: dsa: motorcomm: MIB fixup

[PATCH net-next 0/4] net: dsa: motorcomm: MIB fixup

[David Yang]

Various MIB defects reported by LLM.  While they do actually "fix"
something, the odds are extremely low and all depend on very specified
transient conditions. Since MIBs are not control path related and nearly
nobody would notice them, target them to net-next.

David Yang (4):
  net: dsa: motorcomm: Fix stats64 error counter mapping
  net: dsa: motorcomm: Fix RMON fragments stat
  net: dsa: motorcomm: Use safe 64-bit counter reader
  net: dsa: motorcomm: Prevent rx_bytes/tx_bytes underflow

 drivers/net/dsa/motorcomm/mib.c | 62 ++++++++++++++++++---------------
 drivers/net/dsa/motorcomm/mib.h |  2 ++
 drivers/net/dsa/motorcomm/smi.c | 35 +++++++++++++++++++
 drivers/net/dsa/motorcomm/smi.h |  2 ++
 4 files changed, 73 insertions(+), 28 deletions(-)

-- 
2.53.0

[PATCH net-next 1/4] net: dsa: motorcomm: Fix stats64 error counter mapping

[David Yang]

rtnl_link_stats64 wants aFrameTooLongErrors in rx_length_errors, and the
eth-mac stats already report the oversize counter there as
FrameTooLongErrors; move it from rx_over_errors, which is a RX FIFO
overflow counter.

Drop the tx_undersize to tx_fifo_errors mapping as well; a runt frame is
not a FIFO underrun. Report it as a TxUndersize ethtool stat instead.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/mib.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c
index 6b170fc6e187..5c027edb71e3 100644
--- a/drivers/net/dsa/motorcomm/mib.c
+++ b/drivers/net/dsa/motorcomm/mib.c
@@ -50,7 +50,7 @@ static const struct yt921x_mib_desc yt921x_mib_descs[] = {
 	MIB_DESC(1, YT921X_MIB_DATA_TX_PAUSE, NULL),
 	MIB_DESC(1, YT921X_MIB_DATA_TX_MULTICAST, NULL),
 
-	MIB_DESC(1, YT921X_MIB_DATA_TX_UNDERSIZE_ERR, NULL),
+	MIB_DESC(1, YT921X_MIB_DATA_TX_UNDERSIZE_ERR, "TxUndersize"),
 	MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_64, NULL),
 	MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_65_TO_127, NULL),
 	MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_128_TO_255, NULL),
@@ -376,8 +376,9 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
 		start = u64_stats_fetch_begin(&pm->syncp);
 
 		stats->rx_length_errors = u64_stats_read(&mib->rx_undersize_errors) +
-					  u64_stats_read(&mib->rx_fragment_errors);
-		stats->rx_over_errors = u64_stats_read(&mib->rx_oversize_errors);
+					  u64_stats_read(&mib->rx_fragment_errors) +
+					  u64_stats_read(&mib->rx_oversize_errors);
+		/* stats->rx_over_errors */
 		stats->rx_crc_errors = u64_stats_read(&mib->rx_crc_errors);
 		stats->rx_frame_errors = u64_stats_read(&mib->rx_alignment_errors);
 		/* stats->rx_fifo_errors */
@@ -385,7 +386,7 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
 
 		stats->tx_aborted_errors = u64_stats_read(&mib->tx_aborted_errors);
 		/* stats->tx_carrier_errors */
-		stats->tx_fifo_errors = u64_stats_read(&mib->tx_undersize_errors);
+		/* stats->tx_fifo_errors */
 		/* stats->tx_heartbeat_errors */
 		stats->tx_window_errors = u64_stats_read(&mib->tx_late_collisions);
 
@@ -401,10 +402,9 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
 		stats->collisions = u64_stats_read(&mib->tx_collisions);
 	} while (u64_stats_fetch_retry(&pm->syncp, start));
 
-	stats->rx_errors = stats->rx_length_errors + stats->rx_over_errors +
-			   stats->rx_crc_errors + stats->rx_frame_errors;
-	stats->tx_errors = stats->tx_aborted_errors + stats->tx_fifo_errors +
-			   stats->tx_window_errors;
+	stats->rx_errors = stats->rx_length_errors + stats->rx_crc_errors +
+			   stats->rx_frame_errors;
+	stats->tx_errors = stats->tx_aborted_errors + stats->tx_window_errors;
 }
 
 void
-- 
2.53.0

[PATCH net-next 2/4] net: dsa: motorcomm: Fix RMON fragments stat

[David Yang]

etherStatsFragments counts frames with a bad FCS due to fragmentation;
report the dedicated RX fragment error counter instead of the alignment
error one, which is already exported as AlignmentErrors and
rx_frame_errors.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/mib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c
index 5c027edb71e3..90b0ef670aa3 100644
--- a/drivers/net/dsa/motorcomm/mib.c
+++ b/drivers/net/dsa/motorcomm/mib.c
@@ -336,7 +336,7 @@ yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port,
 
 	rmon_stats->undersize_pkts = u64_stats_read(&mib->rx_undersize_errors);
 	rmon_stats->oversize_pkts = u64_stats_read(&mib->rx_oversize_errors);
-	rmon_stats->fragments = u64_stats_read(&mib->rx_alignment_errors);
+	rmon_stats->fragments = u64_stats_read(&mib->rx_fragment_errors);
 	/* rmon_stats->jabbers */
 
 	rmon_stats->hist[0] = u64_stats_read(&mib->rx_64byte);
-- 
2.53.0

[PATCH net-next 3/4] net: dsa: motorcomm: Use safe 64-bit counter reader

[David Yang]

64-bit counter registers are read as two separate 32-bit transactions.
If the low word wraps from about 0xffffffff to a small value while the
high word increments between the two transactions, the recombined value
becomes ((new_high << 32) | old_low), producing a spurious ~4 GiB jump.

Use a safe counter reader for this.

Signed-off-by: David Yang <mmyangfl@gmail.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/motorcomm/mib.c | 21 +++++++++-----------
 drivers/net/dsa/motorcomm/smi.c | 35 +++++++++++++++++++++++++++++++++
 drivers/net/dsa/motorcomm/smi.h |  2 ++
 3 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c
index 90b0ef670aa3..b45bc8211741 100644
--- a/drivers/net/dsa/motorcomm/mib.c
+++ b/drivers/net/dsa/motorcomm/mib.c
@@ -102,29 +102,26 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
 	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;
-		u32 val0;
-		u64 val;
-
-		res = yt921x_reg_read(priv, reg, &val0);
-		if (res)
-			break;
 
 		if (desc->size <= 1) {
 			u64 old_val = buf[i];
+			u32 val0;
+			u64 val;
+
+			res = yt921x_reg_read(priv, reg, &val0);
+			if (res)
+				break;
 
 			val = (old_val & ~(u64)U32_MAX) | val0;
 			if (val < old_val)
 				val += 1ull << 32;
-		} else {
-			u32 val1;
 
-			res = yt921x_reg_read(priv, reg + 4, &val1);
+			buf[i] = val;
+		} else {
+			res = yt921x_counter_read(priv, reg, &buf[i]);
 			if (res)
 				break;
-			val = ((u64)val1 << 32) | val0;
 		}
-
-		buf[i] = val;
 	}
 
 	flags = u64_stats_update_begin_irqsave(&pm->syncp);
diff --git a/drivers/net/dsa/motorcomm/smi.c b/drivers/net/dsa/motorcomm/smi.c
index bf3adfd64165..16f62409422d 100644
--- a/drivers/net/dsa/motorcomm/smi.c
+++ b/drivers/net/dsa/motorcomm/smi.c
@@ -63,6 +63,41 @@ int yt921x_reg_update_bits(struct yt921x_priv *priv, u32 reg, u32 mask, u32 val)
 	return yt921x_reg_write(priv, reg, u);
 }
 
+/* Reliably read a 64-bit counter, assuming the counter isn't that "fast" */
+int yt921x_counter_read(struct yt921x_priv *priv, u32 reg, u64 *valp)
+{
+	u32 old_lo;
+	int res;
+	u32 hi;
+	u32 lo;
+
+	res = yt921x_reg_read(priv, reg, &old_lo);
+	if (res)
+		return res;
+
+	res = yt921x_reg_read(priv, reg + 4, &hi);
+	if (res)
+		return res;
+
+	res = yt921x_reg_read(priv, reg, &lo);
+	if (res)
+		return res;
+
+	if (lo < old_lo) {
+		u32 new_hi;
+
+		res = yt921x_reg_read(priv, reg + 4, &new_hi);
+		if (res)
+			return res;
+		if (new_hi != hi + 1)
+			return -EIO;
+		hi = new_hi;
+	}
+
+	*valp = ((u64)hi << 32) | lo;
+	return 0;
+}
+
 static int
 yt921x_regs_read(struct yt921x_priv *priv, u32 reg, u32 *vals,
 		 unsigned int num_regs)
diff --git a/drivers/net/dsa/motorcomm/smi.h b/drivers/net/dsa/motorcomm/smi.h
index 212e20f71d80..d34240548af7 100644
--- a/drivers/net/dsa/motorcomm/smi.h
+++ b/drivers/net/dsa/motorcomm/smi.h
@@ -35,6 +35,8 @@ yt921x_reg_toggle_bits(struct yt921x_priv *priv, u32 reg, u32 mask, bool set)
 	return yt921x_reg_update_bits(priv, reg, mask, !set ? 0 : mask);
 }
 
+int yt921x_counter_read(struct yt921x_priv *priv, u32 reg, u64 *valp);
+
 /* Some multi-word registers, like VLANn_CTRL, should be treated as a single
  * long register. More specifically, writes to parts of its words won't become
  * visible, until the last word is written.
-- 
2.53.0

[PATCH net-next 4/4] net: dsa: motorcomm: Prevent rx_bytes/tx_bytes underflow

[David Yang]

There is no direct rx_bytes/tx_bytes counter excluding the FCS; both
have to be derived as good_bytes - ETH_FCS_LEN * frames. Three hazards
can pair a frame count with a mismatched byte count and make the
subtraction underflow, reporting rx_bytes or tx_bytes as about 2^64:

- a register read fails midway and the half-refreshed buffer is
  published as is, pairing fresh frame counters with stale byte
  counters;
- get_stats64() on 64-bit kernels reads the published counters without
  any sequencing across fields (u64_stats_sync is a no-op there), so a
  writer update can slip between the frame and byte reads;
- the MIB bank cannot be read as a whole; counters keep counting while
  the read loop walks the table.

Handle this in the writer: do not publish a half-read buffer, keep the
last complete sample on read failure, do the arithmetic on the complete
sample under priv->reg_lock, and publish the derived byte counters only
when check_sub_overflow() succeeds, so that get_stats64() copies plain
published values.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/motorcomm/mib.c | 25 +++++++++++++++++--------
 drivers/net/dsa/motorcomm/mib.h |  2 ++
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c
index b45bc8211741..cb3912493cfb 100644
--- a/drivers/net/dsa/motorcomm/mib.c
+++ b/drivers/net/dsa/motorcomm/mib.c
@@ -95,6 +95,8 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
 	unsigned long flags;
 	u64 rx_frames;
 	u64 tx_frames;
+	u64 rx_bytes;
+	u64 tx_bytes;
 	int res = 0;
 
 	mib = &pm->stats;
@@ -124,6 +126,12 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
 		}
 	}
 
+	if (res) {
+		dev_err(dev, "Failed to %s port %d: %i\n", "read stats for",
+			port, res);
+		return res;
+	}
+
 	flags = u64_stats_update_begin_irqsave(&pm->syncp);
 	for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++)
 		u64_stats_set(&((u64_stats_t *)mib)[i], buf[i]);
@@ -143,12 +151,15 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
 		    u64_stats_read(&mib->tx_jumbo);
 	u64_stats_set(&pm->rx_frames, rx_frames);
 	u64_stats_set(&pm->tx_frames, tx_frames);
+	if (!check_sub_overflow(u64_stats_read(&mib->rx_good_bytes),
+				ETH_FCS_LEN * rx_frames, &rx_bytes))
+		u64_stats_set(&pm->rx_bytes, rx_bytes);
+	if (!check_sub_overflow(u64_stats_read(&mib->tx_good_bytes),
+				ETH_FCS_LEN * tx_frames, &tx_bytes))
+		u64_stats_set(&pm->tx_bytes, tx_bytes);
 	u64_stats_update_end_irqrestore(&pm->syncp, flags);
 
-	if (res)
-		dev_err(dev, "Failed to %s port %d: %i\n", "read stats for",
-			port, res);
-	return res;
+	return 0;
 }
 
 void yt921x_mib_poll(struct work_struct *work)
@@ -389,10 +400,8 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
 
 		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;
+		stats->rx_bytes = u64_stats_read(&pm->rx_bytes);
+		stats->tx_bytes = u64_stats_read(&pm->tx_bytes);
 		stats->rx_dropped = u64_stats_read(&mib->rx_dropped);
 		/* stats->tx_dropped */
 		stats->multicast = u64_stats_read(&mib->rx_multicast);
diff --git a/drivers/net/dsa/motorcomm/mib.h b/drivers/net/dsa/motorcomm/mib.h
index 9c24a52d46cc..2e2b832cb245 100644
--- a/drivers/net/dsa/motorcomm/mib.h
+++ b/drivers/net/dsa/motorcomm/mib.h
@@ -136,6 +136,8 @@ struct yt921x_mib {
 	struct yt921x_mib_stats stats;
 	u64_stats_t rx_frames;
 	u64_stats_t tx_frames;
+	u64_stats_t rx_bytes;
+	u64_stats_t tx_bytes;
 	/* protected by priv->reg_lock */
 	u64 data[YT921X_MIB_STAT_NUM];
 };
-- 
2.53.0

Re: [PATCH net-next 1/4] net: dsa: motorcomm: Fix stats64 error counter mapping

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In yt921x_dsa_get_stats64(), this patch removes the
  `tx_fifo_errors = tx_undersize_errors` mapping and changes the…
- [Low] The commit "net: dsa: motorcomm: Fix stats64 error counter
  mapping" corrects counters that users see in rtnl_link_stats64 (`ip -s…

Pre-existing issues:
- [Low] yt921x_dsa_get_rmon_stats() sets RMON etherStatsFragments from
  the alignment-error counter (`rmon_stats->fragments =…

--

net: dsa: motorcomm: Fix stats64 error counter mapping

This commit changes yt921x_dsa_get_stats64() to report the RX oversize
counter in rx_length_errors instead of rx_over_errors. It also drops the
mapping of TX undersize to tx_fifo_errors and exposes that counter as a
driver-private TxUndersize ethtool stat instead.

> diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c
> index 6b170fc6e1875..5c027edb71e3e 100644
> --- a/drivers/net/dsa/motorcomm/mib.c
> +++ b/drivers/net/dsa/motorcomm/mib.c

[ ... ]

> @@ -376,8 +376,9 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
>  		start = u64_stats_fetch_begin(&pm->syncp);
>  
>  		stats->rx_length_errors = u64_stats_read(&mib->rx_undersize_errors) +
> -					  u64_stats_read(&mib->rx_fragment_errors);
> -		stats->rx_over_errors = u64_stats_read(&mib->rx_oversize_errors);
> +					  u64_stats_read(&mib->rx_fragment_errors) +
> +					  u64_stats_read(&mib->rx_oversize_errors);
> +		/* stats->rx_over_errors */

[Severity: Low]
Should this patch have a Fixes: tag?

The mapping it corrects came in with the original driver. At that
commit, yt921x_dsa_get_stats64() in drivers/net/dsa/yt921x.c had:

	stats->rx_over_errors = mib->rx_oversize_errors;
	...
	stats->tx_fifo_errors = mib->tx_undersize_errors;

Both values were also summed into rx_errors and tx_errors.

Commit 25265e7ccdb5 later moved this code to
drivers/net/dsa/motorcomm/mib.c, and commit ed4453587dc0 wrapped the
reads in u64_stats_read(). Neither commit changed the mapping.

Would something like this be appropriate?

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

>  		stats->rx_crc_errors = u64_stats_read(&mib->rx_crc_errors);
>  		stats->rx_frame_errors = u64_stats_read(&mib->rx_alignment_errors);

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. Since
this function uses rx_fragment_errors as the fragment count, does
yt921x_dsa_get_rmon_stats() read the wrong counter for RMON fragments?

drivers/net/dsa/motorcomm/mib.c:yt921x_dsa_get_rmon_stats() {
    ...
	rmon_stats->fragments = u64_stats_read(&mib->rx_alignment_errors);
    ...
}

rx_alignment_errors is already reported as AlignmentErrors in
yt921x_dsa_get_eth_mac_stats() and as rx_frame_errors here. That means
the hardware fragment counter (YT921X_MIB_DATA_RX_FRAG_ERR) can't be
seen through the standard RMON stats.

A later patch in this series, "net: dsa: motorcomm: Fix RMON fragments
stat", changes that line to read rx_fragment_errors. Nothing is left to
fix by the end of the series.

>  		/* stats->rx_fifo_errors */
> @@ -385,7 +386,7 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
>  
>  		stats->tx_aborted_errors = u64_stats_read(&mib->tx_aborted_errors);
>  		/* stats->tx_carrier_errors */
> -		stats->tx_fifo_errors = u64_stats_read(&mib->tx_undersize_errors);
> +		/* stats->tx_fifo_errors */
>  		/* stats->tx_heartbeat_errors */
>  		stats->tx_window_errors = u64_stats_read(&mib->tx_late_collisions);
>  
> @@ -401,10 +402,9 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
>  		stats->collisions = u64_stats_read(&mib->tx_collisions);
>  	} while (u64_stats_fetch_retry(&pm->syncp, start));
>  
> -	stats->rx_errors = stats->rx_length_errors + stats->rx_over_errors +
> -			   stats->rx_crc_errors + stats->rx_frame_errors;
> -	stats->tx_errors = stats->tx_aborted_errors + stats->tx_fifo_errors +
> -			   stats->tx_window_errors;
> +	stats->rx_errors = stats->rx_length_errors + stats->rx_crc_errors +
> +			   stats->rx_frame_errors;
> +	stats->tx_errors = stats->tx_aborted_errors + stats->tx_window_errors;

[Severity: Medium]
With the tx_fifo_errors mapping removed, TX undersize events are no
longer counted in any rtnl_link_stats64 field, and that includes
tx_errors. Is this intended?

The tx_errors kernel-doc in include/uapi/linux/if_link.h says:

 * @tx_errors: Total number of transmit problems.
 *   This counter must include events counter by @tx_aborted_errors,
 *   @tx_carrier_errors, @tx_fifo_errors, @tx_heartbeat_errors,
 *   @tx_window_errors and other errors not otherwise counted.

A runt frame may not be a FIFO underrun. Could tx_undersize_errors still
be added to tx_errors as an error not otherwise counted?

As written, a port that sends runt frames now shows a lower tx_errors in
ip -s link and /proc/net/dev than before this patch. The only place these
events still appear is the driver-private TxUndersize ethtool -S string.

The commit message also doesn't mention the change to tx_errors. The
later patches in this series don't change this calculation.

>  }
>  
>  void

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

Re: [PATCH net-next 2/4] net: dsa: motorcomm: Fix RMON fragments stat

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This commit fixes a statistics bug that users can see, but it has
  no Fixes: tag.

--

net: dsa: motorcomm: Fix RMON fragments stat

This commit changes yt921x_dsa_get_rmon_stats() so that the RMON
etherStatsFragments counter is read from rx_fragment_errors, not
rx_alignment_errors. The alignment error counter is already exported as
AlignmentErrors and rx_frame_errors.

> etherStatsFragments counts frames with a bad FCS due to fragmentation;
> report the dedicated RX fragment error counter instead of the alignment
> error one, which is already exported as AlignmentErrors and
> rx_frame_errors.
>
> Signed-off-by: David Yang <mmyangfl@gmail.com>

[Severity: Low]
Should this have a Fixes: tag? The wrong mapping looks like it goes back
to the commit that first added the driver.

In that commit, yt921x_dsa_get_rmon_stats() in drivers/net/dsa/yt921x.c
already had:

	rmon_stats->fragments = mib->rx_alignment_errors;

Since then the line has only been moved by "net: dsa: motorcomm: Split
MIB module" and converted to u64_stats_t. Neither change touched the
mapping.

Without the tag, stable kernels that contain the original driver will
keep reporting alignment errors as fragments in the ethtool RMON stats
for YT921x ports. Would something like this work?

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

Older trees will probably need a hand-adjusted backport. Before the move
to motorcomm/, the code is in drivers/net/dsa/yt921x.c and uses plain u64
fields, not u64_stats_t.

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

Re: [PATCH net-next 3/4] net: dsa: motorcomm: Use safe 64-bit counter reader

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] yt921x_counter_read() in drivers/net/dsa/motorcomm/smi.c
  returns a false -EIO for half of the low-word wraps that race with a…
- [Low] "net: dsa: motorcomm: Use safe 64-bit counter reader" fixes a
  user-visible bug, a transient ~4 GiB jump in the 64-bit RX/TX good-…

--

net: dsa: motorcomm: Use safe 64-bit counter reader

This commit changes how yt921x_mib_read() reads the 64-bit MIB counters.
The old code read the low word and then the high word. That is replaced
with a new helper, yt921x_counter_read(), which reads the low word again
and reads the high word again if the low word wrapped. The aim is to
avoid a spurious ~4 GiB jump when the low word wraps between the two
reads.

> diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c
> index 90b0ef670aa31..b45bc82117412 100644
> --- a/drivers/net/dsa/motorcomm/mib.c
> +++ b/drivers/net/dsa/motorcomm/mib.c
> @@ -102,29 +102,26 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)

[ ... ]

> -		} else {
> -			u32 val1;
>  
> -			res = yt921x_reg_read(priv, reg + 4, &val1);
> +			buf[i] = val;
> +		} else {
> +			res = yt921x_counter_read(priv, reg, &buf[i]);
>  			if (res)
>  				break;
> -			val = ((u64)val1 << 32) | val0;
>  		}

[Severity: Low]
The commit fixes a user-visible problem: a transient ~4 GiB jump in the
64-bit good-bytes and bad-bytes counters. That jump shows up in ethtool -S
and rtnl_link_stats64. Should it carry a Fixes: tag?

The read removed here, which takes the low word and then the high word in
two separate transactions, has been in the driver since it was added in
drivers/net/dsa/yt921x.c. It was later moved into
drivers/net/dsa/motorcomm/mib.c by 25265e7ccdb51b. That would suggest:

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

> diff --git a/drivers/net/dsa/motorcomm/smi.c b/drivers/net/dsa/motorcomm/smi.c
> index bf3adfd64165a..16f62409422d5 100644
> --- a/drivers/net/dsa/motorcomm/smi.c
> +++ b/drivers/net/dsa/motorcomm/smi.c
> @@ -63,6 +63,41 @@ int yt921x_reg_update_bits(struct yt921x_priv *priv, u32 reg, u32 mask, u32 val)
>  	return yt921x_reg_write(priv, reg, u);
>  }
>  
> +/* Reliably read a 64-bit counter, assuming the counter isn't that "fast" */
> +int yt921x_counter_read(struct yt921x_priv *priv, u32 reg, u64 *valp)
> +{
> +	u32 old_lo;
> +	int res;
> +	u32 hi;
> +	u32 lo;
> +
> +	res = yt921x_reg_read(priv, reg, &old_lo);
> +	if (res)
> +		return res;
> +
> +	res = yt921x_reg_read(priv, reg + 4, &hi);
> +	if (res)
> +		return res;
> +
> +	res = yt921x_reg_read(priv, reg, &lo);
> +	if (res)
> +		return res;
> +
> +	if (lo < old_lo) {
> +		u32 new_hi;
> +
> +		res = yt921x_reg_read(priv, reg + 4, &new_hi);
> +		if (res)
> +			return res;
> +		if (new_hi != hi + 1)
> +			return -EIO;

[Severity: Medium]
Can this return -EIO for a read that is actually consistent?

The reads happen in this order:

  t0: old_lo = [reg]
  t1: hi     = [reg + 4]
  t2: lo     = [reg]
  t3: new_hi = [reg + 4]   (only if lo < old_lo)

lo < old_lo only shows that the low word wrapped somewhere between t0 and
t2.

If the wrap happens between t1 and t2, then hi == H and new_hi == H + 1,
so the check passes.

If the wrap happens between t0 and t1, the carry is already in the high
word when hi is read. Then hi == H + 1 and new_hi == H + 1 == hi. The
(hi, lo) pair is a valid snapshot, but new_hi != hi + 1 rejects it with
-EIO.

The t0..t1 window is one SMI transaction, the same size as the t1..t2
window. So roughly half of the wraps that race with a read would take
this path. At 1 Gb/s the low words of the good-bytes and bad-bytes
counters wrap about every 34 seconds.

When it happens, yt921x_mib_read() breaks out of its descriptor loop, so
the remaining counters for that port are not refreshed. The partially
refreshed buffer is still published, and this message is logged:

  Failed to read stats for port N: -5

In addition, yt921x_mib_poll() backs off:

	if (res)
		delay *= 4;

That moves the poll interval from 3 seconds to 12 seconds, for a bus
error that never occurred.

The later "net: dsa: motorcomm: Prevent rx_bytes/tx_bytes underflow"
change drops the whole sample for that cycle instead. It does not touch
yt921x_counter_read(), so the false -EIO is still present after it.

Would it work to accept new_hi when it equals either hi or hi + 1 and
pair it with lo? Another option is a hi/lo/hi read that retries while
the two hi values differ.

> +		hi = new_hi;
> +	}
> +
> +	*valp = ((u64)hi << 32) | lo;
> +	return 0;
> +}

[ ... ]

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

Re: [PATCH net-next 4/4] net: dsa: motorcomm: Prevent rx_bytes/tx_bytes underflow

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] yt921x_counter_read() (drivers/net/dsa/motorcomm/smi.c)
  returns a false -EIO for a valid read.
- [Low] The commit fixes a user-visible bug: rtnl_link_stats64
  rx_bytes/tx_bytes (as shown by `ip -s link`) can read as about 2^64…

--

net: dsa: motorcomm: Prevent rx_bytes/tx_bytes underflow

This commit stops yt921x_mib_read() from publishing a partially refreshed
MIB buffer when a register read fails. It moves the rx_bytes/tx_bytes
derivation (good_bytes - ETH_FCS_LEN * frames) into the writer under
priv->reg_lock, and publishes the result only when check_sub_overflow()
reports no underflow. yt921x_dsa_get_stats64() then copies the published
values.

> diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c
> index b45bc82117412..cb3912493cfb1 100644
> --- a/drivers/net/dsa/motorcomm/mib.c
> +++ b/drivers/net/dsa/motorcomm/mib.c

[ ... ]

> @@ -124,6 +126,12 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
>  		}
>  	}
>  
> +	if (res) {
> +		dev_err(dev, "Failed to %s port %d: %i\n", "read stats for",
> +			port, res);
> +		return res;
> +	}
> +

[Severity: Medium]
With this early return, any error from yt921x_counter_read() now throws
away the whole sample. Can yt921x_counter_read() return -EIO for a read
that is actually consistent?

drivers/net/dsa/motorcomm/smi.c:yt921x_counter_read() {
	...
	res = yt921x_reg_read(priv, reg, &old_lo);
	...
	res = yt921x_reg_read(priv, reg + 4, &hi);
	...
	res = yt921x_reg_read(priv, reg, &lo);
	...
	if (lo < old_lo) {
		u32 new_hi;

		res = yt921x_reg_read(priv, reg + 4, &new_hi);
		...
		if (new_hi != hi + 1)
			return -EIO;
		hi = new_hi;
	}
	...
}

Suppose the low word wraps after old_lo is read but before hi is read.
Then hi and lo are both post-wrap values and already form a valid 64-bit
counter. For example:

  old_lo = 0xfffffff0, hi = H + 1, lo = 0x10, new_hi = H + 1

In this case new_hi == hi, so the check fails and -EIO is returned.

The affected counters are the 64-bit rx_good_bytes, rx_bad_bytes and
tx_good_bytes. Their low words wrap about every 34 seconds at 1 Gb/s
line rate.

When this happens, yt921x_mib_read() logs "Failed to read stats for port
N: -5" and discards the sample. yt921x_mib_poll() then pushes the next
poll back from 3 to 12 seconds:

	if (res)
		delay *= 4;

The ethtool callbacks that ignore the return value would quietly report
the previous sample.

The reader came in with the earlier "net: dsa: motorcomm: Use safe 64-bit
counter reader" commit in this series, but the error only becomes visible
through this patch's discard path.

Could yt921x_counter_read() also accept new_hi == hi? Another option is a
hi/lo/hi2 sequence that re-reads lo when hi changes.

[ ... ]

> @@ -389,10 +400,8 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port,
>  
>  		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;
> +		stats->rx_bytes = u64_stats_read(&pm->rx_bytes);
> +		stats->tx_bytes = u64_stats_read(&pm->tx_bytes);

[Severity: Low]
This fixes rx_bytes/tx_bytes in rtnl_link_stats64 reading as about 2^64,
which users can see with ip -s link. Should the commit carry a Fixes: tag?

The unchecked subtraction dates back to the initial driver commit:

Fixes: 186623f4aa72 ("net: dsa: yt921x: Add support for Motorcomm YT921x")

Later, 25265e7ccdb5 ("net: dsa: motorcomm: Split MIB module") moved it,
and e7b23688d0d3 and ed4453587dc0 rewrote it, but none of them guarded
it against underflow. The driver has been in released kernels since late
2025.

[ ... ]

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