Re: [PATCH net-next v9 2/4] net: dsa: tag_ks8995: Add the KS8995 tag handling
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-08 03:49:46
Also in:
netdev
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 2 - [Low] Claimed incomplete submission: the new tag protocol DSA_TAG_PROTO_KS8995 / CONFIG_NET_DSA_TAG_KS8995 /… - [Low] In ks8995_xmit() (net/dsa/tag_ks8995.c:52-54) the debug print passes the big-endian struct members hdr->h_vlan_proto and… -- net: dsa: tag_ks8995: Add the KS8995 tag handling This adds a new DSA tagger for the Micrel KS8995 / Microchip KSZ8995XA switch, which carries its special tag in a modified 802.1Q header whose last nybble holds the destination port mask. The tagger inserts the outer switch tag on xmit and moves the tag into the hwaccel VLAN area on receive. Two small observations below.
quoted hunk ↗ jump to hunk
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig index 23b4b74004ed5..4f44bf3ede238 100644 --- a/net/dsa/Kconfig +++ b/net/dsa/Kconfig@@ -125,6 +125,12 @@ config NET_DSA_TAG_MXL_GSW1XX Say Y or M if you want to enable support for tagging frames for MaxLinear GSW1xx switches. +config NET_DSA_TAG_KS8995 + tristate "Tag driver for Micrel KS8995 switch" + help + Say Y if you want to enable support for tagging frames for the + Micrel KS8995 switch. +
[Severity: Low] At this commit nothing in the tree selects NET_DSA_TAG_KS8995 and no driver returns DSA_TAG_PROTO_KS8995, so the module can be built but never bound. The existing drivers/net/dsa/ks8995.c still returns DSA_TAG_PROTO_NONE from ks8995_get_tag_protocol(). Looking further in the series this resolves itself: the following patch adds ksz8995xa_get_tag_protocol() in drivers/net/dsa/microchip/ksz8.c returning DSA_TAG_PROTO_KS8995 and makes drivers/net/dsa/microchip/Kconfig select NET_DSA_TAG_KS8995, and the last patch deletes the legacy drivers/net/dsa/ks8995.c. Each patch still compiles on its own. Would it be worth mentioning in the commit message that the switch driver side arrives in the next patch, so the tagger is intentionally without a consumer for one commit?
config NET_DSA_TAG_KSZ tristate "Tag driver for Microchip 8795/937x/9477/9893 families of switches" help
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/net/dsa/tag_ks8995.c b/net/dsa/tag_ks8995.c new file mode 100644 index 0000000000000..db90b3165a853 --- /dev/null +++ b/net/dsa/tag_ks8995.c@@ -0,0 +1,134 @@
[ ... ]
+ /* Always add a distinct outer tag. The user port removes this field on + * egress, so reusing an existing 802.1Q tag would consume that VLAN tag. + */ + skb = vlan_insert_tag(skb, KS8995M_STPID(portmask), 0); + /* vlan_insert_tag() drops the skb on failure */ + if (!skb) + return NULL; + hdr = skb_vlan_eth_hdr(skb); + netdev_dbg(dev, "%s: inserted VLAN TAG %04x TCI %04x\n", + __func__, hdr->h_vlan_proto, hdr->h_vlan_TCI);
[Severity: Low] This isn't a bug in the packet path, but should these two values go through ntohs() first? Both members are big-endian in struct vlan_ethhdr (include/linux/if_vlan.h): __be16 h_vlan_proto; __be16 h_vlan_TCI; On a little-endian host the tag just inserted (0x8101..0x810f) prints as 0x0181..0x0f81, which is not the value that goes on the wire, and sparse with C=1 will complain about a restricted __be16 degrading to integer. ks8995_rcv() in the same file converts before logging: etype = ntohs(*(__be16 *)dsa_etype_header_pos_rx(skb)); ... netdev_dbg(dev, "%s: received ethertype %04x\n", __func__, etype); so printing ntohs(hdr->h_vlan_proto) / ntohs(hdr->h_vlan_TCI) here would also make the two prints consistent.
+ + return skb; +}
[ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906-ks8995-to-ksz8-v9-0-8d8815a91bd8%40kernel.org