[PATCH net-next v1 00/11] lan78xx: Preparations for PHYlink

[PATCH net-next v1 00/11] lan78xx: Preparations for PHYlink

[Oleksij Rempel]

This patch set is a second part of the preparatory work for migrating
the lan78xx USB Ethernet driver to the PHYlink framework. During
extensive testing, I observed that resetting the USB adapter can lead to
various read/write errors. While the errors themselves are acceptable,
they generate excessive log messages, resulting in significant log spam.
This set improves error handling to reduce logging noise by addressing
errors directly and returning early when necessary.

Oleksij Rempel (11):
  net: usb: lan78xx: Add error handling to lan78xx_setup_irq_domain
  net: usb: lan78xx: Add error handling to lan78xx_init_mac_address
  net: usb: lan78xx: Add error handling to lan78xx_set_mac_addr
  net: usb: lan78xx: Add error handling to lan78xx_get_regs
  net: usb: lan78xx: Simplify lan78xx_update_reg
  net: usb: lan78xx: Fix return value handling in lan78xx_set_features
  net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw
  net: usb: lan78xx: Use function-specific label in lan78xx_mac_reset
  net: usb: lan78xx: Improve error handling in lan78xx_phy_wait_not_busy
  net: usb: lan78xx: Rename lan78xx_phy_wait_not_busy to
    lan78xx_mdiobus_wait_not_busy
  net: usb: lan78xx: Improve error handling in WoL operations

 drivers/net/usb/lan78xx.c | 137 +++++++++++++++++++++++++-------------
 1 file changed, 89 insertions(+), 48 deletions(-)

--
2.39.5

[PATCH net-next v1 01/11] net: usb: lan78xx: Add error handling to lan78xx_setup_irq_domain

[Oleksij Rempel]

Update `lan78xx_setup_irq_domain` to handle errors in
`lan78xx_read_reg`.  Return the error code immediately if the read
operation fails, ensuring proper error propagation during IRQ domain
setup.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/usb/lan78xx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index d5f6367d3714..070b21baffaf 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2445,7 +2445,10 @@ static int lan78xx_setup_irq_domain(struct lan78xx_net *dev)
 
 	mutex_init(&dev->domain_data.irq_lock);
 
-	lan78xx_read_reg(dev, INT_EP_CTL, &buf);
+	ret = lan78xx_read_reg(dev, INT_EP_CTL, &buf);
+	if (ret < 0)
+		return ret;
+
 	dev->domain_data.irqenable = buf;
 
 	dev->domain_data.irqchip = &lan78xx_irqchip;
-- 
2.39.5

Re: [PATCH net-next v1 01/11] net: usb: lan78xx: Add error handling to lan78xx_setup_irq_domain

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:41PM +0100, Oleksij Rempel wrote:
> Update `lan78xx_setup_irq_domain` to handle errors in
> `lan78xx_read_reg`.  Return the error code immediately if the read
> operation fails, ensuring proper error propagation during IRQ domain
> setup.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v1 02/11] net: usb: lan78xx: Add error handling to lan78xx_init_mac_address

[Oleksij Rempel]

Convert `lan78xx_init_mac_address` to return error codes and handle
failures in register read and write operations. Update `lan78xx_reset`
to check for errors during MAC address initialization and propagate them
appropriately.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/usb/lan78xx.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 070b21baffaf..26dc43bac84b 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2153,13 +2153,19 @@ static const struct ethtool_ops lan78xx_ethtool_ops = {
 	.get_regs	= lan78xx_get_regs,
 };
 
-static void lan78xx_init_mac_address(struct lan78xx_net *dev)
+static int lan78xx_init_mac_address(struct lan78xx_net *dev)
 {
 	u32 addr_lo, addr_hi;
 	u8 addr[6];
+	int ret;
+
+	ret = lan78xx_read_reg(dev, RX_ADDRL, &addr_lo);
+	if (ret < 0)
+		return ret;
 
-	lan78xx_read_reg(dev, RX_ADDRL, &addr_lo);
-	lan78xx_read_reg(dev, RX_ADDRH, &addr_hi);
+	ret = lan78xx_read_reg(dev, RX_ADDRH, &addr_hi);
+	if (ret < 0)
+		return ret;
 
 	addr[0] = addr_lo & 0xFF;
 	addr[1] = (addr_lo >> 8) & 0xFF;
@@ -2192,14 +2198,26 @@ static void lan78xx_init_mac_address(struct lan78xx_net *dev)
 			  (addr[2] << 16) | (addr[3] << 24);
 		addr_hi = addr[4] | (addr[5] << 8);
 
-		lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
-		lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
+		ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
+		if (ret < 0)
+			return ret;
+
+		ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
+		if (ret < 0)
+			return ret;
 	}
 
