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

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Mon Nov 9, 2026 at 6:31 PM EST, netdev-bot+sashiko wrote:
> 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)++;
>> +	}
>> +}
>
> [ ... ]

pw-bot: cr