On Sat, Feb 29, 2020 at 04:42:15PM +0100, Andrew Lunn wrote:

quoted

-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?

quoted

+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