[PATCH net-next v2 0/2] net: dsa: qca8k: report port statistics from the MIB

[PATCH net-next v2 0/2] net: dsa: qca8k: report port statistics from the MIB

[Stanislaw Pal]

qca8k has no .get_stats64, so a user port's netdev counts only what
crosses the CPU port; traffic the switch forwards in hardware never
shows up. Patch 2 reports the port counters from the switch MIB through
a delayed work, as ar9331 and rtl8365mb do. Patch 1 makes register
access safe for such a reader outside rtnl first.

Changes in v2:
- patch 1 is new: the regmap was created without locking while it
  formats register and value in its shared work_buf, and an autocast
  and qca8k_conduit_change() took the MIB and management mutexes in
  opposite orders (netdev AI review)
- read the MIB with one autocast request for all ports when the
  management path is up, register reads only as the fallback, only for
  ports with link (Christian)
- index ar8327_mib[] by an enum instead of a second table of offsets,
  share the register reads with qca8k_get_ethtool_stats() (Christian)
- MIB instead of STATS in the names, interval define in qca8k.h, one
  spinlock for the cached counters (Christian)
- stop the work on shutdown and suspend, init it in probe; keep the
  user port's tstats if the MIB did not come up; read the high word of
  a 64-bit counter around the low one and drop a 64-bit sample that
  goes backwards; take the FCS off the byte counters (netdev AI review)

On the FCS: on the CPU port of the B3000, with an 802.1Q tag on the
link, the maximum-size frames (1518 bytes without the FCS) land in
RxMaxByte, so the size buckets count the FCS; the CPU port's
RxGoodByte also matches the conduit GMAC's MMC octet counter byte for
byte. Please correct me if the byte counters are documented otherwise.

Tested on a GL-B3000 (QCA8337, CPU port 0), three 15 s uploads over the
WAN port per case: with register reads the wan netdev's TX bytes were
TxByte less 4 bytes per frame within 0.004%; with the qca tagger, where
every read is an autocast, the counters followed TxByte within 0.002%
(measured before the FCS change). With lockdep, a tagger change and
three conduit down/up cycles with the autocast work running and
ethtool -S in a loop gave no report.

v1: https://lore.kernel.org/netdev/20261018153658.209999-1-kuncy7@gmail.com/

Stanislaw Pal (2):
  net: dsa: qca8k: serialize register access
  net: dsa: qca8k: report port statistics from the MIB

 drivers/net/dsa/qca/qca8k-8xxx.c   | 194 ++++++++++++++++++++++++++---
 drivers/net/dsa/qca/qca8k-common.c | 137 ++++++++++++--------
 drivers/net/dsa/qca/qca8k.h        |  72 ++++++++++-
 3 files changed, 332 insertions(+), 71 deletions(-)


base-commit: d1241cdbbcf7bfac3e53482b2054504628f9f047
-- 
2.43.0

[PATCH net-next v2 1/2] net: dsa: qca8k: serialize register access

[Stanislaw Pal]

The regmap is created with .disable_locking, on the ground that
qca8k's read and write callbacks do their own locking. They only lock
the bus transaction, though: regmap formats the register and parses
the value in the map's shared work_buf, outside of them, so two
concurrent users can swap or corrupt each other's register and value.
Most register access runs under rtnl, but not all of it - phylink's
resolve work (mac_link_up/down) and the LED class callbacks do not -
and the next change adds a delayed work that reads the MIB. Let regmap
take its own lock; every user sleeps already, on the MDIO bus or on
the management frame round trip.

An autocast MIB read holds the MIB mutex while its register write
takes the management mutex, and qca8k_conduit_change() takes the two
the other way round. Both run under rtnl today; with a reader outside
rtnl that would be an ABBA deadlock. Take the MIB mutex first there
too.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 07227ae8b..9a8b9e34a 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -579,7 +579,6 @@ static const struct regmap_config qca8k_regmap_config = {
 	.write = qca8k_bulk_write,
 	.reg_update_bits = qca8k_regmap_update_bits,
 	.rd_table = &qca8k_readable_table,
-	.disable_locking = true, /* Locking is handled by qca8k read/write */
 	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
 	.max_raw_read = 32, /* mgmt eth can read up to 8 registers at time */
 	/* ATU regs suffer from a bug where some data are not correctly
@@ -1782,13 +1781,16 @@ qca8k_conduit_change(struct dsa_switch *ds, const struct net_device *conduit,
 	if (dp->index != 0)
 		return;
 
-	mutex_lock(&priv->mgmt_eth_data.mutex);
+	/* Same order as an autocast, which holds the MIB mutex while its
+	 * register write takes the management one.
+	 */
 	mutex_lock(&priv->mib_eth_data.mutex);
+	mutex_lock(&priv->mgmt_eth_data.mutex);
 
 	priv->mgmt_conduit = operational ? (struct net_device *)conduit : NULL;
 
-	mutex_unlock(&priv->mib_eth_data.mutex);
 	mutex_unlock(&priv->mgmt_eth_data.mutex);
+	mutex_unlock(&priv->mib_eth_data.mutex);
 }
 
 static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
-- 
2.43.0

[PATCH net-next v2 2/2] 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, so a delayed work reads the MIB every 3 s and
.get_stats64 hands out the cached result, as ar9331 and rtl8365mb do.

With the Ethernet management path up, one autocast request returns the
MIB of every port: the autocast handler gets a mode that keeps one row
per port instead of only the port that was asked for. Otherwise the
work falls back to register reads, with the loop split out of
qca8k_get_ethtool_stats(), only for the ports that have link, and with
the high word of a 64-bit counter read around the low one.

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, and a 64-bit
sample below the previous one is dropped as a bad read. The byte
counters include the FCS, as the size buckets do, so ETH_FCS_LEN per
frame comes off. If the MIB failed to come up, the user port keeps its
own tstats. The work stops on teardown, shutdown and suspend.

An index enum for ar8327_mib[] lets the stats code name the counters it
uses.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Stanislaw Pal <kuncy7@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c   | 186 +++++++++++++++++++++++++++--
 drivers/net/dsa/qca/qca8k-common.c | 137 ++++++++++++---------
 drivers/net/dsa/qca/qca8k.h        |  72 ++++++++++-
 3 files changed, 327 insertions(+), 68 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 9a8b9e34a..c95df2989 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1450,6 +1450,7 @@ qca8k_phylink_mac_link_down(struct phylink_config *config, unsigned int mode,
 	struct dsa_port *dp = dsa_phylink_to_port(config);
 	struct qca8k_priv *priv = dp->ds->priv;
 
+	clear_bit(dp->index, &priv->mib_stats.link_map);
 	qca8k_port_set_status(priv, dp->index, 0);
 }
 
@@ -1495,6 +1496,7 @@ qca8k_phylink_mac_link_up(struct phylink_config *config,
 	reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
 
 	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg);
+	set_bit(port, &priv->mib_stats.link_map);
 }
 
 static struct qca8k_pcs *pcs_to_qca8k_pcs(struct phylink_pcs *pcs)
