Re: [PATCH net-next 2/6] eth: mpnic: Rx checksum offload

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The CHECKSUM_COMPLETE value in mpnic_rx_csum() is only right
  on hosts of one byte order.
- [Low] mpnic_rx_csum() (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c)
  sets skb->ip_summed = CHECKSUM_UNNECESSARY and ignores the full-frame…

--

eth: mpnic: Rx checksum offload

This commit adds Rx checksum offload to the mpnic driver. When the
completion descriptor's l4_csum_unnecessary bit is set, the driver reports
CHECKSUM_UNNECESSARY; otherwise it reports the l2_csum field as
CHECKSUM_COMPLETE. It also exports csum none/complete/unnecessary counters
through the netdev qstats API.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index edb087b12a1e2..680625810ec03 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> @@ -655,9 +655,34 @@ static void mpnic_put_pkt_buff(struct mpnic_pkt_ctxt *ctxt, bool napi)
>  	page_pool_put_full_page(page->pp, page, napi);
>  }
>  
> +static void mpnic_rx_csum(u64 rcd, struct sk_buff *skb,
> +			  unsigned int *csum_complete,
> +			  unsigned int *csum_unnecessary,
> +			  unsigned int *csum_none)
> +{
> +	skb_checksum_none_assert(skb);
> +
> +	if (!(skb->dev->features & NETIF_F_RXCSUM)) {
> +		(*csum_none)++;
> +		return;
> +	}
> +
> +	if (FIELD_GET(MPNIC_RCD_META_L4_CSUM_UNNECESSARY, rcd)) {
> +		skb->ip_summed = CHECKSUM_UNNECESSARY;
> +		(*csum_unnecessary)++;

[Severity: Low]
This isn't a bug, but should mpnic_rx_csum() prefer CHECKSUM_COMPLETE here,
since the same descriptor also carries MPNIC_RCD_META_L2_CSUM?

The commit message says l2_csum is reported for every frame, and
mpnic_csr.h doesn't define a validity bit for it. The comment in
include/linux/skbuff.h says:

 *   - Even if device supports only some protocols, but is able to produce
 *     skb->csum, it MUST use CHECKSUM_COMPLETE, not CHECKSUM_UNNECESSARY.

With the current ordering, every TCP/UDP packet that the device validates
goes up the stack without skb->csum. Anything the hardware L4 engine did
not check then needs a software checksum. One example is the inner
payload of a UDP tunnel when the checksum conversion path doesn't apply.

The in-tree fbnic driver uses the same pattern in fbnic_rx_csum(), so
this may be intentional. It still doesn't match the documented contract.

> +	} else {
> +		u16 csum = FIELD_GET(MPNIC_RCD_META_L2_CSUM, rcd);
> +
> +		skb->ip_summed = CHECKSUM_COMPLETE;
> +		skb->csum = (__force __wsum)csum;

[Severity: Medium]
Is this skb->csum value correct on big-endian hosts?

mpnic_clean_rcq() converts the descriptor to host order first:

	rcd = le64_to_cpu(*raw_rcd);

So the u16 that FIELD_GET() extracts from bits 47:32 is the same number on
every host. A __wsum, however, is computed by csum_partial() using
host-order 16-bit loads. The correct value therefore differs by a byte
swap between little-endian and big-endian hosts, and storing the raw
field can only be right on one of them.

The MPNIC Kconfig entry only has:

	depends on 64BIT || COMPILE_TEST
	depends on !S390

That still allows ppc64 BE, sparc64 and mips64 BE builds with PCI_MSI.

On such a host, would every good packet reported as CHECKSUM_COMPLETE fail
validation in __skb_checksum_complete()? That would end in
netdev_rx_csum_fault() and "hw csum failure" messages. Decap paths such as
skb_postpull_rcsum() and pskb_trim_rcsum() would also adjust from a wrong
base.

Would a conversion that keeps the device's byte layout help here, for
example (__force __wsum)(__force u16)cpu_to_le16(csum)? Alternatively, the
driver could be restricted to little-endian hosts in Kconfig.

> +		(*csum_complete)++;
> +	}
> +}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-mpnic-lso-v1-0-e943ff1493e4%40gmail.com