-	lan78xx_write_reg(dev, MAF_LO(0), addr_lo);
-	lan78xx_write_reg(dev, MAF_HI(0), addr_hi | MAF_HI_VALID_);
+	ret = lan78xx_write_reg(dev, MAF_LO(0), addr_lo);
+	if (ret < 0)
+		return ret;
+
+	ret = lan78xx_write_reg(dev, MAF_HI(0), addr_hi | MAF_HI_VALID_);
+	if (ret < 0)
+		return ret;
 
 	eth_hw_addr_set(dev->net, addr);
+
+	return 0;
 }
 
 /* MDIO read and write wrappers for phylib */
@@ -2990,7 +3008,9 @@ static int lan78xx_reset(struct lan78xx_net *dev)
 		}
 	} while (buf & HW_CFG_LRST_);
 
-	lan78xx_init_mac_address(dev);
+	ret = lan78xx_init_mac_address(dev);
+	if (ret < 0)
+		return ret;
 
 	/* save DEVID for later usage */
 	ret = lan78xx_read_reg(dev, ID_REV, &buf);
-- 
2.39.5

Re: [PATCH net-next v1 02/11] net: usb: lan78xx: Add error handling to lan78xx_init_mac_address

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:42PM +0100, Oleksij Rempel wrote:
> Convert `lan78xx_init_mac_address` to return error codes and handle
> failures in register read and write operations. Update `lan78xx_reset`
> to check for errors during MAC address initialization and propagate them
> appropriately.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v1 03/11] net: usb: lan78xx: Add error handling to lan78xx_set_mac_addr

[Oleksij Rempel]

Update `lan78xx_set_mac_addr` to handle errors during MAC address
register write operations. Ensure that errors are properly propagated to
the caller, improving the robustness of MAC address updates.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/usb/lan78xx.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 26dc43bac84b..5d318ff8b33d 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2763,6 +2763,7 @@ static int lan78xx_set_mac_addr(struct net_device *netdev, void *p)
 	struct lan78xx_net *dev = netdev_priv(netdev);
 	struct sockaddr *addr = p;
 	u32 addr_lo, addr_hi;
+	int ret;
 
 	if (netif_running(netdev))
 		return -EBUSY;
@@ -2779,14 +2780,20 @@ static int lan78xx_set_mac_addr(struct net_device *netdev, void *p)
 	addr_hi = netdev->dev_addr[4] |
 		  netdev->dev_addr[5] << 8;
 
-	lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
-	lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
+	ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
+	if (ret < 0)
+		return ret;
+
+	ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
+	if (ret < 0)
+		return ret;
 
 	/* Added to support MAC address changes */
-	lan78xx_write_reg(dev, MAF_LO(0), addr_lo);
-	lan78xx_write_reg(dev, MAF_HI(0), addr_hi | MAF_HI_VALID_);
+	ret = lan78xx_write_reg(dev, MAF_LO(0), addr_lo);
+	if (ret < 0)
+		return ret;
 
-	return 0;
+	return lan78xx_write_reg(dev, MAF_HI(0), addr_hi | MAF_HI_VALID_);
 }
 
 /* Enable or disable Rx checksum offload engine */
-- 
2.39.5

Re: [PATCH net-next v1 03/11] net: usb: lan78xx: Add error handling to lan78xx_set_mac_addr

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:43PM +0100, Oleksij Rempel wrote:
> Update `lan78xx_set_mac_addr` to handle errors during MAC address
> register write operations. Ensure that errors are properly propagated to
> the caller, improving the robustness of MAC address updates.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v1 04/11] net: usb: lan78xx: Add error handling to lan78xx_get_regs

