Re: [PATCH net-next 3/4] net: dsa: initial MT7628 tagging driver

[Daniel Golle <daniel@makrotopia.org>]

On Thu, Mar 26, 2026 at 10:44:12PM +0200, Joris Vaisvila wrote:
> Add support for the MT7628 embedded switch's tag.
> [...]

> +++ b/net/dsa/tag_mt7628.c
> [...]
> +
> +#define MT7628_TAG_NAME "mt7628"
> +
> +#define MT7628_TAG_TX_PORT_BIT_MASK GENMASK(5, 0)
> +#define MT7628_TAG_RX_PORT_MASK GENMASK(2, 0)

'MASK' vs. 'BIT_MASK'... Maybe just

#define MT7628_TAG_TX_PORT	GENMASK(5, 0)
#define MT7628_TAG_RX_PORT	GENMASK(2, 0)


> +#define MT7628_TAG_LEN 4
> +
> +static struct sk_buff *mt7628_tag_xmit(struct sk_buff *skb,
> +				       struct net_device *dev)
> +{
> +	struct dsa_port *dp;
> +	u16 xmit_vlan;
> +	__be16 *tag;
> +
> +	dp = dsa_user_to_port(dev);
> +	xmit_vlan = dsa_tag_8021q_standalone_vid(dp);
> +
> +	skb_push(skb, MT7628_TAG_LEN);
> +	dsa_alloc_etype_header(skb, MT7628_TAG_LEN);
> +
> +	tag = dsa_etype_header_pos_tx(skb);
> +
> +	tag[0] = htons(ETH_P_8021Q |
> +		       FIELD_PREP(MT7628_TAG_TX_PORT_BIT_MASK,
> +				  dsa_xmit_port_mask(skb, dev)));
> +	tag[1] = htons(xmit_vlan);
> +
> +	return skb;
> +}
> +
> +static struct sk_buff *mt7628_tag_rcv(struct sk_buff *skb,
> +				      struct net_device *dev)
> +{
> +	int src_port;
> +	__be16 *phdr;
> +	u16 tpid;
> +
> +	if (unlikely(!pskb_may_pull(skb, MT7628_TAG_LEN)))
> +		return NULL;
> +
> +	phdr = dsa_etype_header_pos_rx(skb);
> +	tpid = ntohs(*phdr);

Why not directly

	src_port = FIELD_GET(MT7628_TAG_RX_PORT, ntohs(*phdr));

tpid isn't used for anything else (at this point).

Or even getting rid of both stack variables:

	skb->dev = dsa_conduit_find_user(dev, 0, FIELD_GET(MT7628_TAG_RX_PORT,
							   ntohs(*phdr)));

(though the compiler probably treats both forms equally or
almost equally)

> +	skb_pull_rcsum(skb, MT7628_TAG_LEN);
> +	dsa_strip_etype_header(skb, MT7628_TAG_LEN);
> +
> +	src_port = tpid & MT7628_TAG_RX_PORT_MASK;

As you are using FIELD_PREP above for something which also could as well
just be an & operation, I would say you should then also use FIELD_GET here,
just to maintain an consistent style. And potentially move up to get rid
of the stack variables, see above.

> +
> +	skb->dev = dsa_conduit_find_user(dev, 0, src_port);
> +	if (!skb->dev)
> +		return NULL;
> +	dsa_default_offload_fwd_mark(skb);
> +	return skb;
> +}