[PATCH net v4] net: airoha: fix MIB stats collection to be lossless

[PATCH net v4] net: airoha: fix MIB stats collection to be lossless

[Aniket Negi]

REG_FE_GDM_MIB_CLEAR after every read creates a race window where
packets arriving between read and clear are lost from statistics.

Switch to a delta-based approach instead:

- 64-bit H+L registers (ok pkts/bytes, E64..L1023): read absolute
  hardware total directly; use a local variable and max(new, old)
  clamping to prevent intermediate visibility and torn-read regression.

- 32-bit registers (drops, bc, mc, errors, runt, long): accumulate
  (u32)(curr - prev) into a 64-bit software counter; unsigned
  subtraction handles wrap-around transparently.

- tx/rx_len[0] ([0,64] bucket): combines RUNT_CNT (32-bit, delta via
  tx_runt/rx_runt) and E64_CNT (64-bit, absolute) into a single
  assignment using a local accumulator to avoid double-counting.

Clear MIB counters once in airoha_fe_init() to establish a clean
baseline, preventing spurious stats from pre-driver activity (kexec,
driver rebind, warm reboot).

Merge airoha_dev_get_hw_stats() into airoha_update_hw_stats() and
move stats_lock inside. Plain spin_lock() is correct: the function
is only called from ndo_get_stats64() in process context. Each dev
refreshes only its own MIB counters; sibling devs on a shared GDM3/4
port are polled when their own netdev is queried.

Fixes: 8f4695fb67b2 ("net: airoha: better handle MIBs for GDM ports with multiple devs attached")
Signed-off-by: Aniket Negi <aniket.negi03@gmail.com>
Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Changes in v4:
- Add max(new, old) clamping for 64-bit H+L register pairs to ensure
  monotonically non-decreasing stats despite torn reads between H and L
- Use local variable (tmp) for all 64-bit H+L computations to prevent
  lockless readers from seeing intermediate values during piecewise write
- Add one-shot MIB counter clear in airoha_fe_init() to establish a
  clean baseline (kexec, driver rebind, warm reboot)
- Document sibling dev polling design in commit message

Changes in v3:
- Link to V2: https://lore.kernel.org/20260701173941.314795-1-aniket.negi03@gmail.com/
- Add Acked-by tag from Lorenzo
- Rename from tx_runt_cnt to tx_runt, tx_long_cnt to tx_long,
  tx_runt_accum64 to tx_runt64
- Rename from rx_runt_cnt to rx_runt, rx_long_cnt to rx_long,
  rx_runt_accum64 to rx_runt64
- Condense the marked comments in V2, remove new line after comment

Changes in v2:
- Store _CNT_L register reads in val before adding to stats, improving
  readability (suggested by Lorenzo Bianconi)
- Fix double-counting bug in the RUNT+E64 combined bucket: previously
  "+=" for E64 re-added the full absolute counter each poll; now a
  dedicated tx_runt_accum64/rx_runt_accum64 accumulator holds the
  running RUNT delta, and tx_len[0] is assigned (not accumulated) each
  poll as runt_accum64 + E64_abs
- Replace 7-element tx_len[]/rx_len[] shadow arrays in mib_prev with
  focused tx_runt_cnt/tx_long_cnt and rx_runt_cnt/rx_long_cnt fields;
  only RUNT and LONG are 32-bit and need wrap-around tracking
- Rename inner struct hw_prev_stats to mib_prev; rename accumulator
  fields to tx_runt_accum64/rx_runt_accum64 for clarity
- Fix comment alignment in mib_prev struct block
- Rename airoha_dev_get_hw_stats() to airoha_update_hw_stats() and
  move the port spin_lock inside, removing the separate wrapper

 drivers/net/ethernet/airoha/airoha_eth.c | 183 +++++++++++++++--------
 drivers/net/ethernet/airoha/airoha_eth.h |  27 ++++
 2 files changed, 144 insertions(+), 66 deletions(-)

diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
index 59001fd4b6f7..2d032cc6dfca 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.c
+++ b/drivers/net/ethernet/airoha/airoha_eth.c
@@ -493,6 +493,8 @@ static void airoha_fe_crsn_qsel_init(struct airoha_eth *eth)
 
 static int airoha_fe_init(struct airoha_eth *eth)
 {
+	int i;
+
 	airoha_fe_maccr_init(eth);
 
 	/* PSE IQ reserve */
@@ -586,6 +588,14 @@ static int airoha_fe_init(struct airoha_eth *eth)
 	/* enable 1:N vlan action, init vlan table */
 	airoha_fe_set(eth, REG_MC_VLAN_EN, MC_VLAN_EN_MASK);
 
+	/* Clear MIB counters to establish clean baseline for delta tracking.
+	 * This prevents spurious statistics from pre-driver activity (e.g.,
+	 * kexec, driver rebind, warm reboot) on first poll.
+	 */
+	for (i = AIROHA_GDM1_IDX; i <= AIROHA_GDM4_IDX; i++)
+		airoha_fe_set(eth, REG_FE_GDM_MIB_CLEAR(i),
+			      FE_GDM_MIB_RX_CLEAR_MASK | FE_GDM_MIB_TX_CLEAR_MASK);
+
 	return airoha_fe_mc_vlan_clear(eth);
 }
 
