[PATCH net-next] net: dsa: qca8k: report port statistics from the MIB

[PATCH net-next] net: dsa: qca8k: report port statistics from the MIB

[Stanislaw Pal]

qca8k has no .get_stats64, so a user port's netdev counts only the
frames that cross the CPU port, through the DSA user netdev's own
tstats. Whatever the switch forwards between its own ports in a
hardware-offloaded bridge never reaches the CPU and never shows up in
the counters of either port. The same goes for a conduit that forwards
in hardware past the host: on a GL.iNet GL-B3000 (IPQ5018 with a
QCA8337 on SGMII) with the conduit offloaded, a 482 MB upload over the
WAN port moved the wan netdev's TX bytes by 0.74 MB, while the switch's
own TxByte counter for that port grew by 503 MB.

The switch counts every frame in its per-port MIB. .get_stats64 may be
called in atomic context and the MIB sits behind MDIO (or the Ethernet
management frames), so read it from a delayed work every 3 s, as ar9331
and rtl8365mb do, and hand out the cached result. The counters keep
their value across reads (QCA8K_MIB_CPU_KEEP); only the difference since
the previous read is added, so a 32-bit counter wrapping between two
reads does not corrupt the total.

With the change, on the same board and setup, a 732 MB upload moved the
wan netdev's TX bytes by 764.9 MB, the same as the switch's TxByte.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
---
Tested on a GL.iNet GL-B3000 with the patch on OpenWrt's 6.18 kernel,
where it differs only in the context of struct qca8k_priv. Built for
arm64 on net-next with W=1; checkpatch --strict is clean.

 drivers/net/dsa/qca/qca8k-8xxx.c | 157 +++++++++++++++++++++++++++++++
 drivers/net/dsa/qca/qca8k.h      |  47 +++++++++
 2 files changed, 204 insertions(+)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 07227ae8b..9fef54a88 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1851,6 +1851,156 @@ static void qca8k_setup_hol_fixup(struct qca8k_priv *priv, int port)
 			   mask);
 }
 
