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

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.

[ ... ]

+#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