@@ -1686,11 +1696,14 @@ static void airoha_qdma_stop_napi(struct airoha_qdma *qdma)
 	}
 }
 
-static void airoha_dev_get_hw_stats(struct airoha_gdm_dev *dev)
+static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
 {
 	struct airoha_gdm_port *port = dev->port;
 	struct airoha_eth *eth = dev->eth;
 	u32 val, i = 0;
+	u64 tmp, prev;
+
+	spin_lock(&port->stats_lock);
 
 	/* Read relevant MIB for GDM with multiple port attached */
 	if (port->id == AIROHA_GDM3_IDX || port->id == AIROHA_GDM4_IDX)
@@ -1701,152 +1714,190 @@ static void airoha_dev_get_hw_stats(struct airoha_gdm_dev *dev)
 
 	u64_stats_update_begin(&dev->stats.syncp);
 
-	/* TX */
+	/* TX - 64-bit H+L registers: hw accumulates the total, read directly.
+	 * Use local variable to prevent readers from seeing intermediate values.
+	 * Clamp to prevent regression from torn reads between H and L.
+	 */
+	prev = dev->stats.tx_ok_pkts;
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_H(port->id));
-	dev->stats.tx_ok_pkts += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_L(port->id));
-	dev->stats.tx_ok_pkts += val;
+	tmp |= val;
+	dev->stats.tx_ok_pkts = max(tmp, prev);
 
+	prev = dev->stats.tx_ok_bytes;
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_BYTE_CNT_H(port->id));
-	dev->stats.tx_ok_bytes += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_BYTE_CNT_L(port->id));
-	dev->stats.tx_ok_bytes += val;
+	tmp |= val;
+	dev->stats.tx_ok_bytes = max(tmp, prev);
 
+	/* TX - 32-bit registers: accumulate delta to handle wrap-around. */
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_DROP_CNT(port->id));
-	dev->stats.tx_drops += val;
+	dev->stats.tx_drops += (u32)(val - dev->stats.mib_prev.tx_drops);
+	dev->stats.mib_prev.tx_drops = val;
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_BC_CNT(port->id));
-	dev->stats.tx_broadcast += val;
+	dev->stats.tx_broadcast += (u32)(val - dev->stats.mib_prev.tx_broadcast);
+	dev->stats.mib_prev.tx_broadcast = val;
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_MC_CNT(port->id));
-	dev->stats.tx_multicast += val;
+	dev->stats.tx_multicast += (u32)(val - dev->stats.mib_prev.tx_multicast);
+	dev->stats.mib_prev.tx_multicast = val;
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_RUNT_CNT(port->id));
-	dev->stats.tx_len[i] += val;
+	dev->stats.mib_prev.tx_runt64 +=
+		(u32)(val - dev->stats.mib_prev.tx_runt);
+	dev->stats.mib_prev.tx_runt = val;
 
+	/* tx_len[0]: RUNT (32-bit, delta) + E64 (64-bit, absolute). */
+	tmp = dev->stats.mib_prev.tx_runt64;
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_E64_CNT_H(port->id));
-	dev->stats.tx_len[i] += ((u64)val << 32);
+	tmp += ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_E64_CNT_L(port->id));
-	dev->stats.tx_len[i++] += val;
+	tmp += val;
+	dev->stats.tx_len[i++] = tmp;
 
+	prev = dev->stats.tx_len[i];
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L64_CNT_H(port->id));
-	dev->stats.tx_len[i] += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L64_CNT_L(port->id));
-	dev->stats.tx_len[i++] += val;
+	tmp |= val;
+	dev->stats.tx_len[i++] = max(tmp, prev);
 
+	prev = dev->stats.tx_len[i];
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L127_CNT_H(port->id));
-	dev->stats.tx_len[i] += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L127_CNT_L(port->id));
-	dev->stats.tx_len[i++] += val;
+	tmp |= val;
+	dev->stats.tx_len[i++] = max(tmp, prev);
 
+	prev = dev->stats.tx_len[i];
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L255_CNT_H(port->id));
-	dev->stats.tx_len[i] += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L255_CNT_L(port->id));
-	dev->stats.tx_len[i++] += val;
+	tmp |= val;
+	dev->stats.tx_len[i++] = max(tmp, prev);
 
+	prev = dev->stats.tx_len[i];
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L511_CNT_H(port->id));
-	dev->stats.tx_len[i] += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L511_CNT_L(port->id));
-	dev->stats.tx_len[i++] += val;
+	tmp |= val;
+	dev->stats.tx_len[i++] = max(tmp, prev);
 
+	prev = dev->stats.tx_len[i];
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L1023_CNT_H(port->id));
-	dev->stats.tx_len[i] += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L1023_CNT_L(port->id));
-	dev->stats.tx_len[i++] += val;
+	tmp |= val;
+	dev->stats.tx_len[i++] = max(tmp, prev);
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_LONG_CNT(port->id));
-	dev->stats.tx_len[i++] += val;
+	dev->stats.tx_len[i++] += (u32)(val - dev->stats.mib_prev.tx_long);
+	dev->stats.mib_prev.tx_long = val;
 
 	/* RX */
