netdev - Re: [PATCH net] net: dsa: mv88e6xxx: do not allow inband AN for 2500base-x mode

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, 15 Jan 2021 00:55:16 +0100
From:   Marek Behún <kabel@...nel.org>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     netdev@...r.kernel.org, pavana.sharma@...i.com,
        vivien.didelot@...il.com, f.fainelli@...il.com, kuba@...nel.org,
        lkp@...el.com, davem@...emloft.net, ashkan.boldaji@...i.com,
        Chris Packham <chris.packham@...iedtelesis.co.nz>,
        olteanv@...il.com,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Russell King <rmk+kernel@...linux.org.uk>
Subject: Re: [PATCH net] net: dsa: mv88e6xxx: do not allow inband AN for
 2500base-x mode

On Fri, 15 Jan 2021 00:00:33 +0100
Andrew Lunn <andrew@...n.ch> wrote:

> > diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
> > index 3195936dc5be..b8241820679e 100644
> > --- a/drivers/net/dsa/mv88e6xxx/serdes.c
> > +++ b/drivers/net/dsa/mv88e6xxx/serdes.c
> > @@ -55,9 +55,20 @@ static int mv88e6xxx_serdes_pcs_get_state(struct mv88e6xxx_chip *chip,
> >  {
> >  	if (status & MV88E6390_SGMII_PHY_STATUS_SPD_DPL_VALID) {
> >  		state->link = !!(status & MV88E6390_SGMII_PHY_STATUS_LINK);
> > +
> > +		if (state->interface == PHY_INTERFACE_MODE_2500BASEX) {
> > +			if (state->link) {
> > +				state->speed = SPEED_2500;
> > +				state->duplex = DUPLEX_FULL;
> > +			}
> > +
> > +			return 0;
> > +		}
> > +
> > +		state->an_complete = 1;  
> 
> Should this be here? It seems like a logically different change, it is
> not clear to me it is to do with PHY_INTERFACE_MODE_2500BASEX.

This function does not set an_complete at all, and as I understand it,
it should. But maybe this should be in different commit, and more
thought put into it. I will rethink it and send another version.

> >  		state->duplex = status &
> >  				MV88E6390_SGMII_PHY_STATUS_DUPLEX_FULL ?
> > -			                         DUPLEX_FULL : DUPLEX_HALF;
> > +						DUPLEX_FULL : DUPLEX_HALF;  
> 
> This looks like an unintended white space change.

This change is intended. There were 17 space there istead of 2 tabs + 1
space. And the last space is not needed, since it does not provide any
other alignment. Should this be in separate commit?

BTW Andrew, the code in serdes.c does many read and write calls, and it
could be simplified a lot by implementing modify, setbits and clearbits
methods, like phy.h implements. Or maybe we can use phy_mmd_* methods
here instead of mv88e6390_serdes_read/write ?

I fear such change will make future backporting of new fix commits
a pain. But I still think it should be done. What do you think?

Marek