[Oleksij Rempel]

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.  This ensures that invalid data is not returned to users.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/usb/lan78xx.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 5d318ff8b33d..62445aced7c6 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2112,20 +2112,35 @@ 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);
+	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]);
+			return;
+		}
+	}
 
 	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);
+			return;
+		}
+
+		data[i] = ret;
+	}
 }
 
 static const struct ethtool_ops lan78xx_ethtool_ops = {
-- 
2.39.5

Re: [PATCH net-next v1 04/11] net: usb: lan78xx: Add error handling to lan78xx_get_regs

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:44PM +0100, Oleksij Rempel wrote:
> 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.  This ensures that invalid data is not returned to users.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v1 04/11] net: usb: lan78xx: Add error handling to lan78xx_get_regs

[Jakub Kicinski]

On Mon,  9 Dec 2024 14:07:44 +0100 Oleksij Rempel wrote:
> 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.  This ensures that invalid data is not returned to users.

Should we zero out all the returned regs, too? 
Make it more obvious we failed.

[PATCH net-next v1 05/11] net: usb: lan78xx: Simplify lan78xx_update_reg

[Oleksij Rempel]

Simplify `lan78xx_update_reg` by directly returning the result of
`lan78xx_write_reg`. This eliminates unnecessary checks and improves
code readability.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/usb/lan78xx.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 62445aced7c6..b6e6c090a072 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -674,11 +674,7 @@ static int lan78xx_update_reg(struct lan78xx_net *dev, u32 reg, u32 mask,
 	buf &= ~mask;
 	buf |= (mask & data);
 
-	ret = lan78xx_write_reg(dev, reg, buf);
-	if (ret < 0)
-		return ret;
-
-	return 0;
+	return lan78xx_write_reg(dev, reg, buf);
 }
 
 static int lan78xx_read_stats(struct lan78xx_net *dev,
-- 
2.39.5

Re: [PATCH net-next v1 05/11] net: usb: lan78xx: Simplify lan78xx_update_reg

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:45PM +0100, Oleksij Rempel wrote:
> Simplify `lan78xx_update_reg` by directly returning the result of
> `lan78xx_write_reg`. This eliminates unnecessary checks and improves
> code readability.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v1 06/11] net: usb: lan78xx: Fix return value handling in lan78xx_set_features

[Oleksij Rempel]

Update `lan78xx_set_features` to correctly return the result of
`lan78xx_write_reg`. This ensures that errors during register writes
are propagated to the caller.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 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 b6e6c090a072..2966f7e63617 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -2837,9 +2837,7 @@ static int lan78xx_set_features(struct net_device *netdev,
 
 	spin_unlock_irqrestore(&pdata->rfe_ctl_lock, flags);
 
-	lan78xx_write_reg(dev, RFE_CTL, pdata->rfe_ctl);
-
-	return 0;
+	return lan78xx_write_reg(dev, RFE_CTL, pdata->rfe_ctl);
 }
 
 static void lan78xx_deferred_vlan_write(struct work_struct *param)
-- 
2.39.5

Re: [PATCH net-next v1 06/11] net: usb: lan78xx: Fix return value handling in lan78xx_set_features

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:46PM +0100, Oleksij Rempel wrote:
> Update `lan78xx_set_features` to correctly return the result of
> `lan78xx_write_reg`. This ensures that errors during register writes
> are propagated to the caller.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v1 07/11] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw

[Oleksij Rempel]

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.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 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 2966f7e63617..c66e404f51ac 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)
-- 
2.39.5

Re: [PATCH net-next v1 07/11] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:47PM +0100, Oleksij Rempel wrote:
> 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.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  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 2966f7e63617..c66e404f51ac 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;

I've not looked at the call stack, but tx_complete() and rx_complete()
specifically looks for ETIME. Do they need to change as well?

	Andrew

Re: [PATCH net-next v1 07/11] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw

[Oleksij Rempel]

