Re: [PATCH net-next v2 3/4] net: dsa: Add tag handling for SoC-e switches

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] sdsa_rcv() (net/dsa/tag_sdsa.c, lines 86-87) unconditionally
  destroys every received frame whose SDSA header has the VLAN-info bit…
- [Low] Concern (INVALIDATED by series context): 'the new DSA tag
  protocol DSA_TAG_PROTO_SDSA / CONFIG_NET_DSA_TAG_SDSA has no in-tree…

--

net: dsa: Add tag handling for SoC-e switches

This adds a new DSA tagger for SoC-e switches, which insert an 8-byte SDSA
header with EtherType 0xdcdc between the source MAC address and the original
EtherType.  Both TO_CPU and FROM_CPU directions are handled, while the VLAN
metadata fields in the header are left clear on transmit and rejected on
receive.

> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index 23b4b74004ed5..0032ccd3badf6 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -188,6 +188,12 @@ config NET_DSA_TAG_RZN1_A5PSW
>  	  Renesas RZ/N1 embedded switch that uses an 8 byte tag located after
>  	  destination MAC address.
>  
> +config NET_DSA_TAG_SDSA
> +	tristate "Tag driver for SoC-e switches using EtherType SDSA headers"
> +	help
> +	  Say Y or M if you want to enable support for tagging frames for the
> +	  SoC-e switches.
> +

[Severity: Low]
This isn't a bug, just a note so it does not get raised again during review.
Taken on its own, this patch adds a tagger and a protocol id with no in-tree
user.  The next patch in the series, "net: dsa: soce: Add basic support for
SoC-e switch IP cores", adds drivers/net/dsa/soce/ whose Kconfig contains
select NET_DSA_TAG_SDSA and whose soce_get_tag_protocol() returns
DSA_TAG_PROTO_SDSA, so the tagger is reachable by the end of the series.

[ ... ]

> diff --git a/net/dsa/tag_sdsa.c b/net/dsa/tag_sdsa.c
> new file mode 100644
> index 0000000000000..83bccd65b1b1d
> --- /dev/null
> +++ b/net/dsa/tag_sdsa.c
> @@ -0,0 +1,123 @@

[ ... ]

> +/* SDSA tag byte layout (after the 12-byte MAC header):
> + * Bytes 0-1: SDSA EtherType (0xDCDC)
> + * Bytes 2-3: Reserved
> + * Byte 4:    Frame type (bits 7-6), VLAN-info bit (bit 5), port[9:5] (bits 4-0)
> + * Byte 5:    Port[4:0] (bits 7-3)
> + * Bytes 6-7: PCP (bits 7-5) / CFI (bit 4) / VID (bits 3-0 + byte 7), only
> + *            meaningful when the VLAN-info bit is set.
> + */

[ ... ]

> +static struct sk_buff *sdsa_xmit(struct sk_buff *skb, struct net_device *dev)
> +{

[ ... ]

> +	tag->port_lo = FIELD_PREP(SDSA_TAG_PORT_LO_MASK, dp->index);
> +	tag->vlan = 0;
> +
> +	return skb;
> +}
> +
> +static struct sk_buff *sdsa_rcv(struct sk_buff *skb, struct net_device *dev)
> +{

[ ... ]

> +	/* SDSA VLAN information is not supported. */
> +	if (tag->frame_type_port_hi & SDSA_TAG_VLAN_BIT)
> +		goto out_drop;

[Severity: Medium]
Can this drop every VLAN-tagged frame destined for the CPU?

The layout comment in this file states that bytes 6-7 carry PCP/CFI/VID and
are "only meaningful when the VLAN-info bit is set", so the hardware appears
to move the frame's VLAN identity out of the frame body and into the tag.

The SDSA layout looks like a close relative of Marvell (E)DSA: proprietary
EtherType, two reserved bytes, command in bits 7-6 of the first tag byte,
port in bits 7-3 of the second byte, and the VLAN indication in bit 5.  In
tag_dsa.c that bit means the frame arrived VLAN-tagged and the tagger
reconstructs the 802.1Q header rather than dropping the frame:

net/dsa/tag_dsa.c:dsa_rcv_ll() {
	...
	if (dsa_header[0] & 0x20) {
		u8 new_header[4];

		/* Insert 802.1Q ethertype and copy the VLAN-related
		 * fields, but clear the bit that will hold CFI (since
		 * DSA uses that bit location for another purpose).
		 */
		new_header[0] = (ETH_P_8021Q >> 8) & 0xff;
		new_header[1] = ETH_P_8021Q & 0xff;
		new_header[2] = dsa_header[2] & ~0x10;
		new_header[3] = dsa_header[3];
	...
}

If the SoC-e core sets bit 5 for ordinary tagged ingress in any supported
configuration, all such traffic towards the CPU is freed here.  The driver
added later in the series only programs SOCE_TAG_ALL_FRAMES_ENABLE and
SOCE_CUSTOM_RULES_TAGGING_ENABLE in soce_sw_enable_tagging() and implements
no VLAN operations at all, so nothing turns this hardware behaviour off.
Is it known that the core never sets that bit?

The drop itself is a bare kfree_skb() with no drop reason, counter, or
ratelimited message, so from userspace the resulting loss is hard to tell
apart from a link or MAC problem.  Would kfree_skb_reason() be worth using
for these paths?

On the transmit side, sdsa_xmit() writes tag->vlan = 0 with the VLAN-info
bit left clear while any 802.1Q header stays inline in the payload.  If the
switch expects the egress VLAN identity in the tag, as the Marvell-style
parts do in tag_dsa.c:

net/dsa/tag_dsa.c:dsa_xmit_ll() {
	...
	if (skb->protocol == htons(ETH_P_8021Q) &&
	    (!br_dev || br_vlan_enabled(br_dev))) {
		...
		dsa_header[0] = (cmd << 6) | 0x20 | tag_dev;
	...
}

would FROM_CPU frames then be injected as untagged/PVID-assigned with the
inline 802.1Q header treated as opaque payload?

The commit message does say the field could not be tested:

    SDSA VLAN metadata generation could not be tested on the available
    switch configuration yet. Leave these fields clear on transmit and
    reject receive headers which mark the metadata as valid.

Could the receive side pass such frames up (or at least count them) instead
of discarding them, so an untested field does not turn into silent packet
loss?

> +
> +	/* Determine the source port from the two port fields. */
> +	source_port = FIELD_GET(SDSA_TAG_PORT_HI_MASK,
> +				tag->frame_type_port_hi) <<
> +		      SDSA_TAG_PORT_HI_SHIFT;
> +	source_port |= FIELD_GET(SDSA_TAG_PORT_LO_MASK, tag->port_lo);
> +
> +	skb->dev = dsa_conduit_find_user(dev, 0, source_port);
> +	if (!skb->dev)
> +		goto out_drop;

[ ... ]

> +out_drop:
> +	kfree_skb(skb);
> +	return NULL;
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-devel-vstrassheim-soce-dsa-ml-v2-0-fb0587cb466b%40linutronix.de