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);
+
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, ®);
+ ret = lan78xx_read_reg(dev, HW_CFG, ®);
+ 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.5Hi 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, ®);
> + ret = lan78xx_read_reg(dev, HW_CFG, ®);
> + 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
>
> > 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?
phylink itself implements fixed phy. So it is being removed later
because it is not needed once the conversation to phylink is
performed. If you remove it here, before the conversion to phylink,
you break the driver when it is using fixed phy.
With this sort of refactoring, you should not break the normal
case. But there is some wiggle room for error cases, which should not
happen, so long as by the end of the patch series, it is all clean.
So i personally don't care about this leak of a fixed link, at this
stage.
Andrew
On Wed, Mar 19, 2025 at 09:49:47AM +0100, Oleksij Rempel wrote:
> } else {
> if (!phydev->drv) {
> netdev_err(dev->net, "no PHY driver found\n");
> - return NULL;
> + return ERR_PTR(-EINVAL);
> }
Idly wondering why this driver cares whether the PHY has a driver or
not (it seemingly wants to exclude the generic driver.) Not a reason
to reject the patch, just curious.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
© 2016 - 2025 Red Hat, Inc.