linux-kernel - Re: [PATCH v1 net-next 1/2] net: mscc: ocelot: remove redundant stats

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: <Y3MK9PCz0JQSQNiQ@euler>
Date:   Mon, 14 Nov 2022 19:43:48 -0800
From:   Colin Foster <colin.foster@...advantage.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Vladimir Oltean <olteanv@...il.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH v1 net-next 1/2] net: mscc: ocelot: remove redundant
 stats_layout pointers

Hi Vladimir,

On Mon, Nov 14, 2022 at 03:15:36PM +0000, Vladimir Oltean wrote:
> Hi Colin,
> 
> On Fri, Nov 11, 2022 at 12:49:23PM -0800, Colin Foster wrote:
> > Ever since commit 4d1d157fb6a4 ("net: mscc: ocelot: share the common stat
> > definitions between all drivers") the stats_layout entry in ocelot and
> > felix drivers have become redundant. Remove the unnecessary code.
> > 
> > Suggested-by: Vladimir Oltean <olteanv@...il.com>
> > Signed-off-by: Colin Foster <colin.foster@...advantage.com>
> > ---
> 
> (please also the Microchip development list, people do seem to follow it
> and do respond sometimes)

Apologies there. Honest mistake, as I see get_maintainer was working.

> 
> Before saying anything about this patch set, I wanted to check what's
> the status with my downstream patches for the MAC Merge Layer counters.
> 
> There, vsc9959_stats_layout would get expanded to look like this:
> 
> static const struct ocelot_stat_layout vsc9959_stats_layout[OCELOT_NUM_STATS] = {
> 	OCELOT_COMMON_STATS,
> 	OCELOT_STAT(RX_ASSEMBLY_ERRS),
> 	OCELOT_STAT(RX_SMD_ERRS),
> 	OCELOT_STAT(RX_ASSEMBLY_OK),
> 	OCELOT_STAT(RX_MERGE_FRAGMENTS),
> 	OCELOT_STAT(TX_MERGE_FRAGMENTS),
> 	OCELOT_STAT(RX_PMAC_OCTETS),
> 	OCELOT_STAT(RX_PMAC_UNICAST),
> 	OCELOT_STAT(RX_PMAC_MULTICAST),
> 	OCELOT_STAT(RX_PMAC_BROADCAST),
> 	OCELOT_STAT(RX_PMAC_SHORTS),
> 	OCELOT_STAT(RX_PMAC_FRAGMENTS),
> 	OCELOT_STAT(RX_PMAC_JABBERS),
> 	OCELOT_STAT(RX_PMAC_CRC_ALIGN_ERRS),
> 	OCELOT_STAT(RX_PMAC_SYM_ERRS),
> 	OCELOT_STAT(RX_PMAC_64),
> 	OCELOT_STAT(RX_PMAC_65_127),
> 	OCELOT_STAT(RX_PMAC_128_255),
> 	OCELOT_STAT(RX_PMAC_256_511),
> 	OCELOT_STAT(RX_PMAC_512_1023),
> 	OCELOT_STAT(RX_PMAC_1024_1526),
> 	OCELOT_STAT(RX_PMAC_1527_MAX),
> 	OCELOT_STAT(RX_PMAC_PAUSE),
> 	OCELOT_STAT(RX_PMAC_CONTROL),
> 	OCELOT_STAT(RX_PMAC_LONGS),
> 	OCELOT_STAT(TX_PMAC_OCTETS),
> 	OCELOT_STAT(TX_PMAC_UNICAST),
> 	OCELOT_STAT(TX_PMAC_MULTICAST),
> 	OCELOT_STAT(TX_PMAC_BROADCAST),
> 	OCELOT_STAT(TX_PMAC_PAUSE),
> 	OCELOT_STAT(TX_PMAC_64),
> 	OCELOT_STAT(TX_PMAC_65_127),
> 	OCELOT_STAT(TX_PMAC_128_255),
> 	OCELOT_STAT(TX_PMAC_256_511),
> 	OCELOT_STAT(TX_PMAC_512_1023),
> 	OCELOT_STAT(TX_PMAC_1024_1526),
> 	OCELOT_STAT(TX_PMAC_1527_MAX),
> };
> 
> The issue is that not all Ocelot family switches support the MAC merge
> layer. Namely, only vsc9959 does.
> 
> With your removal of the ability to have a custom per-switch stats layout,
> the only remaining thing for vsc9959 to do is to add a "bool mm_supported"
> to the common struct ocelot, and all the above extra stats will only be read
> from the common code in ocelot_stats.c only if mm_supported is set to true.
> 
> What do you think, is this acceptable?

That's an interesting solution. I don't really have any strong opinions
on this one. I remember we'd had the discussion about making sure the
stats are ordered (so that bulk stat reads don't get fragmented) and that
wasn't an issue here. So I'm happy to go any route, either:

1. I fix up this patch and resubmit
2. I wait until the 9959 code lands, and do some tweaks for mac merge
stats
3. Maybe we deem this patch set unnecessary and drop it, since 9959 will
start using custom stats again.


Or maybe a 4th route, where ocelot->stats_layout remains in tact and
felix->info->stats_layout defaults to the common stats. Only the 9959
would have to override it?