netdev - Re: [PATCH net 6/8] net: mscc: ocelot: make struct ocelot_stat

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: <Yv0FwVuroXgUsWNo@colin-ia-desktop>
Date:   Wed, 17 Aug 2022 08:14:09 -0700
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Maxim Kochetkov <fido_max@...ox.ru>
Subject: Re: [PATCH net 6/8] net: mscc: ocelot: make struct
 ocelot_stat_layout array indexable

On Wed, Aug 17, 2022 at 01:05:32PM +0000, Vladimir Oltean wrote:
> On Wed, Aug 17, 2022 at 02:06:44PM +0300, Vladimir Oltean wrote:
> > I think in practice this means that ocelot_prepare_stats_regions() would
> > need to be modified to first sort the ocelot_stat_layout array by "reg"
> > value (to keep bulking efficient), and then, I think I'd have to keep to
> > introduce another array of u32 *ocelot->stat_indices (to keep specific
> > indexing possible). Then I'd have to go through one extra layer of
> > indirection; RX_OCTETS would be available at
> > 
> > ocelot->stats[port * OCELOT_NUM_STATS + ocelot->stat_indices[OCELOT_STAT_RX_OCTETS]].
> > 
> > (I can wrap this behind a helper, of course)
> > 
> > This is a bit complicated, but I'm not aware of something simpler that
> > would do what you want and what I want. What are your thoughts?
> 
> Or simpler, we can keep enum ocelot_stat sorted in ascending order of
> the associated SYS_COUNT_* register addresses. That should be very much
> possible, we just need to add a comment to watch out for that. Between
> switch revisions, the counter relative ordering won't differ. It's just
> that RX and TX counters have a larger space between each other.

That's what I thought was done... enum order == register order. But
that's a subtle, currently undocumented "feature" of my implementation
of the bulk reads. Also, it now relies on the fact that register order
is the same between hardware products - that's the new requirement that
I'm addressing.

I agree it would be nice to not require specific ordering, either in the
display order of `ethtool -S` or the definition order of enum
ocelot_stat. That's telling me that at some point someone (likely me?)
should probably write a sorting routine to guarantee optimized reads,
regardless of how they're defined or if there are common / unique
register sets.

The good thing about the current implementation is that the worst case
scenario is it will just fall back to the original behavior. That was
intentional.

Tangentially related: I'm having a heck of a time getting the QSGMII
connection to the VSC8514 working correctly. I plan to write a tool to
print out human-readable register names. Am I right to assume this is
the job of a userspace application, translating the output of
/sys/kernel/debug/regmap/ reads to their datasheet-friendly names, and
not something that belongs in some sort of sysfs interface? I took a
peek at mv88e6xxx_dump but it didn't seem to be what I was looking for.