netdev - Re: [PATCH v2 net-next 6/6] net: dsa: mv88e6xxx: Add "rmon" counter group support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231206195550.4bf3qlybd3hoip5h@skbuf>
Date: Wed, 6 Dec 2023 21:55:50 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
	f.fainelli@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 6/6] net: dsa: mv88e6xxx: Add "rmon" counter
 group support

On Wed, Dec 06, 2023 at 09:27:29AM +0100, Tobias Waldekranz wrote:
> > I just noticed that this doesn't populate the TX counters, just RX.
> >
> > I haven't tried it, but I think the Histogram Mode bits (11:10) of the
> > Stats Operation Register might be able to control what gets reported for
> > the Set 4 of counters. Currently AFAICS, the driver always sets it to
> > MV88E6XXX_G1_STATS_OP_HIST_RX_TX, aka what gets reported to
> > "rx-rmon-etherStatsPkts64to64Octets" is actually an RX+TX counter.
> 
> You have a keen eye! Yes, that is what's happening.

It would be nice if my failure-prone keen eye had the safety net of a
selftest that catches this kind of stuff. After all, the ethtool
counters were standardized in order for us to be able to expect standard
behavior out of them, and for nonconformities to stand out easily.

Do you think (bearing in mind that the questions below might make the
rest irrelevant) that you could look into creating a minimal test in
tools/testing/selftests/net/forwarding and symlinking it to
tools/testing/selftests/drivers/net/dsa? You can start from
ethtool_std_stats_get() and take inspiration from the way in which it is
used by ethtool_mm.sh.

> > What's the story behind this?
> 
> I think the story starts, and ends, with this value being the hardware
> default.

I do hope that is where the story actually ends.

But the 88E6097 documentation I have suggests that the Histogram Mode
bits are reserved to the value of 3 (RX+TX), which suggests that this
cannot be written to any other value.

> Seeing as the hardware only has a single set of histogram counters,

"Seeing" means you tested this? calling chip->info->ops->stats_set_histogram()
at runtime, and seeing if the previously hidden histogram counters are
reset to zero, or if they show retroactively counted packets?

I searched through the git logs, but it's not exactly clear that this
was tried and doesn't work.

> it seems to me like we have to prioritize between:
> 
> 1. Keeping Rx+Tx: Backwards-compatible, but we can't export any histogram via
>    the standard RMON group.
> 
> 2. Move to Rx-only: We can export them via the RMON group, but we change
>    the behavior of the "native" counters.
> 
> 3. Move to Tx-only: We can export them via the RMON group, but we change
>    the behavior of the "native" counters.
> 
> Looking at RFC2819, which lays out the original RMON MIB, we find this
> description:
> 
>     etherStatsPkts64Octets OBJECT-TYPE
>         SYNTAX     Counter32
>         UNITS      "Packets"
>         MAX-ACCESS read-only
>         STATUS     current
>         DESCRIPTION
>             "The total number of packets (including bad
>             packets) received that were 64 octets in length
>             (excluding framing bits but including FCS octets)."
>         ::= { etherStatsEntry 14 }
> 
> In my opinion, this gives (2) a clear edge over (3), so we're down to
> choosing between (1) and (2). Personally, I lean towards (2), as I think
> it is more useful because:
> 
> - Most people will tend to assume that the histogram counters refers to
>   those defined in RFC2819 anyway
> 
> - It means we can deliver _something_ rather than nothing to someone
>   building an operating system, who is looking for a hardware
>   independent way of providing diagnostics

If the "reserved to 3" thing is true, then both (2) and (3) become
practically non-options, at least for 88E6097. The waters would be
further muddied if the driver were to make some chips use one
Histogram Mode, and other chips a different one. It implies that as
a user, you would need to know what switch family you have, before
you know what "ethtool -S lan0 | grep hist_64bytes" is counting!