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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Claimed: yt922x_tag_rcv() never calls
  dsa_default_offload_fwd_mark() or decodes a forward/trap code, so…
- [Low] net/dsa/tag_yt922x.c adds a tag driver whose file is not listed
  in any vendor MAINTAINERS entry.
- [Low] The comment block in net/dsa/tag_yt922x.c says 'To define the
  from cpu tag format 8 bytes:' and then describes nothing; the #defines…

--

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

This commit adds a DSA tag driver for Motorcomm YT922x switches. The 8
byte tag reuses the ETH_P_YT921X EtherType (0x9988). On transmit it
carries the destination port mask and priority, and on receive it
carries the source port.

> diff --git a/net/dsa/tag_yt922x.c b/net/dsa/tag_yt922x.c
> new file mode 100644
> index 000000000000..006a6bda90eb
> --- /dev/null
> +++ b/net/dsa/tag_yt922x.c
> @@ -0,0 +1,110 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Motorcomm YT922x Switch Extended CPU Port Tagging
> + *
> + * Copyright (c) 2026 Kyle switch <kyle.switch@xxxxxxxxxxxxxx>
> + *
> + */

[Severity: Low]
Should this new file be added to MAINTAINERS?

The Motorcomm switch entry lists drivers/net/dsa/motorcomm/ and the
YT921x tagger, but not net/dsa/tag_yt922x.c:

MAINTAINERS:
MOTORCOMM YT921X ETHERNET SWITCH DRIVER
...
F:	drivers/net/dsa/motorcomm/
F:	net/dsa/tag_yt921x.c

Right now only the generic net/dsa/ entry covers the file, so
get_maintainer.pl won't send patches for it to the Motorcomm
maintainers. The author of the new file isn't listed in that entry
either.

> +
> +#include <linux/etherdevice.h>
> +
> +#include "tag.h"
> +
> +#define YT922X_TAG_LEN	8
> +
> +/*
> + * To define the from cpu tag format 8 bytes:
> + */
> +#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)

[Severity: Low]
This isn't a bug, but the comment says "To define the from cpu tag
format 8 bytes:" and then no format description follows.

YT922X_TAG_RX_PORT_M is also under the from cpu heading, but only
yt922x_tag_rcv() uses it, to parse the to cpu direction.

Could this get a layout description like the one in tag_yt921x.c (the
DA/SA/TagET/Tag diagram plus a description of each field), covering
both directions?

With that, the TX encoding could be checked against the hardware
format: ports 1..8 in tag[1] bits 8:0, port 0 in tag[2] bit 15, and
the FORCE_DST, PRIO and PRIO_EN bits. The same goes for the RX port
field in tag[2] bits 5:2.

[ ... ]

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

[ ... ]

> +	/* Remove tag and update checksum */
> +	skb_pull_rcsum(skb, YT922X_TAG_LEN);
> +	dsa_strip_etype_header(skb, YT922X_TAG_LEN);
> +
> +	return skb;
> +}

[Severity: Medium]
yt922x_tag_rcv() never calls dsa_default_offload_fwd_mark(), and it
doesn't decode a forward or trap reason from the tag. The sibling
tagger does this for frames the hardware has already forwarded:

net/dsa/tag_yt921x.c:yt921x_tag_rcv() {
    ...
		case YT921X_TAG_CODE_FORWARD:
		case YT921X_TAG_CODE_PORT_COPY:
		case YT921X_TAG_CODE_FDB_COPY:
			/* Already forwarded by hardware */
			dsa_default_offload_fwd_mark(skb);
    ...
}

With the current series this doesn't seem to be reachable. The
yt922x_dsa_switch_ops added later in the series ("net: dsa: motorcomm:
Add support for Motorcomm YT922x") has no .port_bridge_join. That means
dsa_switch_bridge_join() returns -EOPNOTSUPP and bridging falls back to
software.

yt922x_port_setup() also limits each user port to the CPU port:

drivers/net/dsa/motorcomm/chip.c:yt922x_port_setup() {
    ...
	ctrl = ~priv->cpu_ports_mask;
	res = yt921x_reg_write(priv, YT922X_PORTn_ISOLATION(port), ctrl);
    ...
}

Learning is also disabled, and unknown unicast and multicast are
trapped to the CPU. So leaving offload_fwd_mark at 0 is correct for now.

If bridge offload is added for YT922x later, would the software bridge
forward frames the switch has already forwarded, and send duplicate
flooded frames on bridged ports? If the tag carries a YT922x forward
reason, would it make sense to decode it before bridge offload is
added?

skb->priority is also not set on receive, but that is optional and many
taggers don't set it.

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