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

[Florian Fainelli]

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@xxxxxxxxx>
>
> 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