Re: [PATCH net-next 5/5] net: sparx5: Offload tc port policers via matchall

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · 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] The driver refuses police offload on shared ingress blocks,
  but it does not refuse a police action that is shared between ports by…
- [Medium] The new police offload ignores the police action's MTU limit.
- [Medium] The offloaded matchall policer meters line rate, but tc police
  meters data rate.

--

net: sparx5: Offload tc port policers via matchall

This commit offloads a tc matchall filter with a police action to the
sparx5 per-port ingress policer. Offload is limited to ingress, chain 0,
protocol all, non-shared blocks, drop as the exceed action, and one
policer per port. Pass and drop counts are reported through the matchall
stats.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c
> index 146d7f28e8f3d..5bf17c52e564e 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c
> @@ -43,6 +43,13 @@ sparx5_tc_matchall_parse_mirror_action(struct sparx5_mall_entry *entry,
>  	entry->mirror.port = netdev_priv(action->dev);
>  }
>  
> +static void
> +sparx5_tc_matchall_parse_port_policer_action(struct sparx5_mall_entry *entry,
> +					     struct flow_action_entry *action)
> +{
> +	entry->port_policer.police = action->police;
> +}

[Severity: Medium]
Can a police action that is shared by index across ports end up
over-admitting traffic here?

sparx5_add_port_policer() refuses shared ingress blocks, because a shared
police instance has one aggregate rate:

drivers/net/ethernet/microchip/sparx5/sparx5_police.c:sparx5_add_port_policer() {
    ...
	if (entry->port->tc.ingress_block_shared) {
		NL_SET_ERR_MSG_MOD(extack,
				   "Policer is not supported on shared ingress blocks");
    ...
}

A police action shared by index has the same aggregate semantics. Take
this example:

  tc filter add dev eth0 ingress matchall skip_sw \
      action police index 5 rate 1mbit burst 10k conform-exceed drop
  tc filter add dev eth1 ingress matchall skip_sw action police index 5

In software this is a single token bucket covering both ports.
tc_setup_action() passes the shared index to drivers as
entry[k].hw_index = act->tcfa_index. However,
sparx5_tc_matchall_parse_port_policer_action() copies only
action->police, so the index is lost.

Each port then gets its own full-rate policer
(pol.rate = police->rate_bytes_ps * BITS_PER_BYTE). With skip_sw, the
hardware passes N times the configured aggregate rate across N ports.

Should action->hw_index be tracked, and a police action already offloaded
on another port be rejected? It looks like the lan966x and ocelot
matchall paths have the same gap.

>  static int sparx5_tc_matchall_replace(struct net_device *ndev,
>  				      struct tc_cls_matchall_offload *tmo,
>  				      bool ingress)
> @@ -98,6 +105,30 @@ static int sparx5_tc_matchall_replace(struct net_device *ndev,

[ ... ]

> +		sparx5_tc_matchall_parse_port_policer_action(mall_entry,
> +							     action);
> +		err = sparx5_add_port_policer(mall_entry, tmo->common.extack);

[Severity: Medium]
Is the police action's MTU limit dropped on this path?

tcf_police_offload_act_setup() exports the MTU as entry->police.mtu.
sparx5_add_port_policer() rejects peakrate, avrate, overhead and
rate_pkt_ps, but it never checks police->mtu. It programs only type,
rate, burst and idx.

In software, tcf_police_act() takes the exceed action whenever
tcf_police_mtu_check() fails, whatever the token state. When no mtu is
given, tcf_police_init() still sets a finite default:

net/sched/act_police.c:tcf_police_init() {
    ...
		if (R_tab)
			new->tcfp_mtu = 255 << R_tab->rate.cell_log;
    ...
}

With iproute2's default rate table this works out to 2040 bytes.
Software drops frames over that limit, whether it comes from an explicit
"mtu 512" or from jumbo frames under the default. Within rate, the
offloaded hardware policer would forward those same frames.

The flower path in the same driver honours this value
(sf.max_sdu = act->police.mtu in sparx5_tc_flower.c). Should the matchall
path reject, or otherwise handle, a police->mtu the port policer cannot
enforce?

[Severity: Medium]
Does the offloaded policer meter a different rate than tc police
configures?

sparx5_port_policer_conf_set() always programs a 20-byte per-frame gap:

drivers/net/ethernet/microchip/sparx5/sparx5_police.c:sparx5_port_policer_conf_set() {
    ...
	spx5_rmw(ANA_AC_POL_PORT_GAP_GAP_VALUE_SET(SPX5_POLICER_LINE_RATE_GAP),
		 ANA_AC_POL_PORT_GAP_GAP_VALUE,
		 sparx5, ANA_AC_POL_PORT_GAP(portno));
    ...
}

As a result, each frame is charged its length plus 20 bytes of IFG and
preamble. The software police action charges only qdisc_pkt_len(), plus
any user-set overhead.

sparx5_add_port_policer() also rejects a user-supplied police->overhead,
so the user cannot compensate in either direction. With 64-byte frames
the hardware charges 84 bytes per frame and passes only about 55-70% of
the configured rate.

The earlier commit "net: sparx5: Add hardware programming for the port
policer" says this line-rate metering is as on lan966x and ocelot. The
in-tree code seems to do the opposite:

drivers/net/ethernet/mscc/ocelot_police.c:ocelot_port_policer_add() {
    ...
	pp.mode = MSCC_QOS_RATE_MODE_DATA;
    ...
}

ocelot maps that mode to POL_MODE_DATARATE (excl IPG). lan966x_police_add()
uses ANA_POL_MODE_FRM_MODE_SET(1), which is also data rate in this
policer family.

Should the gap be 0 for tc matchall police offload, so the hardware
matches the data-rate semantics of tc police?

> +		if (err) {
> +			kfree(mall_entry);
> +			return err;
> +		}
> +
>  		break;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-b4-lan966x-pci-fdma-sparx5-lan969x-port-policers-v1-0-4916708a7ea2%40microchip.com