Re: [PATCH net-next v16 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 layout comment in net/dsa/tag_yt922x.c does not match what
  yt922x_tag_xmit() puts on the wire, even though the commit message…
- [Low] Concern: yt922x_tag_rcv() never calls
  dsa_default_offload_fwd_mark(), unlike yt921x_tag_rcv(), so the…

--

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

This commit adds a DSA tag driver for Motorcomm YT922x switches. They use
the same ETH_P_YT921X EtherType and 8-byte tag length as YT921x, but a
different tag layout. The commit also adds a co-maintainer and the new
file to the Motorcomm 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 @@

[ ... ]

> +#define YT922X_TAG_LEN 8
> +
> +/*
> + * 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
> + *
> + */

[Severity: Low]
This isn't a bug, but does this From-CPU layout match what
yt922x_tag_xmit() puts on the wire? The commit message says "The new
driver documents both the to-CPU and from-CPU tag layouts".

>From the comment, tag[1] bit 0 would be port 0. The code shifts the mask
by one, though:

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

So tag[1] bit n is port n+1, and port 0 goes in tag[2] bit 15.

On every frame, tag[1] also 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 zero. None of these are in the comment.

Could the comment describe the shifted mask and these extra fields?

Also, the block starts with "To define the from cpu tag format 8 bytes."
but then describes the To-CPU layout first.

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