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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Sat, Jan 04, 2025 at 10:37:00PM +0100, Tobias Waldekranz wrote:
> On tor, jan 02, 2025 at 17:08, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > On Thu, Jan 02, 2025 at 02:06:32PM +0100, Tobias Waldekranz wrote:
> >> 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?
> >
> > You seem confused with that point, somehow confusing the calls to
> > mac_link_up()/mac_link_down() when using in-band-status with something
> > that a PHY would indicate. No, that's just wrong.
> >
> > If using in-band-status, these calls will be made in response to what
> > the PCS says the link state is, possibly in conjunction with a PHY if
> > there is a PHY present. Whether the PCS state gets forwarded to the MAC
> > is hardware specific, and we have at least one DSA switch where this
> > doesn't appear happen.
> >
> > Please realise that there are _three_ distinct modules here:
> >
> > - The MAC
> > - The PCS
> > - The PHY or media
> 
> Right, I sloppily used "PHY" to refer to the link partner on the other
> end of the SERDES.  I realize that the remote PCS does not have to
> reside within a PHY.

Sigh, it seems I'm not making myself clear.

Host system:

  ---------------------------+
    NIC (or DSA switch port) |
     +-------+    +-------+  |
     |       |    |       |  |
     |  MAC  <---->  PCS  <-----------------------> PHY, SFP or media
     |       |    |       |  |     ^
     +-------+    +-------+  |     |
                             |   phy interface type
  ---------------------------+   also in-band signalling
                                 which managed = "in-band-status"
				 applies to

> E.g. what does it mean to have an SGMII link where in-band signaling is
> not used?  Is that not part of what defines SGMII?

There _are_ PHYs out there that implement Cisco SGMII (which is IEEE
802.3 1000BASE-X modified to allow signalling at 10M and 100M speeds by
symbol replication, and changing the format of the 1000BASE-X to provide
the details of the SGMII link speed and duplex) but do _not_ support
that in-band signalling.

The point of SGMII without in-band signalling rather than just using
1000BASE-X without in-band signalling is that SGMII can operate at
10M and 100M, whereas 1000BASE-X can not.

The usual situation, however, is that most devices that support Cisco
SGMII also allow the in-band signalling to be configured to be used or
not used.


Going back to the diagram above, the link between the MAC and PCS is
_not_ described in DT currently, not by the managed property not by
the phy-modes etc properties.

Now, the port configuration register on the Marvell switches controls
the MAC settings. The PCS has a separate register set (normally
referred to as serdes in Marvell's Switch terminology) which is an
IEEE compliant clause 22 register layout.

The problem is, it seems *some* Marvell switches automatically forward
the PCS status to the MAC. Other switches do not. The DT "managed"
property does not describe this - because - as stated above - the
"managed" property applies to the link between the PCS and external
world (which may be a PHY, or may be media) and _not_ between the
MAC and its associated PCS.

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