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

[netdev-bot+sashiko@kernel.org]

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