Re: [PATCH net-next v13 2/4] net: dsa: tag_ks8995: Add the KS8995 tag handling

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] net/dsa/tag_ks8995.c declares KS8995_NEEDED_HEADROOM =
  KS8995_TAG_LEN + VLAN_HLEN = 8 (needed for TX, where…
- [Low] net/dsa/tag_ks8995.c:ks8995_rcv() lines 120-134 use the special
  tag's TCI as the only discriminator between a hardware-inserted…

--

net: dsa: tag_ks8995: Add the KS8995 tag handling

This adds a new DSA tagger for the Micrel KS8995 / Microchip KSZ8995XA
switches, which carry the switch tag in a modified 0x8100 ethertype
where the low nibble is a port mask. Egress inserts the special tag at
offset 12 after pushing any hardware-accelerated VLAN tag inside, and
ingress moves the in-band tag into VLAN metadata before resolving the
source port.

A couple of questions below.

> diff --git a/net/dsa/tag_ks8995.c b/net/dsa/tag_ks8995.c
> new file mode 100644
> index 0000000000000..435665013a864
> --- /dev/null
> +++ b/net/dsa/tag_ks8995.c
> @@ -0,0 +1,153 @@

[ ... ]

> +#define KS8995_NAME "ks8995"
> +
> +#define KS8995_TAG_LEN		VLAN_HLEN
> +/* Reserve room for the switch tag and a hardware-accelerated VLAN tag. */
> +#define KS8995_NEEDED_HEADROOM	(KS8995_TAG_LEN + VLAN_HLEN)

[Severity: Medium]
The transmit side can add two 4-byte tags, so reserving 8 bytes of
headroom makes sense there, but the in-band tag seen on receive is only
VLAN_HLEN.  Since ks8995_netdev_ops does not provide a .flow_dissect
callback, __skb_flow_dissect() falls back to the generic helper, which
takes needed_headroom as the tag length:

include/net/dsa.h:dsa_tag_generic_flow_dissect() {
	const struct dsa_device_ops *ops = skb->dev->dsa_ptr->tag_ops;
	int tag_len = ops->needed_headroom;

	*offset = tag_len;
	*proto = ((__be16 *)skb->data)[(tag_len / 2) - 1];
}

With tag_len == 8 that reads *proto from skb->data + 6 and advances
nhoff by 8, while the real EtherType of a single-tagged frame sits at
skb->data + 2.  For IPv4 the bogus proto is taken from the IP total
length field, which varies per packet, so skb_get_hash() and any tc
classifier on the conduit would see a different pseudo-protocol per
frame and RPS/RFS could steer one flow across CPUs.

Would it make sense to follow the pattern already used in
tag_sja1105.c:sja1110_flow_dissect() and tag_netc.c:netc_flow_dissect(),
which keep the larger needed_headroom but override the dissector with
*offset = VLAN_HLEN and *proto = ((__be16 *)skb->data)[(VLAN_HLEN / 2) - 1]?

[ ... ]

> +	/* The special tag's TCI is now in the hardware-accelerated VLAN
> +	 * metadata. The switch preserves the TCI of tagged
> +	 * frames, but inserts the ingress port's default tag for untagged frames.
> +	 * Since the driver programs the default tag to 0, a zero TCI identifies
> +	 * an originally untagged frame. Preserve every non-zero TCI as an
> +	 * 802.1Q tag, including VID 0 frames with PCP or DEI set.
> +	 */
> +	if (!skb->vlan_tci) {
> +		netdev_dbg(dev, "%s: clear VLAN tag from frame\n", __func__);
> +		__vlan_hwaccel_clear_tag(skb);
> +	} else {
> +		skb->vlan_proto = htons(ETH_P_8021Q);
> +		netdev_dbg(dev, "%s: vlan_tci = 0x%04x VLAN frame\n",
> +			   __func__, skb->vlan_tci);
> +	}

[Severity: Low]
Does this drop a legal 802.1Q tag whose TCI happens to be all zero?

A station sending a priority tag with VID 0, PCP 0 and DEI 0 produces
TCI == 0x0000 on the wire, and after skb_vlan_untag() moved the special
tag into the metadata that is identical to the switch-inserted default
tag, so ks8995_rcv() takes the __vlan_hwaccel_clear_tag() branch and the
frame reaches the stack untagged.  That would be visible to a VLAN 0
upper interface and to a capture on the user port.

To be clear about what was checked: the default-tag == 0 assumption in
the comment does hold at the end of the series.  The later commit
"net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA" writes
REG_PORT_CTRL_VID = 0 for every port in ksz8_port_setup():

	/* Use VID 0 to identify untagged frames in the special tag */
	ksz_pwrite16(dev, port, REG_PORT_CTRL_VID, 0);

ksz8995xa_switch_ops exposes no .port_vlan_filtering or .port_vlan_add,
so a bridge PVID cannot end up in that register, and the default port
priority lives in Port Control 0 (KSZ8995XA_PORT_BASED_PRIO in
ksz_dcb.c) rather than in the inserted TCI.  So the only remaining case
is the all-zero wire tag above.  Is that ambiguity worth a note in the
comment, given it is inherent to the tag format?

> +static const struct dsa_device_ops ks8995_netdev_ops = {
> +	.name = KS8995_NAME,
> +	.proto	= DSA_TAG_PROTO_KS8995,
> +	.xmit = ks8995_xmit,
> +	.rcv = ks8995_rcv,
> +	.needed_headroom = KS8995_NEEDED_HEADROOM,
> +};

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913-ks8995-to-ksz8-v13-0-327b16d309eb%40kernel.org