Re: [net-next,v13,2/4] net: dsa: add tag format for MxL862xx switches

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: dsa: add tag format for MxL862xx switches

This patch adds a DSA tag driver for MaxLinear MxL862xx switches. The
tag format uses an 8-byte header with an ethertype marker that is shared
with GSW1xx switches, though the format itself differs.

> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -104,6 +104,13 @@

[ ... ]

> +config NET_DSA_TAG_MXL_862XX
> +	tristate "Tag driver for MxL862xx switches"
> +	help
> +	  Say Y or M if you want to enable support for tagging frames for the
> +	  Maxlinear MxL86252 and MxL86282 switches using their native 8-byte
          ^^^^^^^^^

This isn't a bug, but there's inconsistent capitalization here. The help
text uses "Maxlinear" while the adjacent NET_DSA_TAG_MXL_GSW1XX entry uses
"MaxLinear" (with capital L). The MODULE_DESCRIPTION in tag_mxl862xx.c also
uses "MaxLinear".

> +	  tagging protocol.

> diff --git a/net/dsa/tag_mxl862xx.c b/net/dsa/tag_mxl862xx.c
> --- /dev/null
> +++ b/net/dsa/tag_mxl862xx.c
> @@ -0,0 +1,110 @@

[ ... ]

> +static struct sk_buff *mxl862_tag_xmit(struct sk_buff *skb,
> +				       struct net_device *dev)
> +{
> +	struct dsa_port *dp = dsa_user_to_port(dev);
> +	struct dsa_port *cpu_dp = dp->cpu_dp;
> +	unsigned int cpu_port, sub_interface;
> +	__be16 *mxl862_tag;
> +
> +	cpu_port = cpu_dp->index;
> +
> +	/* target port sub-interface ID relative to the CPU port */
> +	sub_interface = dp->index + 16 - cpu_port;
                        ^^^^^^^^^^^^^^^^^^^^^^^^

Can sub_interface overflow the 5-bit MXL862_SUBIF_ID field here? With
dp->index = 16 and cpu_port = 0, the calculation would yield 32 which
exceeds the valid range (0-31) for GENMASK(4, 0). FIELD_PREP would silently
truncate this to 0 via masking. Is there a constraint on port configurations
that prevents this from occurring?

> +
> +	/* provide additional space 'MXL862_HEADER_LEN' bytes */
> +	skb_push(skb, MXL862_HEADER_LEN);

[ ... ]

> +	dsa_alloc_etype_header(skb, MXL862_HEADER_LEN);
> +
> +	/* special tag ingress */
                      ^^^^^^^

This comment says "ingress" but mxl862_tag_xmit() is the TX/egress path.
Should this say "egress" instead?

> +	mxl862_tag = dsa_etype_header_pos_tx(skb);
-- 
pw-bot: cr