+	prev = dev->stats.rx_ok_pkts;
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_OK_PKT_CNT_H(port->id));
-	dev->stats.rx_ok_pkts += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_OK_PKT_CNT_L(port->id));
-	dev->stats.rx_ok_pkts += val;
+	tmp |= val;
+	dev->stats.rx_ok_pkts = max(tmp, prev);
 
+	prev = dev->stats.rx_ok_bytes;
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_OK_BYTE_CNT_H(port->id));
-	dev->stats.rx_ok_bytes += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_OK_BYTE_CNT_L(port->id));
-	dev->stats.rx_ok_bytes += val;
+	tmp |= val;
+	dev->stats.rx_ok_bytes = max(tmp, prev);
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_DROP_CNT(port->id));
-	dev->stats.rx_drops += val;
+	dev->stats.rx_drops += (u32)(val - dev->stats.mib_prev.rx_drops);
+	dev->stats.mib_prev.rx_drops = val;
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_BC_CNT(port->id));
-	dev->stats.rx_broadcast += val;
+	dev->stats.rx_broadcast += (u32)(val - dev->stats.mib_prev.rx_broadcast);
+	dev->stats.mib_prev.rx_broadcast = val;
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_MC_CNT(port->id));
-	dev->stats.rx_multicast += val;
+	dev->stats.rx_multicast += (u32)(val - dev->stats.mib_prev.rx_multicast);
+	dev->stats.mib_prev.rx_multicast = val;
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ERROR_DROP_CNT(port->id));
-	dev->stats.rx_errors += val;
+	dev->stats.rx_errors += (u32)(val - dev->stats.mib_prev.rx_errors);
+	dev->stats.mib_prev.rx_errors = val;
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_CRC_ERR_CNT(port->id));
-	dev->stats.rx_crc_error += val;
+	dev->stats.rx_crc_error += (u32)(val - dev->stats.mib_prev.rx_crc_error);
+	dev->stats.mib_prev.rx_crc_error = val;
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_OVERFLOW_DROP_CNT(port->id));
-	dev->stats.rx_over_errors += val;
+	dev->stats.rx_over_errors += (u32)(val - dev->stats.mib_prev.rx_over_errors);
+	dev->stats.mib_prev.rx_over_errors = val;
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_FRAG_CNT(port->id));
-	dev->stats.rx_fragment += val;
+	dev->stats.rx_fragment += (u32)(val - dev->stats.mib_prev.rx_fragment);
+	dev->stats.mib_prev.rx_fragment = val;
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_JABBER_CNT(port->id));
-	dev->stats.rx_jabber += val;
+	dev->stats.rx_jabber += (u32)(val - dev->stats.mib_prev.rx_jabber);
+	dev->stats.mib_prev.rx_jabber = val;
 
 	i = 0;
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_RUNT_CNT(port->id));
-	dev->stats.rx_len[i] += val;
+	dev->stats.mib_prev.rx_runt64 +=
+		(u32)(val - dev->stats.mib_prev.rx_runt);
+	dev->stats.mib_prev.rx_runt = val;
 
+	/* rx_len[0]: RUNT (32-bit, delta) + E64 (64-bit, absolute). */
+	tmp = dev->stats.mib_prev.rx_runt64;
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_E64_CNT_H(port->id));
-	dev->stats.rx_len[i] += ((u64)val << 32);
+	tmp += ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_E64_CNT_L(port->id));
-	dev->stats.rx_len[i++] += val;
+	tmp += val;
+	dev->stats.rx_len[i++] = tmp;
 
+	prev = dev->stats.rx_len[i];
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L64_CNT_H(port->id));
-	dev->stats.rx_len[i] += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L64_CNT_L(port->id));
-	dev->stats.rx_len[i++] += val;
+	tmp |= val;
+	dev->stats.rx_len[i++] = max(tmp, prev);
 
+	prev = dev->stats.rx_len[i];
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L127_CNT_H(port->id));
-	dev->stats.rx_len[i] += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L127_CNT_L(port->id));
-	dev->stats.rx_len[i++] += val;
+	tmp |= val;
+	dev->stats.rx_len[i++] = max(tmp, prev);
 
+	prev = dev->stats.rx_len[i];
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L255_CNT_H(port->id));
-	dev->stats.rx_len[i] += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L255_CNT_L(port->id));
-	dev->stats.rx_len[i++] += val;
+	tmp |= val;
+	dev->stats.rx_len[i++] = max(tmp, prev);
 
+	prev = dev->stats.rx_len[i];
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L511_CNT_H(port->id));
-	dev->stats.rx_len[i] += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L511_CNT_L(port->id));
-	dev->stats.rx_len[i++] += val;
+	tmp |= val;
+	dev->stats.rx_len[i++] = max(tmp, prev);
 
+	prev = dev->stats.rx_len[i];
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L1023_CNT_H(port->id));
-	dev->stats.rx_len[i] += ((u64)val << 32);
+	tmp = ((u64)val << 32);
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L1023_CNT_L(port->id));
-	dev->stats.rx_len[i++] += val;
+	tmp |= val;
+	dev->stats.rx_len[i++] = max(tmp, prev);
 
 	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_LONG_CNT(port->id));
