netdev - Re: [PATCH net] net: dsa: fix phylink_start()/phylink

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: <20200229164538.GB25745@shell.armlinux.org.uk>
Date:   Sat, 29 Feb 2020 16:45:39 +0000
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Vladimir Oltean <olteanv@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: fix phylink_start()/phylink_stop() calls

On Sat, Feb 29, 2020 at 04:42:15PM +0100, Andrew Lunn wrote:
> > -int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
> > +int dsa_port_enable_locked(struct dsa_port *dp, struct phy_device *phy)
> >  {
> >  	struct dsa_switch *ds = dp->ds;
> >  	int port = dp->index;
> >  	int err;
> >  
> > +	if (dp->pl)
> > +		phylink_start(dp->pl);
> > +
> >  	if (ds->ops->port_enable) {
> >  		err = ds->ops->port_enable(ds, port, phy);
> >  		if (err)
> > @@ -81,7 +84,18 @@ int dsa_port_enable(struct dsa_port *dp, struct phy_device *phy)
> >  	return 0;
> >  }
> 
> Hi Russell
> 
> I'm wondering about the order here. You are starting phylink before
> the port is actually enabled in the hardware. Could phylink_start()
> result in synchronous calls into the MAC to configure the port?

Yes, that's possible.

> If the port is disabled, maybe that configuration will not stick?

No idea...

> The current code in dsa_slave_open() first enables the port, then
> calls phylink_start(). So maybe we should keep the ordering the same?

However, dsa_port_setup() does it in the reverse order, so it was a
bit of guess work which is right.  So, if the port needs to be enabled
first, then the dsa_port_setup() path for DSA and CPU ports is wrong.

It's not clear what dsa_port_enable() actually does, and should a port
be enabled before its interface mode and link parameters have been
set?

> > +void dsa_port_disable_locked(struct dsa_port *dp)
> >  {
> >  	struct dsa_switch *ds = dp->ds;
> >  	int port = dp->index;
> > @@ -91,6 +105,16 @@ void dsa_port_disable(struct dsa_port *dp)
> >  
> >  	if (ds->ops->port_disable)
> >  		ds->ops->port_disable(ds, port);
> > +
> > +	if (dp->pl)
> > +		phylink_stop(dp->pl);
> > +}
> 
> The current code first stops phylink, then disables the port...

It depends what order is the correct one, which depends on what
port_disable() does vs phylink_stop(), and whether the network
queues should be stopped before or after port_disable().

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up