netdev - Re: [PATCH net 6/6] net: dsa: mv88e6xxx: Link in pcs_get

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: <20211130170911.6355db44@thinkpad>
Date:   Tue, 30 Nov 2021 17:09:11 +0100
From:   Marek Behún <kabel@...nel.org>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Subject: Re: [PATCH net 6/6] net: dsa: mv88e6xxx: Link in pcs_get_state() if
 AN is bypassed

On Tue, 30 Nov 2021 01:18:12 +0100
Marek Behún <kabel@...nel.org> wrote:

> On Mon, 29 Nov 2021 23:14:17 +0000
> "Russell King (Oracle)" <linux@...linux.org.uk> wrote:
> 
> > On Mon, Nov 29, 2021 at 08:58:23PM +0100, Marek Behún wrote:  
> > >  static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
> > > -					  u16 status, u16 lpa,
> > > +					  u16 ctrl, u16 status, u16 lpa,
> > >  					  struct phylink_link_state *state)
> > >  {
> > > +	state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> > > +
> > >  	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
> > > -		state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> > > +		state->an_complete = !!(ctrl & BMCR_ANENABLE);    
> > 
> > I think I'd much rather report the value of BMSR_ANEGCAPABLE - since
> > an_complete controls the BMSR_ANEGCAPABLE bit in the emulated PHY
> > that userspace sees. Otherwise, an_complete is not used.
> > 
> > state->link is the key that phylink uses to know whether it can
> > trust the status being reported.  
> 
> Isn't BMSR_ANEGCAPABLE set to 1 even if aneg is disabled in BMCR?
> I will test tomorrow.
> 
> The reason why I used BMCR_ANENABLE is that if we don't enable AN, the
> PHY will report SPD_DPL_VALID if link is up. But clearly AN is not
> completed in that case, because it was never enabled in the first place.

Seems that BMSR_ANEGCAPABLE is set even if AN is disabled in BMCR.

If phylink_autoneg_inband() is not true mv88e6*_serdes_pcs_config(),
then we aren't enabling AN. But in this case SPD_DPL_VALID is always 1,
so if we use, as you say, an_complete=BMSR_ANEGCAPABLE in
mv88e6xxx_serdes_pcs_get_state(), we will always set an_complete=true, even
if AN wasn't enabled.

I was under the impression that
  state->an_complete
should only be set to true if AN is enabled.

Marek