-	dev->stats.rx_len[i++] += val;
+	dev->stats.rx_len[i] += (u32)(val - dev->stats.mib_prev.rx_long);
+	dev->stats.mib_prev.rx_long = val;
 
 	u64_stats_update_end(&dev->stats.syncp);
-}
-
-static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
-{
-	struct airoha_gdm_port *port = dev->port;
-	int i;
-
-	spin_lock(&port->stats_lock);
-
-	for (i = 0; i < ARRAY_SIZE(port->devs); i++) {
-		if (port->devs[i])
-			airoha_dev_get_hw_stats(port->devs[i]);
-	}
-
-	/* Reset MIB counters */
-	airoha_fe_set(dev->eth, REG_FE_GDM_MIB_CLEAR(port->id),
-		      FE_GDM_MIB_RX_CLEAR_MASK | FE_GDM_MIB_TX_CLEAR_MASK);
 
 	spin_unlock(&port->stats_lock);
 }
diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
index f6d01a8e8da1..fe934f9ffe8a 100644
--- a/drivers/net/ethernet/airoha/airoha_eth.h
+++ b/drivers/net/ethernet/airoha/airoha_eth.h
@@ -245,6 +245,33 @@ struct airoha_hw_stats {
 	u64 rx_fragment;
 	u64 rx_jabber;
 	u64 rx_len[7];
+
+	struct {
+		/* Previous HW register values for 32-bit counter delta
+		 * tracking. Storing the last seen value and accumulating
+		 * (u32)(curr - prev) into the 64-bit software counter
+		 * handles wrap-around transparently via unsigned arithmetic.
+		 * tx_runt64/rx_runt64 hold the running sum of runt deltas.
+		 * These fields are never reported to userspace.
+		 */
+		u32 tx_drops;
+		u32 tx_broadcast;
+		u32 tx_multicast;
+		u32 tx_runt;
+		u32 tx_long;
+		u64 tx_runt64;
+		u32 rx_drops;
+		u32 rx_broadcast;
+		u32 rx_multicast;
+		u32 rx_errors;
+		u32 rx_crc_error;
+		u32 rx_over_errors;
+		u32 rx_fragment;
+		u32 rx_jabber;
+		u32 rx_runt;
+		u32 rx_long;
+		u64 rx_runt64;
+	} mib_prev;
 };
 
 enum {

base-commit: a225f8c20712713406ae47024b8df42deacddd4a
-- 
2.43.0

Re: [PATCH net v4] net: airoha: fix MIB stats collection to be lossless

[Aniket Negi]

Hi Lorenzo,

I noticed I forgot to include the link to V3 in the v4 changelog.
V3 can be found at:
https://lore.kernel.org/20260702171814.11165-1-aniket.negi03@gmail.com/

Sorry for the oversight.

Regards,
Aniket

Re: [PATCH net v4] net: airoha: fix MIB stats collection to be lossless

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 17239 bytes --]

