linux-kernel - Re: [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure

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: <Z9gjylMFV5zFG-i5@shell.armlinux.org.uk>
Date: Mon, 17 Mar 2025 13:29:46 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Florian Fainelli <florian.fainelli@...adcom.com>
Cc: Jim Liu <jim.t90615@...il.com>, JJLIU0@...oton.com, andrew@...n.ch,
	hkallweit1@...il.com, kuba@...nel.org, edumazet@...gle.com,
	pabeni@...hat.com, netdev@...r.kernel.org,
	giulio.benetti+tekvox@...ettiengineering.com,
	bcm-kernel-feedback-list@...adcom.com, linux-kernel@...r.kernel.org
Subject: Re: [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection
 failure

On Mon, Mar 17, 2025 at 06:18:17AM -0700, Florian Fainelli wrote:
> On 3/17/2025 3:24 AM, Russell King (Oracle) wrote:
> > On Mon, Mar 17, 2025 at 02:34:52PM +0800, Jim Liu wrote:
> > > Use "BRCM_PHY_MODEL" can be applied to the entire 5221 family of PHYs.
> > > 
> > > Fixes: 3abbd0699b67 ("net: phy: broadcom: add support for BCM5221 phy")
> > > Signed-off-by: Jim Liu <jim.t90615@...il.com>
> > 
> > Looking at BRCM_PHY_MODEL() and BRCM_PHY_REV(), I think there's more
> > issues with this driver. E.g.:
> > 
> > #define BRCM_PHY_MODEL(phydev) \
> >          ((phydev)->drv->phy_id & (phydev)->drv->phy_id_mask)
> > 
> > #define BRCM_PHY_REV(phydev) \
> >          ((phydev)->drv->phy_id & ~((phydev)->drv->phy_id_mask))
> > 
> > #define PHY_ID_BCM50610                 0x0143bd60
> > #define PHY_ID_BCM50610M                0x0143bd70
> > 
> >          if ((BRCM_PHY_MODEL(phydev) == PHY_ID_BCM50610 ||
> >               BRCM_PHY_MODEL(phydev) == PHY_ID_BCM50610M) &&
> >              BRCM_PHY_REV(phydev) >= 0x3) {
> > 
> > and from the PHY driver table:
> > 
> >          .phy_id         = PHY_ID_BCM50610,
> >          .phy_id_mask    = 0xfffffff0,
> > 
> >          .phy_id         = PHY_ID_BCM50610M,
> >          .phy_id_mask    = 0xfffffff0,
> > 
> > BRCM_PHY_REV() looks at _this_ .phy_id in the table, and tries to match
> > it against the revision field bits 0-3 being >= 3 - but as we can see,
> > this field is set to the defined value which has bits 0-3 always as
> > zero. So, this if() statement is always false.
> > 
> > So, BRCM_PHY_REV() should be:
> > 
> > #define BRCM_PHY_REV(phydev) \
> > 	((phydev)->phy_id & ~(phydev)->drv->phy_id_mask)
> > 
> > 
> > Next, I question why BRCM_PHY_MODEL() exists in the first place.
> > phydev->drv->phy_id is initialised to the defined value(s), and then
> > we end up doing:
> > 
> > 	(phydev->drv->phy_id & phydev->drv->phy_id_mask) ==
> > 		one-of-those-defined-values
> > 
> > which is pointless, because we know that what is in phydev->drv->phy_id
> > /is/ one-of-those-defined-values.
> > 
> > Therefore, I would suggest:
> > 
> > #define BRCM_PHY_MODEL(phydev) ((phydev)->drv->phy_id)
> > 
> > is entirely sufficient, and with such a simple definition, I question
> > the value of BRCM_PHY_MODEL() existing.
> 
> If I were to make a guess, BRCM_PHY_MODEL() might have existed to ease the
> porting of a non-Linux PHY driver to a Linux PHY driver environment at a
> time where the subsystem was not as mature as it is now.
> 
> In the interest of a targeted bug fix, I would be keen on taking this patch
> in its current form and follow up in net next with a removal of
> BRCM_PHY_MODEL() later on.

Note that commit 32e5a8d651c0 ("tg3 / broadcom: Add code to disable rxc
refclk") is still wrong (which introduced BRCM_PHY_REV().)

Given its age, I would suggest that the commit I reference above was
not properly tested, and *possibly* fixing the bug might actually
cause a regression on TG3.

Also, the original commit description (which references RXC) which is
supposed to be synchronous to the received data, but the code talks
about CLK125 which is *technically* a different clock.

We know that the current driver logic works, even though it doesn't
do what the original author of the code wanted it to do.

Taking these three points together, I don't think it would be wise
to fix the logical error (and actually test the revision field).
Instead, I think getting rid of the always-false if() and simplifying
the code would be better to avoid causing a possible regression on
TG3.

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