Re: [PATCH net 06/12] net: systemport: Fix RUNT MIB counter register offset calculation

[netdev-bot+sashiko@kernel.org]

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] Commit message inaccurately describes the pre-fix behaviour of
  bcm_sysport_update_mib_counters()…

--

net: systemport: Fix RUNT MIB counter register offset calculation

This adds the second 0xC UniMAC MIB gap to the register offset used when
reading the BCM_SYSPORT_STAT_RUNT counters in
bcm_sysport_update_mib_counters(), so offset becomes
2 * UMAC_MIB_STAT_OFFSET for those four entries.  The register offsets
produced by the new code look correct to me.

> In bcm_sysport_update_mib_counters(), 'offset' was only set to
> UMAC_MIB_STAT_OFFSET (0xC) for all non-RX counters, omitting the second
> 0xC gap for BCM_SYSPORT_STAT_RUNT counters. As a result, all 4 RUNT MIB
> counters were read from unmapped gap register space.

[Severity: Low]
Is "all 4 RUNT MIB counters were read from unmapped gap register space"
accurate?  By the driver's own table it looks like only three of the four
reads landed in the reserved gap.

bcm_sysport_gstrings_stats[] has 29 STAT_MIB_RX entries and 29
STAT_MIB_TX entries, all u32, so 'j' is 29 * 4 + 29 * 4 = 0xE8 by the time
the first STAT_RUNT entry is processed.  With the old offset of 0xC the
four reads were:

    UMAC_MIB_START(0x400) + 0xE8 + 0xC = 0x4F4, 0x4F8, 0x4FC, 0x500

The annotations in bcmsysport.h put mib.rx.rcrc at 0x470 and mib.tx.uc at
0x4F0, which places the second 0xC gap at 0x4F4-0x4FF and the start of the
RUNT block at 0x500.  So 0x4F4/0x4F8/0x4FC were gap space, but 0x500 was
the real first RUNT register.

That last read was stored into the fourth STAT_RUNT destination:

	STAT_RUNT("rx_runt_pkts", mib.rx_runt_cnt),
	STAT_RUNT("rx_runt_valid_fcs", mib.rx_runt_fcs),
	STAT_RUNT("rx_runt_inval_fcs_align", mib.rx_runt_fcs_align),
	STAT_RUNT("rx_runt_bytes", mib.rx_runt_bytes),

so rx_runt_bytes reported the runt packet count rather than gap data.

Could the changelog also state the user-visible symptom, i.e. that
ethtool -S reported bogus values for rx_runt_pkts, rx_runt_valid_fcs and
rx_runt_inval_fcs_align, and a mis-assigned value for rx_runt_bytes?  That
would help anyone deciding on a stable backport.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921231305.394773-1-florian.fainelli%40broadcom.com