@@ -1669,17 +1671,23 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 	const struct qca8k_mib_desc *mib;
 	struct mib_ethhdr *mib_ethhdr;
 	__le32 *data2;
+	u64 *data;
 	u8 port;
 	int i;
 
 	mib_ethhdr = (struct mib_ethhdr *)skb_mac_header(skb);
 	mib_eth_data = &priv->mib_eth_data;
 
-	/* The switch autocast every port. Ignore other packet and
-	 * parse only the requested one.
+	/* The switch autocast every port. Parse only the requested one, or
+	 * every port into its own row for QCA8K_MIB_ALL_PORTS.
 	 */
 	port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr));
-	if (port != mib_eth_data->req_port)
+	if (mib_eth_data->req_port == QCA8K_MIB_ALL_PORTS &&
+	    port < QCA8K_NUM_PORTS)
+		data = mib_eth_data->data + port * QCA8K_QCA833X_MIB_COUNT;
+	else if (port == mib_eth_data->req_port)
+		data = mib_eth_data->data;
+	else
 		goto exit;
 
 	data2 = (__le32 *)skb->data;
@@ -1689,15 +1697,15 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 
 		/* First 3 mib are present in the skb head */
 		if (i < 3) {
-			mib_eth_data->data[i] = get_unaligned_le32(mib_ethhdr->data + i);
+			data[i] = get_unaligned_le32(mib_ethhdr->data + i);
 			continue;
 		}
 
 		/* Some mib are 64 bit wide */
 		if (mib->size == 2)
-			mib_eth_data->data[i] = get_unaligned_le64((__le64 *)data2);
+			data[i] = get_unaligned_le64((__le64 *)data2);
 		else
-			mib_eth_data->data[i] = get_unaligned_le32(data2);
+			data[i] = get_unaligned_le32(data2);
 
 		data2 += mib->size;
 	}
@@ -1708,12 +1716,9 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 		complete(&mib_eth_data->rw_done);
 }
 
-static int
-qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
+static int qca8k_mib_autocast(struct qca8k_priv *priv, u8 req_port, u64 *data)
 {
-	struct dsa_port *dp = dsa_to_port(ds, port);
 	struct qca8k_mib_eth_data *mib_eth_data;
-	struct qca8k_priv *priv = ds->priv;
 	int ret;
 
 	mib_eth_data = &priv->mib_eth_data;
@@ -1722,7 +1727,7 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 
 	reinit_completion(&mib_eth_data->rw_done);
 
-	mib_eth_data->req_port = dp->index;
+	mib_eth_data->req_port = req_port;
 	mib_eth_data->data = data;
 	refcount_set(&mib_eth_data->port_parsed, QCA8K_NUM_PORTS);
 
@@ -1747,6 +1752,14 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 	return ret;
 }
 
+static int
+qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+
+	return qca8k_mib_autocast(ds->priv, dp->index, data);
+}
+
 static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)
 {
 	struct qca8k_priv *priv = ds->priv;
@@ -1853,6 +1866,141 @@ static void qca8k_setup_hol_fixup(struct qca8k_priv *priv, int port)
 			   mask);
 }
 
