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: <Z3ZrH9yqtvu2-W7f@shell.armlinux.org.uk>
Date: Thu, 2 Jan 2025 10:31:59 +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
Subject: Re: [PATCH v2 net 3/4] net: dsa: mv88e6xxx: Never force link on
 in-band managed MACs

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

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.

If you have a switch that needs your change, then I think you also need
to adopt the "conservative" approach and only fix the switch you're
working with - short of someone going through all the Marvell DSA
switch datasheets or testing on real hardware, I don't think it's a
good idea to change the behaviour of all Marvell switches like this.

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