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]
Date:   Wed, 17 Aug 2022 22:03:51 +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 01:47:07PM -0700, Colin Foster wrote:
> > 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.
> 
> You get my concern. That's a good comment / addition. Gaps are welcome
> in the register layout, but moving backwards will ensure (in the current
> implementation) inefficiencies.

Ok. The WARN_ON() won't trigger with current code. Do you mind if I add
it as a net-next change, and don't resend this series for net? I'm
worried I'll miss this week's "net" pull request which means I'll have
to wait some more for the other rework surrounding stats handling in the
ocelot driver (which in turn is a dependency for frame preemption).

> > 
> > > 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?
> 
> I suspect the issue I'm seeing is that there's something wrong with the
> HSIO registers that control the QSGMII interface between the 7512 and
> the 8514. Possibly something with PLL configuration / calibration? I
> don't really know yet, and bouncing between the source
> (ocelot_vsc7514.c, {felix,ocelot-ext}.c, phy-ocelot-serdes.c), the
> reference design software, and the datasheet is slowing me down quite a
> bit. Unless I am mistaken, it feels like the problems I'm chasing down
> are at the register <> datasheet interface and not something exposed
> through any existing interfaces.

So you mean you suspect that the HSIO register definitions are somehow
wrong? You mean the phy-ocelot-serdes.c driver seems to behave strangely
in a way that could possibly indicate it's accessing the wrong stuff?
Do you have any indication that this is the case? I'm not familiar at
all with blocks that weren't instantiated on NXP hardware (we have our
own SERDES), and I see you're already monitoring the right source files,
so I'm afraid there isn't much that I can help you with.

> I plan to get some internal support on that front that can hopefully
> point me in the right direction, or find what I have set up incorrectly.
> Otherwise it probably doesn't even make sense to send out anything for
> review until the MFD set gets accepted. Though maybe I'm wrong there.

IDK, if you have a concrete description of the problem, I suppose the
contributors to the SERDES driver may be able to come up with a
suggestion or two?

I suggest you try to cover all bases; is the HSIO PLL locked and at the
right frequency for QSGMII? Does the lane acquire CDR lock? Are in-band
autoneg settings in sync between the PCS and the PHY? Does the PCS
report link up?

> I'd also like to try to keep my patch version count down to one nibble
> next time, so I'm planning on keeping ports 0-3 and ports 4-7+ in
> separate patch sets :D