> REG_FE_GDM_MIB_CLEAR after every read creates a race window where
> packets arriving between read and clear are lost from statistics.
> 
> Switch to a delta-based approach instead:
> 
> - 64-bit H+L registers (ok pkts/bytes, E64..L1023): read absolute
>   hardware total directly; use a local variable and max(new, old)
>   clamping to prevent intermediate visibility and torn-read regression.
> 
> - 32-bit registers (drops, bc, mc, errors, runt, long): accumulate
>   (u32)(curr - prev) into a 64-bit software counter; unsigned
>   subtraction handles wrap-around transparently.
> 
> - tx/rx_len[0] ([0,64] bucket): combines RUNT_CNT (32-bit, delta via
>   tx_runt/rx_runt) and E64_CNT (64-bit, absolute) into a single
>   assignment using a local accumulator to avoid double-counting.
> 
> Clear MIB counters once in airoha_fe_init() to establish a clean
> baseline, preventing spurious stats from pre-driver activity (kexec,
> driver rebind, warm reboot).
> 
> Merge airoha_dev_get_hw_stats() into airoha_update_hw_stats() and
> move stats_lock inside. Plain spin_lock() is correct: the function
> is only called from ndo_get_stats64() in process context. Each dev
> refreshes only its own MIB counters; sibling devs on a shared GDM3/4
> port are polled when their own netdev is queried.
> 
> Fixes: 8f4695fb67b2 ("net: airoha: better handle MIBs for GDM ports with multiple devs attached")
> Signed-off-by: Aniket Negi <aniket.negi03@gmail.com>
> Acked-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
> Changes in v4:
> - Add max(new, old) clamping for 64-bit H+L register pairs to ensure
>   monotonically non-decreasing stats despite torn reads between H and L
> - Use local variable (tmp) for all 64-bit H+L computations to prevent
>   lockless readers from seeing intermediate values during piecewise write
> - Add one-shot MIB counter clear in airoha_fe_init() to establish a
>   clean baseline (kexec, driver rebind, warm reboot)
> - Document sibling dev polling design in commit message
> 
> Changes in v3:
> - Link to V2: https://lore.kernel.org/20260701173941.314795-1-aniket.negi03@gmail.com/
> - Add Acked-by tag from Lorenzo
> - Rename from tx_runt_cnt to tx_runt, tx_long_cnt to tx_long,
>   tx_runt_accum64 to tx_runt64
> - Rename from rx_runt_cnt to rx_runt, rx_long_cnt to rx_long,
>   rx_runt_accum64 to rx_runt64
> - Condense the marked comments in V2, remove new line after comment
> 
> Changes in v2:
> - Store _CNT_L register reads in val before adding to stats, improving
>   readability (suggested by Lorenzo Bianconi)
> - Fix double-counting bug in the RUNT+E64 combined bucket: previously
>   "+=" for E64 re-added the full absolute counter each poll; now a
>   dedicated tx_runt_accum64/rx_runt_accum64 accumulator holds the
>   running RUNT delta, and tx_len[0] is assigned (not accumulated) each
>   poll as runt_accum64 + E64_abs
> - Replace 7-element tx_len[]/rx_len[] shadow arrays in mib_prev with
>   focused tx_runt_cnt/tx_long_cnt and rx_runt_cnt/rx_long_cnt fields;
>   only RUNT and LONG are 32-bit and need wrap-around tracking
> - Rename inner struct hw_prev_stats to mib_prev; rename accumulator
>   fields to tx_runt_accum64/rx_runt_accum64 for clarity
> - Fix comment alignment in mib_prev struct block
> - Rename airoha_dev_get_hw_stats() to airoha_update_hw_stats() and
>   move the port spin_lock inside, removing the separate wrapper
> 
>  drivers/net/ethernet/airoha/airoha_eth.c | 183 +++++++++++++++--------
>  drivers/net/ethernet/airoha/airoha_eth.h |  27 ++++
>  2 files changed, 144 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 59001fd4b6f7..2d032cc6dfca 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -493,6 +493,8 @@ static void airoha_fe_crsn_qsel_init(struct airoha_eth *eth)
>  
>  static int airoha_fe_init(struct airoha_eth *eth)
>  {
> +	int i;
> +
>  	airoha_fe_maccr_init(eth);
>  
>  	/* PSE IQ reserve */
> @@ -586,6 +588,14 @@ static int airoha_fe_init(struct airoha_eth *eth)
>  	/* enable 1:N vlan action, init vlan table */
>  	airoha_fe_set(eth, REG_MC_VLAN_EN, MC_VLAN_EN_MASK);
>  
> +	/* Clear MIB counters to establish clean baseline for delta tracking.
> +	 * This prevents spurious statistics from pre-driver activity (e.g.,
> +	 * kexec, driver rebind, warm reboot) on first poll.
> +	 */
> +	for (i = AIROHA_GDM1_IDX; i <= AIROHA_GDM4_IDX; i++)
> +		airoha_fe_set(eth, REG_FE_GDM_MIB_CLEAR(i),
> +			      FE_GDM_MIB_RX_CLEAR_MASK | FE_GDM_MIB_TX_CLEAR_MASK);

This configuration is not needed since we reset FE at module load.

> +
>  	return airoha_fe_mc_vlan_clear(eth);
>  }
>  
> @@ -1686,11 +1696,14 @@ static void airoha_qdma_stop_napi(struct airoha_qdma *qdma)
>  	}
>  }
>  
> -static void airoha_dev_get_hw_stats(struct airoha_gdm_dev *dev)
> +static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
>  {
>  	struct airoha_gdm_port *port = dev->port;
>  	struct airoha_eth *eth = dev->eth;
>  	u32 val, i = 0;
> +	u64 tmp, prev;

nit: tmp is not so meaningful, maybe better something like data?

> +
> +	spin_lock(&port->stats_lock);
>  
>  	/* Read relevant MIB for GDM with multiple port attached */
>  	if (port->id == AIROHA_GDM3_IDX || port->id == AIROHA_GDM4_IDX)
> @@ -1701,152 +1714,190 @@ static void airoha_dev_get_hw_stats(struct airoha_gdm_dev *dev)
>  
>  	u64_stats_update_begin(&dev->stats.syncp);
>  
> -	/* TX */
> +	/* TX - 64-bit H+L registers: hw accumulates the total, read directly.
> +	 * Use local variable to prevent readers from seeing intermediate values.
> +	 * Clamp to prevent regression from torn reads between H and L.
> +	 */
> +	prev = dev->stats.tx_ok_pkts;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_H(port->id));
> -	dev->stats.tx_ok_pkts += ((u64)val << 32);
> +	tmp = ((u64)val << 32);

nit: data = (u64)val << 32;

>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_PKT_CNT_L(port->id));
> -	dev->stats.tx_ok_pkts += val;
> +	tmp |= val;

nit: I would prefer "+" instead of "|"

> +	dev->stats.tx_ok_pkts = max(tmp, prev);

nit: please drop prev and just do:
	dev->stats.tx_ok_pkts = max(data, dev->stats.tx_ok_pkts);

please redo it for all the occurrences.

Regards,
Lorenzo

