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

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
--- Patch [1]: [PATCH net-next v16 1/4] net: dsa: add tag driver for LAN9645X ---
commit 9f0c74e8611a75335b66615c753e5ac04e4a4bc9
Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>

net: dsa: add tag driver for LAN9645X

This adds a DSA tag driver for the Microchip LAN9645x switch. It uses a
front port as an NPI CPU port, with a long prefix plus a 28 byte IFH on
extraction and a bare IFH on injection. On receive, the classified VID
popped by the rewriter is restored as a hwaccel C-tag. On transmit, a tag
with a non-zero VID is moved from the payload into the IFH.

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

[ ... ]

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

[ ... ]

> +	qos_class = netdev_get_num_tc(ndev) ?
> +		    netdev_get_prio_tc_map(ndev, skb->priority) :
> +		    skb->priority;
> +	qos_class = min_t(u32, qos_class, GENMASK(IFH_QOS_CLASS_SZ - 1, 0));
> +
> +	/* Make room for IFH */
> +	ifh = skb_push(skb, LAN9645X_IFH_LEN_BYTES);
> +	memset(ifh, 0, LAN9645X_IFH_LEN_BYTES);
> +
> +	lan9645x_ifh_set(ifh, 1, IFH_BYPASS, IFH_BYPASS_SZ);
> +	lan9645x_ifh_set(ifh, tag_type, IFH_TAG_TYPE, IFH_TAG_TYPE_SZ);
> +	lan9645x_ifh_set(ifh, vlan_tci, IFH_TCI, IFH_TCI_SZ);
> +	lan9645x_ifh_set(ifh, qos_class, IFH_QOS_CLASS, IFH_QOS_CLASS_SZ);
> +	lan9645x_ifh_set(ifh, BIT(dp->index), IFH_DSTS, IFH_DSTS_SZ);

[Severity: Medium]
[Finding: main-7-6]
[Sources: opus-5-5]
Should IFH_IPV be set here too?

The field table at the top of the file lists IFH_IPV (pos 48, width 3)
under "Extraction/Injection header". Nothing on the xmit path writes it,
so after the memset() it is 0 for every injected frame.

The commit message says LAN9645x is in the same family as lan966x, and the
IFH layouts are close (IFH_TCI is at bit 103 in both). lan966x_ifh.h
describes QOS_CLASS as "Classified internal priority for queuing" and IPV
as "Classified internal priority for resourcemgt, tagging etc".
lan966x_port_xmit() sets both of them:

drivers/net/ethernet/microchip/lan966x/lan966x_main.c:lan966x_port_xmit() {
    ...
	lan966x_ifh_set_qos_class(ifh, skb->priority >= 7 ? 0x7 : skb->priority);
	lan966x_ifh_set_ipv(ifh, skb->priority >= 7 ? 0x7 : skb->priority);
    ...
}

The lan9645x switch driver in this series reserves buffers per
(port, prio) through QSYS_RES_CFG. If IPV stays at 0, would high priority
traffic injected from the CPU be queued by QOS_CLASS but charged against
the prio 0 reservation? Any tagging or mapping based on IPV would also
see priority 0.

Could IFH_IPV be set to the same clamped qos_class? If LAN9645x ignores
IPV on injection when BYPASS=1, could a comment say so?

[ ... ]

> +	/* While we have REW_PORT_CFG_NO_REWRITE=0 on the NPI port, we still
> +	 * disable port VLAN tag pushing with REW_TAG_CFG. A frame ingressing
> +	 * on a vlan aware port, which is forwarded to the CPU, will not carry
> +	 * vlan info in the frame data, because the tag is popped. The
> +	 * classified VID is only communicated via the IFH, never in the
> +	 * payload. We therefore restore it via hwaccel and must not pop an
> +	 * in-band tag here.
> +	 */
> +	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]
[Finding: main-1-1]
[Sources: opus-5-5]
Do lan9645x_xmit_get_vlan_info() and lan9645x_rcv() agree on which bridge
VLAN protocols are supported?

On xmit, the bridge TPID is read with br_vlan_get_proto(). An in-band tag
is stripped only when its TPID matches that protocol, and an S-type tag is
signalled for 802.1ad:

lan9645x_xmit_get_vlan_info() {
    ...
	br_vlan_get_proto(br, &proto);
    ...
		if (ntohs(hdr->h_vlan_proto) == proto &&
		    ntohs(hdr->h_vlan_TCI) & VLAN_VID_MASK) {
    ...
	*tag_type = (proto != ETH_P_8021Q) ? LAN9645X_IFH_TAG_TYPE_S :
					     LAN9645X_IFH_TAG_TYPE_C;
    ...
}

On rcv, IFH_TAG_TYPE is never read, and the tag always comes back with
vlan_proto 0x8100.

With a VLAN-aware bridge set to vlan_protocol 802.1ad, br->vlan_proto is
0x88a8. Wouldn't __allowed_ingress() in net/bridge/br_vlan.c then see a
protocol mismatch?

net/bridge/br_vlan.c:__allowed_ingress() {
    ...
	if (skb->vlan_proto != br->vlan_proto) {
		/* Protocol-mismatch, empty out vlan_tci for new tag */
    ...
}

In that case the bridge pushes the tag back into the payload and puts the
frame on the port pvid. The comment above says the C-tag choice avoids
exactly this, but it only does so when br->vlan_proto is 802.1Q.

The commit message says:

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

Is that enforced anywhere? DSA has no handler for
SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL. __br_vlan_set_proto() sends that
attribute with SWITCHDEV_F_SKIP_EOPNOTSUPP, so changing vlan_protocol to
802.1ad on an offloaded bridge still succeeds.

If 802.1ad really is unsupported, the TAG_TYPE_S branch in xmit looks dead.
Otherwise, could rcv take the TPID from IFH TAG_TYPE or from the bridge
protocol? tag_ocelot.c does this in ocelot_rcv():

	vlan_tpid = tag_type ? ETH_P_8021AD : ETH_P_8021Q;

At the end of this series the lan9645x switch driver does not implement
bridge join or program VLAN classification yet. So this path stays dormant
until bridge and VLAN offload is added, and the tagger is not changed later
in the series.

> +	}
> +
> +	return skb;
> +}

[ ... ]
-- 
This is an AI-generated review.