Re: mv88e6240 configuration broken for B850v3

On Mon, Dec 06, 2021 at 11:44:28PM +0200, Vladimir Oltean wrote:

On Mon, Dec 06, 2021 at 08:18:30PM +0000, Russell King (Oracle) wrote:

quoted

quoted

If we're going to impersonate phylink we could at least provide the same
arguments as phylink will.

What is going on here in terms of impersonation is entirely reasonable.

The only things in this respect that phylink guarantees are:

1) The MAC/PCS configuration will not be substantially reconfigured
   unless a call to mac_link_down() was made if a call to mac_link_up()
   was previously made.

The wording here is unclear. Did you mean "When the MAC/PCS configuration
is substantially reconfigured and the last call was a mac_link_up(), a
follow-up call to mac_link_down() will also be made"?
And what do you mean by "substantially reconfigured"?

I mean what the documentation refers to as a "full initialisation" of
the link, which happens if we have to change the interface mode or
MLO_AN mode. Only minor changes (advertisements or pause modes if under
manual control) will be changed without a prior call to mac_link_down().

For example, phylink will _never_ do:

mac_link_up()
mac_prepare()
mac_config()
pcs_config()
mac_finish()
mac_link_up()

However, with legacy (pre-March 2020) users, it _may_ do:

mac_link_up()
mac_config() (e.g. changing the in-band advertisement)
mac_an_restart()

The problem here is the legacy stuff which clouds the picture by making
extra calls to mac_config() when we're only changing things like the
in-band advert.

phylink_major_config called from the paths that aren't phylink_mac_initial_config
(because that happens with no preceding call to either mac_link_down or
mac_link_up), right?

Where we call phylink_major_config(), we ensure that if we know the link
was previously up, we make a call to mac_link_down() first.

quoted

2) The arguments to mac_link_down() will be the same as the preceeding
   mac_link_up() call - in other words, the "mode" and "interface".

Does this imply that "there will always be a preceding mac_link_up to
every mac_link_down call"?

From the design of phylink, yes it does, since phylink assumes that the
link is initially down.

Because if it does imply that, DSA violates it.

Yes, DSA violates that, but DSA is free to do that if it makes sense,
and from what I understand of DSA, this is a special case. From what
I've seen with Marvell DSA, they start off auto-configuring the
inter-switch and CPU ports in link-up mode, whereas phylink assumes
that the link is down.

When DSA sets stuff up and brings up the CPU port, the very first
thing that phylink does is reconfigure the port - but the port is in
link-up mode, and that can mess up the port. So Andrew introduced
this to fix it. See commit 3be98b2d5fbc.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!