>  
> +	prev = dev->stats.tx_ok_bytes;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_BYTE_CNT_H(port->id));
> -	dev->stats.tx_ok_bytes += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_OK_BYTE_CNT_L(port->id));
> -	dev->stats.tx_ok_bytes += val;
> +	tmp |= val;
> +	dev->stats.tx_ok_bytes = max(tmp, prev);
>  
> +	/* TX - 32-bit registers: accumulate delta to handle wrap-around. */
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_DROP_CNT(port->id));
> -	dev->stats.tx_drops += val;
> +	dev->stats.tx_drops += (u32)(val - dev->stats.mib_prev.tx_drops);
> +	dev->stats.mib_prev.tx_drops = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_BC_CNT(port->id));
> -	dev->stats.tx_broadcast += val;
> +	dev->stats.tx_broadcast += (u32)(val - dev->stats.mib_prev.tx_broadcast);
> +	dev->stats.mib_prev.tx_broadcast = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_MC_CNT(port->id));
> -	dev->stats.tx_multicast += val;
> +	dev->stats.tx_multicast += (u32)(val - dev->stats.mib_prev.tx_multicast);
> +	dev->stats.mib_prev.tx_multicast = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_RUNT_CNT(port->id));
> -	dev->stats.tx_len[i] += val;
> +	dev->stats.mib_prev.tx_runt64 +=
> +		(u32)(val - dev->stats.mib_prev.tx_runt);
> +	dev->stats.mib_prev.tx_runt = val;
>  
> +	/* tx_len[0]: RUNT (32-bit, delta) + E64 (64-bit, absolute). */
> +	tmp = dev->stats.mib_prev.tx_runt64;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_E64_CNT_H(port->id));
> -	dev->stats.tx_len[i] += ((u64)val << 32);
> +	tmp += ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_E64_CNT_L(port->id));
> -	dev->stats.tx_len[i++] += val;
> +	tmp += val;
> +	dev->stats.tx_len[i++] = tmp;
>  
> +	prev = dev->stats.tx_len[i];
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L64_CNT_H(port->id));
> -	dev->stats.tx_len[i] += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L64_CNT_L(port->id));
> -	dev->stats.tx_len[i++] += val;
> +	tmp |= val;
> +	dev->stats.tx_len[i++] = max(tmp, prev);
>  
> +	prev = dev->stats.tx_len[i];
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L127_CNT_H(port->id));
> -	dev->stats.tx_len[i] += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L127_CNT_L(port->id));
> -	dev->stats.tx_len[i++] += val;
> +	tmp |= val;
> +	dev->stats.tx_len[i++] = max(tmp, prev);
>  
> +	prev = dev->stats.tx_len[i];
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L255_CNT_H(port->id));
> -	dev->stats.tx_len[i] += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L255_CNT_L(port->id));
> -	dev->stats.tx_len[i++] += val;
> +	tmp |= val;
> +	dev->stats.tx_len[i++] = max(tmp, prev);
>  
> +	prev = dev->stats.tx_len[i];
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L511_CNT_H(port->id));
> -	dev->stats.tx_len[i] += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L511_CNT_L(port->id));
> -	dev->stats.tx_len[i++] += val;
> +	tmp |= val;
> +	dev->stats.tx_len[i++] = max(tmp, prev);
>  
> +	prev = dev->stats.tx_len[i];
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L1023_CNT_H(port->id));
> -	dev->stats.tx_len[i] += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_L1023_CNT_L(port->id));
> -	dev->stats.tx_len[i++] += val;
> +	tmp |= val;
> +	dev->stats.tx_len[i++] = max(tmp, prev);
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_TX_ETH_LONG_CNT(port->id));
> -	dev->stats.tx_len[i++] += val;
> +	dev->stats.tx_len[i++] += (u32)(val - dev->stats.mib_prev.tx_long);
> +	dev->stats.mib_prev.tx_long = val;
>  
>  	/* RX */
> +	prev = dev->stats.rx_ok_pkts;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_OK_PKT_CNT_H(port->id));
> -	dev->stats.rx_ok_pkts += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_OK_PKT_CNT_L(port->id));
> -	dev->stats.rx_ok_pkts += val;
> +	tmp |= val;
> +	dev->stats.rx_ok_pkts = max(tmp, prev);
>  
> +	prev = dev->stats.rx_ok_bytes;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_OK_BYTE_CNT_H(port->id));
> -	dev->stats.rx_ok_bytes += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_OK_BYTE_CNT_L(port->id));
> -	dev->stats.rx_ok_bytes += val;
> +	tmp |= val;
> +	dev->stats.rx_ok_bytes = max(tmp, prev);
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_DROP_CNT(port->id));
> -	dev->stats.rx_drops += val;
> +	dev->stats.rx_drops += (u32)(val - dev->stats.mib_prev.rx_drops);
> +	dev->stats.mib_prev.rx_drops = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_BC_CNT(port->id));
> -	dev->stats.rx_broadcast += val;
> +	dev->stats.rx_broadcast += (u32)(val - dev->stats.mib_prev.rx_broadcast);
> +	dev->stats.mib_prev.rx_broadcast = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_MC_CNT(port->id));
> -	dev->stats.rx_multicast += val;
> +	dev->stats.rx_multicast += (u32)(val - dev->stats.mib_prev.rx_multicast);
> +	dev->stats.mib_prev.rx_multicast = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ERROR_DROP_CNT(port->id));
> -	dev->stats.rx_errors += val;
> +	dev->stats.rx_errors += (u32)(val - dev->stats.mib_prev.rx_errors);
> +	dev->stats.mib_prev.rx_errors = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_CRC_ERR_CNT(port->id));
> -	dev->stats.rx_crc_error += val;
> +	dev->stats.rx_crc_error += (u32)(val - dev->stats.mib_prev.rx_crc_error);
> +	dev->stats.mib_prev.rx_crc_error = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_OVERFLOW_DROP_CNT(port->id));
> -	dev->stats.rx_over_errors += val;
> +	dev->stats.rx_over_errors += (u32)(val - dev->stats.mib_prev.rx_over_errors);
> +	dev->stats.mib_prev.rx_over_errors = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_FRAG_CNT(port->id));
> -	dev->stats.rx_fragment += val;
> +	dev->stats.rx_fragment += (u32)(val - dev->stats.mib_prev.rx_fragment);
> +	dev->stats.mib_prev.rx_fragment = val;
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_JABBER_CNT(port->id));
> -	dev->stats.rx_jabber += val;
> +	dev->stats.rx_jabber += (u32)(val - dev->stats.mib_prev.rx_jabber);
> +	dev->stats.mib_prev.rx_jabber = val;
>  
>  	i = 0;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_RUNT_CNT(port->id));
> -	dev->stats.rx_len[i] += val;
> +	dev->stats.mib_prev.rx_runt64 +=
> +		(u32)(val - dev->stats.mib_prev.rx_runt);
> +	dev->stats.mib_prev.rx_runt = val;
>  
> +	/* rx_len[0]: RUNT (32-bit, delta) + E64 (64-bit, absolute). */
> +	tmp = dev->stats.mib_prev.rx_runt64;
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_E64_CNT_H(port->id));
> -	dev->stats.rx_len[i] += ((u64)val << 32);
> +	tmp += ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_E64_CNT_L(port->id));
> -	dev->stats.rx_len[i++] += val;
> +	tmp += val;
> +	dev->stats.rx_len[i++] = tmp;
>  
> +	prev = dev->stats.rx_len[i];
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L64_CNT_H(port->id));
> -	dev->stats.rx_len[i] += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L64_CNT_L(port->id));
> -	dev->stats.rx_len[i++] += val;
> +	tmp |= val;
> +	dev->stats.rx_len[i++] = max(tmp, prev);
>  
> +	prev = dev->stats.rx_len[i];
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L127_CNT_H(port->id));
> -	dev->stats.rx_len[i] += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L127_CNT_L(port->id));
> -	dev->stats.rx_len[i++] += val;
> +	tmp |= val;
> +	dev->stats.rx_len[i++] = max(tmp, prev);
>  
> +	prev = dev->stats.rx_len[i];
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L255_CNT_H(port->id));
> -	dev->stats.rx_len[i] += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L255_CNT_L(port->id));
> -	dev->stats.rx_len[i++] += val;
> +	tmp |= val;
> +	dev->stats.rx_len[i++] = max(tmp, prev);
>  
> +	prev = dev->stats.rx_len[i];
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L511_CNT_H(port->id));
> -	dev->stats.rx_len[i] += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L511_CNT_L(port->id));
> -	dev->stats.rx_len[i++] += val;
> +	tmp |= val;
> +	dev->stats.rx_len[i++] = max(tmp, prev);
>  
> +	prev = dev->stats.rx_len[i];
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L1023_CNT_H(port->id));
> -	dev->stats.rx_len[i] += ((u64)val << 32);
> +	tmp = ((u64)val << 32);
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_L1023_CNT_L(port->id));
> -	dev->stats.rx_len[i++] += val;
> +	tmp |= val;
> +	dev->stats.rx_len[i++] = max(tmp, prev);
>  
>  	val = airoha_fe_rr(eth, REG_FE_GDM_RX_ETH_LONG_CNT(port->id));
> -	dev->stats.rx_len[i++] += val;
> +	dev->stats.rx_len[i] += (u32)(val - dev->stats.mib_prev.rx_long);
> +	dev->stats.mib_prev.rx_long = val;
>  
>  	u64_stats_update_end(&dev->stats.syncp);
> -}
> -
> -static void airoha_update_hw_stats(struct airoha_gdm_dev *dev)
> -{
> -	struct airoha_gdm_port *port = dev->port;
> -	int i;
> -
> -	spin_lock(&port->stats_lock);
> -
> -	for (i = 0; i < ARRAY_SIZE(port->devs); i++) {
> -		if (port->devs[i])
> -			airoha_dev_get_hw_stats(port->devs[i]);
> -	}
> -
> -	/* Reset MIB counters */
> -	airoha_fe_set(dev->eth, REG_FE_GDM_MIB_CLEAR(port->id),
> -		      FE_GDM_MIB_RX_CLEAR_MASK | FE_GDM_MIB_TX_CLEAR_MASK);
>  
>  	spin_unlock(&port->stats_lock);
>  }
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.h b/drivers/net/ethernet/airoha/airoha_eth.h
> index f6d01a8e8da1..fe934f9ffe8a 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.h
> +++ b/drivers/net/ethernet/airoha/airoha_eth.h
> @@ -245,6 +245,33 @@ struct airoha_hw_stats {
>  	u64 rx_fragment;
>  	u64 rx_jabber;
>  	u64 rx_len[7];
> +
> +	struct {
> +		/* Previous HW register values for 32-bit counter delta
> +		 * tracking. Storing the last seen value and accumulating
> +		 * (u32)(curr - prev) into the 64-bit software counter
> +		 * handles wrap-around transparently via unsigned arithmetic.
> +		 * tx_runt64/rx_runt64 hold the running sum of runt deltas.
> +		 * These fields are never reported to userspace.
> +		 */
> +		u32 tx_drops;
> +		u32 tx_broadcast;
> +		u32 tx_multicast;
> +		u32 tx_runt;
> +		u32 tx_long;
> +		u64 tx_runt64;
> +		u32 rx_drops;
> +		u32 rx_broadcast;
> +		u32 rx_multicast;
> +		u32 rx_errors;
> +		u32 rx_crc_error;
> +		u32 rx_over_errors;
> +		u32 rx_fragment;
> +		u32 rx_jabber;
> +		u32 rx_runt;
> +		u32 rx_long;
> +		u64 rx_runt64;
> +	} mib_prev;
>  };
>  
>  enum {
> 
> base-commit: a225f8c20712713406ae47024b8df42deacddd4a
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net v4] net: airoha: fix MIB stats collection to be lossless

