Re: [PATCH net-next v3 11/12] net: dsa: add tagging driver for MaxLinear GSW1xx switch family

[Daniel Golle <daniel@makrotopia.org>]

On Tue, Oct 28, 2025 at 02:28:41AM +0200, Vladimir Oltean wrote:
> On Sun, Oct 26, 2025 at 11:48:23PM +0000, Daniel Golle wrote:
> > Add support for a new DSA tagging protocol driver for the MaxLinear
> > GSW1xx switch family. The GSW1xx switches use a proprietary 8-byte
> > special tag inserted between the source MAC address and the EtherType
> > field to indicate the source and destination ports for frames
> > traversing the CPU port.
> > 
> > Implement the tag handling logic to insert the special tag on transmit
> > and parse it on receive.
> > [...]
> > --- /dev/null
> > +++ b/net/dsa/tag_mxl-gsw1xx.c
> > [...]
> > +#define GSW1XX_TX_CLASS_SHIFT		0
> > +#define GSW1XX_TX_CLASS_MASK		GENMASK(3, 0)
> 
> Using FIELD_PREP() would eliminate these _SHIFT definitions and _MASK
> would also go away from the macro names.

Ack, using FIELD_PREP() and FIELD_GET() does improve readability and
I'll use that.

> 
> > +
> > +/* Byte 3 */
> > +#define GSW1XX_TX_PORT_MAP_LOW_SHIFT	0
> > +#define GSW1XX_TX_PORT_MAP_LOW_MASK	GENMASK(7, 0)
> > +
> > +/* Byte 4 */
> > +#define GSW1XX_TX_PORT_MAP_HIGH_SHIFT	0
> > +#define GSW1XX_TX_PORT_MAP_HIGH_MASK	GENMASK(7, 0)
> > +
> > +#define GSW1XX_RX_HEADER_LEN		8
> 
> Usually you use two separate macros when the lengths are not equal, and
> you set .needed_headroom to the largest value.

A single macro
#define GSW1XX_HEADER_LEN			8
will do the trick as they are anyway equal, right?

> > [...]
> > +	u8 *gsw1xx_tag;
> > +
> > +	/* provide additional space 'GSW1XX_TX_HEADER_LEN' bytes */
> > +	skb_push(skb, GSW1XX_TX_HEADER_LEN);
> > +
> > +	/* add space between MAC address and Ethertype */
> > +	dsa_alloc_etype_header(skb, GSW1XX_TX_HEADER_LEN);
> > +
> > +	/* special tag ingress */
> > +	gsw1xx_tag = dsa_etype_header_pos_tx(skb);
> > +	gsw1xx_tag[0] = 0x88;
> > +	gsw1xx_tag[1] = 0xc3;
> 
> Could you write this as a u16 pointer, to make it obvious to everyone
> it's an EtherType, and define the EtherType constant in
> include/uapi/linux/if_ether.h, to make it a bit more visible that it's
> in use?

Defining the EtherType in the appropriate header makes sense (even though
0x88c3 is just the default and configuration of the chip allows to set it
to anything else, or even have it omitted entirely).

Using __be16 to access the tag fields will make the whole thing
sensitive to endianess, which is a bit messy. I would prefer to keep
using u8 type and some shifting and masking of the EtherType constant
similar to how it is done in tag_dsa.c. Also note that the datasheet
describes the special tag byte-by-byte, and there is even a 16-bit field
which crosses word boundaries, GSW1XX_TX_PORT_MAP_LOW and
GSW1XX_TX_PORT_MAP_HIGH (ie. it is obvious that this wasn't intended to
be accessed as 16-bit words). So I'd rather make it easy to understand
how the tag driver matches the datasheet instead of using __be16 just
for the sake of the EtherType.

I've implemented and tested using __be16 now, and it doesn't look very
bad either, especially when skipping the PORT_MAP_HIGH/LOW part because
on the actually produced chips there anyway aren't ever more than 6
ports, so one anyway always only accesses the LOW part of the portmap.

If you like to use __be16 (like eg. the realtek taggers) I will proceed
like that in v4.

> > [...] 
> > +	if (gsw1xx_tag[0] != 0x88 && gsw1xx_tag[1] != 0xc3) {
> > +		dev_warn_ratelimited(&dev->dev, "Dropping packet due to invalid special tag\n");
> > +		dev_warn_ratelimited(&dev->dev,
> > +				     "Tag: 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x, 0x%x\n",
> > +				     gsw1xx_tag[0], gsw1xx_tag[1], gsw1xx_tag[2], gsw1xx_tag[3],
> > +				     gsw1xx_tag[4], gsw1xx_tag[5], gsw1xx_tag[6], gsw1xx_tag[7]);
> 
> I think you could print the tag with %*ph, according to
> https://elixir.bootlin.com/linux/v6.17.5/source/lib/vsprintf.c#L2453
> (needs testing)

I've tested that and it works fine (looks slightly different of course due
to the missing '0x' prefix, but that doesn't matter for debugging)

> > [...]
> > +	/* remove the GSW1xx special tag between MAC addresses and the current
> > +	 * ethertype field.
> > +	 */
> > +	skb_pull_rcsum(skb, GSW1XX_RX_HEADER_LEN);
> > +	dsa_strip_etype_header(skb, GSW1XX_RX_HEADER_LEN);
> 
> You're not setting skb->offload_fwd_mark but you implement
> port_bridge_join() so you offload L2 switching. If a packet gets flooded
> from port A to the CPU and also to port B, don't you see that the
> software bridge also creates a packet copy that it sends to port B a
> second time?

No, the opposite is true. If I set
dsa_default_offload_fwd_mark(skb);
forwarding between the ports no longer works.
It can well be that this is an existing flaw in the driver, as tag_gswip.c
also doesn't set offload_fwd_mark.