+/* Without .get_stats64 a DSA user port counts only what crosses the CPU
+ * port, which is nothing of the traffic the switch forwards between its own
+ * ports. Report the switch's own port counters instead.
+ */
+#define QCA8K_STATS_INTERVAL	(3 * HZ)
+
+static const u8 qca8k_stats_mib[QCA8K_STATS_MIB_COUNT] = {
+	[QCA8K_STATS_RX_MULTI]		= 0x08,
+	[QCA8K_STATS_RX_FCS_ERR]	= 0x0c,
+	[QCA8K_STATS_RX_ALIGN_ERR]	= 0x10,
+	[QCA8K_STATS_RX_RUNT]		= 0x14,
+	[QCA8K_STATS_RX_FRAGMENT]	= 0x18,
+	[QCA8K_STATS_RX_64]		= 0x1c,
+	[QCA8K_STATS_RX_128]		= 0x20,
+	[QCA8K_STATS_RX_256]		= 0x24,
+	[QCA8K_STATS_RX_512]		= 0x28,
+	[QCA8K_STATS_RX_1024]		= 0x2c,
+	[QCA8K_STATS_RX_1518]		= 0x30,
+	[QCA8K_STATS_RX_MAX]		= 0x34,
+	[QCA8K_STATS_RX_TOO_LONG]	= 0x38,
+	[QCA8K_STATS_RX_OVERFLOW]	= 0x4c,
+	[QCA8K_STATS_FILTERED]		= 0x50,
+	[QCA8K_STATS_TX_UNDERRUN]	= 0x60,
+	[QCA8K_STATS_TX_64]		= 0x64,
+	[QCA8K_STATS_TX_128]		= 0x68,
+	[QCA8K_STATS_TX_256]		= 0x6c,
+	[QCA8K_STATS_TX_512]		= 0x70,
+	[QCA8K_STATS_TX_1024]		= 0x74,
+	[QCA8K_STATS_TX_1518]		= 0x78,
+	[QCA8K_STATS_TX_MAX]		= 0x7c,
+	[QCA8K_STATS_TX_OVERSIZE]	= 0x80,
+	[QCA8K_STATS_TX_COLLISION]	= 0x8c,
+	[QCA8K_STATS_TX_ABORT_COL]	= 0x90,
+	[QCA8K_STATS_TX_LATE_COL]	= 0xa4,
+};
+
+#define QCA8K_MIB_RX_GOOD_BYTE	0x3c
+#define QCA8K_MIB_TX_BYTE	0x84
+
+static int qca8k_stats_read64(struct qca8k_priv *priv, u32 reg, u64 *val)
+{
+	u32 lo, hi;
+	int ret;
+
+	ret = qca8k_read(priv, reg, &lo);
+	if (!ret)
+		ret = qca8k_read(priv, reg + 4, &hi);
+	if (!ret)
+		*val = ((u64)hi << 32) | lo;
+
+	return ret;
+}
+
+static u64 qca8k_stats_delta64(u64 now, u64 *last)
+{
+	/* a flushed MIB starts again from zero */
+	u64 delta = now >= *last ? now - *last : now;
+
+	*last = now;
+	return delta;
+}
+
+static void qca8k_stats_read_port(struct qca8k_priv *priv, int port)
+{
+	struct qca8k_port_stats *ps = &priv->port_stats[port];
+	u32 raw[QCA8K_STATS_MIB_COUNT], d[QCA8K_STATS_MIB_COUNT];
+	u32 base = QCA8K_PORT_MIB_COUNTER(port);
+	struct rtnl_link_stats64 *s = &ps->stats;
+	u64 rx_bytes, tx_bytes;
+	int i;
+
+	/* all of the slow MDIO reads before the lock */
+	if (qca8k_stats_read64(priv, base + QCA8K_MIB_RX_GOOD_BYTE, &rx_bytes) ||
+	    qca8k_stats_read64(priv, base + QCA8K_MIB_TX_BYTE, &tx_bytes))
+		return;
+
+	for (i = 0; i < QCA8K_STATS_MIB_COUNT; i++)
+		if (qca8k_read(priv, base + qca8k_stats_mib[i], &raw[i]))
+			return;
+
+	spin_lock_bh(&ps->lock);
+
+	for (i = 0; i < QCA8K_STATS_MIB_COUNT; i++) {
+		d[i] = raw[i] - ps->last[i];
+		ps->last[i] = raw[i];
+	}
+
+	s->rx_bytes += qca8k_stats_delta64(rx_bytes, &ps->last_rx_bytes);
+	s->tx_bytes += qca8k_stats_delta64(tx_bytes, &ps->last_tx_bytes);
+	s->rx_packets += (u64)d[QCA8K_STATS_RX_64] + d[QCA8K_STATS_RX_128] +
+			 d[QCA8K_STATS_RX_256] + d[QCA8K_STATS_RX_512] +
+			 d[QCA8K_STATS_RX_1024] + d[QCA8K_STATS_RX_1518] +
+			 d[QCA8K_STATS_RX_MAX];
+	s->tx_packets += (u64)d[QCA8K_STATS_TX_64] + d[QCA8K_STATS_TX_128] +
+			 d[QCA8K_STATS_TX_256] + d[QCA8K_STATS_TX_512] +
+			 d[QCA8K_STATS_TX_1024] + d[QCA8K_STATS_TX_1518] +
+			 d[QCA8K_STATS_TX_MAX];
+	s->multicast += d[QCA8K_STATS_RX_MULTI];
+	s->rx_length_errors += (u64)d[QCA8K_STATS_RX_RUNT] +
+			       d[QCA8K_STATS_RX_FRAGMENT] +
+			       d[QCA8K_STATS_RX_TOO_LONG];
+	s->rx_crc_errors += d[QCA8K_STATS_RX_FCS_ERR];
+	s->rx_frame_errors += d[QCA8K_STATS_RX_ALIGN_ERR];
+	s->rx_missed_errors += d[QCA8K_STATS_RX_OVERFLOW];
+	s->rx_dropped += d[QCA8K_STATS_FILTERED];
+	s->rx_errors += (u64)d[QCA8K_STATS_RX_FCS_ERR] +
+			d[QCA8K_STATS_RX_ALIGN_ERR] + d[QCA8K_STATS_RX_RUNT] +
+			d[QCA8K_STATS_RX_FRAGMENT] + d[QCA8K_STATS_RX_OVERFLOW] +
+			d[QCA8K_STATS_RX_TOO_LONG];
+	s->tx_fifo_errors += d[QCA8K_STATS_TX_UNDERRUN];
+	s->tx_aborted_errors += d[QCA8K_STATS_TX_ABORT_COL];
+	s->tx_window_errors += d[QCA8K_STATS_TX_LATE_COL];
+	s->tx_errors += (u64)d[QCA8K_STATS_TX_OVERSIZE] +
+			d[QCA8K_STATS_TX_ABORT_COL] + d[QCA8K_STATS_TX_UNDERRUN] +
+			d[QCA8K_STATS_TX_LATE_COL];
+	s->collisions += d[QCA8K_STATS_TX_COLLISION];
+
+	spin_unlock_bh(&ps->lock);
+}
+
+static void qca8k_stats_poll(struct work_struct *work)
+{
+	struct qca8k_priv *priv = container_of(work, struct qca8k_priv,
+					       stats_work.work);
+	struct dsa_port *dp;
+
+	dsa_switch_for_each_user_port(dp, priv->ds)
+		qca8k_stats_read_port(priv, dp->index);
+
+	schedule_delayed_work(&priv->stats_work, QCA8K_STATS_INTERVAL);
+}
+
+static void qca8k_get_stats64(struct dsa_switch *ds, int port,
+			      struct rtnl_link_stats64 *s)
+{
+	struct qca8k_priv *priv = ds->priv;
+	struct qca8k_port_stats *ps = &priv->port_stats[port];
+
+	spin_lock_bh(&ps->lock);
+	memcpy(s, &ps->stats, sizeof(*s));
+	spin_unlock_bh(&ps->lock);
+}
+
+static void qca8k_teardown(struct dsa_switch *ds)
+{
+	struct qca8k_priv *priv = ds->priv;
+
+	cancel_delayed_work_sync(&priv->stats_work);
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -2020,6 +2170,11 @@ qca8k_setup(struct dsa_switch *ds)
 	/* Set max number of LAGs supported */
 	ds->num_lag_ids = QCA8K_NUM_LAGS;
 
+	for (int port = 0; port < QCA8K_NUM_PORTS; port++)
+		spin_lock_init(&priv->port_stats[port].lock);
+	INIT_DELAYED_WORK(&priv->stats_work, qca8k_stats_poll);
+	schedule_delayed_work(&priv->stats_work, 0);
+
 	return 0;
 }
 
