Re: [PATCH net 1/2] net: dsa: mv88e6xxx: fix 88E6171 statistics snapshot

[netdev-bot+sashiko]

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 fix is incomplete.

--

net: dsa: mv88e6xxx: fix 88E6171 statistics snapshot

This patch changes the 88E6171 ops table from the shifted
mv88e6320_g1_stats_snapshot() helper to the unshifted
mv88e6xxx_g1_stats_snapshot() helper. The 6351 family takes the port
number in the low bits of the statistics operation register. With the
shifted helper, ethtool -S returns port 0's MIB counters for every user
port.

> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 2a975ecd79c74..60d2ce80ede01 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -4611,7 +4611,7 @@ static const struct mv88e6xxx_ops mv88e6171_ops = {
>  	.port_disable_pri_override = mv88e6xxx_port_disable_pri_override,
>  	.port_get_cmode = mv88e6352_port_get_cmode,
>  	.port_setup_message_port = mv88e6xxx_setup_message_port,
> -	.stats_snapshot = mv88e6320_g1_stats_snapshot,
> +	.stats_snapshot = mv88e6xxx_g1_stats_snapshot,

[Severity: Medium]
Should the other 6351 family ops tables get the same change?

In chip.c, mv88e6175_ops, mv88e6350_ops and mv88e6351_ops are all marked
/* MV88E6XXX_FAMILY_6351 */. Their info entries set
.family = MV88E6XXX_FAMILY_6351. All three still have:

	.stats_snapshot = mv88e6320_g1_stats_snapshot,

and that helper in global1.c does:

int mv88e6320_g1_stats_snapshot(struct mv88e6xxx_chip *chip, int port)
{
	port = (port + 1) << 5;

	return mv88e6xxx_g1_stats_snapshot(chip, port);
}

For ports 0..6, bits 4:0 of the STATS_OP CAPTURE_PORT write end up as
zero. If those chips decode the port from the low bits, as the commit
message says the 6351 family does, wouldn't every snapshot on
88E6175/88E6350/88E6351 capture port 0?

All stats readers reach the callback through mv88e6xxx_stats_snapshot():
mv88e6xxx_get_ethtool_stats(), get_eth_mac_stats() and get_rmon_stats().
Each of these would then report port 0's counters for every user port on
those chips.

The commit named in the Fixes: tag, a605a0fe7115, appears to have set
mv88e6320_g1_stats_snapshot on all four 6351 family tables. Before that
refactoring, _mv88e6xxx_stats_snapshot() shifted only for:

	mv88e6xxx_6320_family(chip) || mv88e6xxx_6352_family(chip)

so every 6351 family chip used the unshifted encoding.

As far as I can tell, the follow-up patch in the series ("net: dsa:
mv88e6xxx: fix 88E6171 statistics type") only touches the 6171 info
entry. At the end of the series, the three sibling tables would still
use the shifted helper.

>  	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
>  	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
>  	.stats_get_strings = mv88e6095_stats_get_strings,

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