netdev - Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Set link down when changing speed

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: <20200323223934.GA14512@lunn.ch>
Date:   Mon, 23 Mar 2020 23:39:34 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH net-next 2/2] net: dsa: mv88e6xxx: Set link down when
 changing speed

On Mon, Mar 23, 2020 at 10:01:13PM +0000, Russell King - ARM Linux admin wrote:
> On Mon, Mar 23, 2020 at 10:49:00PM +0100, Andrew Lunn wrote:
> > The MAC control register must not be changed unless the link is down.
> > Add the necassary call into mv88e6xxx_mac_link_up. Without it, the MAC
> > does not change state, the link remains at the wrong speed.
> > 
> > Fixes: 30c4a5b0aad8 ("net: mv88e6xxx: use resolved link config in mac_link_up()")
> > Signed-off-by: Andrew Lunn <andrew@...n.ch>
> > ---
> >  drivers/net/dsa/mv88e6xxx/chip.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> > index dd8a5666a584..24ce17503950 100644
> > --- a/drivers/net/dsa/mv88e6xxx/chip.c
> > +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> > @@ -733,6 +733,14 @@ static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
> >  
> >  	mv88e6xxx_reg_lock(chip);
> >  	if (!mv88e6xxx_port_ppu_updates(chip, port) || mode == MLO_AN_FIXED) {
> > +		/* Port's MAC control must not be changed unless the
> > +		 * link is down
> > +		 */
> > +		err = chip->info->ops->port_set_link(chip, port,
> > +						     LINK_FORCED_DOWN);
> > +		if (err)
> > +			goto error;
> > +
> 
> The port should be down at this point, otherwise the link state is not
> matching phylink's idea of the state.  Your patch merely works around
> that.  I think it needs solving properly.

Hi Russell

So the problem here is that CPU and DSA ports should default to up and
at their fastest speed. During setup, the driver is setting the CPU
port to 1G and up. Later on, phylink finds the fixed-link node in DT,
and then sets the port to 100Mbps as requested.

How do you suggest fixing this? If we find a fixed-link, configure it
first down and then up?

     Andrew