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

Florian Fainelli
2025-03-17T13:18:21Z

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@gmail.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