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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] yt921x_acl_add() in drivers/net/dsa/motorcomm/tc.c writes
  YT921X_ACL_PORT before the rule is programmed.

--

net: dsa: motorcomm: Enable ACL on demand

This commit adds a per-port acl_cnt to struct yt921x_port. It now writes
YT921X_ACL_PORT only for ports that have ACL rules, instead of enabling
ACL on all ports at setup time. The register is updated from
yt921x_acl_add() and yt921x_acl_del() when a port's rule count goes from
zero to non-zero or back.

> diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
> index be3321df88683..a302016912b5f 100644
> --- a/drivers/net/dsa/motorcomm/tc.c
> +++ b/drivers/net/dsa/motorcomm/tc.c

[ ... ]

> @@ -1312,21 +1349,39 @@ yt921x_acl_add(struct yt921x_priv *priv,
>  	else
>  		aclrule->action[0] &= ~YT921X_ACL_ACTa_METER_EN;
>  
> +	/* Activate ACL if needed */
> +	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)
> +			goto err;
> +	}
> +
>  	/* Write rules */
>  	aclblk->rules[binid] = aclrule;
>  	res = yt921x_acl_commit(priv, entid, aclrule->mask);
>  	if (res) {
>  		aclblk->rules[binid] = NULL;
> -		kvfree(aclrule);
>  		goto err;
>  	}
>  
> +	/* Bookkeeping */
> +	m = FIELD_GET(YT921X_ACL_KEYb_SPORTS_M, ruleext->r.entries[0].key[1]);
> +	for_each_set_bit(o, &m, YT921X_PORT_NUM)
> +		priv->ports[o].acl_cnt++;
> +
>  	if (meterid < YT921X_METER_NUM)
>  		set_bit(meterid, priv->meters_map);
>  	priv->acl_masks[blkid] |= aclrule->mask;
>  	return 0;
>  
>  err:
> +	kvfree(aclrule);
>  	if (!priv->acl_masks[blkid]) {
>  		kvfree(aclblk);
>  		priv->acl_blks[blkid] = NULL;

[Severity: Low]
If yt921x_acl_commit() fails here, should YT921X_ACL_PORT be restored
before returning?

The enable bits for the new rule's source ports have already been written
by then. The err path frees aclrule (and aclblk if it is now empty), but
it doesn't write YT921X_ACL_PORT back to yt921x_acl_activated_ports(priv).

acl_cnt++ only runs on success, so the driver counts zero rules on those
ports while the hardware still has ACL lookup enabled on them.
yt921x_acl_find() can no longer find the rejected rule, so it can't be
deleted to clean this up.

yt921x_acl_commit() writes several registers in sequence (ACT, BLK_CMD
modify, KEY/MASK, BLK_KEEP, ACLn_ENTRY, BLK_CMD commit), and any of those
writes can fail.

yt921x_acl_del() can end up in the same state:

	if (refresh_en) {
		ctrl = yt921x_acl_activated_ports(priv);
		res = yt921x_reg_write(priv, YT921X_ACL_PORT, ctrl);
		if (res)
			ret = res;
	}

If this write fails after the last rule on a port took acl_cnt to 0, the
rule is still freed. The enable bit stays set, and a second delete
returns -ENOENT.

After either failure, YT921X_ACL_PORT stays wider than
yt921x_acl_activated_ports() until some later add or delete happens to
rewrite it. If that never happens, those ports keep the extra ACL lookup
latency that this patch is meant to remove.

These paths are also unchanged later in the series. "net: dsa:
motorcomm: Add TC prio support" doesn't touch them.

Would it make sense to rewrite YT921X_ACL_PORT from
yt921x_acl_activated_ports() on the add error path?

[ ... ]

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