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: <20220817174226.ih5aikph6qyc2xtz@skbuf>
Date:   Wed, 17 Aug 2022 17:42:27 +0000
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     Colin Foster <colin.foster@...advantage.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 08:14:09AM -0700, Colin Foster wrote:
> 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.

How about we add this extra check?

diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
index d39908c1c6c9..85259de86ec2 100644
--- a/drivers/net/ethernet/mscc/ocelot_stats.c
+++ b/drivers/net/ethernet/mscc/ocelot_stats.c
@@ -385,7 +385,7 @@ EXPORT_SYMBOL(ocelot_port_get_stats64);
 static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
 {
 	struct ocelot_stats_region *region = NULL;
-	unsigned int last;
+	unsigned int last = 0;
 	int i;
 
 	INIT_LIST_HEAD(&ocelot->stats_regions);
@@ -402,6 +402,12 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
 			if (!region)
 				return -ENOMEM;
 
+			/* enum ocelot_stat must be kept sorted in the same
+			 * order as ocelot->stats_layout[i].reg in order to
+			 * have efficient bulking.
+			 */
+			WARN_ON(last >= ocelot->stats_layout[i].reg);
+
 			region->base = ocelot->stats_layout[i].reg;
 			region->count = 1;
 			list_add_tail(&region->node, &ocelot->stats_regions);

If not, help me understand the concern better.

> 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.

Why is the mv88e6xxx_dump kind of program (using devlink regions) not
what you're looking for?

There's also ethtool --register-dump which some DSA drivers have support
for (again, mv88e6xxx), but I think that's mainly per port.

I tried to add support for devlink regions to dump the PGIDs, but doing
this from the common ocelot switch lib is a PITA, because in the devlink
callbacks, we need to access the struct ocelot *ocelot from the
struct devlink *devlink. But DSA keeps the devlink pointer one way, and
the ocelot switchdev driver another. To know how to retrieve the ocelot
pointer from the devlink pointer, you'd need to know where to search for it.

So I'm thinking, if we add devlink regions to ocelot, it would be just
for DSA for now.