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

[Vinay Tilak, Pranav]

Public

Hi Marek,

> -----Original Message-----
> From: Marek Vasut <marek.vasut at mailbox.org>
> Sent: Wednesday, May 13, 2026 7:09 PM
> To: Vinay Tilak, Pranav <Pranav.VinayTilak at amd.com>; u-boot at lists.denx.de;
> Simek, Michal <michal.simek at amd.com>
> Cc: git (AMD-Xilinx) <git at amd.com>; Begari, Padmarao
> <Padmarao.Begari at amd.com>; Jerome Forissier
> <jerome.forissier at arm.com>; Tom Rini <trini at konsulko.com>; Peng Fan
> <peng.fan at nxp.com>; Patrice Chotard <patrice.chotard at foss.st.com>;
> Lucien.Jheng <lucienzx159 at gmail.com>; Siddharth Vadapalli <s-
> vadapalli at ti.com>; SkyLake.Huang <skylake.huang at mediatek.com>; Marek
> Vasut <marek.vasut+renesas at mailbox.org>; Ramon Fried
> <rfried.dev at gmail.com>
> Subject: Re: [PATCH v5] net: phy: fix duplicate eth_phy binding
>
> On 5/13/26 3:33 PM, Pranav Tilak wrote:
> > When both CONFIG_PHY_ETHERNET_ID and CONFIG_DM_ETH_PHY are
> enabled,
> > eth_phy_binds_nodes() called from eth_post_bind() already binds the
> > ethernet PHY node to eth_phy_generic_drv. However,
> > phy_connect_phy_id() called via phy_connect() also binds the same PHY
> > node, resulting in duplicate entries in the DM tree.
> >
> > Fix this by checking at the beginning of phy_connect() whether the PHY
> > is already bound via uclass_find_device_by_phandle(). If so, skip all
> > generic binding methods and fall through to phy_find_by_mask(). Also
> > assign phydev->node from the already bound DM device.
> >
> > Fixes: 68a4d1506109 ("net: phy: Bind ETH_PHY uclass driver to each new
> > PHY")
> > Signed-off-by: Pranav Tilak <pranav.vinaytilak at amd.com>
> > ---
> > Changes in v5:
> > - Use int ret instead of bool phy_bound and goto out
> >
> > Changes in v4:
> > - Removed IS_ENABLED(CONFIG_DM_ETH_PHY), use runtime check via
> >    uclass_find_device_by_phandle()
> > - Fix indentation of binding methods block
> > - Assign phydev->node from the bound DM device's ofnode
> >
> > Changes in v3:
> > - Moved duplicate binding check to beginning of phy_connect()
> >
> > Changes in v2:
> > - Move duplicate binding check to caller phy_connect()
> > - Update commit description
> > ---
> >   drivers/net/phy/phy.c | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > 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;
> > +   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;
> >
> >   #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.

!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.

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);

Thanks & regards,
Pranav Tilak