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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z3bIF7xaXrje79D8@shell.armlinux.org.uk>
Date: Thu, 2 Jan 2025 17:08:39 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Tobias Waldekranz <tobias@...dekranz.com>
Cc: davem@...emloft.net, kuba@...nel.org, andrew@...n.ch,
	f.fainelli@...il.com, olteanv@...il.com, netdev@...r.kernel.org,
	chris.packham@...iedtelesis.co.nz, pabeni@...hat.com,
	marek.behun@....cz
Subject: Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on
 in-band managed MACs

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@...linux.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@...dekranz.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

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.

Thanks.

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