+static u64 qca8k_mib_delta(struct qca8k_priv *priv, int port, int i)
+{
+	struct qca8k_mib_stats *ms = &priv->mib_stats;
+	u64 now = ms->raw[port][i];
+	u64 last = ms->last[port][i];
+
+	/* A 32-bit counter may wrap between two reads. A 64-bit one does
+	 * not, and nothing flushes the MIB after setup: a sample below the
+	 * previous one is a bad read, keep the previous one.
+	 */
+	if (ar8327_mib[i].size == 2 && now < last)
+		return 0;
+
+	ms->last[port][i] = now;
+
+	if (ar8327_mib[i].size == 1)
+		return (u32)(now - last);
+
+	return now - last;
+}
+
+/* The byte counters include the FCS, as the size buckets do */
+static u64 qca8k_mib_bytes(u64 bytes, u64 packets)
+{
+	u64 fcs = packets * ETH_FCS_LEN;
+
+	/* bytes is 0 when its sample was dropped as a bad read */
+	return bytes > fcs ? bytes - fcs : 0;
+}
+
+static void qca8k_mib_stats_update(struct qca8k_priv *priv, int port)
+{
+	struct qca8k_mib_stats *ms = &priv->mib_stats;
+	struct rtnl_link_stats64 *s = &ms->stats[port];
+	u64 d[QCA8K_QCA833X_MIB_COUNT];
+	u64 rx_packets, tx_packets;
+	int i;
+
+	for (i = 0; i < priv->info->mib_count; i++)
+		d[i] = qca8k_mib_delta(priv, port, i);
+
+	rx_packets = d[QCA8K_MIB_RX_64] + d[QCA8K_MIB_RX_128] +
+		     d[QCA8K_MIB_RX_256] + d[QCA8K_MIB_RX_512] +
+		     d[QCA8K_MIB_RX_1024] + d[QCA8K_MIB_RX_1518] +
+		     d[QCA8K_MIB_RX_MAX];
+	tx_packets = d[QCA8K_MIB_TX_64] + d[QCA8K_MIB_TX_128] +
+		     d[QCA8K_MIB_TX_256] + d[QCA8K_MIB_TX_512] +
+		     d[QCA8K_MIB_TX_1024] + d[QCA8K_MIB_TX_1518] +
+		     d[QCA8K_MIB_TX_MAX];
+
+	spin_lock_bh(&ms->lock);
+
+	s->rx_bytes += qca8k_mib_bytes(d[QCA8K_MIB_RX_GOOD_BYTE], rx_packets);
+	s->tx_bytes += qca8k_mib_bytes(d[QCA8K_MIB_TX_BYTE], tx_packets);
+	s->rx_packets += rx_packets;
+	s->tx_packets += tx_packets;
+	s->multicast += d[QCA8K_MIB_RX_MULTI];
+	s->rx_length_errors += d[QCA8K_MIB_RX_RUNT] +
+			       d[QCA8K_MIB_RX_FRAGMENT] +
+			       d[QCA8K_MIB_RX_TOO_LONG];
+	s->rx_crc_errors += d[QCA8K_MIB_RX_FCS_ERR];
+	s->rx_frame_errors += d[QCA8K_MIB_RX_ALIGN_ERR];
+	s->rx_missed_errors += d[QCA8K_MIB_RX_OVERFLOW];
+	s->rx_dropped += d[QCA8K_MIB_FILTERED];
+	s->rx_errors += d[QCA8K_MIB_RX_FCS_ERR] + d[QCA8K_MIB_RX_ALIGN_ERR] +
+			d[QCA8K_MIB_RX_RUNT] + d[QCA8K_MIB_RX_FRAGMENT] +
+			d[QCA8K_MIB_RX_OVERFLOW] + d[QCA8K_MIB_RX_TOO_LONG];
+	s->tx_fifo_errors += d[QCA8K_MIB_TX_UNDERRUN];
+	s->tx_aborted_errors += d[QCA8K_MIB_TX_ABORT_COL];
+	s->tx_window_errors += d[QCA8K_MIB_TX_LATE_COL];
+	s->tx_errors += d[QCA8K_MIB_TX_OVERSIZE] + d[QCA8K_MIB_TX_ABORT_COL] +
+			d[QCA8K_MIB_TX_UNDERRUN] + d[QCA8K_MIB_TX_LATE_COL];
+	s->collisions += d[QCA8K_MIB_TX_COLLISION];
+
+	spin_unlock_bh(&ms->lock);
+}
+
+static void qca8k_mib_stats_poll(struct work_struct *work)
+{
+	struct qca8k_priv *priv = container_of(work, struct qca8k_priv,
+					       mib_stats.work.work);
+	struct qca8k_mib_stats *ms = &priv->mib_stats;
+	bool autocast = false;
+	struct dsa_port *dp;
+	int port;
+
+	/* One autocast request returns the MIB of every port */
+	if (priv->mgmt_conduit)
+		autocast = qca8k_mib_autocast(priv, QCA8K_MIB_ALL_PORTS,
+					      &ms->raw[0][0]) > 0;
+
+	dsa_switch_for_each_user_port(dp, priv->ds) {
+		port = dp->index;
+
+		if (!autocast) {
+			/* Register reads are slow, skip ports without link.
+			 * Their counters keep their value, so the next read
+			 * still adds everything since the previous one.
+			 */
+			if (!test_bit(port, &ms->link_map))
+				continue;
+			if (qca8k_mib_read(priv, port, ms->raw[port]))
+				continue;
+		}
+
+		qca8k_mib_stats_update(priv, port);
+	}
+
+	schedule_delayed_work(&ms->work, QCA8K_MIB_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_mib_stats *ms = &priv->mib_stats;
+
+	/* Without the MIB keep what the user port counts itself */
+	if (!ms->enabled) {
+		dev_get_tstats64(dsa_to_port(ds, port)->user, s);
+		return;
+	}
+
+	spin_lock_bh(&ms->lock);
+	*s = ms->stats[port];
+	spin_unlock_bh(&ms->lock);
+}
+
+static void qca8k_teardown(struct dsa_switch *ds)
+{
+	struct qca8k_priv *priv = ds->priv;
+
+	cancel_delayed_work_sync(&priv->mib_stats.work);
+}
+
 static int
 qca8k_setup(struct dsa_switch *ds)
 {
@@ -1911,6 +2059,7 @@ qca8k_setup(struct dsa_switch *ds)
 	ret = qca8k_mib_init(priv);
 	if (ret)
 		dev_warn(priv->dev, "mib init failed");
+	priv->mib_stats.enabled = !ret;
 
 	/* Initial setup of all ports */
 	dsa_switch_for_each_port(dp, ds) {
@@ -2022,6 +2171,10 @@ qca8k_setup(struct dsa_switch *ds)
 	/* Set max number of LAGs supported */
 	ds->num_lag_ids = QCA8K_NUM_LAGS;
 
+	if (priv->mib_stats.enabled)
+		schedule_delayed_work(&priv->mib_stats.work,
+				      QCA8K_MIB_STATS_INTERVAL);
+
 	return 0;
 }
 
@@ -2035,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,
@@ -2124,6 +2279,9 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	mutex_init(&priv->mib_eth_data.mutex);
 	init_completion(&priv->mib_eth_data.rw_done);
 
+	spin_lock_init(&priv->mib_stats.lock);
+	INIT_DELAYED_WORK(&priv->mib_stats.work, qca8k_mib_stats_poll);
+
 	priv->ds->dev = &mdiodev->dev;
 	priv->ds->num_ports = QCA8K_NUM_PORTS;
 	priv->ds->priv = priv;
@@ -2157,6 +2315,8 @@ static void qca8k_sw_shutdown(struct mdio_device *mdiodev)
 	if (!priv)
 		return;
 
+	/* .teardown does not run on this path */
+	cancel_delayed_work_sync(&priv->mib_stats.work);
 	dsa_switch_shutdown(priv->ds);
 
 	dev_set_drvdata(&mdiodev->dev, NULL);
@@ -2183,6 +2343,7 @@ static int qca8k_suspend(struct device *dev)
 {
 	struct qca8k_priv *priv = dev_get_drvdata(dev);
 
+	cancel_delayed_work_sync(&priv->mib_stats.work);
 	qca8k_set_pm(priv, 0);
 
 	return dsa_switch_suspend(priv->ds);
@@ -2193,6 +2354,9 @@ static int qca8k_resume(struct device *dev)
 	struct qca8k_priv *priv = dev_get_drvdata(dev);
 
 	qca8k_set_pm(priv, 1);
+	if (priv->mib_stats.enabled)
+		schedule_delayed_work(&priv->mib_stats.work,
+				      QCA8K_MIB_STATS_INTERVAL);
 
 	return dsa_switch_resume(priv->ds);
 }
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
index ab978cc83..232367892 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -20,49 +20,51 @@
 	}
 
 const struct qca8k_mib_desc ar8327_mib[] = {
-	MIB_DESC(1, 0x00, "RxBroad"),
-	MIB_DESC(1, 0x04, "RxPause"),
-	MIB_DESC(1, 0x08, "RxMulti"),
-	MIB_DESC(1, 0x0c, "RxFcsErr"),
-	MIB_DESC(1, 0x10, "RxAlignErr"),
-	MIB_DESC(1, 0x14, "RxRunt"),
-	MIB_DESC(1, 0x18, "RxFragment"),
-	MIB_DESC(1, 0x1c, "Rx64Byte"),
-	MIB_DESC(1, 0x20, "Rx128Byte"),
-	MIB_DESC(1, 0x24, "Rx256Byte"),
-	MIB_DESC(1, 0x28, "Rx512Byte"),
-	MIB_DESC(1, 0x2c, "Rx1024Byte"),
-	MIB_DESC(1, 0x30, "Rx1518Byte"),
-	MIB_DESC(1, 0x34, "RxMaxByte"),
-	MIB_DESC(1, 0x38, "RxTooLong"),
-	MIB_DESC(2, 0x3c, "RxGoodByte"),
-	MIB_DESC(2, 0x44, "RxBadByte"),
-	MIB_DESC(1, 0x4c, "RxOverFlow"),
-	MIB_DESC(1, 0x50, "Filtered"),
-	MIB_DESC(1, 0x54, "TxBroad"),
-	MIB_DESC(1, 0x58, "TxPause"),
-	MIB_DESC(1, 0x5c, "TxMulti"),
-	MIB_DESC(1, 0x60, "TxUnderRun"),
-	MIB_DESC(1, 0x64, "Tx64Byte"),
-	MIB_DESC(1, 0x68, "Tx128Byte"),
-	MIB_DESC(1, 0x6c, "Tx256Byte"),
-	MIB_DESC(1, 0x70, "Tx512Byte"),
-	MIB_DESC(1, 0x74, "Tx1024Byte"),
-	MIB_DESC(1, 0x78, "Tx1518Byte"),
-	MIB_DESC(1, 0x7c, "TxMaxByte"),
-	MIB_DESC(1, 0x80, "TxOverSize"),
-	MIB_DESC(2, 0x84, "TxByte"),
-	MIB_DESC(1, 0x8c, "TxCollision"),
-	MIB_DESC(1, 0x90, "TxAbortCol"),
-	MIB_DESC(1, 0x94, "TxMultiCol"),
-	MIB_DESC(1, 0x98, "TxSingleCol"),
-	MIB_DESC(1, 0x9c, "TxExcDefer"),
-	MIB_DESC(1, 0xa0, "TxDefer"),
-	MIB_DESC(1, 0xa4, "TxLateCol"),
-	MIB_DESC(1, 0xa8, "RXUnicast"),
-	MIB_DESC(1, 0xac, "TXUnicast"),
+	[QCA8K_MIB_RX_BROAD]		= MIB_DESC(1, 0x00, "RxBroad"),
+	[QCA8K_MIB_RX_PAUSE]		= MIB_DESC(1, 0x04, "RxPause"),
+	[QCA8K_MIB_RX_MULTI]		= MIB_DESC(1, 0x08, "RxMulti"),
+	[QCA8K_MIB_RX_FCS_ERR]		= MIB_DESC(1, 0x0c, "RxFcsErr"),
+	[QCA8K_MIB_RX_ALIGN_ERR]	= MIB_DESC(1, 0x10, "RxAlignErr"),
+	[QCA8K_MIB_RX_RUNT]		= MIB_DESC(1, 0x14, "RxRunt"),
+	[QCA8K_MIB_RX_FRAGMENT]		= MIB_DESC(1, 0x18, "RxFragment"),
+	[QCA8K_MIB_RX_64]		= MIB_DESC(1, 0x1c, "Rx64Byte"),
+	[QCA8K_MIB_RX_128]		= MIB_DESC(1, 0x20, "Rx128Byte"),
+	[QCA8K_MIB_RX_256]		= MIB_DESC(1, 0x24, "Rx256Byte"),
+	[QCA8K_MIB_RX_512]		= MIB_DESC(1, 0x28, "Rx512Byte"),
+	[QCA8K_MIB_RX_1024]		= MIB_DESC(1, 0x2c, "Rx1024Byte"),
+	[QCA8K_MIB_RX_1518]		= MIB_DESC(1, 0x30, "Rx1518Byte"),
+	[QCA8K_MIB_RX_MAX]		= MIB_DESC(1, 0x34, "RxMaxByte"),
+	[QCA8K_MIB_RX_TOO_LONG]		= MIB_DESC(1, 0x38, "RxTooLong"),
+	[QCA8K_MIB_RX_GOOD_BYTE]	= MIB_DESC(2, 0x3c, "RxGoodByte"),
+	[QCA8K_MIB_RX_BAD_BYTE]		= MIB_DESC(2, 0x44, "RxBadByte"),
+	[QCA8K_MIB_RX_OVERFLOW]		= MIB_DESC(1, 0x4c, "RxOverFlow"),
+	[QCA8K_MIB_FILTERED]		= MIB_DESC(1, 0x50, "Filtered"),
+	[QCA8K_MIB_TX_BROAD]		= MIB_DESC(1, 0x54, "TxBroad"),
+	[QCA8K_MIB_TX_PAUSE]		= MIB_DESC(1, 0x58, "TxPause"),
+	[QCA8K_MIB_TX_MULTI]		= MIB_DESC(1, 0x5c, "TxMulti"),
+	[QCA8K_MIB_TX_UNDERRUN]		= MIB_DESC(1, 0x60, "TxUnderRun"),
+	[QCA8K_MIB_TX_64]		= MIB_DESC(1, 0x64, "Tx64Byte"),
+	[QCA8K_MIB_TX_128]		= MIB_DESC(1, 0x68, "Tx128Byte"),
+	[QCA8K_MIB_TX_256]		= MIB_DESC(1, 0x6c, "Tx256Byte"),
+	[QCA8K_MIB_TX_512]		= MIB_DESC(1, 0x70, "Tx512Byte"),
+	[QCA8K_MIB_TX_1024]		= MIB_DESC(1, 0x74, "Tx1024Byte"),
+	[QCA8K_MIB_TX_1518]		= MIB_DESC(1, 0x78, "Tx1518Byte"),
+	[QCA8K_MIB_TX_MAX]		= MIB_DESC(1, 0x7c, "TxMaxByte"),
+	[QCA8K_MIB_TX_OVERSIZE]		= MIB_DESC(1, 0x80, "TxOverSize"),
+	[QCA8K_MIB_TX_BYTE]		= MIB_DESC(2, 0x84, "TxByte"),
+	[QCA8K_MIB_TX_COLLISION]	= MIB_DESC(1, 0x8c, "TxCollision"),
+	[QCA8K_MIB_TX_ABORT_COL]	= MIB_DESC(1, 0x90, "TxAbortCol"),
+	[QCA8K_MIB_TX_MULTI_COL]	= MIB_DESC(1, 0x94, "TxMultiCol"),
+	[QCA8K_MIB_TX_SINGLE_COL]	= MIB_DESC(1, 0x98, "TxSingleCol"),
+	[QCA8K_MIB_TX_EXC_DEFER]	= MIB_DESC(1, 0x9c, "TxExcDefer"),
+	[QCA8K_MIB_TX_DEFER]		= MIB_DESC(1, 0xa0, "TxDefer"),
+	[QCA8K_MIB_TX_LATE_COL]		= MIB_DESC(1, 0xa4, "TxLateCol"),
+	[QCA8K_MIB_RX_UNICAST]		= MIB_DESC(1, 0xa8, "RXUnicast"),
+	[QCA8K_MIB_TX_UNICAST]		= MIB_DESC(1, 0xac, "TXUnicast"),
 };
 
+static_assert(ARRAY_SIZE(ar8327_mib) == QCA8K_QCA833X_MIB_COUNT);
+
 int qca8k_read(struct qca8k_priv *priv, u32 reg, u32 *val)
 {
 	return regmap_read(priv->regmap, reg, val);
@@ -487,37 +489,62 @@ void qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset,
 		ethtool_puts(&data, ar8327_mib[i].name);
 }
 
-void qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
-			     uint64_t *data)
+/* Read the MIB of one port with register reads, one value per ar8327_mib[]
+ * entry. A counter that cannot be read keeps its old value in data[]; the
+ * last error is returned.
+ */
+int qca8k_mib_read(struct qca8k_priv *priv, int port, u64 *data)
 {
-	struct qca8k_priv *priv = ds->priv;
 	const struct qca8k_mib_desc *mib;
-	u32 reg, i, val;
+	u32 reg, i, val, hi2;
+	int err = 0;
 	u32 hi = 0;
 	int ret;
 
-	if (priv->mgmt_conduit && priv->info->ops->autocast_mib &&
-	    priv->info->ops->autocast_mib(ds, port, data) > 0)
-		return;
-
 	for (i = 0; i < priv->info->mib_count; i++) {
 		mib = &ar8327_mib[i];
 		reg = QCA8K_PORT_MIB_COUNTER(port) + mib->offset;
 
-		ret = qca8k_read(priv, reg, &val);
-		if (ret < 0)
-			continue;
-
+		/* The two halves of a 64-bit counter are separate reads: read
+		 * the high word around the low one and take the low word
+		 * again if a carry got in between.
+		 */
 		if (mib->size == 2) {
 			ret = qca8k_read(priv, reg + 4, &hi);
-			if (ret < 0)
-				continue;
+			if (!ret)
+				ret = qca8k_read(priv, reg, &val);
+			if (!ret)
+				ret = qca8k_read(priv, reg + 4, &hi2);
+			if (!ret && hi2 != hi) {
+				hi = hi2;
+				ret = qca8k_read(priv, reg, &val);
+			}
+		} else {
+			ret = qca8k_read(priv, reg, &val);
+		}
+		if (ret < 0) {
+			err = ret;
+			continue;
 		}
 
 		data[i] = val;
 		if (mib->size == 2)
 			data[i] |= (u64)hi << 32;
 	}
+
+	return err;
+}
+
+void qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
+			     uint64_t *data)
+{
+	struct qca8k_priv *priv = ds->priv;
+
+	if (priv->mgmt_conduit && priv->info->ops->autocast_mib &&
+	    priv->info->ops->autocast_mib(ds, port, data) > 0)
+		return;
+
+	qca8k_mib_read(priv, port, data);
 }
 
 int qca8k_get_sset_count(struct dsa_switch *ds, int port, int sset)
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 956338893..5912117fc 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -31,6 +31,9 @@
 
 #define QCA8K_QCA832X_MIB_COUNT				39
 #define QCA8K_QCA833X_MIB_COUNT				41
