[PATCH v5] net: phy: fix duplicate eth_phy binding

[Marek Vasut]

On 5/15/26 1:13 PM, Vinay Tilak, Pranav wrote:

[...]

>>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index
>>> d7e0c4fe02d..7d698f00c09 100644
>>> --- a/drivers/net/phy/phy.c
>>> +++ b/drivers/net/phy/phy.c
>>> @@ -24,6 +24,7 @@
>>>    #include <linux/delay.h>
>>>    #include <linux/err.h>
>>>    #include <linux/compiler.h>
>>> +#include <dm/uclass-internal.h>
>>>
>>>    /* Generic PHY support and helper functions */
>>>
>>> @@ -927,6 +928,14 @@ struct phy_device *phy_connect(struct mii_dev
>> *bus, int addr,
>>>    {
>>>      struct phy_device *phydev = NULL;
>>>      uint mask = (addr >= 0) ? (1 << addr) : 0xffffffff;
>>> +   struct udevice *phy_dev;

Please pick a variable name which is not phy_dev , there is already a 
phydev variable in this function and the naming is VERY confusing.

>>> +   int ret;
>>> +
>>> +   /* Skip binding if PHY already bound by eth_phy_binds_nodes(). */
>>> +   ret = uclass_find_device_by_phandle(UCLASS_ETH_PHY, dev,
>>> +                                       "phy-handle", &phy_dev);
>>> +   if (!ret)
>>> +           goto out;

You already check if (!ret) here ...

>>>    #ifdef CONFIG_PHY_FIXED
>>>      phydev = phy_connect_fixed(bus, dev); @@ -947,9 +956,13 @@
>> struct
>>> phy_device *phy_connect(struct mii_dev *bus, int addr,
>>>              phydev = phy_connect_gmii2rgmii(bus, dev);
>>>    #endif
>>>
>>> +out:
>>>      if (!phydev)
>>>              phydev = phy_find_by_mask(bus, mask);
>>>
>>> +   if (phydev && !ret && !ofnode_valid(phydev->node))
>>> +           phydev->node = dev_ofnode(phy_dev);
>> Is the !ret test necessary in this conditional ?
> 
> Yes i checked, !ret is necessary as uclass_find_device_by_phandle() always sets phy_dev= NULL on entry, when it fails to find an already bound PHY phy_dev is NULL. Removing !ret would call dev_ofnode(NULL) which dereferences a NULL pointer and crashes.

... so I think the goto jump target needs to be adjusted ?

In fact, I do not understand why does this code call phy_find_by_mask() 
if the PHY udevice was already found by uclass_find_device_by_phandle() 
? Can you not convert that already found PHY udevice into phydev ?

> !ret also serves as a guard for the assignment of phydev->node it should only happen when the PHY is already DM bound. When binding methods run (ret != 0), they already set phydev->node correctly so no need to again set phydev->node in that case.

If uclass_find_device_by_phandle() does locate PHY udevice , it will 
return 0, and the !ret conditional will evaluate to true , so 
phydev->node = dev_ofnode(phy_dev); will run in that case ?

> The !ofnode_valid(phydev->node) check is redundant and can be dropped. In the ret == 0 path, only phy_find_by_mask() runs and it never sets phydev->node, so !ofnode_valid(phydev->node) is always true.
> Hence condition can be simplified to:
> 
> if (phydev && !ret)
>      phydev->node = dev_ofnode(phy_dev);
[...]