netdev - Re: [PATCH net] net: dsa: mv88e6xxx: Drop serdes methods for 88E6172

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>] [day] [month] [year] [list]

Message-ID: <2m2phdcy4pdij7vbi4kknu42knjwq24cgjwc7iogfeyqgqkjk7@elwngvs6bsni>
Date: Tue, 13 Aug 2024 13:16:00 +0200
From: Marek Behún <kabel@...nel.org>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Marek Behún <kabel@...nel.org>, 
	Andrew Lunn <andrew@...n.ch>, Florian Fainelli <f.fainelli@...il.com>, 
	Vladimir Oltean <olteanv@...il.com>, "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: dsa: mv88e6xxx: Drop serdes methods for 88E6172

On Mon, Aug 12, 2024 at 03:48:27PM +0100, Russell King (Oracle) wrote:
> On Sun, Aug 11, 2024 at 06:23:29PM +0200, Marek Behún wrote:
> > Drop serdes methods for 88E6172. This switch from the 6352 family does
> > not have serdes. Until commit 85764555442f ("net: dsa: mv88e6xxx:
> > convert 88e6352 to phylink_pcs") these methods were checking for serdes
> > presence by looking at port's cmode, but in that commit the check was
> > dropped, so now the nonexistent serdes registers are being accessed.
> 
> This attributes blame incorrectly, and shows a lack of understanding.
> No, one can *not* discover presence of the serdes "by looking at the
> port's cmode" - that is total rubbish.
> 
> The MV88E6352 family supports ports that can auto-select between the
> integrated baseT PHY and the serdes depending on which has link. The
> port CMODE tells us *nothing* about whether the port supports serdes
> or not.
> 
> Please fix your description not to spread misinformation, and
> incorrectly indirectly blame others for stuff that is not appropriate.
> 
> Thanks.

Hi,

sorry about this.

It seems that the 6352 family was always written with serdes
support for the whole family, 6172 included, even before the switch
operations structure was introduced.

But it really did check whether it should touch serdes registers by
checking cmode:
  in commit 85764555442f~1 the function mv88e6352_serdes_get_lane()
  checks the port's cmode:
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/mv88e6xxx/serdes.c?id=4aabe35c385ce6c28613ab56b334b4a9521d62b7#n260

  then in chip.c this is used in
    mv88e6xxx_serdes_pcs_get_state
    mv88e6xxx_serdes_pcs_config
    mv88e6xxx_serdes_pcs_an_restart
    mv88e6xxx_serdes_pcs_link_up
    mv88e6xxx_serdes_irq_thread_fn
    mv88e6xxx_serdes_power

  and even before, in commit 10fa5bfcd697, it was decided by looking
  at cmode whether serdes should be powered on
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/mv88e6xxx/chip.c?id=10fa5bfcd697#n1885

I propose dropping the Fixes tag and having just this in the commit
message:
  Drop serdes methods for 88E6172. This switch from the 6352 family does
  not have serdes.

What do you think?

Marek