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

[Andrew Lunn]

On Sat, Oct 24, 2026 at 11:21:23AM -0700, Florian Fainelli wrote:
> The 88E6171 belongs to the 6351 family and takes the port number in the
> low bits of the statistics operation register. The 6320 snapshot helper
> instead encodes the port as (port + 1) << 5. On an EA4500, this causes
> ethtool -S to return physical port 0's MIB counters for every user port.
> Traffic on other ports does not update their reported hardware counters.
> 
> Before the snapshot operation was moved into the chip operations, only
> the 6320 and 6352 families used the shifted encoding. The refactoring
> incorrectly assigned that encoding to the 88E6171 as well.
> 
> Use the unshifted snapshot helper for the 88E6171. Tested on an EA4500:
> pings through different switch ports now update only the corresponding
> port's hardware counters.
> 
> Fixes: a605a0fe7115 ("net: dsa: mv88e6xxx: Abstract stats_snapshot into ops structure")
> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 7f68a0c55802..67daf3b9cb48 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -4610,7 +4610,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,
>  	.stats_set_histogram = mv88e6095_g1_stats_set_histogram,
>  	.stats_get_sset_count = mv88e6095_stats_get_sset_count,
>  	.stats_get_strings = mv88e6095_stats_get_strings,

static const struct mv88e6xxx_ops mv88e6171_ops = {
        /* MV88E6XXX_FAMILY_6351 */
...
        .stats_snapshot = mv88e6320_g1_stats_snapshot,

static const struct mv88e6xxx_ops mv88e6175_ops = {
        /* MV88E6XXX_FAMILY_6351 */
...
        .stats_snapshot = mv88e6320_g1_stats_snapshot,

static const struct mv88e6xxx_ops mv88e6350_ops = {
        /* MV88E6XXX_FAMILY_6351 */
...
        .stats_snapshot = mv88e6320_g1_stats_snapshot,

static const struct mv88e6xxx_ops mv88e6351_ops = {
        /* MV88E6XXX_FAMILY_6351 */
...
       .stats_snapshot = mv88e6320_g1_stats_snapshot,

Should these be changed as well?

Looking at DSDT:

static GT_STATUS statsCapture
(
    IN GT_QD_DEV            *dev,
    IN GT_U8             port,
    IN GT_U8             bank
)
...
    if (IS_IN_DEV_GROUP(dev,DEV_RMON_PORT_BITS))
    {
        portNum = (port + 1) << 5;
    }
    else
    {
        portNum = (GT_U16)port;
    }

and

#define DEV_RMON_PORT_BITS    \
        ( DEV_88E6065 | DEV_88E6065 | DEV_88E6352_FAMILY )

So your change looks correct, just not complete.

       Andrew