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

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

On Tue, 2026-05-05 at 18:45 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: dsa: add tag driver for LAN9645X
> 
> Adds a DSA tag driver for Microchip LAN9645x switches operating with a
> front port configured as an NPI CPU port. RX uses a 16-byte long prefix
> plus a 28-byte internal frame header; TX uses only the IFH.
> 
> > diff --git a/net/dsa/tag_lan9645x.c b/net/dsa/tag_lan9645x.c
> > new file mode 100644
> > index 000000000000..81e7a78e0f81
> > --- /dev/null
> > +++ b/net/dsa/tag_lan9645x.c
> 
> [ ... ]
> 
> > +static struct sk_buff *lan9645x_rcv(struct sk_buff *skb,
> > +                                 struct net_device *ndev)
> > +{
> > +     u32 src_port, qos_class, vlan_tci, tag_type, popcnt, etype_ofs;
> > +     struct dsa_port *dp;
> > +     u32 ifh_gap_len = 0;
> > +     u16 vlan_tpid;
> > +     u8 *ifh;
> > +
> > +     /* DSA master already consumed DMAC,SMAC,ETYPE from long prefix. Go back
> > +      * to beginning of frame.
> > +      */
> > +     skb_push(skb, ETH_HLEN);
> > +
> > +     if (unlikely(!pskb_may_pull(skb, LAN9645X_TOTAL_TAG_LEN)))
> > +             return NULL;
> > +
> > +     /* IFH starts after our long prefix */
> > +     ifh = skb_pull(skb, LAN9645X_LONG_PREFIX_LEN);
> 
> [ ... ]
> 
> > +     skb_pull(skb, LAN9645X_IFH_LEN);
> > +
> > +     if (unlikely(!pskb_may_pull(skb, ifh_gap_len + ETH_HLEN)))
> > +             return NULL;
> > +
> > +     skb_pull(skb, ifh_gap_len);
> > +     skb_reset_mac_header(skb);
> > +     skb_set_network_header(skb, ETH_HLEN);
> > +     skb_reset_mac_len(skb);
> > +
> > +     /* Remove the long prefix + IFH + ifh_gap contribution from
> > +      * skb->csum so the stack sees a checksum consistent with the
> > +      * real Ethernet frame. skb->data currently points at the real
> > +      * MAC header.
> > +      */
> > +     skb_postpull_rcsum(skb,
> > +                        skb->data - LAN9645X_TOTAL_TAG_LEN - ifh_gap_len,
> > +                        LAN9645X_TOTAL_TAG_LEN + ifh_gap_len);
> 
> Does this skb_postpull_rcsum() call subtract the wrong region from
> skb->csum for CHECKSUM_COMPLETE frames?
> 
> By the time lan9645x_rcv() is entered, the conduit's eth_type_trans()
> has already pulled the first ETH_HLEN (14) bytes of the long prefix and
> adjusted skb->csum accordingly, so skb->csum covers bytes starting at
> offset 14 (the remaining 2 bytes of the long prefix plus IFH plus any
> gap plus the real frame).
> 
> At the point of the skb_postpull_rcsum() call, skb->data points at the
> real MAC header, i.e. at offset LAN9645X_TOTAL_TAG_LEN + ifh_gap_len
> from the start of the originally-received frame. So
> skb->data - LAN9645X_TOTAL_TAG_LEN - ifh_gap_len points at offset 0,
> and the subtracted region is [0, LAN9645X_TOTAL_TAG_LEN + ifh_gap_len).
> 
> That region includes the first 14 bytes that eth_type_trans() already
> removed from skb->csum, and it stops short of the real MAC header which
> also needs to be removed. Shouldn't the subtracted region instead start
> at what skb->data was on entry to rcv() (i.e. post-eth_type_trans) and
> cover LAN9645X_TOTAL_TAG_LEN + ifh_gap_len bytes from there, so that it
> matches what skb->csum actually covers?
> 
> For reference, tag_ocelot.c (cited in the commit message as the
> architectural model) saves start = skb->data at function entry before
> any skb_push()/skb_pull() manipulation:
> 
>     net/dsa/tag_ocelot.c:ocelot_rcv() {
>         ...
>         u8 *start = skb->data;
>         ...
>         skb_push(skb, ETH_HLEN);
>         skb_pull(skb, OCELOT_SHORT_PREFIX_LEN);
>         ...
>     }
> 
> and later passes that saved pointer to skb_postpull_rcsum() with the
> total tag length, so the subtracted region exactly matches what the
> NIC's csum_complete covers.
> 
> Would saving skb->data on entry (before the initial skb_push(ETH_HLEN))
> and passing that as the start argument, with length
> LAN9645X_TOTAL_TAG_LEN + ifh_gap_len, produce the correct result here
> as well?

Hehe, this is what I did originally, but in the previous version sashiko
convinced me that was not correct. I do not have a CHECKSUM_COMPLETE 
host to test it with unfortunately. But after looking at mlx5 it does seem
the original was correct and the (real) ethernet header should be subtracted.

We can not save skb->data on entry, as it may have become stale. But just
swapping the order of skb_pull(skb, ETH_HLEN) and skb_postpul_rcsum should
give the desired effect. I will fix this in the next version.