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

["Jens Emil Schulz Østergaard" <jensemil.schulzostergaard@microchip.com>]

On Tue Nov 10, 2026 at 12:28 PM CET, Paolo Abeni 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.
> 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?
>

Correct. With IFH.BYPASS set the analyzer is skipped and the queue
system takes the egress queue from IFH.IPV, so every injected frame was
queued in queue 0. The next version sets IFH.IPV to the same clamped QoS
class as IFH.QOS_CLASS.

> [ ... ]
>
>> +	/* 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.
>

Dismissed, intentional.

The driver will not support 802.1ad bridges. The hardware classifier
treats both TPIDs as VLAN tags and the egress TPID follows the ingress
tag type, so following the bridge vlan_protocol in rcv would hide one
symptom without making 802.1ad work. On xmit the TPID is taken from the
bridge because the stack picks the TPID of the frames it sends.

The bridge offload patches will reject joining a VLAN-aware bridge and
enabling VLAN filtering when vlan_protocol is not 802.1Q. A later
vlan_protocol change on a bridge that already filters is not seen by the
driver, since DSA does not handle
SWITCHDEV_ATTR_ID_BRIDGE_VLAN_PROTOCOL. Rejecting it needs DSA to pass
the attribute to drivers, which is independent of this series.

>> +	}
>> +
>> +	return skb;
>> +}
>
> [ ... ]

pw-bot: cr