Re: [PATCH net-next 5/6] net: dsa: motorcomm: Add limited ACL flow statistics support

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] yt921x_acl_add() (drivers/net/dsa/motorcomm/tc.c:1355-1357) is
  meant to detect whether a rule already has a trap action.
- [Medium] yt921x_dsa_cls_flower_stats() (tc.c:1492) ignores its `port`
  argument.
- [Medium] The patch adds hardware flow statistics for flower rules but
  ignores the per-action hw_stats type the user requested.
- [Medium] yt921x_acl_stat() (tc.c:1177-1179) always passes `jiffies` as
  `lastused` to flow_stats_update(), even when the counter did not…
- [Low] Flow counter slots are released only in the software bitmap,
  without regard to hardware state.

--

net: dsa: motorcomm: Add limited ACL flow statistics support

This commit adds .cls_flower_stats() support to the yt921x driver using
the switch's 64 hardware flow counters. At add time, each flower ACL rule
gets a packet-mode counter, except software-assisted trap rules. The
counter is read back and reported as a delta through flow_stats_update().

> diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
> index d919743d89c17..05d4a5957af95 100644
> --- a/drivers/net/dsa/motorcomm/tc.c
> +++ b/drivers/net/dsa/motorcomm/tc.c

[ ... ]

> @@ -1138,6 +1141,45 @@ 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, struct flow_stats *stats)
> +{

[ ... ]

> +	diff = stat - aclrule->laststat;
> +	aclrule->laststat = stat;
> +	flow_stats_update(stats, aclrule->stat_pkt_mode ? 0 : diff,
> +			  !aclrule->stat_pkt_mode ? 0 : diff, 0,
> +			  jiffies, FLOW_ACTION_HW_STATS_IMMEDIATE);

[Severity: Medium]
Should lastused be set to jiffies even when diff is 0?

flow_stats_update() updates lastused whatever the packet and byte
deltas are:

include/net/flow_offload.h:flow_stats_update() {
    ...
	flow_stats->lastused	= max_t(u64, flow_stats->lastused, lastused);
    ...
}

tcf_gact_stats_update() then copies that into tm->lastuse. As a result,
every offloaded rule with a counter looks just used on every stats
poll, and tc -s filter show always reports it that way.

Userspace that ages out idle offloaded flows by tc lastuse, such as the
OVS tc-offload revalidator, would never see these flows as idle.
sja1105_vl.c has the same pattern. The "Align with sja1105" comment only
covers stat_pkt_mode.

Could jiffies be passed only when diff is non-zero?

> +	return 0;
> +}

[ ... ]

> @@ -1270,9 +1312,13 @@ yt921x_acl_del(struct yt921x_priv *priv, enum tc_setup_type type,
>  	}
>  
>  	if (aclrule->action[0] & YT921X_ACL_ACTa_METER_EN)
> -		clear_bit(FIELD_GET(YT921X_ACL_ACTa_METER_ID_M,
> -				    aclrule->action[0]),
> -			  priv->meters_map);
> +		__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);

[Severity: Low]
Can a statid be reused while the hardware still references it?

Earlier in yt921x_acl_del(), the commit result is saved, but a failure
does not stop the resources from being released:

	aclblk->rules[binid] = NULL;
	ret = yt921x_acl_commit(priv, entid, aclrule->mask);
	/* the kernel never rolls back on failure */

Suppose the first write in yt921x_acl_commit(),
yt921x_reg96_write(YT921X_ACLn_ACT(entid), zeros), fails on a bus error.
The old ACT word with FLOWSTAT_EN | FLOWSTAT_ID(statid), plus the
entry's key and mask, stay live in hardware.

The statid is still cleared here. The next yt921x_acl_add() can then
pick it with find_first_zero_bit(), zero the counter and set
laststat = 0. Hits on the orphaned entry would then show up in the new
rule's stats.

Also, YT921X_FLOWSTATn_CTRL(statid) is never cleared, either here or on
the yt921x_acl_add() error paths after the CTRL write. Those paths are
a STAT zeroing failure, a kvzalloc_obj() or kvmemdup() failure, and a
yt921x_acl_commit() failure. Is it intended to leave those counters
armed while flowstats_map treats them as free?

This needs a bus failure to trigger. The same pattern already exists
for meters and ACL slots.

>  	priv->acl_masks[blkid] &= ~aclrule->mask;
>  	kvfree(aclrule);

[ ... ]

