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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The tag-layout comment in net/dsa/tag_yt922x.c does not match
  what yt922x_tag_xmit() writes.
- [Low] Concern: yt922x_tag_rcv() never calls
  dsa_default_offload_fwd_mark(), so the software bridge might forward…

--

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

This adds a new DSA tag driver for Motorcomm YT922x switches. YT922x uses
the same ETH_P_YT921X EtherType and 8-byte tag length as YT921x, but the
tag layout is different. The patch also adds a co-maintainer to the
Motorcomm DSA MAINTAINERS entry.

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

[ ... ]

> +/*
> + * To define the from cpu tag format 8 bytes.
> + *
> + * To-CPU (RX) tag format, 8 bytes:
> + *
> + *   tag[0]: TPID (default: 0x9988)
> + *   tag[1]: VID lookup
> + *   tag[2]: bit 5:2  - RX source port
> + *
> + * From-CPU (TX) tag format, 8 bytes:
> + *
> + *   tag[0]: TPID (default: 0x9988)
> + *   tag[1]: bit 8:0 - Dst port mask
> + *   tag[2]: bit 15  - Dst port 0
> + *
> + */

[ ... ]

> +	tag[0] = htons(ETH_P_YT921X);
> +	ports = dsa_xmit_port_mask(skb, netdev);
> +	/*To fill in the case where the port index is not 0 */
> +	ctrl = YT922X_TAG_PRIO(skb->priority) | YT922X_TAG_FORCE_DST |
> +	       YT922X_TAG_PORTS(ports >> 1);
> +	tag[1] = htons(ctrl);

[Severity: Low]
This isn't a bug, but does the tag layout comment match what
yt922x_tag_xmit() writes? The commit message says:

    The new driver documents both the to-CPU and from-CPU tag layouts.

The from-CPU part of the comment describes tag[1] bits 8:0 as the
destination port mask. yt922x_tag_xmit() puts ports >> 1 into that field,
though, so bit 0 there is port 1, not port 0.

tag[1] also always carries YT922X_TAG_FORCE_DST (bit 9), YT922X_TAG_PRIO_M
(bits 12:10) and YT922X_TAG_PRIO_EN (bit 13), and tag[3] is written as 0.
The comment doesn't mention any of these.

Separately, the comment block opens with "To define the from cpu tag format
8 bytes." right above the To-CPU (RX) description. Is that line a leftover?

[ ... ]

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

[Severity: Low]
Should yt922x_tag_rcv() call dsa_default_offload_fwd_mark() before it
returns? tag_yt921x.c and most of the other tag drivers do.

As far as I can tell, leaving it out has no effect with the switch driver
later in the series. yt922x_dsa_switch_ops has no .port_bridge_join, so
dp->bridge stays NULL and the bridge falls back to software bridging. In
that case the helper would set the mark to 0 anyway:

net/dsa/tag.h:dsa_default_offload_fwd_mark() {
    ...
	skb->offload_fwd_mark = !!(dp->bridge);
}

The port isolation and trap-to-CPU setup in the switch driver also stop the
hardware from forwarding between user ports.

If bridge offload is added later, would the software bridge forward frames
again that the switch has already forwarded, unless the mark is set here?

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