netdev - Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

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]

Date: Fri, 2 Jun 2023 14:28:04 +0300
From: Ioana Ciornei <ioana.ciornei@....com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: msmulski2@...il.com, andrew@...n.ch, f.fainelli@...il.com,
	olteanv@...il.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, simon.horman@...igine.com,
	kabel@...nel.org, Michal Smulski <michal.smulski@...a.com>
Subject: Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII
 mode for mv88e6393x

On Fri, Jun 02, 2023 at 11:30:36AM +0100, Russell King (Oracle) wrote:
> On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@...il.com wrote:
> > +/* USXGMII registers for Marvell switch 88e639x are undocumented and this function is based
> > + * on some educated guesses. It appears that there are no status bits related to
> > + * autonegotiation complete or flow control.
> > + */
> > +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> > +						  int port, int lane,
> > +						  struct phylink_link_state *state)
> > +{
> > +	u16 status, lp_status;
> > +	int err;
> > +
> > +	err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > +				    MV88E6390_USXGMII_PHY_STATUS, &status);
> > +	if (err) {
> > +		dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> > +		return err;
> > +	}
> > +	dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
> > +
> > +	state->link = !!(status & MDIO_USXGMII_LINK);
> > +
> > +	if (state->link) {
> > +		err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> > +					    MV88E6390_USXGMII_LP_STATUS, &lp_status);
> 
> What's the difference between these two registers? Specifically, what
> I'm asking is why the USXGMII partner status seems to be split between
> two separate registers.
> 
> Note that I think phylink_decode_usxgmii_word() is probably missing a
> check for MDIO_USXGMII_LINK, based on how Cisco SGMII works (and
> USXGMII is pretty similar.)
> 
> MDIO_USXGMII_LINK indicates whether the attached PHY has link on the
> media side or not. It's entirely possible for the USXGMII link to be
> up (thus allowing us to receive the "LPA" from the PHY) but for the
> PHY to be reporting to us that it has no media side link.
> 
> So, I think phylink_decode_usxgmii_word() at least needs at the
> beginning this added:
> 
> 	if (!(lpa & MDIO_USXGMII_LINK)) {
> 		state->link = false;
> 		return;
> 	}
> 
> The only user of this has been the Lynx PCS, so I'll add Ioana to this
> email as well to see whether there's any objection from Lynx PCS users
> to adding it.
>

I just tested this snippet on a LX2160ARDB that has USXGMII on two of
its lanes which go to Aquantia AQR113C PHYs.

The lpa read is 0x5601 and, with the patch, the interface does not link
up. I am not sure what is happening, if it's this PHY in particular that
does not properly set MDIO_USXGMII_LINK.

Ioana