[PATCH net-next] net: dsa: yt921x: Add limited ACL flow statistics support

[PATCH net-next] net: dsa: yt921x: Add limited ACL flow statistics support

[David Yang]

The yt921x supports flow statistics, which might be used to implement
.cls_flower_stats(). However, the number of flow counter are limited,
and you must choose between byte mode or packet mode. As there is no
interface for statistics preference for now, we pick one on our own
initiative.

Signed-off-by: David Yang <mmyangfl@gmail.com>
---
 drivers/net/dsa/yt921x.c | 87 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/yt921x.h | 13 ++++++
 2 files changed, 100 insertions(+)

diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
index 9929676a15e1..a553c966916a 100644
--- a/drivers/net/dsa/yt921x.c
+++ b/drivers/net/dsa/yt921x.c
@@ -373,6 +373,11 @@ yt921x_regs_clear_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
 	return yt921x_regs_write(priv, reg, vs, num_regs);
 }
 
+static int yt921x_reg64_read(struct yt921x_priv *priv, u32 reg, u32 *vals)
+{
+	return yt921x_regs_read(priv, reg, vals, 2);
+}
+
 static int
 yt921x_reg64_write(struct yt921x_priv *priv, u32 reg, const u32 *vals)
 {
@@ -2224,6 +2229,40 @@ yt921x_acl_reserve(struct yt921x_priv *priv, unsigned int entscnt,
 	return UINT_MAX;
 }
 
+static int
+yt921x_acl_stat(struct yt921x_priv *priv, enum tc_setup_type type,
+		unsigned long tag, u64 *statp)
+{
+	const struct yt921x_acl_rule *aclrule;
+	const struct yt921x_acl_blk *aclblk;
+	unsigned int statid;
+	unsigned int binid;
+	unsigned int blkid;
+	unsigned int entid;
+	u32 vals[2];
+	int res;
+
+	entid = yt921x_acl_find(priv, type, tag);
+	if (entid == UINT_MAX)
+		return -ENOENT;
+
+	blkid = entid / YT921X_ACL_ENT_PER_BLK;
+	binid = entid % YT921X_ACL_ENT_PER_BLK;
+	aclblk = priv->acl_blks[blkid];
+	aclrule = aclblk->rules[binid];
+
+	if (!(aclrule->action[0] & YT921X_ACL_ACTa_FLOWSTAT_EN))
+		return -EOPNOTSUPP;
+
+	statid = FIELD_GET(YT921X_ACL_ACTa_FLOWSTAT_ID_M, aclrule->action[0]);
+	res = yt921x_reg64_read(priv, YT921X_FLOWSTATn_STAT(statid), vals);
+	if (res)
+		return res;
+
+	*statp = ((u64)vals[1] << 32) | vals[0];
+	return 0;
+}
+
 static int
 yt921x_acl_commit(struct yt921x_priv *priv, unsigned int entid, u8 entsmask)
 {
@@ -2336,6 +2375,10 @@ yt921x_acl_del(struct yt921x_priv *priv, enum tc_setup_type type,
 		clear_bit(FIELD_GET(YT921X_ACL_ACTa_METER_ID_M,
 				    aclrule->action[0]),
 			  priv->meters_map);
+	if (aclrule->action[0] & YT921X_ACL_ACTa_FLOWSTAT_EN)
+		clear_bit(FIELD_GET(YT921X_ACL_ACTa_FLOWSTAT_ID_M,
+				    aclrule->action[0]),
+			  priv->flowstats_map);
 	priv->acl_masks[blkid] &= ~aclrule->mask;
 	kvfree(aclrule);
 	if (!priv->acl_masks[blkid]) {
@@ -2355,11 +2398,13 @@ yt921x_acl_add(struct yt921x_priv *priv,
 	struct yt921x_acl_blk *aclblk;
 	bool use_trap = false;
 	unsigned int meterid;
+	unsigned int statid;
 	unsigned long mask;
 	unsigned int binid;
 	unsigned int blkid;
 	unsigned int entid;
 	unsigned int o;
+	u32 ctrl;
 	int res;
 
 	/* Allocate resources */
@@ -2386,6 +2431,22 @@ yt921x_acl_add(struct yt921x_priv *priv,
 		}
 	}
 
+	statid = find_first_zero_bit(priv->flowstats_map, YT921X_FLOWSTAT_NUM);
+	if (statid < YT921X_FLOWSTAT_NUM) {
+		u32 zeros[2] = {};
+
+		ctrl = YT921X_FLOWSTAT_CTRL_EN | YT921X_FLOWSTAT_CTRL_TYPE_FLOW;
+		res = yt921x_reg_write(priv, YT921X_FLOWSTATn_CTRL(statid),
+				       ctrl);
+		if (res)
+			return res;
+
+		res = yt921x_reg64_write(priv, YT921X_FLOWSTATn_STAT(statid),
+					 zeros);
+		if (res)
+			return res;
+	}
+
 	/* Prepare acl block ctrlblk */
 	blkid = entid / YT921X_ACL_ENT_PER_BLK;
 	binid = entid % YT921X_ACL_ENT_PER_BLK;
@@ -2426,6 +2487,9 @@ yt921x_acl_add(struct yt921x_priv *priv,
 		aclrule->action[0] |= YT921X_ACL_ACTa_METER_ID(meterid);
 	else
 		aclrule->action[0] &= ~YT921X_ACL_ACTa_METER_EN;
+	if (statid < YT921X_FLOWSTAT_NUM)
+		aclrule->action[0] |= YT921X_ACL_ACTa_FLOWSTAT_EN |
+				      YT921X_ACL_ACTa_FLOWSTAT_ID(statid);
 
 	/* Write rules */
 	aclblk->rules[binid] = aclrule;
@@ -2438,6 +2502,8 @@ yt921x_acl_add(struct yt921x_priv *priv,
 
 	if (meterid < YT921X_METER_NUM)
 		set_bit(meterid, priv->meters_map);
+	if (statid < YT921X_FLOWSTAT_NUM)
+		set_bit(statid, priv->flowstats_map);
 	priv->acl_masks[blkid] |= aclrule->mask;
 	return 0;
 
@@ -2449,6 +2515,26 @@ yt921x_acl_add(struct yt921x_priv *priv,
 	return res;
 }
 
+static int
+yt921x_dsa_cls_flower_stats(struct dsa_switch *ds, int port,
+			    struct flow_cls_offload *cls, bool ingress)
+{
+	struct yt921x_priv *priv = to_yt921x_priv(ds);
+	int res;
+
+	mutex_lock(&priv->reg_lock);
+	res = yt921x_acl_stat(priv, TC_SETUP_CLSFLOWER, cls->cookie,
+			      &cls->stats.bytes);
+	mutex_unlock(&priv->reg_lock);
+
+	if (res)
+		return res;
+
+	cls->stats.used_hw_stats = FLOW_ACTION_HW_STATS_IMMEDIATE;
+	cls->stats.used_hw_stats_valid = true;
+	return 0;
+}
+
 static int
 yt921x_dsa_cls_flower_del(struct dsa_switch *ds, int port,
 			  struct flow_cls_offload *cls, bool ingress)
@@ -4817,6 +4903,7 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = {
 	.port_policer_add	= yt921x_dsa_port_policer_add,
 	.port_setup_tc		= yt921x_dsa_port_setup_tc,
 	/* acl */
+	.cls_flower_stats	= yt921x_dsa_cls_flower_stats,
 	.cls_flower_del		= yt921x_dsa_cls_flower_del,
 	.cls_flower_add		= yt921x_dsa_cls_flower_add,
 	/* hsr */
diff --git a/drivers/net/dsa/yt921x.h b/drivers/net/dsa/yt921x.h
index 555046526669..5952900f1794 100644
--- a/drivers/net/dsa/yt921x.h
+++ b/drivers/net/dsa/yt921x.h
@@ -759,6 +759,16 @@ enum yt921x_l4_type {
 #define  YT921X_METER_CTRLa_EIR_M		GENMASK(17, 0)
 #define   YT921X_METER_CTRLa_EIR(x)			FIELD_PREP(YT921X_METER_CTRLa_EIR_M, (x))
 #define YT921X_METERn_STAT(x)		(0x221000 + 8 * (x))
+#define YT921X_FLOWSTATn_STAT(x)	(0x221400 + 8 * (x))
+#define YT921X_FLOWSTATn_CTRL(x)	(0x221c00 + 4 * (x))
+#define  YT921X_FLOWSTAT_CTRL_EN		BIT(3)
+#define  YT921X_FLOWSTAT_CTRL_PKT_MODE		BIT(2)	/* 0: byte mode */
+#define  YT921X_FLOWSTAT_CTRL_TYPE_M		GENMASK(1, 0)
+#define   YT921X_FLOWSTAT_CTRL_TYPE(x)			FIELD_PREP(YT921X_FLOWSTAT_CTRL_TYPE_M, (x))
+#define   YT921X_FLOWSTAT_CTRL_TYPE_FLOW		YT921X_FLOWSTAT_CTRL_TYPE(0)
+#define   YT921X_FLOWSTAT_CTRL_TYPE_CPU_CODE		YT921X_FLOWSTAT_CTRL_TYPE(1)
+#define   YT921X_FLOWSTAT_CTRL_TYPE_DROP_CODE		YT921X_FLOWSTAT_CTRL_TYPE(2)
+#define   YT921X_FLOWSTAT_CTRL_TYPE_PORT		YT921X_FLOWSTAT_CTRL_TYPE(3)
 
 #define YT921X_PORTn_VLAN_CTRL(port)	(0x230010 + 4 * (port))
 #define  YT921X_PORT_VLAN_CTRL_SVLAN_PRIO_EN	BIT(31)
@@ -830,6 +840,8 @@ enum yt921x_fdb_entry_status {
 #define YT921X_SHAPE_CIR_MAX	((1 << 18) - 1)
 #define YT921X_SHAPE_CBS_MAX	((1 << 14) - 1)
 
+#define YT921X_FLOWSTAT_NUM	64
+
 #define YT921X_LAG_NUM		2
 #define YT921X_LAG_PORT_NUM	4
 
@@ -969,6 +981,7 @@ struct yt921x_priv {
 	u16 eee_ports_mask;
 
 	DECLARE_BITMAP(meters_map, YT921X_METER_NUM);
+	DECLARE_BITMAP(flowstats_map, YT921X_FLOWSTAT_NUM);
 
 	u8 acl_masks[YT921X_ACL_BLK_NUM];
 	struct yt921x_acl_blk *acl_blks[YT921X_ACL_BLK_NUM];
-- 
2.53.0

Re: [PATCH net-next] net: dsa: yt921x: Add limited ACL flow statistics support

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "I thought this is a false positive and the register is
Clear-on-Read but you seem to be writing 0 to it when allocating."
---
net: dsa: yt921x: Add limited ACL flow statistics support

This commit adds .cls_flower_stats() support to the yt921x DSA driver,
backed by a pool of 64 hardware flow counters configured in byte mode.
A counter is allocated at rule-install time and read on demand via
yt921x_acl_stat().

> diff --git a/drivers/net/dsa/yt921x.c b/drivers/net/dsa/yt921x.c
> index 159b16606f6c..7919afcb8136 100644
> --- a/drivers/net/dsa/yt921x.c
> +++ b/drivers/net/dsa/yt921x.c
> @@ -373,6 +373,11 @@ yt921x_regs_clear_bits(struct yt921x_priv *priv, u32 reg, const u32 *masks,
>  	return yt921x_regs_write(priv, reg, vs, num_regs);
>  }
>  
> +static int yt921x_reg64_read(struct yt921x_priv *priv, u32 reg, u32 *vals)
> +{
> +	return yt921x_regs_read(priv, reg, vals, 2);
> +}
> +

[Low]
yt921x_reg64_read() reads the 64-bit FLOWSTATn_STAT register as two
independent 32-bit transactions, one for each word, and combines them
in yt921x_acl_stat() with:

	*statp = ((u64)vals[1] << 32) | vals[0];

The hardware counter is live and can increment between the two reads.
If the low word wraps from ~0xffffffff to a small value while the high
word increments between the two transactions, the recombined value
becomes (new_high << 32) | old_low, producing a spurious ~4 GiB jump.

Does the chip provide a latch/snapshot register, or a documented
read-low/high/low retry protocol, that should be used here instead of
two independent reads?

>  static int
>  yt921x_reg64_write(struct yt921x_priv *priv, u32 reg, const u32 *vals)
>  {
> @@ -2224,6 +2229,40 @@ yt921x_acl_reserve(struct yt921x_priv *priv, unsigned int entscnt,
>  	return UINT_MAX;
>  }
>  
> +static int
> +yt921x_acl_stat(struct yt921x_priv *priv, enum tc_setup_type type,
> +		unsigned long tag, u64 *statp)
> +{
> +	const struct yt921x_acl_rule *aclrule;
> +	const struct yt921x_acl_blk *aclblk;
> +	unsigned int statid;
> +	unsigned int binid;
> +	unsigned int blkid;
> +	unsigned int entid;
> +	u32 vals[2];
> +	int res;
> +
> +	entid = yt921x_acl_find(priv, type, tag);
> +	if (entid == UINT_MAX)
> +		return -ENOENT;
> +
> +	blkid = entid / YT921X_ACL_ENT_PER_BLK;
> +	binid = entid % YT921X_ACL_ENT_PER_BLK;
> +	aclblk = priv->acl_blks[blkid];
> +	aclrule = aclblk->rules[binid];
> +
> +	if (!(aclrule->action[0] & YT921X_ACL_ACTa_FLOWSTAT_EN))
> +		return -EOPNOTSUPP;
> +
> +	statid = FIELD_GET(YT921X_ACL_ACTa_FLOWSTAT_ID_M, aclrule->action[0]);
> +	res = yt921x_reg64_read(priv, YT921X_FLOWSTATn_STAT(statid), vals);
> +	if (res)
> +		return res;
> +
> +	*statp = ((u64)vals[1] << 32) | vals[0];
> +	return 0;
> +}
> +

[High]
The absolute hardware counter is written directly into cls->stats.bytes,
and the flow_offload framework treats this value as a per-poll delta.
Following the call chain tcf_exts_hw_stats_update() ->
tcf_action_stats_update() -> a->ops->stats_update() -> _bstats_update(),
the framework performs:

	u64_stats_add(&bstats->bytes, bytes);

So if the hardware counter currently shows X bytes, every subsequent
poll of an idle counter adds another X to the kernel running total. After
N polls of an unchanged counter the user-visible "tc -s flower" value
will read N*X instead of X.

Two existing drivers handle this correctly:

  - drivers/net/dsa/sja1105/sja1105_vl.c:sja1105_vl_stats() caches a
    "last reported" value and reports (current - last).
  - drivers/net/ethernet/mscc/ocelot_vcap.c:ocelot_vcap_filter_stats_update()
    clears the hardware counter after each read.

Should yt921x_acl_stat() do something similar, either by tracking the
previously reported value per-rule, or by clearing FLOWSTATn_STAT after
the read?

[Medium]
cls->stats.pkts and cls->stats.lastused are never populated. The
hardware is hardwired to byte mode (YT921X_FLOWSTAT_CTRL_TYPE_FLOW with
PKT_MODE cleared), so no packet count is available, but the driver still
sets:

	cls->stats.used_hw_stats = FLOW_ACTION_HW_STATS_IMMEDIATE;
	cls->stats.used_hw_stats_valid = true;

Combined with the additive _bstats_update() semantics above, "tc -s
flower" will always show 0 packets even under traffic. Similarly
flow_stats->lastused stays 0, and any rule-aging logic that consults
lastused (it is updated via max_t(u64, ...)) will see the rule as never
used. For comparison, sja1105_vl_stats() passes
(jiffies - rule->vl.stats.lastused) as a delta.

Should the commit message and/or used_hw_stats_valid acknowledge that
pkts and lastused are not being reported?

>  static int
>  yt921x_acl_commit(struct yt921x_priv *priv, unsigned int entid, u8 entsmask)
>  {
> @@ -2336,6 +2375,10 @@ yt921x_acl_del(struct yt921x_priv *priv, enum tc_setup_type type,
>  		clear_bit(FIELD_GET(YT921X_ACL_ACTa_METER_ID_M,
>  				    aclrule->action[0]),
>  			  priv->meters_map);
> +	if (aclrule->action[0] & YT921X_ACL_ACTa_FLOWSTAT_EN)
> +		clear_bit(FIELD_GET(YT921X_ACL_ACTa_FLOWSTAT_ID_M,
> +				    aclrule->action[0]),
> +			  priv->flowstats_map);

[Low]
On rule delete the priv->flowstats_map bit is cleared, but
YT921X_FLOWSTATn_CTRL(statid) is not written to clear FLOWSTAT_CTRL_EN.
The counter remains physically enabled with TYPE_FLOW until something
else reprograms it.

Today this is benign because acl_commit() zeros the action register so
no rule references the counter, and reuse rewrites CTRL. However, the
header file enumerates other types (TYPE_CPU_CODE, TYPE_DROP_CODE,
TYPE_PORT) for the same slot pool. If a future caller picks one of
these types without first disabling the slot, could it observe stale
counts from the previous flow user?

>  	priv->acl_masks[blkid] &= ~aclrule->mask;
>  	kvfree(aclrule);
>  	if (!priv->acl_masks[blkid]) {
> @@ -2355,11 +2398,13 @@ yt921x_acl_add(struct yt921x_priv *priv,
>  	struct yt921x_acl_blk *aclblk;
>  	bool use_trap = false;
>  	unsigned int meterid;
> +	unsigned int statid;
>  	unsigned long mask;
>  	unsigned int binid;
>  	unsigned int blkid;
>  	unsigned int entid;
>  	unsigned int o;
> +	u32 ctrl;
>  	int res;
>  
>  	/* Allocate resources */
> @@ -2386,6 +2431,22 @@ yt921x_acl_add(struct yt921x_priv *priv,
>  		}
>  	}
>  
> +	statid = find_first_zero_bit(priv->flowstats_map, YT921X_FLOWSTAT_NUM);
> +	if (statid < YT921X_FLOWSTAT_NUM) {
> +		u32 zeros[2] = {};
> +
> +		ctrl = YT921X_FLOWSTAT_CTRL_EN | YT921X_FLOWSTAT_CTRL_TYPE_FLOW;
> +		res = yt921x_reg_write(priv, YT921X_FLOWSTATn_CTRL(statid),
> +				       ctrl);
> +		if (res)
> +			return res;
> +
> +		res = yt921x_reg64_write(priv, YT921X_FLOWSTATn_STAT(statid),
> +					 zeros);
> +		if (res)
> +			return res;
> +	}
> +

[Medium]
This block runs unconditionally for every ACL rule, regardless of
whether the user requested statistics. When all 64 counters are in use
(statid >= YT921X_FLOWSTAT_NUM), the rule is still installed but
silently without stats; the user only learns by querying and getting
-EOPNOTSUPP from yt921x_acl_stat().

There is no NL_SET_ERR_MSG_MOD() at install time and no kernel log
message. The same function explicitly emits

	NL_SET_ERR_MSG_MOD(extack, "No more meters available");

for the meter pool right above this hunk.

Should the flow-stat path either be gated on whether the rule actually
needs stats (so unrelated rules do not consume counters), or surface a
diagnostic via extack when the pool is exhausted? Otherwise an old set
of un-queried rules can starve newer rules that do need stats.

[Low]
After the YT921X_FLOWSTATn_CTRL write succeeds, the bare "return res;"
on a failing YT921X_FLOWSTATn_STAT write leaves the counter enabled in
hardware while priv->flowstats_map remains clear. Likewise, if any
later step (kvmemdup, yt921x_acl_commit) fails, the err: label does not
unwind the CTRL write or the meter configuration done in the prior
hunk.

The same asymmetric rollback pre-exists for meters and is implicitly
healed by the next allocation reprogramming the slot, but the new
flow-stat path's bare returns bypass the err: label entirely. Should
this teardown path be made symmetric?

>  	/* Prepare acl block ctrlblk */
>  	blkid = entid / YT921X_ACL_ENT_PER_BLK;
>  	binid = entid % YT921X_ACL_ENT_PER_BLK;
> @@ -2426,6 +2487,9 @@ yt921x_acl_add(struct yt921x_priv *priv,
>  		aclrule->action[0] |= YT921X_ACL_ACTa_METER_ID(meterid);
>  	else
>  		aclrule->action[0] &= ~YT921X_ACL_ACTa_METER_EN;
> +	if (statid < YT921X_FLOWSTAT_NUM)
> +		aclrule->action[0] |= YT921X_ACL_ACTa_FLOWSTAT_EN |
> +				      YT921X_ACL_ACTa_FLOWSTAT_ID(statid);
>  
>  	/* Write rules */
>  	aclblk->rules[binid] = aclrule;

[ ... ]
-- 
pw-bot: cr