@@ -2033,9 +2188,11 @@ static const struct phylink_mac_ops qca8k_phylink_mac_ops = {
 static const struct dsa_switch_ops qca8k_switch_ops = {
 	.get_tag_protocol	= qca8k_get_tag_protocol,
 	.setup			= qca8k_setup,
+	.teardown		= qca8k_teardown,
 	.get_strings		= qca8k_get_strings,
 	.get_ethtool_stats	= qca8k_get_ethtool_stats,
 	.get_sset_count		= qca8k_get_sset_count,
+	.get_stats64		= qca8k_get_stats64,
 	.set_ageing_time	= qca8k_set_ageing_time,
 	.support_eee		= dsa_supports_eee,
 	.set_mac_eee		= qca8k_set_mac_eee,
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 956338893..47e9e5631 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -442,6 +442,51 @@ struct qca8k_led {
 	struct led_classdev cdev;
 };
 
+/* MIB counters behind .get_stats64, in the order qca8k_stats_mib[] reads them */
+enum qca8k_stats_mib_idx {
+	QCA8K_STATS_RX_MULTI,
+	QCA8K_STATS_RX_FCS_ERR,
+	QCA8K_STATS_RX_ALIGN_ERR,
+	QCA8K_STATS_RX_RUNT,
+	QCA8K_STATS_RX_FRAGMENT,
+	QCA8K_STATS_RX_64,
+	QCA8K_STATS_RX_128,
+	QCA8K_STATS_RX_256,
+	QCA8K_STATS_RX_512,
+	QCA8K_STATS_RX_1024,
+	QCA8K_STATS_RX_1518,
+	QCA8K_STATS_RX_MAX,
+	QCA8K_STATS_RX_TOO_LONG,
+	QCA8K_STATS_RX_OVERFLOW,
+	QCA8K_STATS_FILTERED,
+	QCA8K_STATS_TX_UNDERRUN,
+	QCA8K_STATS_TX_64,
+	QCA8K_STATS_TX_128,
+	QCA8K_STATS_TX_256,
+	QCA8K_STATS_TX_512,
+	QCA8K_STATS_TX_1024,
+	QCA8K_STATS_TX_1518,
+	QCA8K_STATS_TX_MAX,
+	QCA8K_STATS_TX_OVERSIZE,
+	QCA8K_STATS_TX_COLLISION,
+	QCA8K_STATS_TX_ABORT_COL,
+	QCA8K_STATS_TX_LATE_COL,
+	QCA8K_STATS_MIB_COUNT
+};
+
+/* Port counters for .get_stats64. That callback runs in atomic context and
+ * the MIB sits behind MDIO, so a worker reads it and this holds the result.
+ * The MIB counters are cumulative; last[] keeps the previous raw values so
+ * only the difference is added and a 32-bit counter may wrap between reads.
+ */
+struct qca8k_port_stats {
+	spinlock_t lock; /* protects stats against the worker */
+	struct rtnl_link_stats64 stats;
+	u32 last[QCA8K_STATS_MIB_COUNT];
+	u64 last_rx_bytes;
+	u64 last_tx_bytes;
+};
+
 struct qca8k_priv {
 	u8 switch_id;
 	u8 switch_revision;
@@ -469,6 +514,8 @@ struct qca8k_priv {
 	struct qca8k_pcs pcs_port_6;
 	const struct qca8k_match_data *info;
 	struct qca8k_led ports_led[QCA8K_LED_COUNT];
+	struct qca8k_port_stats port_stats[QCA8K_NUM_PORTS];
+	struct delayed_work stats_work;
 };
 
 struct qca8k_mib_desc {
-- 
2.43.0

Re: [PATCH net-next] net: dsa: qca8k: report port statistics from the MIB

[Christian Marangi]

On Sun, Oct 18, 2026 at 05:36:58PM +0200, Stanislaw Pal wrote:
> qca8k has no .get_stats64, so a user port's netdev counts only the
> frames that cross the CPU port, through the DSA user netdev's own
> tstats. Whatever the switch forwards between its own ports in a
> hardware-offloaded bridge never reaches the CPU and never shows up in
> the counters of either port. The same goes for a conduit that forwards
> in hardware past the host: on a GL.iNet GL-B3000 (IPQ5018 with a
> QCA8337 on SGMII) with the conduit offloaded, a 482 MB upload over the
> WAN port moved the wan netdev's TX bytes by 0.74 MB, while the switch's
> own TxByte counter for that port grew by 503 MB.
> 
> The switch counts every frame in its per-port MIB. .get_stats64 may be
> called in atomic context and the MIB sits behind MDIO (or the Ethernet
> management frames), so read it from a delayed work every 3 s, as ar9331
> and rtl8365mb do, and hand out the cached result. The counters keep
> their value across reads (QCA8K_MIB_CPU_KEEP); only the difference since
> the previous read is added, so a 32-bit counter wrapping between two
> reads does not corrupt the total.

If we really want we can reset after each read but that is problematic for
the ethtool OPs unless we also update that function to store local u64 MIB
for everything.

Up to you... I would start with keeping and updating all the below things
and then evaluate the needed changes to reset at every read.

> 
> With the change, on the same board and setup, a 732 MB upload moved the
> wan netdev's TX bytes by 764.9 MB, the same as the switch's TxByte.
>

This really seems a task for the MIB from mgmt frame... May I ask why those
are not used here?
 
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
> ---
> Tested on a GL.iNet GL-B3000 with the patch on OpenWrt's 6.18 kernel,
> where it differs only in the context of struct qca8k_priv. Built for
> arm64 on net-next with W=1; checkpatch --strict is clean.
> 
>  drivers/net/dsa/qca/qca8k-8xxx.c | 157 +++++++++++++++++++++++++++++++
>  drivers/net/dsa/qca/qca8k.h      |  47 +++++++++
>  2 files changed, 204 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 07227ae8b..9fef54a88 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -1851,6 +1851,156 @@ static void qca8k_setup_hol_fixup(struct qca8k_priv *priv, int port)
>  			   mask);
>  }
>  
> +/* Without .get_stats64 a DSA user port counts only what crosses the CPU
> + * port, which is nothing of the traffic the switch forwards between its own
> + * ports. Report the switch's own port counters instead.
> + */
> +#define QCA8K_STATS_INTERVAL	(3 * HZ)
> +

I would like to see define in the header file, not in the middle of
functions and declaration.

> +static const u8 qca8k_stats_mib[QCA8K_STATS_MIB_COUNT] = {
> +	[QCA8K_STATS_RX_MULTI]		= 0x08,
> +	[QCA8K_STATS_RX_FCS_ERR]	= 0x0c,
> +	[QCA8K_STATS_RX_ALIGN_ERR]	= 0x10,
> +	[QCA8K_STATS_RX_RUNT]		= 0x14,
> +	[QCA8K_STATS_RX_FRAGMENT]	= 0x18,
> +	[QCA8K_STATS_RX_64]		= 0x1c,
> +	[QCA8K_STATS_RX_128]		= 0x20,
> +	[QCA8K_STATS_RX_256]		= 0x24,
> +	[QCA8K_STATS_RX_512]		= 0x28,
> +	[QCA8K_STATS_RX_1024]		= 0x2c,
> +	[QCA8K_STATS_RX_1518]		= 0x30,
> +	[QCA8K_STATS_RX_MAX]		= 0x34,
> +	[QCA8K_STATS_RX_TOO_LONG]	= 0x38,
> +	[QCA8K_STATS_RX_OVERFLOW]	= 0x4c,
> +	[QCA8K_STATS_FILTERED]		= 0x50,
> +	[QCA8K_STATS_TX_UNDERRUN]	= 0x60,
> +	[QCA8K_STATS_TX_64]		= 0x64,
> +	[QCA8K_STATS_TX_128]		= 0x68,
> +	[QCA8K_STATS_TX_256]		= 0x6c,
> +	[QCA8K_STATS_TX_512]		= 0x70,
> +	[QCA8K_STATS_TX_1024]		= 0x74,
> +	[QCA8K_STATS_TX_1518]		= 0x78,
> +	[QCA8K_STATS_TX_MAX]		= 0x7c,
> +	[QCA8K_STATS_TX_OVERSIZE]	= 0x80,
> +	[QCA8K_STATS_TX_COLLISION]	= 0x8c,
> +	[QCA8K_STATS_TX_ABORT_COL]	= 0x90,
> +	[QCA8K_STATS_TX_LATE_COL]	= 0xa4,
> +};

Why STATS and not MIB? Also I would like to have all these register
converted to define if really needed and update the ar8327_mib struct.

Also Also why not use that directly?

> +
> +#define QCA8K_MIB_RX_GOOD_BYTE	0x3c
> +#define QCA8K_MIB_TX_BYTE	0x84
> +

Why this? For 64 bit?

> +static int qca8k_stats_read64(struct qca8k_priv *priv, u32 reg, u64 *val)
> +{
> +	u32 lo, hi;
> +	int ret;
> +
> +	ret = qca8k_read(priv, reg, &lo);
> +	if (!ret)
> +		ret = qca8k_read(priv, reg + 4, &hi);
> +	if (!ret)
> +		*val = ((u64)hi << 32) | lo;
> +
> +	return ret;
> +}
> +

Maybe generalize the logic in get_ethtool_stats?

> +static u64 qca8k_stats_delta64(u64 now, u64 *last)
> +{
> +	/* a flushed MIB starts again from zero */
> +	u64 delta = now >= *last ? now - *last : now;
> +
> +	*last = now;
> +	return delta;
> +}
> +
> +static void qca8k_stats_read_port(struct qca8k_priv *priv, int port)
> +{
> +	struct qca8k_port_stats *ps = &priv->port_stats[port];
> +	u32 raw[QCA8K_STATS_MIB_COUNT], d[QCA8K_STATS_MIB_COUNT];
> +	u32 base = QCA8K_PORT_MIB_COUNTER(port);
> +	struct rtnl_link_stats64 *s = &ps->stats;
> +	u64 rx_bytes, tx_bytes;
> +	int i;
> +
> +	/* all of the slow MDIO reads before the lock */
> +	if (qca8k_stats_read64(priv, base + QCA8K_MIB_RX_GOOD_BYTE, &rx_bytes) ||
> +	    qca8k_stats_read64(priv, base + QCA8K_MIB_TX_BYTE, &tx_bytes))
> +		return;
> +
> +	for (i = 0; i < QCA8K_STATS_MIB_COUNT; i++)
> +		if (qca8k_read(priv, base + qca8k_stats_mib[i], &raw[i]))
> +			return;
> +

As said MDIO here is extra slow especially for MIB every x seconds...

Autocast MIB is a MUCH BETTER WAY.

> +	spin_lock_bh(&ps->lock);
> +
> +	for (i = 0; i < QCA8K_STATS_MIB_COUNT; i++) {
> +		d[i] = raw[i] - ps->last[i];
> +		ps->last[i] = raw[i];
> +	}
> +
> +	s->rx_bytes += qca8k_stats_delta64(rx_bytes, &ps->last_rx_bytes);
> +	s->tx_bytes += qca8k_stats_delta64(tx_bytes, &ps->last_tx_bytes);
> +	s->rx_packets += (u64)d[QCA8K_STATS_RX_64] + d[QCA8K_STATS_RX_128] +
> +			 d[QCA8K_STATS_RX_256] + d[QCA8K_STATS_RX_512] +
> +			 d[QCA8K_STATS_RX_1024] + d[QCA8K_STATS_RX_1518] +
> +			 d[QCA8K_STATS_RX_MAX];
> +	s->tx_packets += (u64)d[QCA8K_STATS_TX_64] + d[QCA8K_STATS_TX_128] +
> +			 d[QCA8K_STATS_TX_256] + d[QCA8K_STATS_TX_512] +
> +			 d[QCA8K_STATS_TX_1024] + d[QCA8K_STATS_TX_1518] +
> +			 d[QCA8K_STATS_TX_MAX];
> +	s->multicast += d[QCA8K_STATS_RX_MULTI];
> +	s->rx_length_errors += (u64)d[QCA8K_STATS_RX_RUNT] +
> +			       d[QCA8K_STATS_RX_FRAGMENT] +
> +			       d[QCA8K_STATS_RX_TOO_LONG];
> +	s->rx_crc_errors += d[QCA8K_STATS_RX_FCS_ERR];
> +	s->rx_frame_errors += d[QCA8K_STATS_RX_ALIGN_ERR];
> +	s->rx_missed_errors += d[QCA8K_STATS_RX_OVERFLOW];
> +	s->rx_dropped += d[QCA8K_STATS_FILTERED];
> +	s->rx_errors += (u64)d[QCA8K_STATS_RX_FCS_ERR] +
> +			d[QCA8K_STATS_RX_ALIGN_ERR] + d[QCA8K_STATS_RX_RUNT] +
> +			d[QCA8K_STATS_RX_FRAGMENT] + d[QCA8K_STATS_RX_OVERFLOW] +
> +			d[QCA8K_STATS_RX_TOO_LONG];
> +	s->tx_fifo_errors += d[QCA8K_STATS_TX_UNDERRUN];
> +	s->tx_aborted_errors += d[QCA8K_STATS_TX_ABORT_COL];
> +	s->tx_window_errors += d[QCA8K_STATS_TX_LATE_COL];

Is it correct to give window_errors to TX late collision?

> +	s->tx_errors += (u64)d[QCA8K_STATS_TX_OVERSIZE] +
> +			d[QCA8K_STATS_TX_ABORT_COL] + d[QCA8K_STATS_TX_UNDERRUN] +
> +			d[QCA8K_STATS_TX_LATE_COL];
> +	s->collisions += d[QCA8K_STATS_TX_COLLISION];
> +
> +	spin_unlock_bh(&ps->lock);
> +}
> +
> +static void qca8k_stats_poll(struct work_struct *work)
> +{
> +	struct qca8k_priv *priv = container_of(work, struct qca8k_priv,
> +					       stats_work.work);
> +	struct dsa_port *dp;
> +
> +	dsa_switch_for_each_user_port(dp, priv->ds)
> +		qca8k_stats_read_port(priv, dp->index);
> +

No reason to scan for a port that is disabled or not connected...

> +	schedule_delayed_work(&priv->stats_work, QCA8K_STATS_INTERVAL);
> +}
> +
> +static void qca8k_get_stats64(struct dsa_switch *ds, int port,
> +			      struct rtnl_link_stats64 *s)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +	struct qca8k_port_stats *ps = &priv->port_stats[port];
> +
> +	spin_lock_bh(&ps->lock);
> +	memcpy(s, &ps->stats, sizeof(*s));
> +	spin_unlock_bh(&ps->lock);
> +}
> +
> +static void qca8k_teardown(struct dsa_switch *ds)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +
> +	cancel_delayed_work_sync(&priv->stats_work);
> +}
> +
>  static int
>  qca8k_setup(struct dsa_switch *ds)
>  {
> @@ -2020,6 +2170,11 @@ qca8k_setup(struct dsa_switch *ds)
>  	/* Set max number of LAGs supported */
>  	ds->num_lag_ids = QCA8K_NUM_LAGS;
>  
> +	for (int port = 0; port < QCA8K_NUM_PORTS; port++)
> +		spin_lock_init(&priv->port_stats[port].lock);

Considering everything is run under the same delayed work, does it make
sense to have a spinlock for each port?

Either have a delayed work for each port or a global spinlock for the task.

> +	INIT_DELAYED_WORK(&priv->stats_work, qca8k_stats_poll);
> +	schedule_delayed_work(&priv->stats_work, 0);
> +
>  	return 0;
>  }
>  
> @@ -2033,9 +2188,11 @@ static const struct phylink_mac_ops qca8k_phylink_mac_ops = {
>  static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.get_tag_protocol	= qca8k_get_tag_protocol,
>  	.setup			= qca8k_setup,
> +	.teardown		= qca8k_teardown,
>  	.get_strings		= qca8k_get_strings,
>  	.get_ethtool_stats	= qca8k_get_ethtool_stats,
>  	.get_sset_count		= qca8k_get_sset_count,
> +	.get_stats64		= qca8k_get_stats64,
>  	.set_ageing_time	= qca8k_set_ageing_time,
>  	.support_eee		= dsa_supports_eee,
>  	.set_mac_eee		= qca8k_set_mac_eee,
> diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> index 956338893..47e9e5631 100644
> --- a/drivers/net/dsa/qca/qca8k.h
> +++ b/drivers/net/dsa/qca/qca8k.h
> @@ -442,6 +442,51 @@ struct qca8k_led {
>  	struct led_classdev cdev;
>  };
>  
> +/* MIB counters behind .get_stats64, in the order qca8k_stats_mib[] reads them */
> +enum qca8k_stats_mib_idx {
> +	QCA8K_STATS_RX_MULTI,
> +	QCA8K_STATS_RX_FCS_ERR,
> +	QCA8K_STATS_RX_ALIGN_ERR,
> +	QCA8K_STATS_RX_RUNT,
> +	QCA8K_STATS_RX_FRAGMENT,
> +	QCA8K_STATS_RX_64,
> +	QCA8K_STATS_RX_128,
> +	QCA8K_STATS_RX_256,
> +	QCA8K_STATS_RX_512,
> +	QCA8K_STATS_RX_1024,
> +	QCA8K_STATS_RX_1518,
> +	QCA8K_STATS_RX_MAX,
> +	QCA8K_STATS_RX_TOO_LONG,
> +	QCA8K_STATS_RX_OVERFLOW,
> +	QCA8K_STATS_FILTERED,
> +	QCA8K_STATS_TX_UNDERRUN,
> +	QCA8K_STATS_TX_64,
> +	QCA8K_STATS_TX_128,
> +	QCA8K_STATS_TX_256,
> +	QCA8K_STATS_TX_512,
> +	QCA8K_STATS_TX_1024,
> +	QCA8K_STATS_TX_1518,
> +	QCA8K_STATS_TX_MAX,
> +	QCA8K_STATS_TX_OVERSIZE,
> +	QCA8K_STATS_TX_COLLISION,
> +	QCA8K_STATS_TX_ABORT_COL,
> +	QCA8K_STATS_TX_LATE_COL,
> +	QCA8K_STATS_MIB_COUNT
> +};
> +
> +/* Port counters for .get_stats64. That callback runs in atomic context and
> + * the MIB sits behind MDIO, so a worker reads it and this holds the result.
> + * The MIB counters are cumulative; last[] keeps the previous raw values so
> + * only the difference is added and a 32-bit counter may wrap between reads.
> + */
> +struct qca8k_port_stats {
> +	spinlock_t lock; /* protects stats against the worker */
> +	struct rtnl_link_stats64 stats;
> +	u32 last[QCA8K_STATS_MIB_COUNT];
> +	u64 last_rx_bytes;
> +	u64 last_tx_bytes;
> +};
> +
>  struct qca8k_priv {
>  	u8 switch_id;
>  	u8 switch_revision;
> @@ -469,6 +514,8 @@ struct qca8k_priv {
>  	struct qca8k_pcs pcs_port_6;
>  	const struct qca8k_match_data *info;
>  	struct qca8k_led ports_led[QCA8K_LED_COUNT];
> +	struct qca8k_port_stats port_stats[QCA8K_NUM_PORTS];
> +	struct delayed_work stats_work;
>  };
>  
>  struct qca8k_mib_desc {
> -- 
> 2.43.0
> 

-- 
	Ansuel

Re: [PATCH net-next] net: dsa: qca8k: report port statistics from the MIB

[Stanislaw Pal]

On Sun, Oct 18, 2026 at 11:55:42PM +0200, Christian Marangi wrote:
> If we really want we can reset after each read but that is problematic for
> the ethtool OPs unless we also update that function to store local u64 MIB
> for everything.
>
> Up to you... I would start with keeping and updating all the below things
> and then evaluate the needed changes to reset at every read.

Agreed. v2 keeps QCA8K_MIB_CPU_KEEP and adds only the differences, so
ethtool -S stays as it is.

> This really seems a task for the MIB from mgmt frame... May I ask why those
> are not used here?

Only because of where I tested. On my GL-B3000 the switch runs with a
tag_8021q based tagger from the OpenWrt tree I work on (the conduit is
offloaded to the SoC's NSS engine there), so mgmt_conduit is never set
and register reads over MDIO were all I had. That is no reason for
mainline.

v2 will request one autocast per interval when mgmt_conduit is set and
take every port from that one burst, and fall back to register reads
otherwise (another tagger, CPU port 6, conduit down). To get all ports
from one request, the autocast handler gets a mode that fills a buffer
per port instead of dropping the ports that were not asked for.

> I would like to see define in the header file, not in the middle of
> functions and declaration.

Will move it.

> Why STATS and not MIB? Also I would like to have all these register
> converted to define if really needed and update the ar8327_mib struct.
>
> Also Also why not use that directly?

Right, the second table is not needed. v2 drops it: an enum of indices
into ar8327_mib[] (the table turned into designated initializers, so
the two cannot drift apart), the counters read by the code that
get_ethtool_stats uses, split out into a helper, and get_stats64 built
from that array.

> Why this? For 64 bit?

Yes, RxGoodByte and TxByte are the two 64-bit counters. With the shared
helper and ar8327_mib[].size they go away.

> Maybe generalize the logic in get_ethtool_stats?

Yes, as above: one helper for both.

> As said MDIO here is extra slow especially for MIB every x seconds...
>
> Autocast MIB is a MUCH BETTER WAY.

Agreed, register reads stay only as the fallback.

> Is it correct to give window_errors to TX late collision?

I went by include/uapi/linux/if_link.h: "@tx_window_errors: Number of
frame transmission errors due to late collisions (for Ethernet - after
the first 64B of transmission)". The same file says tx_errors must
include them, which the patch does.

> No reason to scan for a port that is disabled or not connected...

Ports disabled in DT are not user ports, so the loop already skips
them. In the register-read fallback v2 also skips ports without link
(a bit set in mac_link_up and cleared in mac_link_down). Since the
counters keep their value, nothing is lost: the next read picks up the
difference. With autocast the switch sends every port anyway.

> Considering everything is run under the same delayed work, does it make
> sense to have a spinlock for each port?
>
> Either have a delayed work for each port or a global spinlock for the task.

One spinlock for the whole cache in v2.

v2 is written and tested on the B3000, both paths. Over three 15 s
uploads on the WAN port each, the wan netdev's TX bytes matched the
MIB's TxByte within 0.002% with the qca tagger, where every read was an
autocast, and within 0.01% with register reads. I will send it
tomorrow, so there is time to object to the approach first.

Thanks for the review.

Stanislaw

Re: [PATCH net-next] net: dsa: qca8k: report port statistics from the MIB

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 4 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Once qca8k_sw_shutdown() has run, nothing cancels the self-
  rearming stats_work.
- [High] The new stats worker is an unsynchronized regmap user that runs
  outside rtnl and reg_mutex.
- [Medium] qca8k_stats_read64() builds RxGoodByte/TxByte from two
  independent 32-bit reads, `qca8k_read(reg)` then `qca8k_read(reg +…
- [Medium] The new statistics path assumes qca8k_mib_init() succeeded,
  but qca8k_setup() still treats MIB init failure as non-fatal (`if…
- [Medium] The self-rearming stats_work keeps accessing the switch over
  MDIO/mgmt Ethernet across system suspend and resume, because the…
- [Medium] rx_bytes/tx_bytes are copied directly from the switch's
  RxGoodByte/TxByte MIB counters with no FCS adjustment.

--

net: dsa: qca8k: report port statistics from the MIB

This adds a .get_stats64 implementation to qca8k that reports per-port
counters from the switch MIB. A delayed work polls the MIB every 3 seconds
and adds the deltas into a cached rtnl_link_stats64, which .get_stats64
copies out under a spinlock. qca8k_setup() starts the work and a new
qca8k_teardown() cancels it.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 07227ae8bc040..9fef54a888502 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -1851,6 +1851,156 @@ static void qca8k_setup_hol_fixup(struct qca8k_priv *priv, int port)

[ ... ]

> +static int qca8k_stats_read64(struct qca8k_priv *priv, u32 reg, u64 *val)
> +{
> +	u32 lo, hi;
> +	int ret;
> +
> +	ret = qca8k_read(priv, reg, &lo);
> +	if (!ret)
> +		ret = qca8k_read(priv, reg + 4, &hi);
> +	if (!ret)
> +		*val = ((u64)hi << 32) | lo;
> +
> +	return ret;
> +}
> +
> +static u64 qca8k_stats_delta64(u64 now, u64 *last)
> +{
> +	/* a flushed MIB starts again from zero */
> +	u64 delta = now >= *last ? now - *last : now;
> +
> +	*last = now;
> +	return delta;
> +}

[Severity: Medium]
Can the two reads in qca8k_stats_read64() tear? RxGoodByte and TxByte are
each built from two separate qca8k_read() transactions. There is no
hi/lo/hi re-read and no latch. If the low word carries between the two
reads, the sample comes out about 2^32 too high.

The existing ethtool -S path reads the halves the same way. The code
doesn't show whether the switch latches the high word, so this part may
depend on the hardware.

If a sample ever goes backwards, qca8k_stats_delta64() treats it as a flush
and returns the full absolute value of now. A torn read or a bogus value
from a racing regmap access would both do this. Would the next good sample
then add the whole byte count since setup to s->rx_bytes or s->tx_bytes a
second time?

As far as I can see, the only MIB flush is the QCA8K_MIB_FLUSH in
qca8k_mib_init(). That runs before the worker starts, and
QCA8K_MIB_FLUSH_PORT is never used. So the "flushed MIB" branch has no
legitimate trigger at runtime.

The 32-bit counters in qca8k_stats_read_port() use plain modular
subtraction instead:

    d[i] = raw[i] - ps->last[i];

So the two paths disagree on what a decrease means. In either case, one
bad sample permanently corrupts the cumulative totals.

> +
> +static void qca8k_stats_read_port(struct qca8k_priv *priv, int port)
> +{

[ ... ]

> +	/* all of the slow MDIO reads before the lock */
> +	if (qca8k_stats_read64(priv, base + QCA8K_MIB_RX_GOOD_BYTE, &rx_bytes) ||
> +	    qca8k_stats_read64(priv, base + QCA8K_MIB_TX_BYTE, &tx_bytes))
> +		return;
> +
> +	for (i = 0; i < QCA8K_STATS_MIB_COUNT; i++)
> +		if (qca8k_read(priv, base + qca8k_stats_mib[i], &raw[i]))
> +			return;

[Severity: High]
Is this worker safe against other regmap users? The qca8k regmap is
created in qca8k_regmap_config with:

    .disable_locking = true, /* Locking is handled by qca8k read/write */

However, regmap_read() and regmap_write() both put the formatted register
and value into the shared map->work_buf, through _regmap_bus_read(),
_regmap_raw_read() and _regmap_bus_raw_write().

The mdio_lock and mgmt_eth_data->mutex cover only the bus transaction.
qca8k_bulk_read() reads *(u16 *)reg_buf before taking any lock.
qca8k_bulk_gather_write() also reads reg and *val before locking. And
_regmap_bus_read() parses work_val after the transport has unlocked.

This worker issues about 31 regmap_read() calls per user port every 3
seconds. It holds neither rtnl nor reg_mutex. It can run alongside
rtnl-held paths such as qca8k_get_ethtool_stats(), FDB/VLAN/ATU
programming through qca8k_write() and qca8k_busy_wait(), and port setup.

Could an interleaving give the worker another caller's value, or the 0
from *val = 0 in qca8k_mii_read32()? The delta logic would then add about
2^32, or the whole counter, for good.

Could it go the other way and give an rtnl reader the worker's MIB value?
For example, qca8k_busy_wait() could see a bogus busy bit.

Could it overwrite the register or value of a pending write in work_buf?
The config write could then be lost, land on a MIB register, or write a
MIB value into a config register.

The unlocked regmap design is older than this patch. However, this patch
adds a reader that runs all the time outside rtnl and relies on it.

> +
> +	spin_lock_bh(&ps->lock);
> +
> +	for (i = 0; i < QCA8K_STATS_MIB_COUNT; i++) {
> +		d[i] = raw[i] - ps->last[i];
> +		ps->last[i] = raw[i];
> +	}
> +
> +	s->rx_bytes += qca8k_stats_delta64(rx_bytes, &ps->last_rx_bytes);
> +	s->tx_bytes += qca8k_stats_delta64(tx_bytes, &ps->last_tx_bytes);

[Severity: Medium]
Do RxGoodByte and TxByte include the FCS? In
include/uapi/linux/if_link.h, struct rtnl_link_stats64 says rx_bytes and
tx_bytes:

    For IEEE 802.3 devices should count the length of Ethernet Frames
    excluding the FCS.

The commit message cites rtl8365mb as a model, and
rtl8365mb_stats_update() removes the FCS:

    /* if{In,Out}Octets includes FCS - remove it */

ksz_common.c and ksz8.c also subtract stats->rx_packets * ETH_FCS_LEN.

The 64..1518 size buckets used in this same function count the FCS in the
frame length. That suggests the byte counters include it too. If so,
every packet is overcounted by 4 bytes, which is about 6% for 64-byte
frames.

The test in the commit message shows the netdev now matches the switch's
own TxByte. That is agreement with the hardware number, not with the uAPI
definition.

The code doesn't confirm whether the QCA8K counters include the FCS, and
ar9331 doesn't subtract it either.

[ ... ]

> +static void qca8k_stats_poll(struct work_struct *work)
> +{
> +	struct qca8k_priv *priv = container_of(work, struct qca8k_priv,
> +					       stats_work.work);
> +	struct dsa_port *dp;
> +
> +	dsa_switch_for_each_user_port(dp, priv->ds)
> +		qca8k_stats_read_port(priv, dp->index);
> +
> +	schedule_delayed_work(&priv->stats_work, QCA8K_STATS_INTERVAL);
> +}

[ ... ]

> +static void qca8k_teardown(struct dsa_switch *ds)
> +{
> +	struct qca8k_priv *priv = ds->priv;
> +
> +	cancel_delayed_work_sync(&priv->stats_work);
> +}

[Severity: High]
What stops this work after qca8k_sw_shutdown()? qca8k_stats_poll()
re-arms itself with no stop condition. qca8k_teardown() is the only place
that cancels stats_work, and it is reached only through
dsa_unregister_switch() and the DSA setup error paths.

qca8k_sw_shutdown() calls dsa_switch_shutdown(), which doesn't call
ds->ops->teardown. It then clears drvdata:

    dsa_switch_shutdown(priv->ds);

    dev_set_drvdata(&mdiodev->dev, NULL);

The driver keeps this so that .remove can still come after .shutdown,
when the parent bus driver unregisters its children during its own
shutdown. In that case qca8k_sw_remove() takes the early return:

    if (!priv)
        return;

That skips dsa_unregister_switch(), so qca8k_teardown() never runs. devm
then frees priv, which embeds stats_work and port_stats[].

Wouldn't the still-queued timer then fire on freed memory? And wouldn't
qca8k_stats_poll() dereference priv->ds and priv->regmap and write into
the freed priv->port_stats[]?

Even without a later remove, the worker keeps sending MDIO or management
Ethernet traffic to a switch and conduit that are already shut down.

The commit message cites ar9331 and rtl8365mb. Both cancel their MIB work
in mac_link_down, which runs when dsa_switch_shutdown() closes the
conduit. The global work added here gets no such call.

> @@ -2020,6 +2170,11 @@ qca8k_setup(struct dsa_switch *ds)
>  	/* Set max number of LAGs supported */
>  	ds->num_lag_ids = QCA8K_NUM_LAGS;
>  
> +	for (int port = 0; port < QCA8K_NUM_PORTS; port++)
> +		spin_lock_init(&priv->port_stats[port].lock);
> +	INIT_DELAYED_WORK(&priv->stats_work, qca8k_stats_poll);
> +	schedule_delayed_work(&priv->stats_work, 0);

[Severity: Medium]
Should the poller and .get_stats64 depend on qca8k_mib_init() succeeding?
Earlier in qca8k_setup(), a failure there is still only a warning:

    ret = qca8k_mib_init(priv);
    if (ret)
        dev_warn(priv->dev, "mib init failed");

qca8k_mib_init() flushes the MIB, busy-waits, sets QCA8K_MIB_CPU_KEEP and
then writes QCA8K_MODULE_EN_MIB. It stops at the first error. That can
leave the MIB module disabled, so the counters are frozen. It can also
leave CPU_KEEP unset, so the counters may clear on read.

dsa_user_get_stats64() uses ds->ops->get_stats64 whenever it is set:

    if (ds->ops->get_stats64)
        ds->ops->get_stats64(ds, dp->index, s);
    else
        dev_get_tstats64(dev, s);

So the software tstats fallback is no longer used.

Would every user port then show zero traffic for the life of the device?
That would include the CPU-terminated traffic that was counted before.

With clear-on-read counters, would raw[i] - ps->last[i] wrap to about 2^32
on every decrease?

Before this patch, a MIB init failure only affected ethtool -S. The code
doesn't show the hardware defaults for MODULE_EN and CPU_KEEP, so the
clear-on-read case is speculative.

[Severity: Medium]
Does this work also need to be stopped across system suspend?
qca8k_suspend() and qca8k_resume() are not updated, and qca8k_suspend()
only does:

    qca8k_set_pm(priv, 0);

    return dsa_switch_suspend(priv->ds);

dsa_switch_suspend() doesn't touch stats_work. The work is queued on
system_percpu_wq, which isn't freezable.

The work can still run after the switch is suspended, and later after the
parent MDIO controller or conduit is suspended too. On resume after any
sleep longer than 3 seconds, the timer has already expired. qca8k_stats_poll()
can then run before dpm_resume() reaches the MDIO controller.

Couldn't qca8k_read() then be issued on a suspended bus? Depending on the
SoC, the result ranges from a skipped sample to a bus fault, or a hang on
a clock-gated controller.

ar9331 and rtl8365mb stop their MIB work in mac_link_down, which
phylink_stop() triggers during dsa_switch_suspend(). The global work added
here has no such hook.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018153658.209999-1-kuncy7%40gmail.com