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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 17.03.2026 08:59, Anand Moon wrote:
> Hi Heiner,
> 
> Thanks for your review comments.
> 
> On Tue, 17 Mar 2026 at 12:21, Heiner Kallweit <hkallweit1@gmail.com> 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@arm.com/
>>> Fixes: ec392abc9593 ("PCI: dw-rockchip: Enable async probe by default")
>>> Cc: Robin Murphy <robin.murphy@arm.com>
>>> Cc: Danilo Krummrich <dakr@kernel.org>
>>> Cc: Niklas Cassel <cassel@kernel.org>
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>>  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.
> 
I'm not aware of any realistic scenario where these calls would fail (w/o printing
an error message before). So adding an error message wouldn't hurt, but it also
wouldn't be beneficial enough to justify a patch.

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