+#define QCA8K_MIB_STATS_INTERVAL			(3 * HZ)
+/* qca8k_mib_autocast() request for every port at once */
+#define QCA8K_MIB_ALL_PORTS				0xff
 
 #define QCA8K_BUSY_WAIT_TIMEOUT				2000
 
@@ -373,6 +376,52 @@ enum qca8k_mid_cmd {
 	QCA8K_MIB_CAST = 3,
 };
 
+/* Index of each counter in ar8327_mib[] */
+enum qca8k_mib_idx {
+	QCA8K_MIB_RX_BROAD,
+	QCA8K_MIB_RX_PAUSE,
+	QCA8K_MIB_RX_MULTI,
+	QCA8K_MIB_RX_FCS_ERR,
+	QCA8K_MIB_RX_ALIGN_ERR,
+	QCA8K_MIB_RX_RUNT,
+	QCA8K_MIB_RX_FRAGMENT,
+	QCA8K_MIB_RX_64,
+	QCA8K_MIB_RX_128,
+	QCA8K_MIB_RX_256,
+	QCA8K_MIB_RX_512,
+	QCA8K_MIB_RX_1024,
+	QCA8K_MIB_RX_1518,
+	QCA8K_MIB_RX_MAX,
+	QCA8K_MIB_RX_TOO_LONG,
+	QCA8K_MIB_RX_GOOD_BYTE,
+	QCA8K_MIB_RX_BAD_BYTE,
+	QCA8K_MIB_RX_OVERFLOW,
+	QCA8K_MIB_FILTERED,
+	QCA8K_MIB_TX_BROAD,
+	QCA8K_MIB_TX_PAUSE,
+	QCA8K_MIB_TX_MULTI,
+	QCA8K_MIB_TX_UNDERRUN,
+	QCA8K_MIB_TX_64,
+	QCA8K_MIB_TX_128,
+	QCA8K_MIB_TX_256,
+	QCA8K_MIB_TX_512,
+	QCA8K_MIB_TX_1024,
+	QCA8K_MIB_TX_1518,
+	QCA8K_MIB_TX_MAX,
+	QCA8K_MIB_TX_OVERSIZE,
+	QCA8K_MIB_TX_BYTE,
+	QCA8K_MIB_TX_COLLISION,
+	QCA8K_MIB_TX_ABORT_COL,
+	QCA8K_MIB_TX_MULTI_COL,
+	QCA8K_MIB_TX_SINGLE_COL,
+	QCA8K_MIB_TX_EXC_DEFER,
+	QCA8K_MIB_TX_DEFER,
+	QCA8K_MIB_TX_LATE_COL,
+	/* QCA833x only */
+	QCA8K_MIB_RX_UNICAST,
+	QCA8K_MIB_TX_UNICAST,
+};
+
 struct qca8k_priv;
 
 struct qca8k_info_ops {
@@ -403,8 +452,25 @@ struct qca8k_mib_eth_data {
 	struct completion rw_done;
 	struct mutex mutex; /* Process one command at time */
 	refcount_t port_parsed; /* Counter to track parsed port */
-	u8 req_port;
-	u64 *data; /* pointer to ethtool data */
+	u8 req_port; /* or QCA8K_MIB_ALL_PORTS */
+	/* ethtool data, or one QCA8K_QCA833X_MIB_COUNT row per port */
+	u64 *data;
+};
+
+/* .get_stats64 is called in atomic context and the MIB sits behind MDIO or
+ * the Ethernet management frames, so a delayed work reads it and keeps the
+ * result. The MIB counters keep their value across reads; last[] holds the
+ * previous raw values so only the difference is added, and a 32-bit counter
+ * may wrap between two reads.
+ */
+struct qca8k_mib_stats {
+	struct delayed_work work;
+	spinlock_t lock; /* protects stats[] against the work */
+	bool enabled; /* the MIB came up, the work runs */
+	unsigned long link_map; /* ports with link up */
+	struct rtnl_link_stats64 stats[QCA8K_NUM_PORTS];
+	u64 last[QCA8K_NUM_PORTS][QCA8K_QCA833X_MIB_COUNT];
+	u64 raw[QCA8K_NUM_PORTS][QCA8K_QCA833X_MIB_COUNT];
 };
 
 struct qca8k_ports_config {
@@ -469,6 +535,7 @@ 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_mib_stats mib_stats;
 };
 
 struct qca8k_mib_desc {
@@ -514,6 +581,7 @@ int qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val);
 void qca8k_fdb_flush(struct qca8k_priv *priv);
 
 /* Common ethtool stats function */
+int qca8k_mib_read(struct qca8k_priv *priv, int port, u64 *data);
 void qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data);
 void qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
 			     uint64_t *data);
