Re: [PATCH next v1] r8169: Fix PHY lookup for mdiobus_get_phy() and improve error reporting during probe

[Anand Moon]

Hi Heiner,

Thanks for your review comments.

On Tue, 17 Mar 2026 at 12:21, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote:
>
> On 17.03.2026 07:16, Anand Moon wrote:
> > The driver currently assumes the PHY is always at address 0. On some
> > hardware or buggy BIOS implementations, the PHY may reside at a
> > different address on the MDIO bus. Update r8169_mdio_register()
> > to scan the bus for the first available PHY instead of hardcoding
> > address 0.
>
> No. The PHY always shows up at (virtual) address 0, due to the
> MAC register based way of accessing the internal PHY.
> Do you have any concrete example of such a "hardware or buggy bios"?
>
Sorry about that, I didn't realize. Unfortunately, my testing shows
this still doesn't
fix the problem, but I still perceive. I modeled the change after some
other drivers,
but clearly, that approach isn't working in this case.
> >
> > Additionally, switch to dev_err_probe() in the main probe path to
> > provide standardized error handling and cleaner log output for
> > registration failures.
> >
> > Link: https://lore.kernel.org/all/87bc37ee-234c-4568-b72e-955c130a6838@xxxxxxx/
> > Fixes: ec392abc9593 ("PCI: dw-rockchip: Enable async probe by default")
> > Cc: Robin Murphy <robin.murphy@xxxxxxx>
> > Cc: Danilo Krummrich <dakr@xxxxxxxxxx>
> > Cc: Niklas Cassel <cassel@xxxxxxxxxx>
> > Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx>
> > ---
> >  drivers/net/ethernet/realtek/r8169_main.c | 34 ++++++++++++++++-------
> >  1 file changed, 24 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> > index 791277e750ba..0fd735beff92 100644
> > --- a/drivers/net/ethernet/realtek/r8169_main.c
> > +++ b/drivers/net/ethernet/realtek/r8169_main.c
> > @@ -5427,7 +5427,8 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
> >  {
> >       struct pci_dev *pdev = tp->pci_dev;
> >       struct mii_bus *new_bus;
> > -     int ret;
> > +     struct phy_device *phydev;
> > +     int ret, addr;
> >
> >       /* On some boards with this chip version the BIOS is buggy and misses
> >        * to reset the PHY page selector. This results in the PHY ID read
> > @@ -5462,18 +5463,31 @@ static int r8169_mdio_register(struct rtl8169_private *tp)
> >       if (ret)
> >               return ret;
> >
> > -     tp->phydev = mdiobus_get_phy(new_bus, 0);
> > -     if (!tp->phydev) {
> > +     /* find the first (lowest address) PHY on the current MAC's MII bus */
> > +     for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
> > +             struct phy_device *tmp = mdiobus_get_phy(new_bus, addr);
> > +
> > +             if (tmp) {
> > +                     phydev = tmp;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     if (!phydev) {
> > +             dev_err(&pdev->dev, "no PHY found on bus\n");
> >               return -ENODEV;
> > -     } else if (!tp->phydev->drv) {
> > -             /* Most chip versions fail with the genphy driver.
> > -              * Therefore ensure that the dedicated PHY driver is loaded.
> > -              */
>
> This check is needed. It's seems your actual intention is something completely
> different from what you state in the commit message. You try to find a workaround
> for the issues with request_module() from async contect, caused by the referenced
> change to Rockchip PCI.
That makes sense, sorry for the noise.

What are your thoughts on using dev_err_probe() for these registration failures?
I thought it might clean up the error path.

Thanks
-Anand
>
> > +     }
> > +
> > +     /* Most chip versions fail with the genphy driver.
> > +      * Therefore ensure that the dedicated PHY driver is loaded.
> > +      */
> > +     if (!phydev->drv) {
> >               dev_err(&pdev->dev, "no dedicated PHY driver found for PHY ID 0x%08x, maybe realtek.ko needs to be added to initramfs?\n",
> > -                     tp->phydev->phy_id);
> > +                     phydev->phy_id);
> >               return -EUNATCH;
> >       }
> >
> > +     tp->phydev = phydev;
> >       tp->phydev->mac_managed_pm = true;
> >       if (rtl_supports_eee(tp))
> >               phy_support_eee(tp->phydev);
> > @@ -5790,11 +5804,11 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> >
> >       rc = r8169_mdio_register(tp);
> >       if (rc)
> > -             return rc;
> > +             return dev_err_probe(&pdev->dev, rc, "mdio register failure\n");
> >
> >       rc = register_netdev(dev);
> >       if (rc)
> > -             return rc;
> > +             return dev_err_probe(&pdev->dev, rc, "register newdev failure\n");
> >
> >       if (IS_ENABLED(CONFIG_R8169_LEDS)) {
> >               if (rtl_is_8125(tp))
> >
> > base-commit: 95c541ddfb0815a0ea8477af778bb13bb075079a
>