[PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value

[PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value

[Heiner Kallweit]

We have only two users of fixed_phy_add(), both use address 0 and
ignore the return value. So simplify fixed_phy_add() accordingly.

Whilst at it, constify the fixed_phy_status configs.

Note:
fixed_phy_add() is a legacy function which shouldn't be used in new
code, as it's use may be problematic:
- No check whether a fixed phy exists already at the given address
- If fixed_phy_register() is called afterwards by any other driver,
  then it will also use phy_addr 0, because fixed_phy_add() ignores
  the ida which manages address assignment
Drivers using a fixed phy created by fixed_phy_add() in platform code,
should dynamically create a fixed phy with fixed_phy_register()
instead.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 arch/m68k/coldfire/m5272.c  | 4 ++--
 arch/mips/bcm47xx/setup.c   | 4 ++--
 drivers/net/phy/fixed_phy.c | 4 ++--
 include/linux/phy_fixed.h   | 8 ++------
 4 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/m68k/coldfire/m5272.c b/arch/m68k/coldfire/m5272.c
index 5b70dfdab..918e2a323 100644
--- a/arch/m68k/coldfire/m5272.c
+++ b/arch/m68k/coldfire/m5272.c
@@ -108,7 +108,7 @@ void __init config_BSP(char *commandp, int size)
  * an ethernet switch. In this case we need to use the fixed phy type,
  * and we need to declare it early in boot.
  */
-static struct fixed_phy_status nettel_fixed_phy_status __initdata = {
+static const struct fixed_phy_status nettel_fixed_phy_status __initconst = {
 	.link	= 1,
 	.speed	= 100,
 	.duplex	= 0,
@@ -119,7 +119,7 @@ static struct fixed_phy_status nettel_fixed_phy_status __initdata = {
 static int __init init_BSP(void)
 {
 	m5272_uarts_init();
-	fixed_phy_add(0, &nettel_fixed_phy_status);
+	fixed_phy_add(&nettel_fixed_phy_status);
 	clkdev_add_table(m5272_clk_lookup, ARRAY_SIZE(m5272_clk_lookup));
 	return 0;
 }
diff --git a/arch/mips/bcm47xx/setup.c b/arch/mips/bcm47xx/setup.c
index de426a474..a93a4266d 100644
--- a/arch/mips/bcm47xx/setup.c
+++ b/arch/mips/bcm47xx/setup.c
@@ -256,7 +256,7 @@ static int __init bcm47xx_cpu_fixes(void)
 }
 arch_initcall(bcm47xx_cpu_fixes);
 
-static struct fixed_phy_status bcm47xx_fixed_phy_status __initdata = {
+static const struct fixed_phy_status bcm47xx_fixed_phy_status __initconst = {
 	.link	= 1,
 	.speed	= SPEED_100,
 	.duplex	= DUPLEX_FULL,
@@ -282,7 +282,7 @@ static int __init bcm47xx_register_bus_complete(void)
 	bcm47xx_leds_register();
 	bcm47xx_workarounds();
 
-	fixed_phy_add(0, &bcm47xx_fixed_phy_status);
+	fixed_phy_add(&bcm47xx_fixed_phy_status);
 	return 0;
 }
 device_initcall(bcm47xx_register_bus_complete);
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 7902b35c5..b39532abf 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -153,9 +153,9 @@ static int fixed_phy_add_gpiod(unsigned int irq, int phy_addr,
 	return 0;
 }
 
-int fixed_phy_add(int phy_addr, const struct fixed_phy_status *status)
+void fixed_phy_add(const struct fixed_phy_status *status)
 {
-	return fixed_phy_add_gpiod(PHY_POLL, phy_addr, status, NULL);
+	fixed_phy_add_gpiod(PHY_POLL, 0, status, NULL);
 }
 EXPORT_SYMBOL_GPL(fixed_phy_add);
 
diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
index 5399b9e41..6227a1bde 100644
--- a/include/linux/phy_fixed.h
+++ b/include/linux/phy_fixed.h
@@ -17,7 +17,7 @@ struct net_device;
 
 #if IS_ENABLED(CONFIG_FIXED_PHY)
 extern int fixed_phy_change_carrier(struct net_device *dev, bool new_carrier);
-int fixed_phy_add(int phy_id, const struct fixed_phy_status *status);
+void fixed_phy_add(const struct fixed_phy_status *status);
 struct phy_device *fixed_phy_register(const struct fixed_phy_status *status,
 				      struct device_node *np);
 
@@ -26,11 +26,7 @@ extern int fixed_phy_set_link_update(struct phy_device *phydev,
 			int (*link_update)(struct net_device *,
 					   struct fixed_phy_status *));
 #else
-static inline int fixed_phy_add(int phy_id,
-				const struct fixed_phy_status *status)
-{
-	return -ENODEV;
-}
+static inline void fixed_phy_add(const struct fixed_phy_status *status) {}
 static inline struct phy_device *
 fixed_phy_register(const struct fixed_phy_status *status,
 		   struct device_node *np)
-- 
2.50.1

Re: [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value

[Hauke Mehrtens]

On 8/22/25 22:36, Heiner Kallweit wrote:
> We have only two users of fixed_phy_add(), both use address 0 and
> ignore the return value. So simplify fixed_phy_add() accordingly.
> 
> Whilst at it, constify the fixed_phy_status configs.
> 
> Note:
> fixed_phy_add() is a legacy function which shouldn't be used in new
> code, as it's use may be problematic:
> - No check whether a fixed phy exists already at the given address
> - If fixed_phy_register() is called afterwards by any other driver,
>    then it will also use phy_addr 0, because fixed_phy_add() ignores
>    the ida which manages address assignment
> Drivers using a fixed phy created by fixed_phy_add() in platform code,
> should dynamically create a fixed phy with fixed_phy_register()
> instead.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>   arch/m68k/coldfire/m5272.c  | 4 ++--
>   arch/mips/bcm47xx/setup.c   | 4 ++--
>   drivers/net/phy/fixed_phy.c | 4 ++--
>   include/linux/phy_fixed.h   | 8 ++------
>   4 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/m68k/coldfire/m5272.c b/arch/m68k/coldfire/m5272.c
> index 5b70dfdab..918e2a323 100644
> --- a/arch/m68k/coldfire/m5272.c
> +++ b/arch/m68k/coldfire/m5272.c
> @@ -108,7 +108,7 @@ void __init config_BSP(char *commandp, int size)
>    * an ethernet switch. In this case we need to use the fixed phy type,
>    * and we need to declare it early in boot.
>    */
> -static struct fixed_phy_status nettel_fixed_phy_status __initdata = {
> +static const struct fixed_phy_status nettel_fixed_phy_status __initconst = {
>   	.link	= 1,
>   	.speed	= 100,
>   	.duplex	= 0,
> @@ -119,7 +119,7 @@ static struct fixed_phy_status nettel_fixed_phy_status __initdata = {
>   static int __init init_BSP(void)
>   {
>   	m5272_uarts_init();
> -	fixed_phy_add(0, &nettel_fixed_phy_status);
> +	fixed_phy_add(&nettel_fixed_phy_status);
>   	clkdev_add_table(m5272_clk_lookup, ARRAY_SIZE(m5272_clk_lookup));
>   	return 0;
>   }
> diff --git a/arch/mips/bcm47xx/setup.c b/arch/mips/bcm47xx/setup.c
> index de426a474..a93a4266d 100644
> --- a/arch/mips/bcm47xx/setup.c
> +++ b/arch/mips/bcm47xx/setup.c
> @@ -256,7 +256,7 @@ static int __init bcm47xx_cpu_fixes(void)
>   }
>   arch_initcall(bcm47xx_cpu_fixes);
>   
> -static struct fixed_phy_status bcm47xx_fixed_phy_status __initdata = {
> +static const struct fixed_phy_status bcm47xx_fixed_phy_status __initconst = {
>   	.link	= 1,
>   	.speed	= SPEED_100,
>   	.duplex	= DUPLEX_FULL,
> @@ -282,7 +282,7 @@ static int __init bcm47xx_register_bus_complete(void)
>   	bcm47xx_leds_register();
>   	bcm47xx_workarounds();
>   
> -	fixed_phy_add(0, &bcm47xx_fixed_phy_status);
> +	fixed_phy_add(&bcm47xx_fixed_phy_status);
>   	return 0;
>   }
>   device_initcall(bcm47xx_register_bus_complete);
> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
> index 7902b35c5..b39532abf 100644
> --- a/drivers/net/phy/fixed_phy.c
> +++ b/drivers/net/phy/fixed_phy.c
> @@ -153,9 +153,9 @@ static int fixed_phy_add_gpiod(unsigned int irq, int phy_addr,
>   	return 0;
>   }
>   
> -int fixed_phy_add(int phy_addr, const struct fixed_phy_status *status)
> +void fixed_phy_add(const struct fixed_phy_status *status)
>   {
> -	return fixed_phy_add_gpiod(PHY_POLL, phy_addr, status, NULL);
> +	fixed_phy_add_gpiod(PHY_POLL, 0, status, NULL);
>   }
>   EXPORT_SYMBOL_GPL(fixed_phy_add);
>   
> diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
> index 5399b9e41..6227a1bde 100644
> --- a/include/linux/phy_fixed.h
> +++ b/include/linux/phy_fixed.h
> @@ -17,7 +17,7 @@ struct net_device;
>   
>   #if IS_ENABLED(CONFIG_FIXED_PHY)
>   extern int fixed_phy_change_carrier(struct net_device *dev, bool new_carrier);
> -int fixed_phy_add(int phy_id, const struct fixed_phy_status *status);
> +void fixed_phy_add(const struct fixed_phy_status *status);
>   struct phy_device *fixed_phy_register(const struct fixed_phy_status *status,
>   				      struct device_node *np);
>   
> @@ -26,11 +26,7 @@ extern int fixed_phy_set_link_update(struct phy_device *phydev,
>   			int (*link_update)(struct net_device *,
>   					   struct fixed_phy_status *));
>   #else
> -static inline int fixed_phy_add(int phy_id,
> -				const struct fixed_phy_status *status)
> -{
> -	return -ENODEV;
> -}
> +static inline void fixed_phy_add(const struct fixed_phy_status *status) {}
>   static inline struct phy_device *
>   fixed_phy_register(const struct fixed_phy_status *status,
>   		   struct device_node *np)

Hi,

I do not use this hardware any more, but Takumi reported that 
fixed_phy_add() is not working for the PHY registration on brcm47xx any 
more and we have to use fixed_phy_register(), see:
https://github.com/openwrt/openwrt/pull/19610

Does this need a bigger refactoring anyway?

Hauke

Re: [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value

[Heiner Kallweit]

On 8/24/2025 6:05 PM, Hauke Mehrtens wrote:
> On 8/22/25 22:36, Heiner Kallweit wrote:
>> We have only two users of fixed_phy_add(), both use address 0 and
>> ignore the return value. So simplify fixed_phy_add() accordingly.
>>
>> Whilst at it, constify the fixed_phy_status configs.
>>
>> Note:
>> fixed_phy_add() is a legacy function which shouldn't be used in new
>> code, as it's use may be problematic:
>> - No check whether a fixed phy exists already at the given address
>> - If fixed_phy_register() is called afterwards by any other driver,
>>    then it will also use phy_addr 0, because fixed_phy_add() ignores
>>    the ida which manages address assignment
>> Drivers using a fixed phy created by fixed_phy_add() in platform code,
>> should dynamically create a fixed phy with fixed_phy_register()
>> instead.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>   arch/m68k/coldfire/m5272.c  | 4 ++--
>>   arch/mips/bcm47xx/setup.c   | 4 ++--
>>   drivers/net/phy/fixed_phy.c | 4 ++--
>>   include/linux/phy_fixed.h   | 8 ++------
>>   4 files changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/m68k/coldfire/m5272.c b/arch/m68k/coldfire/m5272.c
>> index 5b70dfdab..918e2a323 100644
>> --- a/arch/m68k/coldfire/m5272.c
>> +++ b/arch/m68k/coldfire/m5272.c
>> @@ -108,7 +108,7 @@ void __init config_BSP(char *commandp, int size)
>>    * an ethernet switch. In this case we need to use the fixed phy type,
>>    * and we need to declare it early in boot.
>>    */
>> -static struct fixed_phy_status nettel_fixed_phy_status __initdata = {
>> +static const struct fixed_phy_status nettel_fixed_phy_status __initconst = {
>>       .link    = 1,
>>       .speed    = 100,
>>       .duplex    = 0,
>> @@ -119,7 +119,7 @@ static struct fixed_phy_status nettel_fixed_phy_status __initdata = {
>>   static int __init init_BSP(void)
>>   {
>>       m5272_uarts_init();
>> -    fixed_phy_add(0, &nettel_fixed_phy_status);
>> +    fixed_phy_add(&nettel_fixed_phy_status);
>>       clkdev_add_table(m5272_clk_lookup, ARRAY_SIZE(m5272_clk_lookup));
>>       return 0;
>>   }
>> diff --git a/arch/mips/bcm47xx/setup.c b/arch/mips/bcm47xx/setup.c
>> index de426a474..a93a4266d 100644
>> --- a/arch/mips/bcm47xx/setup.c
>> +++ b/arch/mips/bcm47xx/setup.c
>> @@ -256,7 +256,7 @@ static int __init bcm47xx_cpu_fixes(void)
>>   }
>>   arch_initcall(bcm47xx_cpu_fixes);
>>   -static struct fixed_phy_status bcm47xx_fixed_phy_status __initdata = {
>> +static const struct fixed_phy_status bcm47xx_fixed_phy_status __initconst = {
>>       .link    = 1,
>>       .speed    = SPEED_100,
>>       .duplex    = DUPLEX_FULL,
>> @@ -282,7 +282,7 @@ static int __init bcm47xx_register_bus_complete(void)
>>       bcm47xx_leds_register();
>>       bcm47xx_workarounds();
>>   -    fixed_phy_add(0, &bcm47xx_fixed_phy_status);
>> +    fixed_phy_add(&bcm47xx_fixed_phy_status);
>>       return 0;
>>   }
>>   device_initcall(bcm47xx_register_bus_complete);
>> diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
>> index 7902b35c5..b39532abf 100644
>> --- a/drivers/net/phy/fixed_phy.c
>> +++ b/drivers/net/phy/fixed_phy.c
>> @@ -153,9 +153,9 @@ static int fixed_phy_add_gpiod(unsigned int irq, int phy_addr,
>>       return 0;
>>   }
>>   -int fixed_phy_add(int phy_addr, const struct fixed_phy_status *status)
>> +void fixed_phy_add(const struct fixed_phy_status *status)
>>   {
>> -    return fixed_phy_add_gpiod(PHY_POLL, phy_addr, status, NULL);
>> +    fixed_phy_add_gpiod(PHY_POLL, 0, status, NULL);
>>   }
>>   EXPORT_SYMBOL_GPL(fixed_phy_add);
>>   diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h
>> index 5399b9e41..6227a1bde 100644
>> --- a/include/linux/phy_fixed.h
>> +++ b/include/linux/phy_fixed.h
>> @@ -17,7 +17,7 @@ struct net_device;
>>     #if IS_ENABLED(CONFIG_FIXED_PHY)
>>   extern int fixed_phy_change_carrier(struct net_device *dev, bool new_carrier);
>> -int fixed_phy_add(int phy_id, const struct fixed_phy_status *status);
>> +void fixed_phy_add(const struct fixed_phy_status *status);
>>   struct phy_device *fixed_phy_register(const struct fixed_phy_status *status,
>>                         struct device_node *np);
>>   @@ -26,11 +26,7 @@ extern int fixed_phy_set_link_update(struct phy_device *phydev,
>>               int (*link_update)(struct net_device *,
>>                          struct fixed_phy_status *));
>>   #else
>> -static inline int fixed_phy_add(int phy_id,
>> -                const struct fixed_phy_status *status)
>> -{
>> -    return -ENODEV;
>> -}
>> +static inline void fixed_phy_add(const struct fixed_phy_status *status) {}
>>   static inline struct phy_device *
>>   fixed_phy_register(const struct fixed_phy_status *status,
>>              struct device_node *np)
> 
> Hi,
> 
> I do not use this hardware any more, but Takumi reported that fixed_phy_add() is not working for the PHY registration on brcm47xx any more and we have to use fixed_phy_register(), see:
> https://github.com/openwrt/openwrt/pull/19610
> 
I suspected already that fixed_phy_add() usage doesn't work any longer, but don't
have hw to test. I think bfa54812f0bc ("net: phy: fixed_phy: set phy_mask before
calling mdiobus_register()") is to blame.
Reverting this commit or changing the line to fmb->mii_bus->phy_mask = ~1;
should fix the problem. Would be great if Takumi could test this.

The cleaner alternative would be:
- remove call to fixed_phy_add() from platform code
- let b44 call fixed_phy_register() if a fixed phy is needed
  Below is a WIP version of this change.

> Does this need a bigger refactoring anyway?
> 
> Hauke

Heiner


---
 drivers/net/ethernet/broadcom/Kconfig |  1 +
 drivers/net/ethernet/broadcom/b44.c   | 33 ++++++++++++++-------------
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index 0fc10e6c6..99292392d 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -25,6 +25,7 @@ config B44
 	select SSB
 	select MII
 	select PHYLIB
+	select FIXED_PHY
 	help
 	  If you have a network (Ethernet) controller of this type, say Y
 	  or M here.
diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index 0353359c3..abbb5953c 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -31,6 +31,7 @@
 #include <linux/ssb/ssb.h>
 #include <linux/slab.h>
 #include <linux/phy.h>
+#include <linux/phy_fixed.h>
 
 #include <linux/uaccess.h>
 #include <asm/io.h>
@@ -2233,7 +2234,6 @@ static int b44_register_phy_one(struct b44 *bp)
 	struct mii_bus *mii_bus;
 	struct ssb_device *sdev = bp->sdev;
 	struct phy_device *phydev;
-	char bus_id[MII_BUS_ID_SIZE + 3];
 	struct ssb_sprom *sprom = &sdev->bus->sprom;
 	int err;
 
@@ -2260,27 +2260,26 @@ static int b44_register_phy_one(struct b44 *bp)
 		goto err_out_mdiobus;
 	}
 
-	if (!mdiobus_is_registered_device(bp->mii_bus, bp->phy_addr) &&
-	    (sprom->boardflags_lo & (B44_BOARDFLAG_ROBO | B44_BOARDFLAG_ADM))) {
-
+	phydev = mdiobus_get_phy(bp->mii_bus, bp->phy_addr);
+	if (!phydev &&
+	    sprom->boardflags_lo & (B44_BOARDFLAG_ROBO | B44_BOARDFLAG_ADM)) {
 		dev_info(sdev->dev,
 			 "could not find PHY at %i, use fixed one\n",
 			 bp->phy_addr);
 
-		bp->phy_addr = 0;
-		snprintf(bus_id, sizeof(bus_id), PHY_ID_FMT, "fixed-0",
-			 bp->phy_addr);
-	} else {
-		snprintf(bus_id, sizeof(bus_id), PHY_ID_FMT, mii_bus->id,
-			 bp->phy_addr);
+		phydev = fixed_phy_register(NULL, NULL);
+		if (!IS_ERR(phydev))
+			bp->phy_addr = phydev->mdio.addr;
 	}
 
-	phydev = phy_connect(bp->dev, bus_id, &b44_adjust_link,
-			     PHY_INTERFACE_MODE_MII);
-	if (IS_ERR(phydev)) {
+	if (IS_ERR_OR_NULL(phydev))
+		err = -ENODEV;
+	else
+		err = phy_connect_direct(bp->dev, phydev, &b44_adjust_link,
+					 PHY_INTERFACE_MODE_MII);
+	if (err) {
 		dev_err(sdev->dev, "could not attach PHY at %i\n",
 			bp->phy_addr);
-		err = PTR_ERR(phydev);
 		goto err_out_mdiobus_unregister;
 	}
 
@@ -2293,7 +2292,6 @@ static int b44_register_phy_one(struct b44 *bp)
 	linkmode_copy(phydev->advertising, phydev->supported);
 
 	bp->old_link = 0;
-	bp->phy_addr = phydev->mdio.addr;
 
 	phy_attached_info(phydev);
 
@@ -2313,8 +2311,11 @@ static void b44_unregister_phy_one(struct b44 *bp)
 {
 	struct net_device *dev = bp->dev;
 	struct mii_bus *mii_bus = bp->mii_bus;
+	struct phy_device *phydev = dev->phydev);
 
-	phy_disconnect(dev->phydev);
+	phy_disconnect(phydev);
+	if (phy_is_pseudo_fixed_link(phydev))
+		fixed_phy_unregister(phydev);
 	mdiobus_unregister(mii_bus);
 	mdiobus_free(mii_bus);
 }
-- 
2.50.1

Re: [PATCH net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 22 Aug 2025 22:36:11 +0200 you wrote:
> We have only two users of fixed_phy_add(), both use address 0 and
> ignore the return value. So simplify fixed_phy_add() accordingly.
> 
> Whilst at it, constify the fixed_phy_status configs.
> 
> Note:
> fixed_phy_add() is a legacy function which shouldn't be used in new
> code, as it's use may be problematic:
> - No check whether a fixed phy exists already at the given address
> - If fixed_phy_register() is called afterwards by any other driver,
>   then it will also use phy_addr 0, because fixed_phy_add() ignores
>   the ida which manages address assignment
> Drivers using a fixed phy created by fixed_phy_add() in platform code,
> should dynamically create a fixed phy with fixed_phy_register()
> instead.
> 
> [...]

Here is the summary with links:
  - [net-next] net: phy: fixed: let fixed_phy_add always use addr 0 and remove return value
    https://git.kernel.org/netdev/net-next/c/39e94fdce45f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html