-- 
2.43.0

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

[Jakub Kicinski]

On Mon, 19 Oct 2026 21:11:35 +0200 Stanislaw Pal wrote:
> qca8k has no .get_stats64, so a user port's netdev counts only what
> crosses the CPU port; traffic the switch forwards in hardware never
> shows up. Patch 2 reports the port counters from the switch MIB through
> a delayed work, as ar9331 and rtl8365mb do. Patch 1 makes register
> access safe for such a reader outside rtnl first.

## Form letter - net-next-closed

The merge window for Linux v7.4 has begun, and the net-next
tree is now closed for new drivers, features, code refactoring
and optimizations. We are currently accepting bug fixes only.

Please repost when net-next reopens. There is some uncertainty
about how long this merge window will last, so please check
https://netdev.bots.linux.dev/net-next.html
before reposting. net-next _may_ reopen on Nov 2nd.

See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle
-- 
pw-bot: defer
pv-bot: close

Re: [PATCH net-next v2 1/2] net: dsa: qca8k: serialize register access

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · 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).

- [Medium] The patch fixes a regmap work_buf race that already exists in
  released kernels, but it has no Fixes: tag and no stable Cc.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a high-
  severity bug the series builds on.

--

net: dsa: qca8k: serialize register access

This patch drops .disable_locking from the qca8k regmap config, so regmap
serializes use of its shared work_buf. It also swaps the order in which
qca8k_conduit_change() takes the MIB and management mutexes, so the order
matches the autocast path.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 07227ae8bc04..9a8b9e34a27c 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -579,7 +579,6 @@ static const struct regmap_config qca8k_regmap_config = {
>  	.write = qca8k_bulk_write,
>  	.reg_update_bits = qca8k_regmap_update_bits,
>  	.rd_table = &qca8k_readable_table,
> -	.disable_locking = true, /* Locking is handled by qca8k read/write */

[Severity: Medium]
The work_buf race described in the commit message is already in released
kernels. Should this patch carry a Fixes: tag and go to net, not ride
along as groundwork for the MIB stats change?

The race seems to come from commit c766e077d927 ("net: dsa: qca8k: convert
to regmap read/write API"). Before that commit, qca8k_regmap_config used
.reg_read/.reg_write. In that case __regmap_init() skips format
initialization and work_buf is never used, so .disable_locking was safe
then.

c766e077d927 switched to .read = qca8k_bulk_read and .write =
qca8k_bulk_write but kept .disable_locking. In __regmap_init(), this
branch:

    if (config->read && config->write) {
        map->reg_read  = _regmap_bus_read;

routes reads and writes through the formatted path. That path writes the
register (and value) into the shared map->work_buf and then passes it to
the driver callbacks.

Callers outside rtnl can already hit this without the MIB work this
series adds. One path is per-port phylink resolve work:

    phylink_resolve()
      qca8k_phylink_mac_link_up()
        qca8k_write(priv, QCA8K_REG_PORT_STATUS(port), reg)
          regmap_write()

Another is the LED class callbacks, such as regmap_read() in
qca8k_cled_hw_control_get(). Both can run concurrently and end up writing
the wrong switch register or returning the wrong value.

Would something like this be appropriate?

Fixes: c766e077d927 ("net: dsa: qca8k: convert to regmap read/write API")

That would let stable kernels since v6.3 pick up the regmap part.

>  	.cache_type = REGCACHE_NONE, /* Explicitly disable CACHE */
>  	.max_raw_read = 32, /* mgmt eth can read up to 8 registers at time */
>  	/* ATU regs suffer from a bug where some data are not correctly
> @@ -1782,13 +1781,16 @@ qca8k_conduit_change(struct dsa_switch *ds, const struct net_device *conduit,
>  	if (dp->index != 0)
>  		return;
>  
> -	mutex_lock(&priv->mgmt_eth_data.mutex);
> +	/* Same order as an autocast, which holds the MIB mutex while its
> +	 * register write takes the management one.
> +	 */
>  	mutex_lock(&priv->mib_eth_data.mutex);
> +	mutex_lock(&priv->mgmt_eth_data.mutex);

[Severity: High]
This is a pre-existing issue, not one introduced by this patch, but the
series builds on the autocast path, so it seems worth raising here.

qca8k_get_ethtool_stats_eth() publishes the caller's ethtool buffer and
then waits with a short timeout:

    mib_eth_data->req_port = dp->index;
    mib_eth_data->data = data;
    refcount_set(&mib_eth_data->port_parsed, QCA8K_NUM_PORTS);
    ...
    ret = wait_for_completion_timeout(&mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);

exit:
    mutex_unlock(&mib_eth_data->mutex);

QCA8K_ETHERNET_TIMEOUT is msecs_to_jiffies(5), which can be a single
jiffy. On timeout, mib_eth_data->data and ->req_port are left pointing at
the buffer. ethtool_get_stats() then vfree()s that buffer once
qca8k_get_ethtool_stats() returns.

qca8k_mib_autocast_handler() runs in conduit rx softirq context with no
lock and no check that a request is still pending:

    port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr));
    if (port != mib_eth_data->req_port)
        goto exit;
    ...
        mib_eth_data->data[i] = get_unaligned_le32(mib_ethhdr->data + i);

If the MIB frames arrive late, for example because conduit rx is delayed,
can the frame for req_port write into the vfree'd buffer from softirq?
ETHTOOL_GSTATS does not need CAP_NET_ADMIN, so the request side can be
triggered by an unprivileged user.

The exit path also runs for every frame:

exit:
    if (refcount_dec_and_test(&mib_eth_data->port_parsed))
        complete(&mib_eth_data->rw_done);

Can stale frames underflow port_parsed after it has already reached 0?
That would trigger a refcount WARN.

If a stale frame lands after the next request's refcount_set(), can it
also complete that request early with partly stale counters?

Separately, qca_tag_rcv() only does pskb_may_pull(skb, QCA_HDR_LEN) before
it dispatches to this handler. The handler then reads mib_count counters
from skb->data without checking skb->len. Can a short MIB-type frame cause
an out-of-bounds read here?

The later patch in this series, "net: dsa: qca8k: report port statistics
from the MIB", keeps the lockless handler. It adds a periodic autocast and
a QCA8K_MIB_ALL_PORTS mode that indexes mib_eth_data->data +
port * QCA8K_QCA833X_MIB_COUNT. Doesn't that make timed-out requests and
late frames more frequent?

>  
>  	priv->mgmt_conduit = operational ? (struct net_device *)conduit : NULL;
>  
> -	mutex_unlock(&priv->mib_eth_data.mutex);
>  	mutex_unlock(&priv->mgmt_eth_data.mutex);
> +	mutex_unlock(&priv->mib_eth_data.mutex);
>  }

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 5 (1) · Low: 1

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] With the new QCA8K_MIB_ALL_PORTS mode,
  qca8k_mib_autocast_handler() (qca8k-8xxx.c:1685-1691) uses…
