Re: [PATCH net-next v3 4/4] net: dsa: motorcomm: Add TC prio support

[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] net: dsa: motorcomm: strict TC priority modulo check breaks
  auto-allocation

--

> diff --git a/drivers/net/dsa/motorcomm/tc.c b/drivers/net/dsa/motorcomm/tc.c
> --- a/drivers/net/dsa/motorcomm/tc.c
> +++ b/drivers/net/dsa/motorcomm/tc.c
> @@ -1042,6 +1045,36 @@ yt921x_acl_rule_ext_parse_flow(struct yt921x_acl_rule_ext *ruleext, int port,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	/* Lower TC priorities take precedence, while larger orders win in
> +	 * hardware. Spread the 512 hardware orders over the TC priority range
> +	 * as evenly as possible, retaining the relative order:
> +	 *
> +	 *   prio 65535 -> ORD 0    catchall filter
> +	 *   prio 65408 -> ORD 1    highest mappable TC priority
> +	 *   ...
> +	 *   prio 49152 -> ORD 128  default value for the first tc flower rule
> +	 *   ...
> +	 *   prio   128 -> ORD 511
> +	 *   prio     0             (invalid TC priority)
> +	 *
> +	 * The catchall filter always comes with priority 65535; pin it to ORD
> +	 * 0, the lowest hardware order, instead of rejecting it.
> +	 *
> +	 * Prefer explicit rejects over implicit behaviour changes: reject
> +	 * the priorities which do not map onto a hardware order instead of
> +	 * silently colliding with a neighbouring one.
> +	 */
> +	if (prio == U16_MAX) {
> +		ord = 0;
> +	} else if (!(prio % scale)) {
> +		ord = YT921X_ACL_ORD_NUM - prio / scale;
> +	} else {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "Invalid priority %u, must be a multiple of %u or 65535",
> +				       prio, scale);
> +		return -EOPNOTSUPP;
> +	}
> +
>  	res = yt921x_acl_rule_ext_parse_flow_action(ruleext, cls, priv, port);
>  	if (res)
>  		return res;

[Severity: Medium]
This rejects every priority that is not a multiple of 128, but the
kernel's own auto-allocation does not produce such values.

When a filter is added without an explicit pref, tcf_auto_prio() in
net/sched/cls_api.c gives the first filter 49152. Each later filter
gets the previous priority minus one, so the second gets 49151, the
third 49150, and so on.

With this patch, the first plain "tc filter add ... flower" on a port
is offloaded at ORD 128. Every following one added without pref then
fails here with -EOPNOTSUPP:

  - With skip_sw this is a hard failure for a very common usage
    pattern.
  - Without skip_sw the rule quietly stays software-only.

Before this patch the same sequence was offloaded, with every rule at
ORD 0. That makes this a behaviour regression for anyone who does not
hand-pick prefs in steps of 128.

Is a fixed linear prio-to-ORD mapping really what we want here? For
example, could yt921x_acl_rule_ext_parse_flow() assign hardware
orders from the relative priority of the rules currently installed?
Or could it at least handle the auto-allocated range below 49152
sensibly? I'd like to understand how you expect users to drive this
in practice.

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