Re: [PATCH net] net: dsa: fix phylink_start()/phylink_stop() calls

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

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