Re: [PATCH net-next v17 1/4] net: dsa: add tag driver for LAN9645X

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The receive and transmit halves of the LAN9645x tagger
  disagree about which bridge VLAN protocol they support, and nothing…

--

net: dsa: add tag driver for LAN9645X

This adds a DSA tag driver for the Microchip LAN9645x switch, which uses a
front port as an NPI CPU port. Extraction uses a 16 byte long prefix plus a
28 byte IFH, and injection uses the IFH only. On receive, VLAN tags popped by
the hardware are restored as hwaccel tags; on transmit, VLAN tags are moved
into the IFH.

> diff --git a/net/dsa/tag_lan9645x.c b/net/dsa/tag_lan9645x.c
> new file mode 100644
> index 0000000000000..95caf4e0cc663
> --- /dev/null
> +++ b/net/dsa/tag_lan9645x.c
> @@ -0,0 +1,514 @@

[ ... ]

> +static struct sk_buff *lan9645x_xmit_get_vlan_info(struct sk_buff *skb,
> +						   struct net_device *br,
> +						   u32 *vlan_tci,
> +						   u32 *tag_type)
> +{

[ ... ]

> +	br_vlan_get_proto(br, &proto);
> +
> +	*vlan_tci = 0;
> +	if (skb->len >= VLAN_ETH_HLEN) {
> +		if (unlikely(!pskb_may_pull(skb, VLAN_ETH_HLEN))) {
> +			kfree_skb(skb);
> +			return NULL;
> +		}
> +
> +		hdr = skb_vlan_eth_hdr(skb);
> +		if (ntohs(hdr->h_vlan_proto) == proto &&
> +		    ntohs(hdr->h_vlan_TCI) & VLAN_VID_MASK) {
> +			vlan_remove_tag(skb, &tci);
> +			*vlan_tci = tci;
> +		}
> +	}
> +
> +	*tag_type = (proto != ETH_P_8021Q) ? LAN9645X_IFH_TAG_TYPE_S :
> +					     LAN9645X_IFH_TAG_TYPE_C;
> +
> +	return skb;
> +}

[ ... ]

> +static struct sk_buff *lan9645x_rcv(struct sk_buff *skb,
> +				    struct net_device *ndev)
> +{

[ ... ]

> +	if (ifh_gap_len) {
> +		/* The tag is restored as a C-tag, not as the TAG_TYPE the IFH
> +		 * reports. The classifier recognizes both TPIDs as VLAN tags,
> +		 * so an S-tag has already been used for classification by the
> +		 * time we get here. Restoring it as 802.1AD would make the
> +		 * bridge push it back into the payload and reclassify the frame
> +		 * to the port pvid, on a different VID than the one the
> +		 * hardware forwarded it on.
> +		 */
> +		__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vlan_tci);
> +	}

[Severity: Medium]
Do the transmit and receive paths agree on which bridge vlan_protocol is
supported?

The commit message says:

  This assumes a bridge vlan_protocol of 802.1Q, which is the only
  protocol the driver offloads.

lan9645x_xmit_get_vlan_info() does three things:

- It reads the bridge protocol with br_vlan_get_proto().
- It strips a payload tag whose TPID matches that protocol, and the protocol
  can be 0x88a8.
- It sets LAN9645X_IFH_TAG_TYPE_S when the protocol is not ETH_P_8021Q.

So the transmit path looks written for 802.1ad bridges. lan9645x_rcv(), on
the other hand, always restores the popped tag as ETH_P_8021Q.

Does anything enforce the 802.1Q-only assumption?

__br_vlan_set_proto() in net/bridge/br_vlan.c sends
SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL with SWITCHDEV_F_SKIP_EOPNOTSUPP and
ignores -EOPNOTSUPP:

	err = switchdev_port_attr_set(br->dev, &attr, extack);
	if (err && err != -EOPNOTSUPP)
		return err;

Nothing under net/dsa/ handles that attribute. So it looks like
"ip link set br0 type bridge vlan_protocol 802.1ad" cannot be vetoed, even
after lan9645x ports have joined the bridge.

Here is what would happen on such a bridge:

- Every popped S-tag reaches the bridge as a 0x8100 hwaccel tag.
- br_allowed_ingress() sees that skb->vlan_proto != br->vlan_proto.
- It pushes the tag back into the payload as a C-tag that was never on the
  wire.
- It classifies the frame to the port pvid, not the VID the hardware used.

Meanwhile the transmit path treats S-tags as bridge VLAN tags.

At this point in the series the switch driver has no .port_bridge_join and
does not program VLAN popping. So dp->bridge is NULL and no tags get popped.
The problem would only become reachable once VLAN-aware bridge offload is
added.

Would it make sense to use one model in both directions? One option is to
drop the LAN9645X_IFH_TAG_TYPE_S handling on xmit and reject 802.1ad. The
other is to handle the TPID the same way on transmit and receive.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-dsa_lan9645x_switch_driver_base-v17-0-83a471c5bddb%40microchip.com