> @@ -1305,6 +1352,10 @@ yt921x_acl_add(struct yt921x_priv *priv,
>  	if (entid == UINT_MAX)
>  		return -EOPNOTSUPP;
>  
> +	use_trap = (ruleext->r.action[2] & YT921X_ACL_ACTc_FWD_EN) &&
> +		   (FIELD_GET(YT921X_ACL_ACTc_FWD_M,
> +			      ruleext->r.action[2]) == YT921X_ACL_ACTc_FWD_TRAP);

[Severity: Medium]
Can this comparison ever be true? The definitions are:

drivers/net/dsa/motorcomm/tc.h:
#define  YT921X_ACL_ACTc_FWD_M			GENMASK(22, 21)
#define   YT921X_ACL_ACTc_FWD(x)			FIELD_PREP(YT921X_ACL_ACTc_FWD_M, (x))
...
#define   YT921X_ACL_ACTc_FWD_TRAP			YT921X_ACL_ACTc_FWD(3)

FIELD_GET() returns a value in 0..3, but YT921X_ACL_ACTc_FWD_TRAP is
the shifted value 0x600000.

That would leave use_trap false for explicit FLOW_ACTION_TRAP rules. It
would also be false for rules that fell back to trap in
yt921x_acl_rule_ext_parse_flow_action(). Only the meter exhaustion path
further down sets it.

The new check below then goes to the else branch for software-assisted
trap rules:

	if (ruleext->r.sw_assisted && use_trap) {
		statid = YT921X_FLOWSTAT_NUM;
	} else {
		statid = find_first_zero_bit(priv->flowstats_map,

So these rules get a hardware counter, which is the case the patch
meant to exclude. The software flower filter also counts the trapped
packets, so tc stats count them twice. The rules also use up counters
from the 64-entry pool.

Would comparing the masked value work instead?

	(ruleext->r.action[2] & YT921X_ACL_ACTc_FWD_M) == YT921X_ACL_ACTc_FWD_TRAP

> +
>  	if (!(ruleext->r.action[0] & YT921X_ACL_ACTa_METER_EN)) {
>  		meterid = YT921X_METER_NUM;
>  	} else {

[ ... ]

> @@ -1336,6 +1387,35 @@ yt921x_acl_add(struct yt921x_priv *priv,
>  			return res;
>  	}
>  
> +	if (ruleext->r.sw_assisted && use_trap) {
> +		statid = YT921X_FLOWSTAT_NUM;
> +	} else {
> +		statid = find_first_zero_bit(priv->flowstats_map,
> +					     YT921X_FLOWSTAT_NUM);
> +		if (statid >= YT921X_FLOWSTAT_NUM) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "No more flowstats, HW stats disabled");
> +		} else {

[Severity: Medium]
Should the per-action hw_stats type be checked here?

tc_setup_action() copies the requested type into each action entry:

	entry[k].hw_stats = tc_act_hw_stats(act->hw_stats);

Nothing under drivers/net/dsa/motorcomm/ reads act->hw_stats or calls
flow_action_hw_stats_check() or flow_action_basic_hw_stats_check().
Drivers such as ocelot, sparx5, lan966x, bnxt and mlx5 do. This has
three effects.

A rule added with "hw_stats disabled" still takes a counter from the
64-entry flowstats_map. There are 384 ACL entries, so such rules can use
up the pool and leave later rules that want stats with none.

A rule that explicitly asks for "hw_stats immediate" is still accepted
when the pool is empty. Only the extack message is set, and 0 is
returned. Later stats queries get -EOPNOTSUPP from yt921x_acl_stat(),
which the core ignores, so the user sees no stats and no error.

yt921x_acl_stat() always reports FLOW_ACTION_HW_STATS_IMMEDIATE, even
for "hw_stats delayed" requests.

The commit message says:

    As there is no interface for statistics preference for now, we pick
    one on our own initiative.

That covers byte vs packet mode. The enabled, disabled, immediate and
delayed preference does exist in the uAPI.

Could the driver reject unsupported hw_stats types and skip counter
allocation when stats are disabled?

> +			u32 zeros[2] = {};

[ ... ]

> @@ -1403,6 +1488,21 @@ yt921x_acl_add(struct yt921x_priv *priv,
>  	return res;
>  }
>  
> +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);

[Severity: Medium]
What happens when a shared block offloads the same filter on several
ports?

The port argument is dropped here. yt921x_acl_find() matches only on
tag and type and returns the first match:

	if (aclblk->rules[i] && aclblk->rules[i]->tag == tag &&
	    aclblk->rules[i]->type == type)
		return YT921X_ACL_ENT_PER_BLK * blkid + i;

For example:

    tc qdisc add dev swpA ingress_block 1 clsact
    tc qdisc add dev swpB ingress_block 1 clsact
    tc filter add block 1 flower ... skip_sw

yt921x_acl_rule_ext_parse_flow() creates one ACL rule per port, with
SPORTS = BIT(port) and tag == (unsigned long)f. Each rule has its own
flowstat counter. fl_hw_update_stats() -> tc_setup_cb_call() then calls
each port's callback with the same cookie.

Each of those calls finds the same first rule. The first call reports
its delta and updates laststat, and the later calls report about zero.
The other ports' counters are never read, so their hits never reach
tc -s filter show.

Would matching on the port avoid losing those hits? The port is already
encoded in the SPORTS field of entries[0].key[1].

> +	mutex_unlock(&priv->reg_lock);
> +
> +	return res;
> +}

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