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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 8/24/2025 6:36 PM, Heiner Kallweit wrote:
> 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.
> 
Did you have the chance to 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);
>  }