Paths

Table of Contentst

dpaa2: Extract checksum statuses on ingress
Needs ReviewPublic
Actions

Authored by dsl on Mon, Apr 13, 8:33 PM.

Details

Reviewers

tuexen
bz

Summary

This is effectively a refined version of the code written by bz@ in D55320.

In order to enable RX checksum offloading we need to check the
meta-information for the (good) frames to see if the L3/4 checksums
were calculated and if there was an error.

The way the buffere are setup, the needed frame meta-information is
already requested. All we have to do is make sure it is really part
of the RX frame, that it is valid, and if the respective bits are set.

Also do not forget to set the (dummy) csum_data as otherwise upper
layers will just be cranky. An artefact of the past which likely
should disappear.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

dsl created this revision.Mon, Apr 13, 8:33 PM

Herald added a subscriber: imp. · View Herald TranscriptMon, Apr 13, 8:33 PM

dsl requested review of this revision.Mon, Apr 13, 8:33 PM

XB! I am likely finding my own faults here; sorry about that!

sys/dev/dpaa2/dpaa2_frame.c
151	Unused function; will give you an annoying warning possibly.
188	If you rewrite this as: if (((fd->ctrl >> DPAA2_FD_ASAL_SHIFT) & DPAA2_FD_ASAL_MASK) == 0) { *hwa = NULL return (ENOENT); } Basically checking for the else path first and return, it saves you a level of indentation for the other rather awkward to read part and at least the >= 0x4u) ? should fit on the same line. I am still wondering if using a temporary variable would help more; the compiler can then do its thing on single use and optimize it...
223	Unused function; will give you an annoying warning possibly.
sys/dev/dpaa2/dpaa2_ni.c
3133	I would highly recommend to spell it out and not call it "upd" as some brains will read that "udp" on first glance; also we call them csum_flags on the mbuf not chksum_flags; why not keep that terminology for the new code for the variable and function names?
3675	style says to use != 0 but again if you change it to predict false and do the else path with == 0, the code will be easier to read

This revision now requires changes to proceed.Mon, Apr 13, 10:05 PM

dsl mentioned this in D56366: Status/2026Q1/dpaa2.adoc: Add report.Wed, Apr 15, 10:31 AM

dsl updated this revision to Diff 175557.Wed, Apr 15, 11:55 AM

dsl marked 5 inline comments as done.Wed, Apr 15, 12:00 PM

dsl added inline comments.

sys/dev/dpaa2/dpaa2_frame.c
188	Okay, done. I've also declared PTA/PTV1/PTV2 masks to make the ">= 0x4u" less awkward and applied your advice in the dpaa2_fa_get_swa as well. Looks neat :) Thanks!
sys/dev/dpaa2/dpaa2_ni.c
3133	It was probably the last pieces I refined and just didn't think about a possible confusion. Fixed.

dsl marked 2 inline comments as done.Wed, Apr 15, 12:02 PM

dsl added inline comments.

sys/dev/dpaa2/dpaa2_ni.c
7	@bz Do you mind if I add you?
7	@tuexen and you :)

Looks good to me, will test it tomorrow. Right now my test system is not reachable and I am only back to my lab tomorrow.

dsl added inline comments.Wed, Apr 15, 12:26 PM

sys/dev/dpaa2/dpaa2_ni.c
3705	Shouldn't csum_data be set to 0xffff above (at the L3) as well? I wonder whether any packet with L3CV set and L4CV reset would cause a problem with the stack or not.

In D56383#1291193, @tuexen wrote:

Looks good to me, will test it tomorrow. Right now my test system is not reachable and I am only back to my lab tomorrow.

Sounds good! Thanks!

tuexen added inline comments.Wed, Apr 15, 1:50 PM

sys/dev/dpaa2/dpaa2_ni.c
7	I haven't written anything in this file. At most, found bugs. So: No. Thanks for asking.

When enabling rx csum offload, I do the the expected flags in incoming TCP segments. So this works now. But when disabling rx csum offload, I still see the flags:

