Re: [PATCH net-next v7 5/6] net: dsa: tag_yt922x: add support for Motorcomm YT922x tags

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 4

- [Low] Claim: the new DSA tag protocol DSA_TAG_PROTO_YT922X /
  CONFIG_NET_DSA_TAG_YT922X has no consumer in the tree (dead code).
- [Low] The YT922x tag layout is effectively undocumented:
  net/dsa/tag_yt922x.c:15-17 contains only the placeholder comment "/*…
- [Low] The RX priority field declared by the new tag format is never
  decoded.
- [Low] The new tagger emits and validates the uAPI constant ETH_P_YT921X
  for a YT922x tag: net/dsa/tag_yt922x.c:40 (`tag[0] =…

--

net: dsa: tag_yt922x: add support for Motorcomm YT922x tags

This adds a new DSA tagging driver for Motorcomm YT922x switches with an
8-byte tag preceded by an EtherType of 0x9988, plus the new
DSA_TAG_PROTO_YT922X protocol value, Kconfig symbol and Makefile entry.

A few questions below.

> diff --git a/net/dsa/tag_yt922x.c b/net/dsa/tag_yt922x.c
> new file mode 100644
> index 0000000000000..1ee9d17735988
> --- /dev/null
> +++ b/net/dsa/tag_yt922x.c
> @@ -0,0 +1,111 @@

[ ... ]

> +#define YT922X_TAG_LEN	8
> +
> +/*
> + * To define the from cpu tag format 8 bytes:
> + */

[Severity: Low]
This isn't a bug, but could the tag layout be documented here?  The
comment above currently describes none of the fields, while the sibling
tagger for the same vendor, net/dsa/tag_yt921x.c, carries a full frame
diagram and a per-bit description of every 16-bit tag word:

net/dsa/tag_yt921x.c:
 * +----+----+-------+-----+----+---------
 * | DA | SA | TagET | Tag | ET | Payload ...
 * +----+----+-------+-----+----+---------
 *   6    6      2      6    2       N
 ...
 *   2:
 *     15b: Tx Port(s) Valid
 *     10b-0b: Tx Port(s) Mask

Without that, the reuse of tag[2] in opposite directions (transmit writes
YT922X_TAG_PORTMASK_0 = BIT(15), receive reads YT922X_TAG_RX_PORT_M =
GENMASK(5, 2)) and the split of the destination port mask across tag[1]
bits 8:0 and tag[2] bit 15 cannot be checked against the hardware by a
reviewer.

> +#define YT922X_TAG_NAME		"yt922x"
> +#define YT922X_TAG_PORTMASK_0	BIT(15)
> +#define YT922X_TAG_PORTMASK_M	GENMASK(8, 0)
> +#define  YT922X_TAG_PORTS(x)		FIELD_PREP(YT922X_TAG_PORTMASK_M, (x))
> +#define YT922X_TAG_FORCE_DST	BIT(9)
> +#define YT922X_TAG_PRIO_M	GENMASK(12, 10)
> +#define YT922X_TAG_PRIO_EN	BIT(13)
> +#define  YT922X_TAG_PRIO(x)		(FIELD_PREP(YT922X_TAG_PRIO_M, (x)) | YT922X_TAG_PRIO_EN)
> +#define YT922X_TAG_RX_PORT_M	GENMASK(5, 2)
> +#define YT922X_TAG_RX_PRIO_M	GENMASK(15, 13)
                                         ^^^^

[Severity: Low]
Is YT922X_TAG_RX_PRIO_M meant to be used?  It is defined here but never
referenced anywhere in the file, and yt922x_tag_rcv() only extracts the
source port from tag[2] and never assigns skb->priority, so the ingress
priority reported by the switch is dropped.

The transmit path does encode skb->priority into the tag:

	ctrl = YT922X_TAG_PRIO(skb->priority) | YT922X_TAG_FORCE_DST |
	       YT922X_TAG_PORTS(ports >> 1);

and comparable taggers decode it on receive, for example
net/dsa/tag_yt921x.c:yt921x_tag_rcv():

	skb->priority = FIELD_GET(YT921X_TAG_PRIO_M, rx);

Should yt922x_tag_rcv() do the same with YT922X_TAG_RX_PRIO_M?

> +
> +static struct sk_buff *
> +yt922x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
> +{

[ ... ]

> +	tag[0] = htons(ETH_P_YT921X);

[Severity: Low]
This isn't a bug, but the YT922x tagger emits and validates a uAPI
constant named for YT921x, both here and in yt922x_tag_rcv():

	if (unlikely(tag[0] != htons(ETH_P_YT921X))) {

include/uapi/linux/if_ether.h currently scopes that value to one family:

#define ETH_P_YT921X	0x9988		/* Motorcomm YT921x DSA [ NOT AN OFFICIALLY REGISTERED ID ] */

Should the header comment be broadened (something like "Motorcomm YT92xx
DSA") or a YT922x alias added, so the uAPI still describes all in-tree
users of 0x9988?  It would also help if the commit message stated that the
two chip families intentionally share the tag EtherType while using
different tag bodies.

> +	ports = dsa_xmit_port_mask(skb, netdev);
> +	/*To fill in the case where the port index is not 0 */

[ ... ]

> +static const struct dsa_device_ops yt922x_netdev_ops = {
> +	.name = YT922X_TAG_NAME,
> +	.proto = DSA_TAG_PROTO_YT922X,

[Severity: Low]
This isn't a bug, but could the commit message mention the companion
patch that consumes this protocol?

At this commit nothing requests DSA_TAG_PROTO_YT922X, and the consumer
only appears in the last patch of the series, "net: dsa: motorcomm: Add
support for Motorcomm YT922x", which adds
yt922x_dsa_get_tag_protocol() returning DSA_TAG_PROTO_YT922X and adds
select NET_DSA_TAG_YT922X to drivers/net/dsa/motorcomm/Kconfig.

So there is no dead code in the merged series, and the ordering is the
usual DSA practice; a note in the commit message pointing at the driver
patch would make that clear on its own.

> +	.xmit = yt922x_tag_xmit,
> +	.rcv = yt922x_tag_rcv,
> +	.needed_headroom = YT922X_TAG_LEN,
> +};

Also, in the commit message, "Add support for Motorcomm YT922x tags with
8bytes. which includes ethertype field (default to 0x9988)." reads a bit
oddly: "8bytes" is missing a space, and the sentence break before "which"
should probably be a comma.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914103009.3007019-1-kyle.switch%40motor-comm.com