[PATCH net-next 2/3] net: dsa: yt921x: Return early for failed MIB read

David Yang posted 3 patches 4 days, 16 hours ago
[PATCH net-next 2/3] net: dsa: yt921x: Return early for failed MIB read
Posted by David Yang 4 days, 16 hours ago
This patch does not change anything effectively, but serves as a
prerequisite for another patch.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/yt921x.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
index ce2878ef9b24..b9f507b07d1f 100644
--- a/drivers/net/dsa/yt921x.c
+++ b/drivers/net/dsa/yt921x.c
@@ -714,6 +714,11 @@ static int yt921x_read_mib(struct yt921x_priv *priv, int port)
 		WRITE_ONCE(*valp, val);
 	}
 
+	if (res) {
+		print_port_err(dev, port, res, "read stats for");
+		return res;
+	}
+
 	pp->rx_frames = mib->rx_64byte + mib->rx_65_127byte +
 			mib->rx_128_255byte + mib->rx_256_511byte +
 			mib->rx_512_1023byte + mib->rx_1024_1518byte +
@@ -723,9 +728,7 @@ static int yt921x_read_mib(struct yt921x_priv *priv, int port)
 			mib->tx_512_1023byte + mib->tx_1024_1518byte +
 			mib->tx_jumbo;
 
-	if (res)
-		print_port_err(dev, port, res, "read stats for");
-	return res;
+	return 0;
 }
 
 static void yt921x_poll_mib(struct work_struct *work)
-- 
2.51.0
Re: [net-next,2/3] net: dsa: yt921x: Return early for failed MIB read
Posted by Jakub Kicinski 3 days, 4 hours ago
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: yt921x: Return early for failed MIB read

This patch refactors error handling in yt921x_read_mib() by checking
for read failures earlier and returning immediately, before attempting
to compute statistics from potentially incomplete data.

> net: dsa: yt921x: Return early for failed MIB read
>
> This patch does not change anything effectively, but serves as a
> prerequisite for another patch.

The claim "This patch does not change anything effectively" is not
accurate. The patch changes the behavior when MIB register reads fail.

Before the patch: If yt921x_reg_read() fails partway through the loop,
the function would still compute pp->rx_frames and pp->tx_frames using
the partially-read MIB values before returning the error code. The frame
counts would contain a mix of old values (for registers read before the
error) and new values (for registers read successfully).

After the patch: If yt921x_reg_read() fails, the function returns
immediately without updating pp->rx_frames and pp->tx_frames, leaving
them at their previous values from the last successful read.

This affects how five callers behave when reads fail:
  - yt921x_dsa_get_pause_stats()
  - yt921x_dsa_get_eth_mac_stats()
  - yt921x_dsa_get_rmon_stats()
  - yt921x_dsa_get_eth_ctrl_stats()
  - yt921x_dsa_get_ethtool_stats()

These functions don't check the return value, so they will now report
stale data from the last successful read instead of partially-updated
data from a failed read.

Should the commit message be updated to accurately describe this
behavior change?

> diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> --- a/drivers/net/dsa/yt921x.c
> +++ b/drivers/net/dsa/yt921x.c
> @@ -714,6 +714,11 @@ static int yt921x_read_mib(struct yt921x_priv *priv, int port)
>  		WRITE_ONCE(*valp, val);
>  	}
>
> +	if (res) {
> +		print_port_err(dev, port, res, "read stats for");
> +		return res;
> +	}
> +
>  	pp->rx_frames = mib->rx_64byte + mib->rx_65_127byte +
>  			mib->rx_128_255byte + mib->rx_256_511byte +
>  			mib->rx_512_1023byte + mib->rx_1024_1518byte +
> @@ -723,9 +728,7 @@
>  			mib->tx_512_1023byte + mib->tx_1024_1518byte +
>  			mib->tx_jumbo;
>
> -	if (res)
> -		print_port_err(dev, port, res, "read stats for");
> -	return res;
> +	return 0;
>  }
-- 
pw-bot: cr