tuexen@ten64:~ % ifconfig dpni1
dpni1: flags=1008843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=48002a<TXCSUM,VLAN_MTU,JUMBO_MTU,LINKSTATE,TXCSUM_IPV6>
	ether 00:0a:fa:24:2f:c6
	inet 212.201.121.124 netmask 0xffffffe0 broadcast 212.201.121.127
	inet6 fe80::20a:faff:fe24:2fc6%dpni1 prefixlen 64 scopeid 0x2
	inet6 2a02:c6a0:4015:10::124 prefixlen 64
	media: Ethernet autoselect (1000baseT <full-duplex,master>)
	status: active
	nd6 options=821<PERFORMNUD,AUTO_LINKLOCAL,STABLEADDR>

Output of:

#!/usr/sbin/dtrace -s

#pragma D option quiet

/* 
 * Trace tcp_input to capture incoming segments. 
 * arg0 is typically a pointer to the mbuf (struct mbuf *).
 */
fbt::tcp_input:entry
{
    /* Cast arg0 to access the mbuf structure and its packet header */
    this->m = *(struct mbuf **)arg0;
    this->flags = this->m->m_pkthdr.csum_flags;

    printf("%-20Y  Flags: 0x%08x\n", walltimestamp, this->flags);
}
fbt::tcp6_input:entry
{
    /* Cast arg0 to access the mbuf structure and its packet header */
    this->m = *(struct mbuf **)arg0;
    this->flags = this->m->m_pkthdr.csum_flags;

    printf("%-20Y  Flags: 0x%08x\n", walltimestamp, this->flags);
}

is:

2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000

This is for TCP/IPv6. I would have expected 0. Is my expectation wrong?

In D56383#1292028, @tuexen wrote:

When enabling rx csum offload, I do the the expected flags in incoming TCP segments. So this works now. But when disabling rx csum offload, I still see the flags:

You didn't show the command you used.

IMHO this is offloading on or off independent of address family; so both v4 and v6 need to go on/off at the same time; I have no clue how we normally handle that case. I could be wrong; my brain is meshed potatoes currently; I can check in a few hours possibly again or tomorrow.

In D56383#1292035, @bz wrote:

In D56383#1292028, @tuexen wrote:

When enabling rx csum offload, I do the the expected flags in incoming TCP segments. So this works now. But when disabling rx csum offload, I still see the flags:

You didn't show the command you used.

I ran the dtrace script provided above.

IMHO this is offloading on or off independent of address family; so both v4 and v6 need to go on/off at the same time; I have no clue how we normally handle that case. I could be wrong; my brain is meshed potatoes currently; I can check in a few hours possibly again or tomorrow.

If you disable rxsum or rxcsum6, both are turned off. This is done also in other drivers the same way and it works correctly.

In D56383#1292042, @tuexen wrote:

In D56383#1292035, @bz wrote:

In D56383#1292028, @tuexen wrote:

When enabling rx csum offload, I do the the expected flags in incoming TCP segments. So this works now. But when disabling rx csum offload, I still see the flags:

You didn't show the command you used.

I ran the dtrace script provided above.

Sorry, I meant to ifconfig to turn rxcsum off. But you say that works as expected (turning the flag off but not the mbuf bits)?

So do I get it right that initially it worked? [Y/n]
You turned rxcsum on and that worked? [Y/n]
And then you turned rxcsum off and it didn't work? [Y/n] Or in other words, the flag on ifconfig vanished but the mbuf pkthdr still says they were checked?

bz added inline comments.Thu, Apr 16, 8:43 PM

sys/dev/dpaa2/dpaa2_ni.c
7	I do not mind. Copyright (c) 2026 Bjoern A. Zeeb Please no UTF-8. I'll add more to it once this is sorted but parts of what's now in dpaa2_frame came from the other review you said so it should be fine.

In D56383#1292043, @bz wrote:

In D56383#1292042, @tuexen wrote:

In D56383#1292035, @bz wrote:

In D56383#1292028, @tuexen wrote:

When enabling rx csum offload, I do the the expected flags in incoming TCP segments. So this works now. But when disabling rx csum offload, I still see the flags:

You didn't show the command you used.

I ran the dtrace script provided above.