- [High] qca8k_mib_autocast() leaves mib_eth_data->data and req_port set
  after wait_for_completion_timeout() times out, and nothing marks the…
- [Medium] qca8k_mib_delta() (drivers/net/dsa/qca/qca8k-8xxx.c:1869-1888)
  handles a backwards step differently by counter width.
- [Medium] Nothing resets or re-baselines mib_stats.last[] or stats[]
  when the hardware MIB is reset.
- [Medium] On the register fallback path, qca8k_mib_stats_poll() skips
  any port whose link_map bit is clear (qca8k-8xxx.c:1968-1969).
- [Medium] Arming and cancelling the work are not symmetric.
- [Medium] ms->raw[][] has no common lock between its writers and
  readers.
- [Low] qca8k_mib_bytes() (qca8k-8xxx.c:1891-1897) clamps each poll's
  FCS-corrected byte delta to zero.

Pre-existing issues:
- [Medium] Not introduced by this patch.

--

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

This adds a .get_stats64 implementation for qca8k that reports per-port
counters from the switch MIB. A delayed work polls the MIB every 3 s. It
uses one autocast request covering all ports when the Ethernet management
path is up, or register reads otherwise, and adds the deltas into a cached
rtnl_link_stats64. The autocast handler gains a QCA8K_MIB_ALL_PORTS mode,
and the register read loop is split out into qca8k_mib_read().

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 9a8b9e34a27c..c95df298988c 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -1450,6 +1450,7 @@ qca8k_phylink_mac_link_down(struct phylink_config *config, unsigned int mode,
>  	struct dsa_port *dp = dsa_phylink_to_port(config);
>  	struct qca8k_priv *priv = dp->ds->priv;
>  
> +	clear_bit(dp->index, &priv->mib_stats.link_map);
>  	qca8k_port_set_status(priv, dp->index, 0);
>  }

