| Message ID | 20250317063452.3072784-1-JJLIU0@nuvoton.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: phy: broadcom: Correct BCM5221 PHY model detection failure | expand |
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.
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.
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@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. 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.
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c index 22edb7e4c1a1..3529289e9d13 100644 --- a/drivers/net/phy/broadcom.c +++ b/drivers/net/phy/broadcom.c @@ -859,7 +859,7 @@ static int brcm_fet_config_init(struct phy_device *phydev) return reg; /* Unmask events we are interested in and mask interrupts globally. */ - if (phydev->phy_id == PHY_ID_BCM5221) + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM5221) reg = MII_BRCM_FET_IR_ENABLE | MII_BRCM_FET_IR_MASK; else @@ -888,7 +888,7 @@ static int brcm_fet_config_init(struct phy_device *phydev) return err; } - if (phydev->phy_id != PHY_ID_BCM5221) { + if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM5221) { /* Set the LED mode */ reg = __phy_read(phydev, MII_BRCM_FET_SHDW_AUXMODE4); if (reg < 0) { @@ -1009,7 +1009,7 @@ static int brcm_fet_suspend(struct phy_device *phydev) return err; } - if (phydev->phy_id == PHY_ID_BCM5221) + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM5221) /* Force Low Power Mode with clock enabled */ reg = BCM5221_SHDW_AM4_EN_CLK_LPM | BCM5221_SHDW_AM4_FORCE_LPM; else
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> --- v2: - add target tree - add maintainer in mail thread - modify checkpatch warning --- drivers/net/phy/broadcom.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)