Sorry, I meant to ifconfig to turn rxcsum off. But you say that works as expected (turning the flag off but not the mbuf bits)?

Yes, it does. Here are the commands:

tuexen@ten64:~ % ifconfig dpni1
dpni1: flags=1008843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=68002b<RXCSUM,TXCSUM,VLAN_MTU,JUMBO_MTU,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
	ether 00:0a:fa:24:2f:c6
	inet 212.201.121.124 netmask 0xffffffe0 broadcast 212.201.121.127
	inet6 fe80::20a:faff:fe24:2fc6%dpni1 prefixlen 64 scopeid 0x2
	inet6 2a02:c6a0:4015:10::124 prefixlen 64
	media: Ethernet autoselect (1000baseT <full-duplex,master>)
	status: active
	nd6 options=821<PERFORMNUD,AUTO_LINKLOCAL,STABLEADDR>
tuexen@ten64:~ % sudo ifconfig dpni1 -rxcsum
tuexen@ten64:~ % ifconfig dpni1
dpni1: flags=1008843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=48002a<TXCSUM,VLAN_MTU,JUMBO_MTU,LINKSTATE,TXCSUM_IPV6>
	ether 00:0a:fa:24:2f:c6
	inet 212.201.121.124 netmask 0xffffffe0 broadcast 212.201.121.127
	inet6 fe80::20a:faff:fe24:2fc6%dpni1 prefixlen 64 scopeid 0x2
	inet6 2a02:c6a0:4015:10::124 prefixlen 64
	media: Ethernet autoselect (1000baseT <full-duplex,master>)
	status: active
	nd6 options=821<PERFORMNUD,AUTO_LINKLOCAL,STABLEADDR>
tuexen@ten64:~ % sudo ifconfig dpni1 rxcsum
tuexen@ten64:~ % ifconfig dpni1
dpni1: flags=1008843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=68002b<RXCSUM,TXCSUM,VLAN_MTU,JUMBO_MTU,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
	ether 00:0a:fa:24:2f:c6
	inet 212.201.121.124 netmask 0xffffffe0 broadcast 212.201.121.127
	inet6 fe80::20a:faff:fe24:2fc6%dpni1 prefixlen 64 scopeid 0x2
	inet6 2a02:c6a0:4015:10::124 prefixlen 64
	media: Ethernet autoselect (1000baseT <full-duplex,master>)
	status: active
	nd6 options=821<PERFORMNUD,AUTO_LINKLOCAL,STABLEADDR>
tuexen@ten64:~ % sudo ifconfig dpni1 -rxcsum6
tuexen@ten64:~ % ifconfig dpni1
dpni1: flags=1008843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=48002a<TXCSUM,VLAN_MTU,JUMBO_MTU,LINKSTATE,TXCSUM_IPV6>
	ether 00:0a:fa:24:2f:c6
	inet 212.201.121.124 netmask 0xffffffe0 broadcast 212.201.121.127
	inet6 fe80::20a:faff:fe24:2fc6%dpni1 prefixlen 64 scopeid 0x2
	inet6 2a02:c6a0:4015:10::124 prefixlen 64
	media: Ethernet autoselect (1000baseT <full-duplex,master>)
	status: active
	nd6 options=821<PERFORMNUD,AUTO_LINKLOCAL,STABLEADDR>
tuexen@ten64:~ % sudo ifconfig dpni1 rxcsum6
tuexen@ten64:~ % ifconfig dpni1
dpni1: flags=1008843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=68002b<RXCSUM,TXCSUM,VLAN_MTU,JUMBO_MTU,LINKSTATE,RXCSUM_IPV6,TXCSUM_IPV6>
	ether 00:0a:fa:24:2f:c6
	inet 212.201.121.124 netmask 0xffffffe0 broadcast 212.201.121.127
	inet6 fe80::20a:faff:fe24:2fc6%dpni1 prefixlen 64 scopeid 0x2
	inet6 2a02:c6a0:4015:10::124 prefixlen 64
	media: Ethernet autoselect (1000baseT <full-duplex,master>)
	status: active
	nd6 options=821<PERFORMNUD,AUTO_LINKLOCAL,STABLEADDR>
