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

> and the managed property is about whether in-band signalling is used
> from the PCS towards the media, not from the PCS towards the MAC.
>
> So, if the MAC doesn't get updated with the PCS' link state, then
> mac_link_up()/mac_link_down() need to do that manually, even if the
> link from the PCS towards the media is using in-band signalling.
>
> I think you're confusing in-band-status as meaning that the MAC
> gets automatically updated with the PCS media-side link state -
> the DT property has no bearing on that.

If `managed` does not declare a hardware capability of the controller,
then what information does it convey that is not already present in the
`phy-connection-type`?

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?

I.e. could you provide an example `$TYPE`, where both of the following
configs are valid, and what the difference between the two would be?

    &eth0 {
        phy-connection-type = "$TYPE";
        managed = "auto";
    };

    &eth0 {
        phy-connection-type = "$TYPE";
        managed = "in-band-status";
    };

> Thanks.

Thank you for taking the time to explain!