[Aniket Negi]

Hi Lorenzo,
Thanks for the review.

> > +	for (i = AIROHA_GDM1_IDX; i <= AIROHA_GDM4_IDX; i++)
> > +		airoha_fe_set(eth, REG_FE_GDM_MIB_CLEAR(i),
> > +			      FE_GDM_MIB_RX_CLEAR_MASK | FE_GDM_MIB_TX_CLEAR_MASK);
>
> This configuration is not needed since we reset FE at module load.

Agreed. The SCU FE reset clears MIB counters, so the explicit loop
is redundant. Will drop it in v5.

On Mon, 2 Jul 2026 22:59:32 +0000, Sashiko <sashiko@kernel.org> wrote:
> With this patch the driver no longer has any users of
> REG_FE_GDM_MIB_CLEAR, FE_GDM_MIB_RX_CLEAR_MASK or
> FE_GDM_MIB_TX_CLEAR_MASK; grep in drivers/net/ethernet/airoha/
> matches only the definitions in airoha_regs.h. Should these macros
> be removed in the same patch that eliminates their only caller?

What is your suggestions here, I prefer that definitions should 
be retained in airoha_regs.h as register documentation for future 
reference. Only the loop will be dropped.

> nit: tmp is not so meaningful, maybe better something like data?
> nit: I would prefer "+" instead of "|"
> nit: please drop prev and just do:
>       dev->stats.tx_ok_pkts = max(data, dev->stats.tx_ok_pkts);
> please redo it for all the occurrences.

