Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on in-band managed MACs

[Tobias Waldekranz <tobias@waldekranz.com>]

On tor, jan 02, 2025 at 10:31, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Thu, Dec 19, 2024 at 01:30:42PM +0100, Tobias Waldekranz wrote:
>> NOTE: This issue was addressed in the referenced commit, but a
>> conservative approach was chosen, where only 6095, 6097 and 6185 got
>> the fix.
>> 
>> Before the referenced commit, in the following setup, when the PHY
>> detected loss of link on the MDI, mv88e6xxx would force the MAC
>> down. If the MDI-side link was then re-established later on, there was
>> no longer any MII link over which the PHY could communicate that
>> information back to the MAC.
>> 
>>         .-SGMII/USXGMII
>>         |
>> .-----. v .-----.   .--------------.
>> | MAC +---+ PHY +---+ MDI (Cu/SFP) |
>> '-----'   '-----'   '--------------'
>> 
>> Since this a generic problem on all MACs connected to a SERDES - which
>> is the only time when in-band-status is used - move all chips to a
>> common mv88e6xxx_port_sync_link() implementation which avoids forcing
>> links on _all_ in-band managed ports.
>> 
>> Fixes: 4efe76629036 ("net: dsa: mv88e6xxx: Don't force link when using in-band-status")
>> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
>
> I'm feeling uneasy about this change.
>
> The history of the patch you refer to is - original v1:
>
> https://lore.kernel.org/r/20201013021858.20530-2-chris.packham@alliedtelesis.co.nz
>
> When v3 was submitted, it was unchanged:
>
> https://lore.kernel.org/r/20201020034558.19438-2-chris.packham@alliedtelesis.co.nz
>
> Both of these applied the in-band-status thing to all Marvell DSA
> switches, but as Marek states here:
>
> https://lore.kernel.org/r/20201020165115.3ecfd601@nic.cz

Thanks for that context!

> doing so breaks last least one Marvell DSA switch (88E6390). Hence why
> this approach is taken, rather than not forcing the link status on all
> DSA switches.
>
> Your patch appears to be reverting us back to what was effectively in
> Chris' v1 patch from back then, so I don't think we can accept this
> change. Sorry.

Before I abandon this broader fix, maybe you can help me understand
something:

If a user explicitly selects `managed = "in-band-status"`, why would we
ever interpret that as "let's force the MAC's settings according to what
the PHY says"? Is that not what `managed = "auto"` is for?

Could Marek's issue not stem from the fact that his DT stated that the
MAC would pick up the link information in-band, when in fact it was
unable to do so?

Looking at the functional spec for the 6390, it only seems to support
in-band-status when SGMII is used. So would it not be more accurate to
require `managed = "auto"` on boards with 6390 SERDES lanes connected to
an SFP cage, if hot-swap support between SGMII/1000BASE-X/2500BASE-X
transceivers is needed?

I.e. users requiring maximum flexibility can still have it, while
someone with a fixed connection to an SGMII PHY can still get the
benefit of using in-band signaling.