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

On Tue, 2026-03-03 at 18:11 +0200, Vladimir Oltean wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Tue, Mar 03, 2026 at 01:22:27PM +0100, Jens Emil Schulz Østergaard wrote:

quoted

Use long prefix on extraction (RX) and no prefix on injection (TX). A
long prefix on extraction helps get through the conduit port on host
side, since it will see a broadcast MAC.

(...)

quoted

The format can be configured asymmetrically on RX and TX.

Do you foresee a need to configure the prefix length? It would be
possible to do that by changing the tagging protocol. But it implies
that the "lan9645x" string as found in /sys/class/net/.../dsa/tagging
becomes user ABI that is set in stone. It will mean long extraction
prefix and no injection prefix. Otherwise user space will get very
confused (libpcap, XDP, whatever else might get written).

That is a good point, so the prefix choice could be baked into the name like
lan9645x-long? I do not forsee a need to configure the prefix. I wanted to
mention it because it is a difference with the ocelot driver.

quoted

+static inline u32 lan9645x_ifh_get(const u8 *ifh, size_t pos, size_t length)
+{
+     size_t end = (pos + length) - 1;
+     size_t start_u8 = pos >> 3;
+     size_t end_u8 = end >> 3;
+     size_t end_rem = end & 0x7;
+     size_t pos_rem = pos & 0x7;
+     u8 end_mask, start_mask;
+     const u8 *ptr;
+     u32 val;
+
+     end_mask = BTM_MSK(end_rem);
+     start_mask = TOP_MSK(pos_rem);
+
+     ptr = &ifh[LAN9645X_IFH_LEN - 1 - end_u8];
+
+     if (end_u8 == start_u8)
+             return (*ptr & end_mask & start_mask) >> pos_rem;
+
+     val = *ptr++ & end_mask;
+
+     for (size_t j = 1; j < end_u8 - start_u8; j++)
+             val = val << 8 | *ptr++;
+
+     return val << (8 - pos_rem) | (*ptr & start_mask) >> pos_rem;
+}

If performance isn't a huge concern, pack() and unpack() certainly seem
simpler than having your own implementation.

I did see pack/unpack, but I had some trouble getting gcc to inline them.
The performance on the CPU port with our development hosts is less than 
expected, so I tried to make this reasonably fast.

quoted

+
+static inline void lan9645x_xmit_get_vlan_info(struct sk_buff *skb,
+                                            struct net_device *br,
+                                            u32 *vlan_tci, u32 *tag_type)
+{
+     struct vlan_ethhdr *hdr;
+     u16 proto, tci;
+
+     if (!br || !br_vlan_enabled(br)) {
+             *vlan_tci = 0;
+             *tag_type = LAN9645X_IFH_TAG_TYPE_C;
+             return;
+     }
+
+     hdr = (struct vlan_ethhdr *)skb_mac_header(skb);
+     br_vlan_get_proto(br, &proto);
+
+     if (ntohs(hdr->h_vlan_proto) == proto) {
+             vlan_remove_tag(skb, &tci);
+             *vlan_tci = tci;
+     } else {
+             rcu_read_lock();
+             br_vlan_get_pvid_rcu(br, &tci);
+             rcu_read_unlock();
+             *vlan_tci = tci;
+     }
+
+     *tag_type = (proto != ETH_P_8021Q) ? LAN9645X_IFH_TAG_TYPE_S :
+                                          LAN9645X_IFH_TAG_TYPE_C;
+}
+
+#endif /* _NET_DSA_TAG_LAN9645X_H_ */

Why do these need to live in a separate include file? Who else needs
access to them other than the tagger?

You are right I will move them to the .c file.

quoted

+static const struct dsa_device_ops lan9645x_netdev_ops = {
+     .name = LAN9645X_NAME,
+     .proto = DSA_TAG_PROTO_LAN9645X,
+     .xmit = lan9645x_xmit,
+     .rcv = lan9645x_rcv,
+     .needed_headroom = LAN9645X_TOTAL_TAG_LEN,
+     .promisc_on_conduit = false,

Initializing with false is unnecessary.

I will remove it in the next version.

quoted

+};
+
+MODULE_DESCRIPTION("DSA tag driver for LAN9645x family of switches, using NPI port");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_LAN9645X, LAN9645X_NAME);
+
+module_dsa_tag_driver(lan9645x_netdev_ops);

--
2.52.0

Thanks,
Emil