Understood. Will rename tmp to data, and use '+' instead of '|' in v5.

Thanks,
Aniket

Re: [PATCH net v4] net: airoha: fix MIB stats collection to be lossless

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]

> Hi Lorenzo,
> Thanks for the review.
> 
> > > +	for (i = AIROHA_GDM1_IDX; i <= AIROHA_GDM4_IDX; i++)
> > > +		airoha_fe_set(eth, REG_FE_GDM_MIB_CLEAR(i),
> > > +			      FE_GDM_MIB_RX_CLEAR_MASK | FE_GDM_MIB_TX_CLEAR_MASK);
> >
> > This configuration is not needed since we reset FE at module load.
> 
> Agreed. The SCU FE reset clears MIB counters, so the explicit loop
> is redundant. Will drop it in v5.
> 
> On Mon, 2 Jul 2026 22:59:32 +0000, Sashiko <sashiko@kernel.org> wrote:
> > With this patch the driver no longer has any users of
> > REG_FE_GDM_MIB_CLEAR, FE_GDM_MIB_RX_CLEAR_MASK or
> > FE_GDM_MIB_TX_CLEAR_MASK; grep in drivers/net/ethernet/airoha/
> > matches only the definitions in airoha_regs.h. Should these macros
> > be removed in the same patch that eliminates their only caller?
> 
> What is your suggestions here, I prefer that definitions should 
> be retained in airoha_regs.h as register documentation for future 
> reference. Only the loop will be dropped.

correct.

Regards,
Lorenzo

> 
> > nit: tmp is not so meaningful, maybe better something like data?
> > nit: I would prefer "+" instead of "|"
> > nit: please drop prev and just do:
> >       dev->stats.tx_ok_pkts = max(data, dev->stats.tx_ok_pkts);
> > please redo it for all the occurrences.
> 
> Understood. Will rename tmp to data, and use '+' instead of '|' in v5.
> 
> Thanks,
> Aniket

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]