Re: [PATCH net-next v3 02/10] net: mvpp2: phylink support

[Antoine Tenart]

Hello Russell,

On Mon, Aug 27, 2018 at 05:50:59PM +0100, Russell King - ARM Linux wrote:
> On Thu, May 17, 2018 at 10:29:31AM +0200, Antoine Tenart wrote:
> > +static void mvpp2_mac_config(struct net_device *dev, unsigned int mode,
> > +			     const struct phylink_link_state *state)
> > +{
> > +	struct mvpp2_port *port = netdev_priv(dev);
> > +
> > +	/* Check for invalid configuration */
> > +	if (state->interface == PHY_INTERFACE_MODE_10GKR && port->gop_id != 0) {
> > +		netdev_err(dev, "Invalid mode on %s\n", dev->name);
> > +		return;
> > +	}
> > +
> > +	netif_tx_stop_all_queues(port->dev);
> > +	if (!port->has_phy)
> > +		netif_carrier_off(port->dev);
> ...
> > +	/* If the port already was up, make sure it's still in the same state */
> > +	if (state->link || !port->has_phy) {
> > +		mvpp2_port_enable(port);
> > +
> > +		mvpp2_egress_enable(port);
> > +		mvpp2_ingress_enable(port);
> > +		if (!port->has_phy)
> > +			netif_carrier_on(dev);
> > +		netif_tx_wake_all_queues(dev);
> > +	}
> 
> The above appears to cause a problem when testing on a singleshot
> mcbin board - as a direct result of the above, I find that _all_
> the network devices apparently have link, including those which
> have no SFP inserted into the cage.  This results in debian jessie
> hanging at boot for over 1 minute while systemd tries to bring up
> all network devices.

OK, we should fix that. Removing the above code does work for most of
the configurations, including the double shot MacchiatoBin.

> Have you identified a case where mac_config() is called with the
> link up for which a major reconfiguration is required?  mac_config()
> will get called for small reconfigurations such as changing the
> pause parameters (which should _not_ require the queues to be
> restarted) but the link should always be down for major
> reconfigurations such sa changing the PHY interface mode.

With the above code remove one case did not worked anymore: when the
port is configured as a fixed-link because the SFP cage can't be
described and used (on the 7040-db and 8040-db boards). In such cases
phylink is called, mac_config() is called, but link_up() is never
called. I'm not sure this is actually an issue in phylink, but the PPv2
driver should probably take care of this weird case itself (by calling
explicitly link_up()). What do you think?

> mac_config() can also be called when nothing has changed - if we've
> decided to re-run the link state resolve, it can be called with the
> same parameters as previously, especially now that we have polling
> of a GPIO for link state monitoring.  With your code above, this will
> cause mvpp2 to down/up the carrier state every second.

OK, that's bad.

> Note that state->link here is the _future_ state of the link, which
> may change state before the mac_link_up()/down() functions have been
> called - turning the carrier on/off like this will prevent these
> callbacks being made, and the link state being printed by phylink.

OK. Hopefully the fix will drop all use of state->link.


I've made a patch (see below) to stop messing with the carrier link
state in mac_config(), while also fixing the fixed-link handling
directly from within the PPv2 driver. If the patch is OK for you, I'll
sent it upstream to the net tree.

-- >8 --