Re: [PATCH net-next v9 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: 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.

> 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

[ ... ]

> 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