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: <Z9f4W86z90PgtkBc@shell.armlinux.org.uk>
Date: Mon, 17 Mar 2025 10:24:27 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Jim Liu <jim.t90615@...il.com>
Cc: JJLIU0@...oton.com, florian.fainelli@...adcom.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 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.

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