On Tue, Dec 10, 2024 at 03:08:18AM +0100, Andrew Lunn wrote:
> On Mon, Dec 09, 2024 at 02:07:47PM +0100, Oleksij Rempel wrote:
> > 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.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  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 2966f7e63617..c66e404f51ac 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;
> 
> I've not looked at the call stack, but tx_complete() and rx_complete()
> specifically looks for ETIME. Do they need to change as well?

No, the tx_complete() and rx_complete() will get error values in to the
urb->status from the USB stack. In this case it is a different ETIME.

I'll update commit message.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH net-next v1 08/11] net: usb: lan78xx: Use function-specific label in lan78xx_mac_reset

[Oleksij Rempel]

Rename the generic `done` label to the function-specific
`mac_reset_done` label in `lan78xx_mac_reset`. This improves clarity and
aligns with best practices for error handling and cleanup labels.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 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 c66e404f51ac..fdeb95db529b 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_phy_wait_not_busy(dev);
 	if (ret < 0)
-		goto done;
+		goto mac_reset_done;
 
 	ret = lan78xx_read_reg(dev, MAC_CR, &val);
 	if (ret < 0)
-		goto done;
+		goto mac_reset_done;
 
 	val |= MAC_CR_RST_;
 	ret = lan78xx_write_reg(dev, MAC_CR, val);
 	if (ret < 0)
-		goto done;
+		goto mac_reset_done;
 
 	/* 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 mac_reset_done;
 
 		if (!(val & MAC_CR_RST_)) {
 			ret = 0;
-			goto done;
+			goto mac_reset_done;
 		}
 	} while (!time_after(jiffies, start_time + HZ));
 
 	ret = -ETIMEDOUT;
-done:
+mac_reset_done:
 	mutex_unlock(&dev->phy_mutex);
 
 	return ret;
-- 
2.39.5

Re: [PATCH net-next v1 08/11] net: usb: lan78xx: Use function-specific label in lan78xx_mac_reset

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:48PM +0100, Oleksij Rempel wrote:
> Rename the generic `done` label to the function-specific
> `mac_reset_done` label in `lan78xx_mac_reset`. This improves clarity and
> aligns with best practices for error handling and cleanup labels.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v1 08/11] net: usb: lan78xx: Use function-specific label in lan78xx_mac_reset

[Jakub Kicinski]

On Mon,  9 Dec 2024 14:07:48 +0100 Oleksij Rempel wrote:
> Rename the generic `done` label to the function-specific
> `mac_reset_done` label in `lan78xx_mac_reset`. This improves clarity and
> aligns with best practices for error handling and cleanup labels.

What rules do you refer to?
AFAIK the best practice for exits is to name the label after what
you're jumping to. In this case I'd use exit_unlock.

> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index c66e404f51ac..fdeb95db529b 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_phy_wait_not_busy(dev);
>  	if (ret < 0)
> -		goto done;
> +		goto mac_reset_done;

...

>  	ret = -ETIMEDOUT;
> -done:
> +mac_reset_done:
>  	mutex_unlock(&dev->phy_mutex);
>  
>  	return ret;

[PATCH net-next v1 09/11] net: usb: lan78xx: Improve error handling in lan78xx_phy_wait_not_busy

[Oleksij Rempel]

Update `lan78xx_phy_wait_not_busy` to forward errors from
`lan78xx_read_reg` instead of overwriting them with `-EIO`. Replace
`-EIO` with `-ETIMEDOUT` for timeout cases, providing more specific and
appropriate error codes.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/usb/lan78xx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index fdeb95db529b..9852526be002 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -961,14 +961,14 @@ static int lan78xx_phy_wait_not_busy(struct lan78xx_net *dev)
 
 	do {
 		ret = lan78xx_read_reg(dev, MII_ACC, &val);
-		if (unlikely(ret < 0))
-			return -EIO;
+		if (ret < 0)
+			return ret;
 
 		if (!(val & MII_ACC_MII_BUSY_))
 			return 0;
 	} while (!time_after(jiffies, start_time + HZ));
 
-	return -EIO;
+	return -ETIMEDOUT;
 }
 
 static inline u32 mii_access(int id, int index, int read)
-- 
2.39.5

Re: [PATCH net-next v1 09/11] net: usb: lan78xx: Improve error handling in lan78xx_phy_wait_not_busy

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:49PM +0100, Oleksij Rempel wrote:
> Update `lan78xx_phy_wait_not_busy` to forward errors from
> `lan78xx_read_reg` instead of overwriting them with `-EIO`. Replace
> `-EIO` with `-ETIMEDOUT` for timeout cases, providing more specific and
> appropriate error codes.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/usb/lan78xx.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
> index fdeb95db529b..9852526be002 100644
> --- a/drivers/net/usb/lan78xx.c
> +++ b/drivers/net/usb/lan78xx.c
> @@ -961,14 +961,14 @@ static int lan78xx_phy_wait_not_busy(struct lan78xx_net *dev)
>  
>  	do {
>  		ret = lan78xx_read_reg(dev, MII_ACC, &val);
> -		if (unlikely(ret < 0))
> -			return -EIO;
> +		if (ret < 0)
> +			return ret;
>  
>  		if (!(val & MII_ACC_MII_BUSY_))
>  			return 0;
>  	} while (!time_after(jiffies, start_time + HZ));
>  
> -	return -EIO;
> +	return -ETIMEDOUT;

It would be better to replace this with an iopoll.h helper.

However, the change you made is O.K.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v1 10/11] net: usb: lan78xx: Rename lan78xx_phy_wait_not_busy to lan78xx_mdiobus_wait_not_busy

[Oleksij Rempel]

Rename `lan78xx_phy_wait_not_busy` to `lan78xx_mdiobus_wait_not_busy`
for clarity and accuracy, as the function operates on the MII bus rather
than a specific PHY. Update all references to reflect the new name.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 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 9852526be002..0403aea1a9fa 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -953,7 +953,7 @@ static int lan78xx_flush_rx_fifo(struct lan78xx_net *dev)
 }
 
 /* Loop until the read is completed with timeout called with phy_mutex held */
