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

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

[netdev-bot+sashiko]

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

[PATCH net-next 0/5] net: sparx5: add tc matchall port policer offload

[Daniel Machon]

This series adds offload of tc matchall filters with a police action to
the ingress port policers of Sparx5 and lan969x, which are both served
by the sparx5 driver.

In hardware, each ingress port has four single-rate leaky bucket
policers. A policer drops the frames that exceed its rate and burst.
The four policers of a port share the pass and drop statistics counters
of the port.

The driver uses one policer per port, because with more than one, the
statistics reported to tc could not be attributed to the right policer.
This matches lan966x and ocelot. The policer meters each frame
including 20 bytes of preamble and inter-frame gap, so the configured
rate is a line rate, also as on lan966x and ocelot.

Since the policer meters all ingress frames on the port, only filters on
chain 0 with protocol all are offloaded, and filters on shared blocks
are rejected. Only drop is supported as the exceed action, and the
police overhead parameter is not supported.

Patch #1 drops a redundant prefix from the existing POL_UPD_INT_CFG
register macros.

Patch #2 adds the registers used by the port policer.

Patch #3 tracks whether the ingress block of a port is shared, and adds
the per-port policer state.

Patch #4 adds the hardware programming of the port policer.

Patch #5 adds the tc matchall offload.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
Daniel Machon (5):
      net: sparx5: Drop redundant POL_ prefix from POL_UPD_INT_CFG
      net: sparx5: Add registers used by the port policer
      net: sparx5: Track shared tc blocks and per-port policer state
      net: sparx5: Add hardware programming for the port policer
      net: sparx5: Offload tc port policers via matchall

 .../microchip/sparx5/lan969x/lan969x_regs.c        |   7 +
 .../net/ethernet/microchip/sparx5/sparx5_ethtool.c |   1 +
 .../net/ethernet/microchip/sparx5/sparx5_main.c    |   6 +-
 .../net/ethernet/microchip/sparx5/sparx5_main.h    |  36 +-
 .../ethernet/microchip/sparx5/sparx5_main_regs.h   | 117 +++++-
 .../net/ethernet/microchip/sparx5/sparx5_police.c  | 404 ++++++++++++++++++++-
 drivers/net/ethernet/microchip/sparx5/sparx5_qos.c |   4 +
 .../net/ethernet/microchip/sparx5/sparx5_regs.c    |   7 +
 .../net/ethernet/microchip/sparx5/sparx5_regs.h    |   7 +
 drivers/net/ethernet/microchip/sparx5/sparx5_tc.c  |  14 +-
 .../ethernet/microchip/sparx5/sparx5_tc_matchall.c |  38 ++
 11 files changed, 627 insertions(+), 14 deletions(-)
---
base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
change-id: 20260930-b4-lan966x-pci-fdma-sparx5-lan969x-port-policers-474610756e26

Best regards,
-- 
Daniel Machon <daniel.machon@microchip.com>

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

[Daniel Machon]

Offload a tc matchall filter with a police action to the port policer:

  tc qdisc add dev <eth> clsact
  tc filter add dev <eth> ingress matchall skip_sw \
      action police rate <bps> burst <bytes> conform-exceed drop

The port policer meters all ingress frames on the port, so offload the
filter only on ingress, on chain 0, with protocol all, and on a block
that is not shared. Only drop is supported as the exceed action, and
only one policer per port. Report the pass and drop counts through the
matchall stats.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 .../ethernet/microchip/sparx5/sparx5_tc_matchall.c | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c b/drivers/net/ethernet/microchip/sparx5/sparx5_tc_matchall.c
index 146d7f28e8f3..5bf17c52e564 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;
+}
+
 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,
 		}
 		/* Get baseline stats for this port */
 		sparx5_mirror_stats(mall_entry, &tmo->stats);
+		break;
+	case FLOW_ACTION_POLICE:
+		if (tmo->common.protocol != htons(ETH_P_ALL)) {
+			NL_SET_ERR_MSG_MOD(tmo->common.extack,
+					   "Policer is only supported with protocol all");
+			kfree(mall_entry);
+			return -EOPNOTSUPP;
+		}
+
+		if (tmo->common.chain_index) {
+			NL_SET_ERR_MSG_MOD(tmo->common.extack,
+					   "Policer is only supported on chain 0");
+			kfree(mall_entry);
+			return -EOPNOTSUPP;
+		}
+
+		sparx5_tc_matchall_parse_port_policer_action(mall_entry,
+							     action);
+		err = sparx5_add_port_policer(mall_entry, tmo->common.extack);
+		if (err) {
+			kfree(mall_entry);
+			return err;
+		}
+
 		break;
 	case FLOW_ACTION_GOTO:
 		err = vcap_enable_lookups(sparx5->vcap_ctrl, ndev,
@@ -151,6 +182,11 @@ static int sparx5_tc_matchall_destroy(struct net_device *ndev,
 
 	if (entry->type == FLOW_ACTION_MIRRED) {
 		sparx5_mirror_del(entry);
+	} else if (entry->type == FLOW_ACTION_POLICE) {
+		err = sparx5_delete_port_policer(entry);
+		if (err)
+			NL_SET_ERR_MSG_MOD(tmo->common.extack,
+					   "Could not delete port policer");
 	} else if (entry->type == FLOW_ACTION_GOTO) {
 		err = vcap_enable_lookups(sparx5->vcap_ctrl, ndev,
 					  0, 0, tmo->cookie, false);
@@ -180,6 +216,8 @@ static int sparx5_tc_matchall_stats(struct net_device *ndev,
 
 	if (entry->type == FLOW_ACTION_MIRRED) {
 		sparx5_mirror_stats(entry, &tmo->stats);
+	} else if (entry->type == FLOW_ACTION_POLICE) {
+		sparx5_update_port_policer_stats(ndev, tmo);
 	} else {
 		NL_SET_ERR_MSG_MOD(tmo->common.extack, "Unsupported action");
 		return -EOPNOTSUPP;

-- 
2.34.1