Re: [PATCH net-next] net: dsa: qca8k: report port statistics from the MIB

[Stanislaw Pal <kuncy7@gmail.com>]

On Sun, Oct 18, 2026 at 11:55:42PM +0200, Christian Marangi wrote:
> If we really want we can reset after each read but that is problematic for
> the ethtool OPs unless we also update that function to store local u64 MIB
> for everything.
>
> Up to you... I would start with keeping and updating all the below things
> and then evaluate the needed changes to reset at every read.

Agreed. v2 keeps QCA8K_MIB_CPU_KEEP and adds only the differences, so
ethtool -S stays as it is.

> This really seems a task for the MIB from mgmt frame... May I ask why those
> are not used here?

Only because of where I tested. On my GL-B3000 the switch runs with a
tag_8021q based tagger from the OpenWrt tree I work on (the conduit is
offloaded to the SoC's NSS engine there), so mgmt_conduit is never set
and register reads over MDIO were all I had. That is no reason for
mainline.

v2 will request one autocast per interval when mgmt_conduit is set and
take every port from that one burst, and fall back to register reads
otherwise (another tagger, CPU port 6, conduit down). To get all ports
from one request, the autocast handler gets a mode that fills a buffer
per port instead of dropping the ports that were not asked for.

> I would like to see define in the header file, not in the middle of
> functions and declaration.

Will move it.

> Why STATS and not MIB? Also I would like to have all these register
> converted to define if really needed and update the ar8327_mib struct.
>
> Also Also why not use that directly?

Right, the second table is not needed. v2 drops it: an enum of indices
into ar8327_mib[] (the table turned into designated initializers, so
the two cannot drift apart), the counters read by the code that
get_ethtool_stats uses, split out into a helper, and get_stats64 built
from that array.

> Why this? For 64 bit?

Yes, RxGoodByte and TxByte are the two 64-bit counters. With the shared
helper and ar8327_mib[].size they go away.

> Maybe generalize the logic in get_ethtool_stats?

Yes, as above: one helper for both.

> As said MDIO here is extra slow especially for MIB every x seconds...
>
> Autocast MIB is a MUCH BETTER WAY.

Agreed, register reads stay only as the fallback.

> Is it correct to give window_errors to TX late collision?

I went by include/uapi/linux/if_link.h: "@tx_window_errors: Number of
frame transmission errors due to late collisions (for Ethernet - after
the first 64B of transmission)". The same file says tx_errors must
include them, which the patch does.

> No reason to scan for a port that is disabled or not connected...

Ports disabled in DT are not user ports, so the loop already skips
them. In the register-read fallback v2 also skips ports without link
(a bit set in mac_link_up and cleared in mac_link_down). Since the
counters keep their value, nothing is lost: the next read picks up the
difference. With autocast the switch sends every port anyway.

> Considering everything is run under the same delayed work, does it make
> sense to have a spinlock for each port?
>
> Either have a delayed work for each port or a global spinlock for the task.

One spinlock for the whole cache in v2.

v2 is written and tested on the B3000, both paths. Over three 15 s
uploads on the WAN port each, the wan netdev's TX bytes matched the
MIB's TxByte within 0.002% with the qca tagger, where every read was an
autocast, and within 0.01% with register reads. I will send it
tomorrow, so there is time to object to the approach first.

Thanks for the review.

Stanislaw