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

[Jakub Kicinski <kuba@kernel.org>]

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?