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: <9391fb55-11c4-4fa9-b38f-500cb1ae325c@broadcom.com>
Date: Mon, 17 Mar 2025 06:18:17 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
 Jim Liu <jim.t90615@...il.com>
Cc: 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 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.
-- 
Florian