-static int lan78xx_phy_wait_not_busy(struct lan78xx_net *dev)
+static int lan78xx_mdiobus_wait_not_busy(struct lan78xx_net *dev)
 {
 	unsigned long start_time = jiffies;
 	u32 val;
@@ -1602,7 +1602,7 @@ static int lan78xx_mac_reset(struct lan78xx_net *dev)
 	 * bus can result in the MAC interface locking up and not
 	 * completing register access transactions.
 	 */
-	ret = lan78xx_phy_wait_not_busy(dev);
+	ret = lan78xx_mdiobus_wait_not_busy(dev);
 	if (ret < 0)
 		goto mac_reset_done;
 
@@ -2243,7 +2243,7 @@ static int lan78xx_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
 	mutex_lock(&dev->phy_mutex);
 
 	/* confirm MII not busy */
-	ret = lan78xx_phy_wait_not_busy(dev);
+	ret = lan78xx_mdiobus_wait_not_busy(dev);
 	if (ret < 0)
 		goto done;
 
@@ -2253,7 +2253,7 @@ static int lan78xx_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
 	if (ret < 0)
 		goto done;
 
-	ret = lan78xx_phy_wait_not_busy(dev);
+	ret = lan78xx_mdiobus_wait_not_busy(dev);
 	if (ret < 0)
 		goto done;
 
@@ -2284,7 +2284,7 @@ static int lan78xx_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
 	mutex_lock(&dev->phy_mutex);
 
 	/* confirm MII not busy */
-	ret = lan78xx_phy_wait_not_busy(dev);
+	ret = lan78xx_mdiobus_wait_not_busy(dev);
 	if (ret < 0)
 		goto done;
 
@@ -2299,7 +2299,7 @@ static int lan78xx_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
 	if (ret < 0)
 		goto done;
 
-	ret = lan78xx_phy_wait_not_busy(dev);
+	ret = lan78xx_mdiobus_wait_not_busy(dev);
 	if (ret < 0)
 		goto done;
 
-- 
2.39.5

Re: [PATCH net-next v1 10/11] net: usb: lan78xx: Rename lan78xx_phy_wait_not_busy to lan78xx_mdiobus_wait_not_busy

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:50PM +0100, Oleksij Rempel wrote:
> Rename `lan78xx_phy_wait_not_busy` to `lan78xx_mdiobus_wait_not_busy`
> for clarity and accuracy, as the function operates on the MII bus rather
> than a specific PHY. Update all references to reflect the new name.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net-next v1 11/11] net: usb: lan78xx: Improve error handling in WoL operations

