Re: [PATCH net v2] net: dsa: motorcomm: Publish a consistent MIB sample

[netdev-bot+sashiko@kernel.org]

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

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

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] The underflow this patch says it fixes can still happen when
  the MIB sample is fully consistent.

--

net: dsa: motorcomm: Publish a consistent MIB sample

This patch moves the "good_bytes - ETH_FCS_LEN * frames" calculation out
of yt921x_dsa_get_stats64() and into the writer, yt921x_mib_read(). The
writer then publishes the result as pm->rx_bytes and pm->tx_bytes. It
also skips publishing the sample when a register read fails.

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

[ ... ]

> @@ -144,14 +152,15 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port)
>  		    u64_stats_read(&mib->tx_512_1023byte) +
>  		    u64_stats_read(&mib->tx_1024_1518byte) +
>  		    u64_stats_read(&mib->tx_jumbo);
> +	rx_bytes = u64_stats_read(&mib->rx_good_bytes) - ETH_FCS_LEN * rx_frames;
> +	tx_bytes = u64_stats_read(&mib->tx_good_bytes) - ETH_FCS_LEN * tx_frames;

[Severity: Medium]
Can this subtraction still underflow when the sample is fully consistent?

rx_frames and tx_frames are sums of the packet-length histogram buckets
(rx_64byte through rx_jumbo, and tx_64byte through tx_jumbo). The byte
term comes from a separate counter, RX_GOOD_BYTES or TX_GOOD_BYTES, which
appears to count good frames only. The hardware also has a separate
RxBadBytes counter:

	MIB_DESC(2, YT921X_MIB_DATA_RX_BAD_BYTES, "RxBadBytes"),

yt921x_dsa_get_rmon_stats() exports the same buckets as RMON histogram
counters:

	rmon_stats->hist[0] = u64_stats_read(&mib->rx_64byte);
	...
	rmon_stats->hist_tx[0] = u64_stats_read(&mib->tx_64byte);

Those are the RFC 2819 etherStatsPktsXtoYOctets counters, and RFC 2819
counts bad packets in them.

If the YT921x buckets also count CRC or alignment errored frames, a port
that receives only errored frames would see rx_64byte grow while
rx_good_bytes stays the same. rx_bytes would then wrap to about 2^64.
This is the same symptom the commit message describes.

Because the value is recomputed and published on every poll, would the
wrapped value persist? The commit message says:

    The issue is transient and abnormal values will be flushed
    by the next function call.

That claim would not hold in this case.

There also seems to be a mismatch with struct rtnl_link_stats64. It says
rx_packets counts good packets and rx_bytes corresponds to rx_packets.
For TX, the driver already reads a good-frame counter into mib->tx_good:

	MIB_DESC(1, YT921X_MIB_DATA_TX_PKT, NULL),

Would tx_good be a better match for tx_good_bytes than the histogram sum?
Alternatively, should the subtraction be clamped to avoid wrapping?

>  	u64_stats_set(&pm->rx_frames, rx_frames);
>  	u64_stats_set(&pm->tx_frames, tx_frames);
> +	u64_stats_set(&pm->rx_bytes, rx_bytes);
> +	u64_stats_set(&pm->tx_bytes, tx_bytes);
>  	u64_stats_update_end_irqrestore(&pm->syncp, flags);

[ ... ]

> @@ -391,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);

[ ... ]

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