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 for Android: free password hash cracker in your pocket

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

Message-ID: <YvyO1kjPKPQM0Zw8@euler>
Date:   Tue, 16 Aug 2022 23:46:46 -0700
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     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,
        "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 Tue, Aug 16, 2022 at 04:53:50PM +0300, Vladimir Oltean wrote:
> The ocelot counters are 32-bit and require periodic reading, every 2
> seconds, by ocelot_port_update_stats(), so that wraparounds are
> detected.
> 
> Currently, the counters reported by ocelot_get_stats64() come from the
> 32-bit hardware counters directly, rather than from the 64-bit
> accumulated ocelot->stats, and this is a problem for their integrity.
> 
> The strategy is to make ocelot_get_stats64() able to cherry-pick
> individual stats from ocelot->stats the way in which it currently reads
> them out from SYS_COUNT_* registers. But currently it can't, because
> ocelot->stats is an opaque u64 array that's used only to feed data into
> ethtool -S.
> 
> To solve that problem, we need to make ocelot->stats indexable, and
> associate each element with an element of struct ocelot_stat_layout used
> by ethtool -S.
> 
> This makes ocelot_stat_layout a fat (and possibly sparse) array, so we
> need to change the way in which we access it. We no longer need
> OCELOT_STAT_END as a sentinel, because we know the array's size
> (OCELOT_NUM_STATS). We just need to skip the array elements that were
> left unpopulated for the switch revision (ocelot, felix, seville).

Hi Vladimir,

I'm not sure if this is an issue here, and I'm not sure it will ever
be... ocelot_stat_layout as you know relies on consecutive register
addresses to be most efficient. This was indirectly enforced by
*_stats_layout[] always being laid out in ascending order.

If the order of ocelot_stat doesn't match each device's register
offset order, there'll be unnecessary overhead. I tried to test
this just now, but I'll have to deal with a few conflicts that I won't
be able to get to immediately.

Do you see this as a potential issue in the future? Or do you expect all
hardware is simliar enough that they'll all be ordered the same?

Or, because I'm the lucky one running on a slow SPI bus, this will be my
problem to monitor and fix if need be :)