[Oleksij Rempel]

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 <o.rempel@pengutronix.de>
---
 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 0403aea1a9fa..99c19ec1cb88 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 set_wol_done;
 
-	phy_ethtool_set_wol(netdev->phydev, wol);
+	ret = phy_ethtool_set_wol(netdev->phydev, wol);
 
+set_wol_done:
 	usb_autopm_put_interface(dev->intf);
 
 	return ret;
-- 
2.39.5

Re: [PATCH net-next v1 11/11] net: usb: lan78xx: Improve error handling in WoL operations

[Andrew Lunn]

On Mon, Dec 09, 2024 at 02:07:51PM +0100, Oleksij Rempel wrote:
> 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 <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v1 11/11] net: usb: lan78xx: Improve error handling in WoL operations

[Jakub Kicinski]

On Mon,  9 Dec 2024 14:07:51 +0100 Oleksij Rempel wrote:
> +set_wol_done:

exit_pm_put: ?

>  	usb_autopm_put_interface(dev->intf);

Re: [PATCH net-next v1 00/11] lan78xx: Preparations for PHYlink

[Jakub Kicinski]

On Mon,  9 Dec 2024 14:07:40 +0100 Oleksij Rempel wrote:
> This patch set is a second part of the preparatory work for migrating
> the lan78xx USB Ethernet driver to the PHYlink framework. During
> extensive testing, I observed that resetting the USB adapter can lead to
> various read/write errors. While the errors themselves are acceptable,
> they generate excessive log messages, resulting in significant log spam.
> This set improves error handling to reduce logging noise by addressing
> errors directly and returning early when necessary.

These look independent so let me apply most...

Re: [PATCH net-next v1 00/11] lan78xx: Preparations for PHYlink

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  9 Dec 2024 14:07:40 +0100 you wrote:
> This patch set is a second part of the preparatory work for migrating
> the lan78xx USB Ethernet driver to the PHYlink framework. During
> extensive testing, I observed that resetting the USB adapter can lead to
> various read/write errors. While the errors themselves are acceptable,
> they generate excessive log messages, resulting in significant log spam.
> This set improves error handling to reduce logging noise by addressing
> errors directly and returning early when necessary.
> 
> [...]

Here is the summary with links:
  - [net-next,v1,01/11] net: usb: lan78xx: Add error handling to lan78xx_setup_irq_domain
    https://git.kernel.org/netdev/net-next/c/d354d008255f
  - [net-next,v1,02/11] net: usb: lan78xx: Add error handling to lan78xx_init_mac_address
    https://git.kernel.org/netdev/net-next/c/6f31135894ec
  - [net-next,v1,03/11] net: usb: lan78xx: Add error handling to lan78xx_set_mac_addr
    https://git.kernel.org/netdev/net-next/c/9a46956c72cb
  - [net-next,v1,04/11] net: usb: lan78xx: Add error handling to lan78xx_get_regs
    (no matching commit)
  - [net-next,v1,05/11] net: usb: lan78xx: Simplify lan78xx_update_reg
    https://git.kernel.org/netdev/net-next/c/41b774e4f327
  - [net-next,v1,06/11] net: usb: lan78xx: Fix return value handling in lan78xx_set_features
    https://git.kernel.org/netdev/net-next/c/bf361b18d91e
  - [net-next,v1,07/11] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw
    (no matching commit)
  - [net-next,v1,08/11] net: usb: lan78xx: Use function-specific label in lan78xx_mac_reset
    (no matching commit)
  - [net-next,v1,09/11] net: usb: lan78xx: Improve error handling in lan78xx_phy_wait_not_busy
    https://git.kernel.org/netdev/net-next/c/21fff45a6cc1
  - [net-next,v1,10/11] net: usb: lan78xx: Rename lan78xx_phy_wait_not_busy to lan78xx_mdiobus_wait_not_busy
    (no matching commit)
  - [net-next,v1,11/11] net: usb: lan78xx: Improve error handling in WoL operations
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html