[Severity: Medium]
On the register fallback path, does clearing the bit here lose the last
poll interval of traffic for this port?

qca8k_mib_stats_poll() skips ports whose link_map bit is clear:

    if (!test_bit(port, &ms->link_map))
        continue;

Nothing reads the port's MIB one last time before the bit is cleared. Up
to 3 s of frames that the MAC counted before the link dropped stay in the
hardware MIB. They are missing from stats64 until the link comes back up.

The fallback is the only path when mgmt_conduit is NULL. That includes
every setup with the CPU on port 6, because qca8k_conduit_change() returns
early for dp->index != 0. The fallback is also used whenever the autocast
request times out.

[ ... ]

> @@ -1669,17 +1671,23 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
>  	const struct qca8k_mib_desc *mib;
>  	struct mib_ethhdr *mib_ethhdr;
>  	__le32 *data2;
> +	u64 *data;
>  	u8 port;
>  	int i;
>  
>  	mib_ethhdr = (struct mib_ethhdr *)skb_mac_header(skb);
>  	mib_eth_data = &priv->mib_eth_data;
>  
> -	/* The switch autocast every port. Ignore other packet and
> -	 * parse only the requested one.
> +	/* The switch autocast every port. Parse only the requested one, or
> +	 * every port into its own row for QCA8K_MIB_ALL_PORTS.
>  	 */
>  	port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr));
> -	if (port != mib_eth_data->req_port)
> +	if (mib_eth_data->req_port == QCA8K_MIB_ALL_PORTS &&
> +	    port < QCA8K_NUM_PORTS)
> +		data = mib_eth_data->data + port * QCA8K_QCA833X_MIB_COUNT;
> +	else if (port == mib_eth_data->req_port)
> +		data = mib_eth_data->data;
> +	else
>  		goto exit;

[Severity: High]
Can the row offset come from one request and the base pointer from
another?

The handler loads req_port and data separately, with no lock, barrier or
request generation check. qca8k_mib_autocast() publishes them with two
plain stores, under a mutex that the RX handler never takes:

    mib_eth_data->req_port = req_port;
    mib_eth_data->data = data;

After a timeout the pair is left in place, so late frames from the
previous request keep using it. The poll now sends ALL_PORTS requests
every 3 s outside rtnl, and these interleave with single-port
ETHTOOL_GSTATS requests. ETHTOOL_GSTATS does not need CAP_NET_ADMIN. A
late frame could then see:

  req_port == QCA8K_MIB_ALL_PORTS   (from the poll)
  data     == the ethtool buffer    (from the new request)

It would then write rows 1 through 6 past the 41 entries that
dsa_user_get_ethtool_stats() reserves at data + 4 in the vzalloc'd buffer
from ethtool_get_stats().

The reverse case also seems possible: req_port comes from a new ALL_PORTS
request while data still points at a timed-out ethtool buffer that
ethtool_get_stats() has already vfree'd. That would write seven rows into
freed memory from softirq.

Before this patch, req_port only decided whether a single row was written.
It never scaled the offset.

>  
>  	data2 = (__le32 *)skb->data;

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch.
qca_tag_rcv() only makes QCA_HDR_LEN bytes linear before it calls
mib_autocast_handler. The loop below then reads up to 164 bytes of
counters straight from skb->data. It never checks skb->len or calls
pskb_may_pull().

Could a truncated or non-linear MIB frame make this read past the packet
data? With this patch those values would also reach stats64.

MIB frames are generated by the switch itself, not by traffic on the
ports.

[ ... ]

