Re: Heads up: phylink changes for next merge window

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

On Thu, Feb 13, 2020 at 05:16:02PM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Feb 13, 2020 at 05:00:04PM +0100, Andrew Lunn wrote:
> > On Thu, Feb 13, 2020 at 02:46:16PM +0000, Russell King - ARM Linux admin wrote:
> > > [Recipient list updated; removed addresses that bounce, added Ioana
> > > Ciornei for dpaa2 and DSA issue mentioned below.]
> > > 
> > > On Thu, Feb 13, 2020 at 01:38:31PM +0000, Russell King - ARM Linux admin wrote:
> > > > Hi,
> > > > 
> > > > During the next round of development changes, I wish to make some
> > > > changes to phylink which will affect almost every user out there,
> > > > as it affects the interfaces to the MAC drivers.
> > > > 
> > > > The reason behind the change is to allow us to support situations
> > > > where the MAC is not closely coupled with its associated PCS, such
> > > > as is found in mvneta and mvpp2.  This is necessary to support
> > > > already existing hardware properly, such as Marvell DSA and Xilinx
> > > > AXI ethernet drivers, and there seems to be a growing need for this.
> > > > 
> > > > What I'm proposing to do is to move the MAC setup for the negotiated
> > > > speed, duplex and pause settings to the mac_link_up() method, out of
> > > > the existing mac_config() method.  I have already converted the
> > > > axienet, dpaa2-mac, macb, mvneta, mvpp2 and mv88e6xxx (dsa) drivers,
> > > > but I'm not able to test all those.  Thus far, I've tested dpaa2-mac,
> > > > mvneta, and mv88e6xxx.  There's a bunch of other drivers that I don't
> > > > know enough about the hardware to do the conversion myself.
> > > 
> > > I should also have pointed out that with mv88e6xxx, the patch
> > > "net: mv88e6xxx: use resolved link config in mac_link_up()" "fixes" by
> > > side-effect an issue that Andrew has mentioned, where inter-DSA ports
> > > get configured down to 10baseHD speed.  This is by no means a true fix
> > > for that problem - which is way deeper than this series can address.
> > > The reason it fixes it is because we no longer set the speed/duplex
> > > in mac_config() but set it in mac_link_up() - but mac_link_up() is
> > > never called for CPU and DSA ports.
> > > 
> > > However, I think there may be another side-effect of that - any fixed
> > > link declaration in DT may not be respected after this patch.
> > > 
> > > I believe the root of this goes back to this commit:
> > > 
> > >   commit 0e27921816ad99f78140e0c61ddf2bc515cc7e22
> > >   Author: Ioana Ciornei <ioana.ciornei@nxp.com>
> > >   Date:   Tue May 28 20:38:16 2019 +0300
> > > 
> > >   net: dsa: Use PHYLINK for the CPU/DSA ports
> > > 
> > > and, in the case of no fixed-link declaration, phylink has no idea what
> > > the link parameters should be (and hence the initial bug, where
> > > mac_config gets called with speed=0 duplex=0, which gets interpreted as
> > > 10baseHD.)  Moreover, as far as phylink is concerned, these links never
> > > come up. Essentially, this commit was not fully tested with inter-DSA
> > > links, and probably was never tested with phylink debugging enabled.
> > > 
> > > There is currently no fix for this, and it is not an easy problem to
> > > resolve, irrespective of the patches I'm proposing.
> > 
> > Hi Russell
> > 
> > I've been playing around with this a bit. I have a partial fix:
> > 
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index 9b54e5a76297..dc4da4dc44f5 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -629,9 +629,14 @@ static int dsa_port_phylink_register(struct dsa_port *dp)
> >  int dsa_port_link_register_of(struct dsa_port *dp)
> >  {
> >         struct dsa_switch *ds = dp->ds;
> > +       struct device_node *phy_np;
> >  
> > -       if (!ds->ops->adjust_link)
> > -               return dsa_port_phylink_register(dp);
> > +       if (!ds->ops->adjust_link) {
> > +               phy_np = of_parse_phandle(dp->dn, "phy-handle", 0);
> > +               if (of_phy_is_fixed_link(dp->dn) || phy_np)
> > +                       return dsa_port_phylink_register(dp);
> > +               return 0;
> > +       }
> >  
> >         dev_warn(ds->dev,
> >                  "Using legacy PHYLIB callbacks. Please migrate to PHYLINK!\n");
> > @@ -646,11 +651,12 @@ void dsa_port_link_unregister_of(struct dsa_port *dp)
> >  {
> >         struct dsa_switch *ds = dp->ds;
> >  
> > -       if (!ds->ops->adjust_link) {
> > +       if (!ds->ops->adjust_link && dp->pl) {
> >                 rtnl_lock();
> >                 phylink_disconnect_phy(dp->pl);
> >                 rtnl_unlock();
> >                 phylink_destroy(dp->pl);
> > +               dp->pl = NULL;
> >                 return;
> >         }
> > 
> > So basically only instantiate phylink if there is a fixed-link
> > property, or a phy-handle.
> > 
> > What i think is still broken is if there is a phy-mode property, and
> > nothing else. e.g. to set RGMII delays. I think that will get ignored.
> 
> Can you please verify that mac_link_up() gets called for these if
> there is a fixed-link property or phy-handle?
> 
> Also, there is another way around this, which is for phylink_create()
> to callback through the mac_ops to request the default configuration.
> That could be plumbed down through the various DSA layers such that
> the old "max speed / max link" business could be setup.  However,
> that brings with it a new problem: if we default to a fixed-link, then
> attempting to connect a phy later will be ignored.  However, deferring
> the default create-time configuration setup to phylink_start() would
> work around it, but brings with it a bit more complexity.

Hmm.

Andrew, it is possible that what I have in the ZII branch does end
up fixing this problem by accident without requiring any further
code changes in DSA.

If a node has no fixed-link nor PHY, phylink defaults to MLO_AN_PHY
mode - and without a PHY, phylink will never believe that the link
has come up.  Hence, mac_link_up() will never be called.

If a node has a fixed-link property, then phylink will parse it,
and by default, fixed-links without GPIOs or a callback function
will come up at initialisation.  Hence, we get a mac_link_up()
call with the fixed-link parameters.

If a node has a PHY, then DSA will instruct phylink to connect to
it, and it will behave as any other PHY-attached MAC, calling
mac_link_up() at the appropriate time.

The mv88e6xxx driver by default configures CPU and DSA ports to
maximum speed, which is what we get if mac_link_up() is never
called.  On the other hand, if there's a fixed-link or PHY, we'll
call mac_link_up() to program those parameters.

So, provided all drivers are converted to program the speed/duplex
etc in mac_link_up(), we might not have a problem at all - and
that would be an additional motivation for everyone to update to
the changes I've proposed!

What I don't like - if the above is correct - is that this is
merely coincidental.

Now, while that sounds great, something's niggling me about it.
If the link is forced up by the DSA driver initialisation defaulting
the speed to maximum speed, but phylink believes the link to be down,
and then we get a mac_link_up() call, with the patches as they stand
we end up changing the port configuration while the link is up, which
I'm sure I've read shouldn't be done in some of the DSA switch
functional specs.  Hmm.

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