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

[Andrew Lunn <andrew@lunn.ch>]

> -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? If the
port is disabled, maybe that configuration will not stick?

The current code in dsa_slave_open() first enables the port, then
calls phylink_start(). So maybe we should keep the ordering the same?
  
> +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...

Apart from this ordering issue, the code looks good.

Thanks
    Andrew