Re: [PATCH net-next v5 1/6] net: usb: lan78xx: Improve error handling in PHY initialization

[<Rengarajan.S@microchip.com>]

Hi Oleksij,

On Wed, 2025-03-19 at 09:49 +0100, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Ensure that return values from `lan78xx_write_reg()`,
> `lan78xx_read_reg()`, and `phy_find_first()` are properly checked and
> propagated. Use `ERR_PTR(ret)` for error reporting in
> `lan7801_phy_init()` and replace `-EIO` with `-ENODEV` where
> appropriate
> to provide more accurate error codes.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
> changes v5:
> - make sure lan7801_phy_init() caller is testing against IS_ERR
>   instead of NULL.
> changes v4:
> - split the patch and move part of it before PHYlink migration
> ---
>  drivers/net/usb/lan78xx.c | 47 ++++++++++++++++++++++++++-----------
> --
>  1 file changed, 31 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index 137adf6d5b08..13b5da18850a 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -2510,14 +2510,13 @@ static void lan78xx_remove_irq_domain(struct
> lan78xx_net *dev)
> 
>  static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
>  {
> -       u32 buf;
> -       int ret;
>         struct fixed_phy_status fphy_status = {
>                 .link = 1,
>                 .speed = SPEED_1000,
>                 .duplex = DUPLEX_FULL,
>         };
>         struct phy_device *phydev;
> +       int ret;
> 
>         phydev = phy_find_first(dev->mdiobus);
>         if (!phydev) {
> @@ -2525,30 +2524,40 @@ static struct phy_device
> *lan7801_phy_init(struct lan78xx_net *dev)
>                 phydev = fixed_phy_register(PHY_POLL, &fphy_status,
> NULL);
>                 if (IS_ERR(phydev)) {
>                         netdev_err(dev->net, "No PHY/fixed_PHY
> found\n");
> -                       return NULL;
> +                       return ERR_PTR(-ENODEV);
>                 }
>                 netdev_dbg(dev->net, "Registered FIXED PHY\n");
>                 dev->interface = PHY_INTERFACE_MODE_RGMII;
>                 ret = lan78xx_write_reg(dev, MAC_RGMII_ID,
>                                         MAC_RGMII_ID_TXC_DELAY_EN_);
> +               if (ret < 0)
> +                       return ERR_PTR(ret);
> +

I noticed that fixed_phy_register is removed in later patches. However,
in the above implementation, if a failure occurs we exit without
unregistering it. To avoid this issue, can we include the patch that
removes fixed_phy_register first to avoid the cleanup scenario?

>                 ret = lan78xx_write_reg(dev, RGMII_TX_BYP_DLL,
> 0x3D00);
> -               ret = lan78xx_read_reg(dev, HW_CFG, &buf);
> -               buf |= HW_CFG_CLK125_EN_;
> -               buf |= HW_CFG_REFCLK25_EN_;
> -               ret = lan78xx_write_reg(dev, HW_CFG, buf);
> +               if (ret < 0)
> +                       return ERR_PTR(ret);
> +
> +               ret = lan78xx_update_reg(dev, HW_CFG,
> HW_CFG_CLK125_EN_ |
> +                                        HW_CFG_REFCLK25_EN_,
> +                                        HW_CFG_CLK125_EN_ |
> HW_CFG_REFCLK25_EN_);
> +               if (ret < 0)
> +                       return ERR_PTR(ret);
>         } else {
>                 if (!phydev->drv) {
>                         netdev_err(dev->net, "no PHY driver
> found\n");
> -                       return NULL;
> +                       return ERR_PTR(-EINVAL);
>                 }
>                 dev->interface = PHY_INTERFACE_MODE_RGMII_ID;
>                 /* The PHY driver is responsible to configure proper
> RGMII
>                  * interface delays. Disable RGMII delays on MAC
> side.
>                  */
> -               lan78xx_write_reg(dev, MAC_RGMII_ID, 0);
> +               ret = lan78xx_write_reg(dev, MAC_RGMII_ID, 0);
> +               if (ret < 0)
> +                       return ERR_PTR(ret);
> 
>                 phydev->is_internal = false;
>         }
> +
>         return phydev;
>  }
> 
> @@ -2562,9 +2571,10 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
>         switch (dev->chipid) {
>         case ID_REV_CHIP_ID_7801_:
>                 phydev = lan7801_phy_init(dev);
> -               if (!phydev) {
> -                       netdev_err(dev->net, "lan7801: PHY Init
> Failed");
> -                       return -EIO;
> +               if (IS_ERR(phydev)) {
> +                       netdev_err(dev->net, "lan7801: failed to init
> PHY: %pe\n",
> +                                  phydev);
> +                       return PTR_ERR(phydev);
>                 }
>                 break;
> 
> @@ -2573,7 +2583,7 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
>                 phydev = phy_find_first(dev->mdiobus);
>                 if (!phydev) {
>                         netdev_err(dev->net, "no PHY found\n");
> -                       return -EIO;
> +                       return -ENODEV;
>                 }
>                 phydev->is_internal = true;
>                 dev->interface = PHY_INTERFACE_MODE_GMII;
> @@ -2581,7 +2591,7 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
> 
>         default:
>                 netdev_err(dev->net, "Unknown CHIP ID found\n");
> -               return -EIO;
> +               return -ENODEV;
>         }
> 
>         /* if phyirq is not set, use polling mode in phylib */
> @@ -2633,7 +2643,10 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
>                                                       sizeof(u32));
>                 if (len >= 0) {
>                         /* Ensure the appropriate LEDs are enabled */
> -                       lan78xx_read_reg(dev, HW_CFG, &reg);
> +                       ret = lan78xx_read_reg(dev, HW_CFG, &reg);
> +                       if (ret < 0)
> +                               return ret;
> +
>                         reg &= ~(HW_CFG_LED0_EN_ |
>                                  HW_CFG_LED1_EN_ |
>                                  HW_CFG_LED2_EN_ |
> @@ -2642,7 +2655,9 @@ static int lan78xx_phy_init(struct lan78xx_net
> *dev)
>                                 (len > 1) * HW_CFG_LED1_EN_ |
>                                 (len > 2) * HW_CFG_LED2_EN_ |
>                                 (len > 3) * HW_CFG_LED3_EN_;
> -                       lan78xx_write_reg(dev, HW_CFG, reg);
> +                       ret = lan78xx_write_reg(dev, HW_CFG, reg);
> +                       if (ret < 0)
> +                               return ret;
>                 }
>         }
> 
> --
> 2.39.5
>