From 30c63abaee9024ed7524325b3eeb7f2d26727c31 Mon Sep 17 00:00:00 2001 From: Oleksij Rempel Date: Mon, 16 Dec 2024 13:09:36 +0100 Subject: [PATCH 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs Update `lan78xx_get_regs` to handle errors during register and PHY reads. Log warnings for failed reads and exit the function early if an error occurs. Drop all previously logged registers to signal inconsistent readings to the user space. This ensures that invalid data is not returned to users. Signed-off-by: Oleksij Rempel Link: https://patch.msgid.link/20241216120941.1690908-2-o.rempel@pengutronix.de Signed-off-by: Jakub Kicinski --- drivers/net/usb/lan78xx.c | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 4661d131b190..270345fcad65 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -2108,20 +2108,44 @@ static void lan78xx_get_regs(struct net_device *netdev, struct ethtool_regs *regs, void *buf) { - u32 *data = buf; - int i, j; struct lan78xx_net *dev = netdev_priv(netdev); + unsigned int data_count = 0; + u32 *data = buf; + int i, j, ret; /* Read Device/MAC registers */ - for (i = 0; i < ARRAY_SIZE(lan78xx_regs); i++) - lan78xx_read_reg(dev, lan78xx_regs[i], &data[i]); + for (i = 0; i < ARRAY_SIZE(lan78xx_regs); i++) { + ret = lan78xx_read_reg(dev, lan78xx_regs[i], &data[i]); + if (ret < 0) { + netdev_warn(dev->net, + "failed to read register 0x%08x\n", + lan78xx_regs[i]); + goto clean_data; + } + + data_count++; + } if (!netdev->phydev) return; /* Read PHY registers */ - for (j = 0; j < 32; i++, j++) - data[i] = phy_read(netdev->phydev, j); + for (j = 0; j < 32; i++, j++) { + ret = phy_read(netdev->phydev, j); + if (ret < 0) { + netdev_warn(dev->net, + "failed to read PHY register 0x%02x\n", j); + goto clean_data; + } + + data[i] = ret; + data_count++; + } + + return; + +clean_data: + memset(data, 0, data_count * sizeof(u32)); } static const struct ethtool_ops lan78xx_ethtool_ops = { From 18bdefe62439c75227021ddbbf6510aa2f2f4e54 Mon Sep 17 00:00:00 2001 From: Oleksij Rempel Date: Mon, 16 Dec 2024 13:09:37 +0100 Subject: [PATCH 2/6] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw Update lan78xx_stop_hw to return -ETIMEDOUT instead of -ETIME when a timeout occurs. While -ETIME indicates a general timer expiration, -ETIMEDOUT is more commonly used for signaling operation timeouts and provides better consistency with standard error handling in the driver. The -ETIME checks in tx_complete() and rx_complete() are unrelated to this error handling change. In these functions, the error values are derived from urb->status, which reflects USB transfer errors. The error value from lan78xx_stop_hw will be exposed in the following cases: - usb_driver::suspend - net_device_ops::ndo_stop (potentially, though currently the return value is not used). Signed-off-by: Oleksij Rempel Reviewed-by: Mateusz Polchlopek Link: https://patch.msgid.link/20241216120941.1690908-3-o.rempel@pengutronix.de Signed-off-by: Jakub Kicinski --- drivers/net/usb/lan78xx.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 270345fcad65..4674051f5c9c 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -844,9 +844,7 @@ static int lan78xx_stop_hw(struct lan78xx_net *dev, u32 reg, u32 hw_enabled, } while (!stopped && !time_after(jiffies, timeout)); } - ret = stopped ? 0 : -ETIME; - - return ret; + return stopped ? 0 : -ETIMEDOUT; } static int lan78xx_flush_fifo(struct lan78xx_net *dev, u32 reg, u32 fifo_flush) From 7433d022b915977a0e361a036aa06a0d382a9630 Mon Sep 17 00:00:00 2001 From: Oleksij Rempel Date: Mon, 16 Dec 2024 13:09:38 +0100 Subject: [PATCH 3/6] net: usb: lan78xx: Use action-specific label in lan78xx_mac_reset Rename the generic `done` label to the action-specific `exit_unlock` label in `lan78xx_mac_reset`. This improves clarity by indicating the specific cleanup action (mutex unlock) and aligns with best practices for error handling and cleanup labels. Signed-off-by: Oleksij Rempel Reviewed-by: Andrew Lunn Reviewed-by: Mateusz Polchlopek Link: https://patch.msgid.link/20241216120941.1690908-4-o.rempel@pengutronix.de Signed-off-by: Jakub Kicinski --- drivers/net/usb/lan78xx.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 4674051f5c9c..30301af29ab2 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -1604,16 +1604,16 @@ static int lan78xx_mac_reset(struct lan78xx_net *dev) */ ret = lan78xx_mdiobus_wait_not_busy(dev); if (ret < 0) - goto done; + goto exit_unlock; ret = lan78xx_read_reg(dev, MAC_CR, &val); if (ret < 0) - goto done; + goto exit_unlock; val |= MAC_CR_RST_; ret = lan78xx_write_reg(dev, MAC_CR, val); if (ret < 0) - goto done; + goto exit_unlock; /* Wait for the reset to complete before allowing any further * MAC register accesses otherwise the MAC may lock up. @@ -1621,16 +1621,16 @@ static int lan78xx_mac_reset(struct lan78xx_net *dev) do { ret = lan78xx_read_reg(dev, MAC_CR, &val); if (ret < 0) - goto done; + goto exit_unlock; if (!(val & MAC_CR_RST_)) { ret = 0; - goto done; + goto exit_unlock; } } while (!time_after(jiffies, start_time + HZ)); ret = -ETIMEDOUT; -done: +exit_unlock: mutex_unlock(&dev->phy_mutex); return ret; From 3a59437ed9072fa812e3e30bf0637ca94a239652 Mon Sep 17 00:00:00 2001 From: Oleksij Rempel Date: Mon, 16 Dec 2024 13:09:39 +0100 Subject: [PATCH 4/6] net: usb: lan78xx: rename phy_mutex to mdiobus_mutex Rename `phy_mutex` to `mdiobus_mutex` for clarity, as the mutex protects MDIO bus access rather than PHY-specific operations. Update all references to ensure consistency. Signed-off-by: Oleksij Rempel Link: https://patch.msgid.link/20241216120941.1690908-5-o.rempel@pengutronix.de Signed-off-by: Jakub Kicinski --- drivers/net/usb/lan78xx.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 30301af29ab2..78c75599b8f1 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -439,7 +439,7 @@ struct lan78xx_net { struct usb_anchor deferred; struct mutex dev_mutex; /* serialise open/stop wrt suspend/resume */ - struct mutex phy_mutex; /* for phy access */ + struct mutex mdiobus_mutex; /* for MDIO bus access */ unsigned int pipe_in, pipe_out, pipe_intr; unsigned int bulk_in_delay; @@ -952,7 +952,7 @@ static int lan78xx_flush_rx_fifo(struct lan78xx_net *dev) return lan78xx_flush_fifo(dev, FCT_RX_CTL, FCT_RX_CTL_RST_); } -/* Loop until the read is completed with timeout called with phy_mutex held */ +/* Loop until the read is completed with timeout called with mdiobus_mutex held */ static int lan78xx_mdiobus_wait_not_busy(struct lan78xx_net *dev) { unsigned long start_time = jiffies; @@ -1596,7 +1596,7 @@ static int lan78xx_mac_reset(struct lan78xx_net *dev) u32 val; int ret; - mutex_lock(&dev->phy_mutex); + mutex_lock(&dev->mdiobus_mutex); /* Resetting the device while there is activity on the MDIO * bus can result in the MAC interface locking up and not @@ -1631,7 +1631,7 @@ static int lan78xx_mac_reset(struct lan78xx_net *dev) ret = -ETIMEDOUT; exit_unlock: - mutex_unlock(&dev->phy_mutex); + mutex_unlock(&dev->mdiobus_mutex); return ret; } @@ -2249,7 +2249,7 @@ static int lan78xx_mdiobus_read(struct mii_bus *bus, int phy_id, int idx) if (ret < 0) return ret; - mutex_lock(&dev->phy_mutex); + mutex_lock(&dev->mdiobus_mutex); /* confirm MII not busy */ ret = lan78xx_mdiobus_wait_not_busy(dev); @@ -2273,7 +2273,7 @@ static int lan78xx_mdiobus_read(struct mii_bus *bus, int phy_id, int idx) ret = (int)(val & 0xFFFF); done: - mutex_unlock(&dev->phy_mutex); + mutex_unlock(&dev->mdiobus_mutex); usb_autopm_put_interface(dev->intf); return ret; @@ -2290,7 +2290,7 @@ static int lan78xx_mdiobus_write(struct mii_bus *bus, int phy_id, int idx, if (ret < 0) return ret; - mutex_lock(&dev->phy_mutex); + mutex_lock(&dev->mdiobus_mutex); /* confirm MII not busy */ ret = lan78xx_mdiobus_wait_not_busy(dev); @@ -2313,7 +2313,7 @@ static int lan78xx_mdiobus_write(struct mii_bus *bus, int phy_id, int idx, goto done; done: - mutex_unlock(&dev->phy_mutex); + mutex_unlock(&dev->mdiobus_mutex); usb_autopm_put_interface(dev->intf); return ret; } @@ -4476,7 +4476,7 @@ static int lan78xx_probe(struct usb_interface *intf, skb_queue_head_init(&dev->rxq_done); skb_queue_head_init(&dev->txq_pend); skb_queue_head_init(&dev->rxq_overflow); - mutex_init(&dev->phy_mutex); + mutex_init(&dev->mdiobus_mutex); mutex_init(&dev->dev_mutex); ret = lan78xx_urb_config_init(dev); From d09de7ebd4abf26d9aee072b82a514c372d278b5 Mon Sep 17 00:00:00 2001 From: Oleksij Rempel Date: Mon, 16 Dec 2024 13:09:40 +0100 Subject: [PATCH 5/6] net: usb: lan78xx: remove PHY register access from ethtool get_regs Remove PHY register handling from `lan78xx_get_regs` and `lan78xx_get_regs_len`. Since the controller can have different PHYs attached, the first 32 registers are not universally relevant or the most interesting. Simplify the implementation to focus on MAC and device registers. Signed-off-by: Oleksij Rempel Link: https://patch.msgid.link/20241216120941.1690908-6-o.rempel@pengutronix.de Signed-off-by: Jakub Kicinski --- drivers/net/usb/lan78xx.c | 23 ++--------------------- 1 file changed, 2 insertions(+), 21 deletions(-) diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 78c75599b8f1..6c9dab290f3f 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -2096,10 +2096,7 @@ static int lan78xx_set_pause(struct net_device *net, static int lan78xx_get_regs_len(struct net_device *netdev) { - if (!netdev->phydev) - return (sizeof(lan78xx_regs)); - else - return (sizeof(lan78xx_regs) + PHY_REG_SIZE); + return sizeof(lan78xx_regs); } static void @@ -2109,7 +2106,7 @@ lan78xx_get_regs(struct net_device *netdev, struct ethtool_regs *regs, struct lan78xx_net *dev = netdev_priv(netdev); unsigned int data_count = 0; u32 *data = buf; - int i, j, ret; + int i, ret; /* Read Device/MAC registers */ for (i = 0; i < ARRAY_SIZE(lan78xx_regs); i++) { @@ -2124,22 +2121,6 @@ lan78xx_get_regs(struct net_device *netdev, struct ethtool_regs *regs, data_count++; } - if (!netdev->phydev) - return; - - /* Read PHY registers */ - for (j = 0; j < 32; i++, j++) { - ret = phy_read(netdev->phydev, j); - if (ret < 0) { - netdev_warn(dev->net, - "failed to read PHY register 0x%02x\n", j); - goto clean_data; - } - - data[i] = ret; - data_count++; - } - return; clean_data: From 01e2f4d55bda0e24548e1458e77975898683a2cd Mon Sep 17 00:00:00 2001 From: Oleksij Rempel Date: Mon, 16 Dec 2024 13:09:41 +0100 Subject: [PATCH 6/6] net: usb: lan78xx: Improve error handling in WoL operations Enhance error handling in Wake-on-LAN (WoL) operations: - Log a warning in `lan78xx_get_wol` if `lan78xx_read_reg` fails. - Check and handle errors from `device_set_wakeup_enable` and `phy_ethtool_set_wol` in `lan78xx_set_wol`. - Ensure proper cleanup with a unified error handling path. Signed-off-by: Oleksij Rempel Reviewed-by: Andrew Lunn Link: https://patch.msgid.link/20241216120941.1690908-7-o.rempel@pengutronix.de Signed-off-by: Jakub Kicinski --- drivers/net/usb/lan78xx.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c index 6c9dab290f3f..a91bf9c7e31d 100644 --- a/drivers/net/usb/lan78xx.c +++ b/drivers/net/usb/lan78xx.c @@ -1857,6 +1857,7 @@ static void lan78xx_get_wol(struct net_device *netdev, ret = lan78xx_read_reg(dev, USB_CFG0, &buf); if (unlikely(ret < 0)) { + netdev_warn(dev->net, "failed to get WoL %pe", ERR_PTR(ret)); wol->supported = 0; wol->wolopts = 0; } else { @@ -1888,10 +1889,13 @@ static int lan78xx_set_wol(struct net_device *netdev, pdata->wol = wol->wolopts; - device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts); + ret = device_set_wakeup_enable(&dev->udev->dev, (bool)wol->wolopts); + if (ret < 0) + goto exit_pm_put; - phy_ethtool_set_wol(netdev->phydev, wol); + ret = phy_ethtool_set_wol(netdev->phydev, wol); +exit_pm_put: usb_autopm_put_interface(dev->intf); return ret;