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

[Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>]

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:
> > 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.
> (...)
> > 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.

> > +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.

> > +
> > +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.

> > +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.

> 
> > +};
> > +
> > +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