tuexen@ten64:~ %

So do I get it right that initially it worked? [Y/n]
You turned rxcsum on and that worked? [Y/n]
And then you turned rxcsum off and it didn't work? [Y/n] Or in other words, the flag on ifconfig vanished but the mbuf pkthdr still says they were checked?

Correct. rxcsum offload is on by default. What is unexpected:
After disabling rxcsum, the mbuf flags are still set. This is unexpected for me, since we have dpaa2_ni_setup_if_caps(). Maybe I am misunderstanding the purpose of that function.

In D56383#1292028, @tuexen wrote:

When enabling rx csum offload, I do the the expected flags in incoming TCP segments. So this works now. But when disabling rx csum offload, I still see the flags:

tuexen@ten64:~ % ifconfig dpni1
dpni1: flags=1008843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST,LOWER_UP> metric 0 mtu 1500
	options=48002a<TXCSUM,VLAN_MTU,JUMBO_MTU,LINKSTATE,TXCSUM_IPV6>
	ether 00:0a:fa:24:2f:c6
	inet 212.201.121.124 netmask 0xffffffe0 broadcast 212.201.121.127
	inet6 fe80::20a:faff:fe24:2fc6%dpni1 prefixlen 64 scopeid 0x2
	inet6 2a02:c6a0:4015:10::124 prefixlen 64
	media: Ethernet autoselect (1000baseT <full-duplex,master>)
	status: active
	nd6 options=821<PERFORMNUD,AUTO_LINKLOCAL,STABLEADDR>

Output of:

#!/usr/sbin/dtrace -s

#pragma D option quiet

/* 
 * Trace tcp_input to capture incoming segments. 
 * arg0 is typically a pointer to the mbuf (struct mbuf *).
 */
fbt::tcp_input:entry
{
    /* Cast arg0 to access the mbuf structure and its packet header */
    this->m = *(struct mbuf **)arg0;
    this->flags = this->m->m_pkthdr.csum_flags;

    printf("%-20Y  Flags: 0x%08x\n", walltimestamp, this->flags);
}
fbt::tcp6_input:entry
{
    /* Cast arg0 to access the mbuf structure and its packet header */
    this->m = *(struct mbuf **)arg0;
    this->flags = this->m->m_pkthdr.csum_flags;

    printf("%-20Y  Flags: 0x%08x\n", walltimestamp, this->flags);
}

is:

2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000
2026 Apr 16 22:04:38  Flags: 0x0c000000

This is for TCP/IPv6. I would have expected 0. Is my expectation wrong?

Could you dtrace entering dpaa2_ni_setup_if_caps? I've tried to toggle rx/tx csums via ifconfig on my side and seems that dpaa2_ni_setup_if_caps wasn't called at all. It's weird...

In D56383#1292505, @dsl wrote:

Could you dtrace entering dpaa2_ni_setup_if_caps? I've tried to toggle rx/tx csums via ifconfig on my side and seems that dpaa2_ni_setup_if_caps wasn't called at all. It's weird...

or simply boot verbose; it'll either log on error or on successful completion from what I can see.

Unrelated but is that

1637         (void)DPAA2_CMD_NI_CLOSE(dev, child, &cmd);

at the end of dpaa2_ni_setup_if_caps correct passing &cmd instead of DPAA2_CMD_TK(&cmd, ni_token) ? I think that should be fixed.
Reported by: bz

Uhh, I've double-checked and dpaa2_ni_setup_if_caps is actually called on my side and reports correct changes in the csum validation/generation, but it doesn't affect the ingress frames at all:

dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
                ifconfig dpni0 dpaa2_ni_update_csum_flags: frc=0x63a000, fas=0x200000000021100, csum_flags=0, csum_data=0
                               -rxcsum
dpaa2_ni0: dpaa2_ni_setup_if_caps: L3/L4 checksum validation disabled
dpaa2_ni0: dpaa2_ni_setup_if_caps: L3/L4 checksum generation disabled
root@castle:~ dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0xa00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0xa00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0xa00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff

