Re: [PATCH net-next 3/6] net: dsa: motorcomm: Enable ACL on demand

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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] In yt921x_acl_add() (drivers/net/dsa/motorcomm/tc.c), ACL
  lookup is switched on in hardware for every source port whose acl_cnt…

--

net: dsa: motorcomm: Enable ACL on demand

This patch adds a per-port acl_cnt field to struct yt921x_port to count
ACL rules. YT921X_ACL_PORT is now initialised to 0 in
yt921x_chip_setup_acl(). yt921x_acl_add() and yt921x_acl_del() then turn
the per-port enable bits on and off based on those counts.

> diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
> index 7717f1e179235..0f6d7a4675fcd 100644
> --- a/drivers/net/dsa/motorcomm/tc.c
> +++ b/drivers/net/dsa/motorcomm/tc.c

[ ... ]

> @@ -1274,6 +1309,18 @@ yt921x_acl_add(struct yt921x_priv *priv,
>  		}
>  	}
>  
> +	ctrl = 0;
> +	m = FIELD_GET(YT921X_ACL_KEYb_SPORTS_M, ruleext->r.entries[0].key[1]);
> +	for_each_set_bit(o, &m, YT921X_PORT_NUM)
> +		if (!priv->ports[o].acl_cnt)
> +			ctrl |= YT921X_ACL_PORT_PORTn(o);
> +	if (ctrl) {
> +		ctrl |= yt921x_acl_activated_ports(priv);
> +		res = yt921x_reg_write(priv, YT921X_ACL_PORT, ctrl);
> +		if (res)
> +			return res;
> +	}

[Severity: Medium]

If yt921x_acl_add() fails after this write, can the switch and the driver
end up disagreeing about YT921X_ACL_PORT?

The register is written before several steps that can still fail.
However, acl_cnt is only incremented after yt921x_acl_commit() succeeds.
None of the later failure paths write YT921X_ACL_PORT again:

	aclblk = kvzalloc_obj(*aclblk);
	if (!aclblk)
		return -ENOMEM;
	...
	aclrule = kvmemdup(...);
	if (!aclrule) {
		res = -ENOMEM;
		goto err;
	}
	...
	res = yt921x_acl_commit(priv, entid, aclrule->mask);
	if (res) {
		aclblk->rules[binid] = NULL;
		kvfree(aclrule);
		goto err;
	}

On any of these paths, the newly enabled port bits stay set in the switch.
Meanwhile, yt921x_acl_activated_ports() still reports those ports as
inactive.

Deleting a rule on the same port can't clear the stale bit, because the
failed rule was never recorded. The bit is only cleared if a later
yt921x_acl_del() drops some other port's acl_cnt to zero, sets
refresh_en, and rewrites the register from the counts.

A later patch in the series, "net: dsa: motorcomm: Add limited ACL flow
statistics support", adds two more direct-return failure points after
this write: the YT921X_FLOWSTATn_CTRL write and the YT921X_FLOWSTATn_STAT
reg64 write. That makes the window wider.

Would it work to write YT921X_ACL_PORT only after yt921x_acl_commit()
succeeds? Another option is to write yt921x_acl_activated_ports(priv)
back on each error path after this point. That value is still the one
from before the add, because acl_cnt hasn't changed yet.

>  	/* Prepare acl block ctrlblk */
>  	blkid = entid / YT921X_ACL_ENT_PER_BLK;
>  	binid = entid % YT921X_ACL_ENT_PER_BLK;

[ ... ]

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