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

[Daniel Golle <daniel@makrotopia.org>]

On Thu, Feb 05, 2026 at 06:21:34PM -0800, Jakub Kicinski wrote:
> 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".

+1 Will fix that.

> 
> > +	  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?

Yes. Only the two SerDes interfaces (port 9 and port 13) can be used as
CPU ports. Also, when using a USXGMII-M 4-port PHY, it is recommended to
connect the PHY to SerDes 1 (port 9 to 12) while using port 13 as CPU
port.

Addressing port 16 is problematic when using the switch with DSA also
because the 4-bit EGP/IGP field used in the receive function can only
present port 0 to 15 obviously. There is a way to do it by also using
the sub-interface ID on switch-to-host tagged frames, but it is worth
avoiding that due to the performance hit which comes with the additional
lookup and multiple pointer dereference steps required when doing so.

As there are only disadvantages when using SerDes 1 for a USXGMII-M
quad-PHY while using SerDes 0 as CPU port it is reasonable to expect
that board designers will opt for the convenient and also
vendor-recommended way and use SerDes 0 for the quad-PHY while using
SerDes 1 as CPU port.

Hence this theoretical limitation will play no role in practise.

> 
> > +
> > +	/* 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?

"ingress" is the perspective of the switch, and how the tag to be used
to send frames from the host to the switch is called in the datasheet[1]

[1]: https://www.mouser.com/datasheet/2/146/621181_MxL86282I_DS_Rev1_1-3675315.pdf
     page 95