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:57:36PM +0200, Vladimir Oltean wrote:
> Hi Russell,
> 
> On Thu, 13 Feb 2020 at 16:46, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> 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,
> >
> > 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.
> >
> > --
> > 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
> 
> Correct me if I'm wrong, but if the lack of fixed-link specifier for
> CPU and DSA ports was not a problem before, but has suddenly started
> to become a problem with that patch, then simply reverting to the old
> "legacy" logic from dsa_port_link_register_of() should restore the
> behavior for those device tree blobs that don't specify an explicit
> fixed-link?

That's a good idea, but presumably the change was done in order to
be able to support something, so reverting it may also cause
regressions.  For example, the mv88e6xxx driver has no adjust_link
support anymore as of:

commit 7fb5a711545d7d25fe9726a9ad277474dd83bd06
Author: Hubert Feurstein <h.feurstein@gmail.com>
Date:   Wed Jul 31 17:42:39 2019 +0200

    net: dsa: mv88e6xxx: drop adjust_link to enabled phylink

Since DSA has been whinging about adjust_link being set, I suspect
this is not the only DSA driver to have dropped adjust_link support.

The problem has basically been spotted way too late, and there's
too much on top for a simple revert to work.

> In the longer term, can't we just require fixed-link specifiers for
> non-netdev ports on new boards, keep the adjust_link code in DSA for
> the legacy blobs (which works through some sort of magic), and be done
> with it?

That would be a nice idea - making them behave exactly the same way
as any other network connection, but that is something for the DSA
maintainers to decide.

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