netdev - Re: [PATCH net-next v3 4/9] net: phy: add Lynx PCS module

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: <20200623120301.GU1551@shell.armlinux.org.uk>
Date:   Tue, 23 Jun 2020 13:03:02 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Ioana Ciornei <ioana.ciornei@....com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandru Marginean <alexandru.marginean@....com>,
        "michael@...le.cc" <michael@...le.cc>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "olteanv@...il.com" <olteanv@...il.com>
Subject: Re: [PATCH net-next v3 4/9] net: phy: add Lynx PCS module

On Tue, Jun 23, 2020 at 11:49:28AM +0000, Ioana Ciornei wrote:
> > This should be added to phylink_mii_c45_pcs_get_state().  There is nothing that
> > is Lynx PCS specific here.
> 
> The USXGMII standard only describes the auto-negotiation word, not the MMD
> where this can be found (MMD_VEND2 in this case).
> I would not add a generic phylink herper that reads the MMD and also
> decodes it.
> Maybe a helper that just decodes the USXGMII word read from the
> Lynx module - phylink_decode_usxgmii_word(state, lpa) ?

Yes, you're right - as they come from the vendor 2 MMD, there's no
standard.  So yes, just a helper to decode the USXGMII word please.

> > > +static void lynx_pcs_get_state_2500basex(struct mdio_device *pcs,
> > > +					 struct phylink_link_state *state) {
> > > +	struct mii_bus *bus = pcs->bus;
> > > +	int addr = pcs->addr;
> > > +	int bmsr, lpa;
> > > +
> > > +	bmsr = mdiobus_read(bus, addr, MII_BMSR);
> > > +	lpa = mdiobus_read(bus, addr, MII_LPA);
> > > +	if (bmsr < 0 || lpa < 0) {
> > > +		state->link = false;
> > > +		return;
> > > +	}
> > > +
> > > +	state->link = !!(bmsr & BMSR_LSTATUS);
> > > +	state->an_complete = !!(bmsr & BMSR_ANEGCOMPLETE);
> > > +	if (!state->link)
> > > +		return;
> > > +
> > > +	state->speed = SPEED_2500;
> > > +	state->pause |= MLO_PAUSE_TX | MLO_PAUSE_RX;
> > 
> > How do you know the other side is using pause frames, or is capable of dealing
> > with them?
> 
> Isn't this done by also looking into the PHY's pause frame bits and
> enabling pause frames in the MAC only when both the PCS and the PHY
> have flow enabled?

You are assuming that there is a PHY to read that information from.

There may not be a PHY - I have 2500base-X fibre links here, there is
no PHY to read that from, there is only the PCS - but this runs with
the configuration word present, so is not supported by this code (at
least at the moment.)

If there is a PHY, these bits will not be used anyway, so there's no
point setting them.

> Yep, will remove. I've gone through the documentation and the register
> should be initialized to 0x0001 when in SGMII mode
> (as done by phylink_mii_c22_pcs_config()).

Yep, that was actually written referring to the LX2160A documentation.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!