> @@ -1708,12 +1716,9 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
>  		complete(&mib_eth_data->rw_done);
>  }
>  
> -static int
> -qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
> +static int qca8k_mib_autocast(struct qca8k_priv *priv, u8 req_port, u64 *data)
>  {

[ ... ]

> @@ -1722,7 +1727,7 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
>  
>  	reinit_completion(&mib_eth_data->rw_done);
>  
> -	mib_eth_data->req_port = dp->index;
> +	mib_eth_data->req_port = req_port;
>  	mib_eth_data->data = data;
>  	refcount_set(&mib_eth_data->port_parsed, QCA8K_NUM_PORTS);
>  

[Severity: High]
What happens to late frames from a request that timed out?

Later in this function:

    ret = wait_for_completion_timeout(&mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);

exit:
    mutex_unlock(&mib_eth_data->mutex);

On timeout, the function returns with data and req_port still set.
port_parsed is still shared with the next request.
qca8k_mib_autocast_handler() runs from qca_tag_rcv() without the mutex.

Before this patch, every request came from ETHTOOL_GSTATS under rtnl. The
new poll sends requests outside rtnl, so this sequence looks possible:

  1. The poll's ALL_PORTS request times out under conduit load, and its
     7 frames are still queued.
  2. An unprivileged ethtool -S starts a request for port N
     (req_port = N, data = buf, port_parsed = 7).
  3. The poll's stale frames bring port_parsed to 0 and complete the
     request early. The stale port N frame fills buf.
  4. qca8k_get_ethtool_stats() returns, and ethtool_get_stats() copies
     buf to user space and vfree()s it.
  5. The ethtool request's own port N frame arrives and writes 41 u64
     into the freed buffer. refcount_dec_and_test() also underflows.

Could this write into a vfree'd buffer from softirq? Would it help to
clear data under a lock on timeout, or to tag each request so that stale
frames are ignored?

[ ... ]

> @@ -1853,6 +1866,141 @@ static void qca8k_setup_hol_fixup(struct qca8k_priv *priv, int port)
>  			   mask);
>  }
>  
> +static u64 qca8k_mib_delta(struct qca8k_priv *priv, int port, int i)
> +{
> +	struct qca8k_mib_stats *ms = &priv->mib_stats;
> +	u64 now = ms->raw[port][i];
> +	u64 last = ms->last[port][i];
> +
> +	/* A 32-bit counter may wrap between two reads. A 64-bit one does
> +	 * not, and nothing flushes the MIB after setup: a sample below the
> +	 * previous one is a bad read, keep the previous one.
> +	 */
> +	if (ar8327_mib[i].size == 2 && now < last)
> +		return 0;
> +
> +	ms->last[port][i] = now;
> +
> +	if (ar8327_mib[i].size == 1)
> +		return (u32)(now - last);

[Severity: Medium]
Is it safe to treat every backwards 32-bit sample as a wrap?

A 32-bit sample below last[] returns (u32)(now - last), which is close to
2^32. That value is added permanently to rx_packets and tx_packets (seven
buckets each), to multicast and to the error counters.

A 64-bit sample that comes in too high is stored in last[]. Every correct
sample after it is then dropped until the hardware counter catches up.

A real wrap cannot happen twice in 3 s, but non-monotonic samples can come
from other sources.

On the register path, qca8k_mii_read32() reads each 32-bit MIB register
over MDIO as two 16-bit transfers, low then high. The hi/lo/hi handling in
qca8k_mib_read() suggests the halves are not latched.

On the autocast path, a late frame from a timed-out ALL_PORTS request can
write an older snapshot into raw[p] after the fallback qca8k_mib_read()
has already advanced last[p]. port_parsed is shared, so stale frames can
complete the next poll's request early. That older row is then used, and
every 32-bit counter of the port jumps by about 2^32.

Would it be more robust to treat implausibly large 32-bit deltas as bad
reads as well?

> +
> +	return now - last;
> +}
> +
> +/* The byte counters include the FCS, as the size buckets do */
> +static u64 qca8k_mib_bytes(u64 bytes, u64 packets)
> +{
> +	u64 fcs = packets * ETH_FCS_LEN;
> +
> +	/* bytes is 0 when its sample was dropped as a bad read */
> +	return bytes > fcs ? bytes - fcs : 0;

[Severity: Low]
Does clamping to 0 here lose the FCS correction for good?

The byte and packet baselines in last[] have both moved on already, so a
negative correction is not carried into the next poll.

On the register path, qca8k_mib_read() reads the size buckets before
RxGoodByte and TxByte. A frame that arrives between those reads can show
up as bytes=64, packets=0 in one poll and bytes=0, packets=1 in the next.
The total is 64 bytes where 60 is correct.

The dropped 64-bit sample path in qca8k_mib_delta() causes the same kind
of error. The packet baselines advance but the byte baseline does not, so
the bytes recovered later are corrected only for the latest interval's
packets.

> +}
> +

[ ... ]

> +static void qca8k_mib_stats_poll(struct work_struct *work)
> +{
> +	struct qca8k_priv *priv = container_of(work, struct qca8k_priv,
> +					       mib_stats.work.work);
> +	struct qca8k_mib_stats *ms = &priv->mib_stats;
> +	bool autocast = false;
> +	struct dsa_port *dp;
> +	int port;
> +
> +	/* One autocast request returns the MIB of every port */
> +	if (priv->mgmt_conduit)
> +		autocast = qca8k_mib_autocast(priv, QCA8K_MIB_ALL_PORTS,
> +					      &ms->raw[0][0]) > 0;
> +
> +	dsa_switch_for_each_user_port(dp, priv->ds) {
> +		port = dp->index;
> +
> +		if (!autocast) {
> +			/* Register reads are slow, skip ports without link.
> +			 * Their counters keep their value, so the next read
> +			 * still adds everything since the previous one.
> +			 */
> +			if (!test_bit(port, &ms->link_map))
> +				continue;
> +			if (qca8k_mib_read(priv, port, ms->raw[port]))
> +				continue;

[Severity: Medium]
Can late autocast frames and this fallback write ms->raw[] at the same
time?

When the ALL_PORTS request times out, mib_eth_data->data still points at
&ms->raw[0][0]. Late frames from that request keep writing rows from
softirq. Meanwhile qca8k_mib_read() fills the same rows and
qca8k_mib_stats_update() computes deltas from process context. No lock is
common to both sides.

On 32-bit SMP (for example IPQ806x with a QCA8337), the u64 load of
RxGoodByte or TxByte in qca8k_mib_delta() could tear between an older and
a newer value. If a carry into the high word happened in between, the
value is about 4 GiB too high. last[] takes it, every later 64-bit sample
is dropped as below last, and rx_bytes and tx_bytes freeze.

Older late rows can also trigger the 32-bit wrap issue in
qca8k_mib_delta() described above.

> +		}
> +
> +		qca8k_mib_stats_update(priv, port);
> +	}
> +
> +	schedule_delayed_work(&ms->work, QCA8K_MIB_STATS_INTERVAL);
> +}
> +

[ ... ]

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

[Severity: Medium]
Should teardown also clear mib_stats.enabled, or should qca8k_sw_remove()
cancel the work itself?

qca8k_resume() re-arms the work whenever enabled is set. It does not check
whether the switch is still set up. The tree can be torn down while qca8k
stays bound, for example when another member switch of the same tree is
unregistered. A later system resume then restarts the self-rescheduling
work.

qca8k_sw_remove() relies on dsa_unregister_switch() -> dsa_tree_teardown()
to reach ops->teardown. dsa_tree_teardown() returns early in that state:

    if (!dst->setup)
        return;

So nothing cancels the work, and devm frees priv while the work is still
queued. Could this be a use-after-free of priv in qca8k_mib_stats_poll()?

If the tree is set up again instead, qca8k_mib_init() flushes the MIB
while last[] keeps its old values. That leads to the reset issue below.

[ ... ]

> @@ -2193,6 +2354,9 @@ static int qca8k_resume(struct device *dev)
>  	struct qca8k_priv *priv = dev_get_drvdata(dev);
>  
>  	qca8k_set_pm(priv, 1);
> +	if (priv->mib_stats.enabled)
> +		schedule_delayed_work(&priv->mib_stats.work,
> +				      QCA8K_MIB_STATS_INTERVAL);
>  
>  	return dsa_switch_resume(priv->ds);
>  }

[Severity: Medium]
Does anything reset last[] when the hardware counters may have been
reset?

SIMPLE_DEV_PM_OPS() also uses qca8k_resume() as the hibernation .restore
callback. Before the image is restored, the boot kernel's qca8k_setup() ->
qca8k_mib_init() has flushed the MIB with QCA8K_MIB_FLUSH, or the switch
was power cycled. The restored image still has its old last[] and
stats[].

On the first poll, every 32-bit counter with now < last returns
(u32)(now - last) in qca8k_mib_delta(). That adds about 2^32 to
rx_packets, tx_packets, multicast and the error counters.

RxGoodByte and TxByte are also below last[]. They return 0 without
updating last[], so rx_bytes and tx_bytes stay frozen until the hardware
counters pass their pre-hibernation values.

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