* [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink
@ 2024-12-16 12:09 Oleksij Rempel
2024-12-16 12:09 ` [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs Oleksij Rempel
` (6 more replies)
0 siblings, 7 replies; 13+ messages in thread
From: Oleksij Rempel @ 2024-12-16 12:09 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
This patch set is a third 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 (6):
net: usb: lan78xx: Add error handling to lan78xx_get_regs
net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw
net: usb: lan78xx: Use action-specific label in lan78xx_mac_reset
net: usb: lan78xx: rename phy_mutex to mdiobus_mutex
net: usb: lan78xx: remove PHY register access from ethtool get_regs
net: usb: lan78xx: Improve error handling in WoL operations
drivers/net/usb/lan78xx.c | 73 +++++++++++++++++++++------------------
1 file changed, 40 insertions(+), 33 deletions(-)
--
2.39.5
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs
2024-12-16 12:09 [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink Oleksij Rempel
@ 2024-12-16 12:09 ` Oleksij Rempel
2024-12-16 13:07 ` Mateusz Polchlopek
2024-12-16 12:09 ` [PATCH net-next v1 2/6] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw Oleksij Rempel
` (5 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Oleksij Rempel @ 2024-12-16 12:09 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
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 <o.rempel@pengutronix.de>
---
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 = {
--
2.39.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net-next v1 2/6] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw
2024-12-16 12:09 [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink Oleksij Rempel
2024-12-16 12:09 ` [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs Oleksij Rempel
@ 2024-12-16 12:09 ` Oleksij Rempel
2024-12-16 13:08 ` Mateusz Polchlopek
2024-12-16 12:09 ` [PATCH net-next v1 3/6] net: usb: lan78xx: Use action-specific label in lan78xx_mac_reset Oleksij Rempel
` (4 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Oleksij Rempel @ 2024-12-16 12:09 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
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 <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 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)
--
2.39.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net-next v1 3/6] net: usb: lan78xx: Use action-specific label in lan78xx_mac_reset
2024-12-16 12:09 [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink Oleksij Rempel
2024-12-16 12:09 ` [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs Oleksij Rempel
2024-12-16 12:09 ` [PATCH net-next v1 2/6] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw Oleksij Rempel
@ 2024-12-16 12:09 ` Oleksij Rempel
2024-12-16 13:13 ` Mateusz Polchlopek
2024-12-16 12:09 ` [PATCH net-next v1 4/6] net: usb: lan78xx: rename phy_mutex to mdiobus_mutex Oleksij Rempel
` (3 subsequent siblings)
6 siblings, 1 reply; 13+ messages in thread
From: Oleksij Rempel @ 2024-12-16 12:09 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn
Cc: Oleksij Rempel, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
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 <o.rempel@pengutronix.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
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;
--
2.39.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net-next v1 4/6] net: usb: lan78xx: rename phy_mutex to mdiobus_mutex
2024-12-16 12:09 [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink Oleksij Rempel
` (2 preceding siblings ...)
2024-12-16 12:09 ` [PATCH net-next v1 3/6] net: usb: lan78xx: Use action-specific label in lan78xx_mac_reset Oleksij Rempel
@ 2024-12-16 12:09 ` Oleksij Rempel
2024-12-16 12:09 ` [PATCH net-next v1 5/6] net: usb: lan78xx: remove PHY register access from ethtool get_regs Oleksij Rempel
` (2 subsequent siblings)
6 siblings, 0 replies; 13+ messages in thread
From: Oleksij Rempel @ 2024-12-16 12:09 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
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 <o.rempel@pengutronix.de>
---
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);
--
2.39.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net-next v1 5/6] net: usb: lan78xx: remove PHY register access from ethtool get_regs
2024-12-16 12:09 [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink Oleksij Rempel
` (3 preceding siblings ...)
2024-12-16 12:09 ` [PATCH net-next v1 4/6] net: usb: lan78xx: rename phy_mutex to mdiobus_mutex Oleksij Rempel
@ 2024-12-16 12:09 ` Oleksij Rempel
2024-12-16 12:09 ` [PATCH net-next v1 6/6] net: usb: lan78xx: Improve error handling in WoL operations Oleksij Rempel
2024-12-18 4:10 ` [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink patchwork-bot+netdevbpf
6 siblings, 0 replies; 13+ messages in thread
From: Oleksij Rempel @ 2024-12-16 12:09 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn
Cc: Oleksij Rempel, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
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 <o.rempel@pengutronix.de>
---
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:
--
2.39.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH net-next v1 6/6] net: usb: lan78xx: Improve error handling in WoL operations
2024-12-16 12:09 [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink Oleksij Rempel
` (4 preceding siblings ...)
2024-12-16 12:09 ` [PATCH net-next v1 5/6] net: usb: lan78xx: remove PHY register access from ethtool get_regs Oleksij Rempel
@ 2024-12-16 12:09 ` Oleksij Rempel
2024-12-18 4:10 ` [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink patchwork-bot+netdevbpf
6 siblings, 0 replies; 13+ messages in thread
From: Oleksij Rempel @ 2024-12-16 12:09 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn
Cc: Oleksij Rempel, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
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>
---
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;
--
2.39.5
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs
2024-12-16 12:09 ` [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs Oleksij Rempel
@ 2024-12-16 13:07 ` Mateusz Polchlopek
2024-12-16 13:19 ` Oleksij Rempel
0 siblings, 1 reply; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-12-16 13:07 UTC (permalink / raw)
To: Oleksij Rempel, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn
Cc: kernel, linux-kernel, netdev, UNGLinuxDriver, Phil Elwell
On 12/16/2024 1:09 PM, 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. 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 <o.rempel@pengutronix.de>
> ---
> 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++) {
Maybe during this refactor is it worth to add some #define with
number of registers to be read?
> + 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 = {
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v1 2/6] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw
2024-12-16 12:09 ` [PATCH net-next v1 2/6] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw Oleksij Rempel
@ 2024-12-16 13:08 ` Mateusz Polchlopek
0 siblings, 0 replies; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-12-16 13:08 UTC (permalink / raw)
To: Oleksij Rempel, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn
Cc: kernel, linux-kernel, netdev, UNGLinuxDriver, Phil Elwell
On 12/16/2024 1:09 PM, 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.
>
> 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 <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 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)
This looks good, nice cleanup.
Reviewed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v1 3/6] net: usb: lan78xx: Use action-specific label in lan78xx_mac_reset
2024-12-16 12:09 ` [PATCH net-next v1 3/6] net: usb: lan78xx: Use action-specific label in lan78xx_mac_reset Oleksij Rempel
@ 2024-12-16 13:13 ` Mateusz Polchlopek
0 siblings, 0 replies; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-12-16 13:13 UTC (permalink / raw)
To: Oleksij Rempel, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Woojung Huh, Andrew Lunn
Cc: Andrew Lunn, kernel, linux-kernel, netdev, UNGLinuxDriver,
Phil Elwell
On 12/16/2024 1:09 PM, Oleksij Rempel wrote:
[...]
> 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;
Nice cleanup, now the label indicates what it does.
Reviewed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs
2024-12-16 13:07 ` Mateusz Polchlopek
@ 2024-12-16 13:19 ` Oleksij Rempel
2024-12-16 13:35 ` Mateusz Polchlopek
0 siblings, 1 reply; 13+ messages in thread
From: Oleksij Rempel @ 2024-12-16 13:19 UTC (permalink / raw)
To: Mateusz Polchlopek
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On Mon, Dec 16, 2024 at 02:07:37PM +0100, Mateusz Polchlopek wrote:
>
>
> On 12/16/2024 1:09 PM, Oleksij Rempel wrote:
> > 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++) {
>
> Maybe during this refactor is it worth to add some #define with
> number of registers to be read?
I prefer to remove this part. Please see patch 5
--
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 |
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs
2024-12-16 13:19 ` Oleksij Rempel
@ 2024-12-16 13:35 ` Mateusz Polchlopek
0 siblings, 0 replies; 13+ messages in thread
From: Mateusz Polchlopek @ 2024-12-16 13:35 UTC (permalink / raw)
To: Oleksij Rempel
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Woojung Huh, Andrew Lunn, kernel, linux-kernel, netdev,
UNGLinuxDriver, Phil Elwell
On 12/16/2024 2:19 PM, Oleksij Rempel wrote:
> On Mon, Dec 16, 2024 at 02:07:37PM +0100, Mateusz Polchlopek wrote:
>>
>>
>> On 12/16/2024 1:09 PM, Oleksij Rempel wrote:
>>> 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++) {
>>
>> Maybe during this refactor is it worth to add some #define with
>> number of registers to be read?
>
> I prefer to remove this part. Please see patch 5
>
Ach, now I see that. So please forget about my comment
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink
2024-12-16 12:09 [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink Oleksij Rempel
` (5 preceding siblings ...)
2024-12-16 12:09 ` [PATCH net-next v1 6/6] net: usb: lan78xx: Improve error handling in WoL operations Oleksij Rempel
@ 2024-12-18 4:10 ` patchwork-bot+netdevbpf
6 siblings, 0 replies; 13+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-12-18 4:10 UTC (permalink / raw)
To: Oleksij Rempel
Cc: davem, edumazet, kuba, pabeni, woojung.huh, andrew+netdev, kernel,
linux-kernel, netdev, UNGLinuxDriver, phil
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Mon, 16 Dec 2024 13:09:35 +0100 you wrote:
> This patch set is a third 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,1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs
https://git.kernel.org/netdev/net-next/c/30c63abaee90
- [net-next,v1,2/6] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw
https://git.kernel.org/netdev/net-next/c/18bdefe62439
- [net-next,v1,3/6] net: usb: lan78xx: Use action-specific label in lan78xx_mac_reset
https://git.kernel.org/netdev/net-next/c/7433d022b915
- [net-next,v1,4/6] net: usb: lan78xx: rename phy_mutex to mdiobus_mutex
https://git.kernel.org/netdev/net-next/c/3a59437ed907
- [net-next,v1,5/6] net: usb: lan78xx: remove PHY register access from ethtool get_regs
https://git.kernel.org/netdev/net-next/c/d09de7ebd4ab
- [net-next,v1,6/6] net: usb: lan78xx: Improve error handling in WoL operations
https://git.kernel.org/netdev/net-next/c/01e2f4d55bda
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-12-18 4:10 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-16 12:09 [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink Oleksij Rempel
2024-12-16 12:09 ` [PATCH net-next v1 1/6] net: usb: lan78xx: Add error handling to lan78xx_get_regs Oleksij Rempel
2024-12-16 13:07 ` Mateusz Polchlopek
2024-12-16 13:19 ` Oleksij Rempel
2024-12-16 13:35 ` Mateusz Polchlopek
2024-12-16 12:09 ` [PATCH net-next v1 2/6] net: usb: lan78xx: Use ETIMEDOUT instead of ETIME in lan78xx_stop_hw Oleksij Rempel
2024-12-16 13:08 ` Mateusz Polchlopek
2024-12-16 12:09 ` [PATCH net-next v1 3/6] net: usb: lan78xx: Use action-specific label in lan78xx_mac_reset Oleksij Rempel
2024-12-16 13:13 ` Mateusz Polchlopek
2024-12-16 12:09 ` [PATCH net-next v1 4/6] net: usb: lan78xx: rename phy_mutex to mdiobus_mutex Oleksij Rempel
2024-12-16 12:09 ` [PATCH net-next v1 5/6] net: usb: lan78xx: remove PHY register access from ethtool get_regs Oleksij Rempel
2024-12-16 12:09 ` [PATCH net-next v1 6/6] net: usb: lan78xx: Improve error handling in WoL operations Oleksij Rempel
2024-12-18 4:10 ` [PATCH net-next v1 0/6] lan78xx: Preparations for PHYlink patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).