I went through the implementation of the command in dpaa2_rc_ni_set_offload, but it seems correct. @tuexen As you've experimentally proved that that Tx csum generation actually works, I'm not sure how Rx csum validation is broken. I've also tried to remove a call to dpaa2_ni_setup_if_caps from the dpaa2_ni_attach, but it looks like checksums are validated still.

In D56383#1292507, @bz wrote:
Unrelated but is that
1637         (void)DPAA2_CMD_NI_CLOSE(dev, child, &cmd);
at the end of dpaa2_ni_setup_if_caps correct passing &cmd instead of DPAA2_CMD_TK(&cmd, ni_token) ? I think that should be fixed.
Reported by: bz

Token from the last open operation (which was DPAA2_CMD_NI_OPEN) is preserved inside the "cmd" itself, i.e. no need to update it there. I've placed it in

close_ni:
	(void)DPAA2_CMD_NI_CLOSE(dev, child, DPAA2_CMD_TK(&cmd, ni_token));

just as a defense against further changes in case somebody will decide to open something else and forget to set the token later on.

In D56383#1292509, @dsl wrote:
Uhh, I've double-checked and dpaa2_ni_setup_if_caps is actually called on my side and reports correct changes in the csum validation/generation, but it doesn't affect the ingress frames at all:
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff

These FAS values make no sense if you look at the bits. Is this on the 2160 or on 1088?

In D56383#1292511, @bz wrote:
In D56383#1292509, @dsl wrote:
Uhh, I've double-checked and dpaa2_ni_setup_if_caps is actually called on my side and reports correct changes in the csum validation/generation, but it doesn't affect the ingress frames at all:
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
These FAS values make no sense if you look at the bits. Is this on the 2160 or on 1088?

even FRC has reserved bits set.

Can you check FD[ERR] is 0?

In D56383#1292509, @dsl wrote:

Uhh, I've double-checked and dpaa2_ni_setup_if_caps is actually called on my side and reports correct changes in the csum validation/generation, but it doesn't affect the ingress frames at all:

OK. I added a printf() and can confirm that it is called. So there is no difference between your observations and mine.

dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
                ifconfig dpni0 dpaa2_ni_update_csum_flags: frc=0x63a000, fas=0x200000000021100, csum_flags=0, csum_data=0
                               -rxcsum
dpaa2_ni0: dpaa2_ni_setup_if_caps: L3/L4 checksum validation disabled
dpaa2_ni0: dpaa2_ni_setup_if_caps: L3/L4 checksum generation disabled
root@castle:~ dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0xa00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0xa00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0xa00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff

OK. Some NICs do this. But let me later double check that it does not report that the checksum is correct when it actually isn't.

In D56383#1292511, @bz wrote:
In D56383#1292509, @dsl wrote:
Uhh, I've double-checked and dpaa2_ni_setup_if_caps is actually called on my side and reports correct changes in the csum validation/generation, but it doesn't affect the ingress frames at all:
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
These FAS values make no sense if you look at the bits. Is this on the 2160 or on 1088?

Why? Take a look:

This is for LX2160.

In D56383#1292559, @dsl wrote:
In D56383#1292511, @bz wrote:
In D56383#1292509, @dsl wrote:
Uhh, I've double-checked and dpaa2_ni_setup_if_caps is actually called on my side and reports correct changes in the csum validation/generation, but it doesn't affect the ingress frames at all:
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
These FAS values make no sense if you look at the bits. Is this on the 2160 or on 1088?
Why? Take a look:

This is for LX2160.

I just checked again after being AFK freeing my mind.
The FRC has different bits compared to the LS1088; I checked the other manual for the parse status and it seems it's IPv6 UDP packets, and IPv4 UDP packets? On the LS1088 those bits are reserved.
And IPv6 correctly means no L3 checksum as it doesn't exist for it.

Sorry for the noise earlier.

In D56383#1292536, @tuexen wrote:

In D56383#1292509, @dsl wrote:

Uhh, I've double-checked and dpaa2_ni_setup_if_caps is actually called on my side and reports correct changes in the csum validation/generation, but it doesn't affect the ingress frames at all:

OK. I added a printf() and can confirm that it is called. So there is no difference between your observations and mine.

dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
                ifconfig dpni0 dpaa2_ni_update_csum_flags: frc=0x63a000, fas=0x200000000021100, csum_flags=0, csum_data=0
                               -rxcsum
dpaa2_ni0: dpaa2_ni_setup_if_caps: L3/L4 checksum validation disabled
dpaa2_ni0: dpaa2_ni_setup_if_caps: L3/L4 checksum generation disabled
root@castle:~ dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x30a000, fas=0x400000200021100, csum_flags=0xc000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0xa00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0xa00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0xa00021100, csum_flags=0xf000000, csum_data=0xffff
dpaa2_ni_update_csum_flags: frc=0x10a000, fas=0x400000a00021100, csum_flags=0xf000000, csum_data=0xffff

OK. Some NICs do this. But let me later double check that it does not report that the checksum is correct when it actually isn't.

I double checked and can say that the checksum is not reported as correct, if the checksum is actually bad. So that seems to be OK.

So why only set the csum_flags, if the flags are set on the interface. This does not disable the hardware doing the validation, but it does disable reporting it. This approach is used also in some other drivers, for example dwc1000_dma.c. I added a proposed change.

sys/dev/dpaa2/dpaa2_ni.c
3189	Maybe add: /* The hardware always verifies the checksum */ if ((if_getcapenable(sc->ifp) & (IFCAP_RXCSUM \| IFCAP_RXCSUM_IPV6)) == 0) update_csum_flags = false;

bz added inline comments.Fri, Apr 17, 6:01 PM

sys/dev/dpaa2/dpaa2_ni.c
3189	I would highly suggest to move this down to the if_inc_counter() call. There the ifp is host and update_csum_flags would also be hot. I am not sure given the locks in between if otherwise we take the cache hit twice. Sorry, microoptimizing..

tuexen added inline comments.Fri, Apr 17, 6:17 PM

sys/dev/dpaa2/dpaa2_ni.c
3189	I am fine with this. One could also combine the check into the `if` condition. something like: if (update_csum_flags && (if_getcapenable(sc->ifp) & (IFCAP_RXCSUM \| IFCAP_RXCSUM_IPV6)) != 0)) { }

dsl updated this revision to Diff 175828.Sat, Apr 18, 11:43 AM

dsl marked 3 inline comments as done.

As soon as we were interested in the FD[ERR} field, I added another counter to track other ingress frames with errors. It can be detailed in future.

dsl marked an inline comment as not done.Sat, Apr 18, 11:48 AM

dsl added inline comments.

sys/dev/dpaa2/dpaa2_ni.c
3705	I still don't know whether csum_data should be set at L3 or not.

dsl marked 2 inline comments as not done.Sat, Apr 18, 11:49 AM

dsl added inline comments.

sys/dev/dpaa2/dpaa2_ni.c
3705	@bz, @tuexen Do you have any comments?

You could write this "This is effectively a refined version of the code written by bz@ in D55320." as part of the final commit message as:

Obtained from: bz (initial version, D55320)
or
Based on: initial version from bz (D55320)

I'll try to give it a pass again tonight once I am done with merges from vendor trees but I think we are good to go if @tuexen has not further comments.

bz added inline comments.Sat, Apr 18, 1:30 PM

sys/dev/dpaa2/dpaa2_ni.c
3705	I believe no. This is used by L4. You could always check the network stack or other drivers like cxgbe or a simple on like (hmm what's left of all the Bill Paul ones?) sk(4) or fxp(4) maybe?

dsl marked an inline comment as not done.Sat, Apr 18, 4:58 PM

dsl added inline comments.

sys/dev/dpaa2/dpaa2_ni.c
3705	Okay, I see https://cgit.freebsd.org/src/tree/sys/dev/fxp/if_fxp.c#n1774 now. Thanks for pointing out!

In D56383#1293150, @bz wrote:

You could write this "This is effectively a refined version of the code written by bz@ in D55320." as part of the final commit message as:

Obtained from: bz (initial version, D55320)
or
Based on: initial version from bz (D55320)

I'll try to give it a pass again tonight once I am done with merges from vendor trees but I think we are good to go if @tuexen has not further comments.