Re: [PATCH 17/20] net: dsa: xilinx: opt into TX forwarding offload on bridge join

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: xilinx: opt into TX forwarding offload on bridge join

The patch adds xlnx_tsn_port_bridge_join() which unconditionally sets
*tx_fwd_offload = true, so the bridge sends a single copy and relies on the
switch fabric for per-port replication.  It also sets ds->max_num_bridges
to 1 in xlnx_tsn_probe().

A few questions below.

> diff --git a/drivers/net/dsa/xilinx/xilinx_tsn.c b/drivers/net/dsa/xilinx/xilinx_tsn.c
> index e48c81f18d6e..9a4d4a2fb892 100644
> --- a/drivers/net/dsa/xilinx/xilinx_tsn.c
> +++ b/drivers/net/dsa/xilinx/xilinx_tsn.c
> @@ -495,6 +495,19 @@ static int xlnx_tsn_port_set_mac_address(struct dsa_switch *ds, int port,
>  	return 0;
>  }
>  
> +static int xlnx_tsn_port_bridge_join(struct dsa_switch *ds, int port,
> +				     struct dsa_bridge bridge,
> +				     bool *tx_fwd_offload,
> +				     struct netlink_ext_ack *extack)
> +{
> +	/* The switch fabric replicates flooded frames per egress port
> +	 * on its own, so the bridge does not need to clone-and-send.
> +	 */
> +	*tx_fwd_offload = true;

[Severity: High]
Does this drop PTP frames on all user ports but one?

The comment's premise does not seem to hold for the frame class the tagger
diverts.  In net/dsa/tag_xlnx_tsn.c, xlnx_tsn_xmit() takes untagged
ETH_P_1588 frames out of the fabric path entirely:

	if (eth_hdr(skb)->h_proto != htons(ETH_P_1588) ||
	    skb_vlan_tag_present(skb))
		return skb;

	tagger_data->ptp_tx(dp, skb);

	return NULL;

That handler, xlnx_tsn_ptp_tx() in
drivers/net/dsa/xilinx/xilinx_tsn_ptp.c, writes into the TX buffer of one
specific MAC:

	m = &sw->mac[dp->index];

so the frame goes into the per-port PTP TX FIFO of the single egress MAC
and never reaches the fabric that would replicate it.

Before this patch the bridge cloned a flooded frame once per member port,
so each clone landed in its own MAC's PTP FIFO.  With
BR_TX_FWD_OFFLOAD requested, net/bridge/br_switchdev.c suppresses the
remaining copies for the same hwdom:

	if (nbp_switchdev_can_offload_tx_fwd(p, skb))
		set_bit(p->hwdom, &BR_INPUT_SKB_CB(skb)->fwd_hwdoms);
	...
	return !test_bit(p->hwdom, &cb->fwd_hwdoms) &&
		(!skb->offload_fwd_mark || cb->src_hwdom != p->hwdom);

With br0 containing both swp1 and swp2, an untagged PTP frame flooded from
another bridge port then loads only one MAC's PTP FIFO and the other user
port transmits nothing, with no counter or log for the loss.  Should PTP
traffic be excluded from the offload, or should the tagger encode a flood
so the second copy is regenerated?

> +
> +	return 0;
> +}
> +

[ ... ]

> @@ -786,6 +800,9 @@ static int xlnx_tsn_probe(struct platform_device *pdev)
>  	ds->phylink_mac_ops = &xlnx_tsn_phylink_mac_ops;
>  	ds->priv = sw;
>  
> +	/* The fabric offloads a single bridge across the user ports. */
> +	ds->max_num_bridges = 1;

[Severity: Medium]
The commit message says:

  "Set ds->max_num_bridges to 1 so the DSA core rejects a second bridge
  instead of silently merging the ports."

Does the core actually reject it?  In net/dsa/port.c,
dsa_port_bridge_create() only returns -EOPNOTSUPP when the bridge number
allocation comes back empty:

	bridge->num = dsa_bridge_num_get(br, ds->max_num_bridges);
	if (ds->max_num_bridges && !bridge->num) {
		NL_SET_ERR_MSG_MOD(extack,
				   "Range of offloadable bridges exceeded");
		kfree(bridge);
		return -EOPNOTSUPP;
	}

and dsa_user_changeupper() in net/dsa/user.c turns that into success:

			if (err == -EOPNOTSUPP) {
				NL_SET_ERR_MSG_WEAK_MOD(extack,
							"Offloading not supported");
				err = 0;
			}

So "ip link set swp2 master br1" succeeds and the port joins br1
unoffloaded with dp->bridge rolled back to NULL.  Since the driver
programs no hardware forwarding domain (the new
xlnx_tsn_port_bridge_join() touches no register and ignores its bridge
argument), and dsa_port_enable_rt() puts a non-bridged port into hardware
forwarding:

	if (!dp->bridge)
		dsa_port_set_state_now(dp, BR_STATE_FORWARDING, false);

both MACs stay forwarding and the fabric keeps bridging them.  Does this
mean the two L2 domains are merged in hardware anyway, which is the case
the commit message claims is prevented?  If rejection is the intent,
would the driver need to track the offloaded bridge in port_bridge_join
and return something other than -EOPNOTSUPP (for example -EBUSY) with an
extack?

[Severity: Medium]
Is max_num_bridges = 1 workable given the bridge number allocator is
system-global rather than per-switch?  In net/dsa/dsa.c,
dsa_bridge_num_get() picks the lowest free bit of the file-scope
dsa_fwd_offloading_bridges bitmap and compares that global index against
this driver's max:

		bridge_num = find_next_zero_bit(&dsa_fwd_offloading_bridges,
						DSA_MAX_NUM_OFFLOADING_BRIDGES,
						1);
		if (bridge_num > max)
			return 0;

With max == 1 the only acceptable global index is 1.  If any other DSA
switch, or a second instance of this platform driver, already holds global
bridge number 1, then the very first bridge on this switch gets index 2,
2 > 1, and dsa_port_bridge_create() returns -EOPNOTSUPP, so
xlnx_tsn_port_bridge_join() is never called and *tx_fwd_offload is never
set.  dsa_user_changeupper() rewrites the error to 0, so the join looks
successful while the bridge silently resumes clone-and-send and the wire
sees duplicated frames again.

Other drivers size max_num_bridges from a hardware resource count (for
example DSA_TAG_8021Q_MAX_NUM_BRIDGES or ds->num_ports).  Would that be
more appropriate here?