[PATCH net 0/2] net: macb: fix zero UDPv4 checksum on transmit

[PATCH net 0/2] net: macb: fix zero UDPv4 checksum on transmit

[Nicolai Buchwitz]

When a UDPv4 checksum computes to zero, RFC 768 wants 0xffff on the
wire, but the GEM sends the raw 0x0000, which receivers read as "no
checksum". Raspberry Pi confirmed in a simulation of the Cadence IP
that the engine skips the substitution for UDPv4. UDPv6 and TCP are
fine, and the IP changelog suggests all GEM revisions could be affected.
Reported and analyzed on the Raspberry Pi 5:
https://github.com/raspberrypi/linux/issues/7550

Patch 2 does UDPv4 checksums in software and lets the existing pad
and FCS path keep the hardware off the frame, UDPv6 and TCP keep
the offload. That path miscounts tx_bytes by the software FCS, so
patch 1 fixes the accounting first.

Tested on CM5. To reproduce, send bulk UDP to another host and capture
zero checksums on the receiving side:

  iperf3 -c <host> -u -b 200M -t 30
  tcpdump -ni <iface> 'src host <dut> and udp[6:2] == 0'

Without the fix a packet shows up every few seconds:

  12:46:20.002739 IP <dut>.36309 > <host>.5201: UDP, length 1448
  12:46:23.773968 IP <dut>.36309 > <host>.5201: UDP, length 1448
  12:46:40.227460 IP <dut>.36309 > <host>.5201: UDP, length 1448

With the fix the capture showed none.

Some numbers (udpgso_bench):
- 18-byte UDP throughput dropped by ~ 8%
- 256-byte UDP throughput dropped by ~ 10%
- MTU-sized UDP and UDP GSO were unchanged at the 116 MiB/s link limit

Unfortunately I don't see a cheaper fix, the zero only shows up after
computing the checksum, the MAC inserts it in flight, and AFAIU there is
no register or descriptor bit for the substitution.

I'd appreciate testing on other silicon.

Nicolai Buchwitz (2):
  net: macb: exclude software FCS from TX byte statistics
  net: macb: fix zero UDPv4 checksum on transmit

 drivers/net/ethernet/cadence/macb.h      |  3 +++
 drivers/net/ethernet/cadence/macb_main.c | 28 +++++++++++++++++++++-------
 2 files changed, 24 insertions(+), 7 deletions(-)

-- 
2.53.0

[PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[Nicolai Buchwitz]

Frames going through macb_pad_and_fcs() get padded and four FCS
bytes appended, and TX completion then accounts the grown skb->len.
tx_bytes is supposed to exclude the FCS, and frames padded by the
hardware are counted without the padding anyway, so these frames
show up too large in the statistics.

Remember the length the stack handed over and use that for the byte
counters. BQL stays on the padded skb->len that
netdev_tx_sent_queue() saw.

Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation")
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/cadence/macb.h      |  3 +++
 drivers/net/ethernet/cadence/macb_main.c | 17 ++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 1e1f52285a39..802971c9d344 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -968,6 +968,8 @@ struct macb_dma_desc_ptp {
  *       of the frame
  * @mapping: DMA address of the skb's fragment buffer
  * @size: size of the DMA mapped buffer
+ * @skb_len: skb->len as handed over by the stack, before padding and
+ *           software FCS, only set for the last buffer of the frame
  * @mapped_as_page: true when buffer was mapped with skb_frag_dma_map(),
  *                  false when buffer was mapped with dma_map_single()
  */
@@ -975,6 +977,7 @@ struct macb_tx_skb {
 	struct sk_buff		*skb;
 	dma_addr_t		mapping;
 	size_t			size;
+	unsigned int		skb_len;
 	bool			mapped_as_page;
 };
 
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 76ee4f506033..623d072a271d 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1322,8 +1322,8 @@ static void macb_tx_error_task(struct work_struct *work)
 				bp->netdev->stats.tx_packets++;
 				queue->stats.tx_packets++;
 				packets++;
-				bp->netdev->stats.tx_bytes += skb->len;
-				queue->stats.tx_bytes += skb->len;
+				bp->netdev->stats.tx_bytes += tx_skb->skb_len;
+				queue->stats.tx_bytes += tx_skb->skb_len;
 				bytes += skb->len;
 			}
 		} else {
@@ -1450,8 +1450,8 @@ static int macb_tx_complete(struct macb_queue *queue, int budget)
 					    skb->data);
 				bp->netdev->stats.tx_packets++;
 				queue->stats.tx_packets++;
-				bp->netdev->stats.tx_bytes += skb->len;
-				queue->stats.tx_bytes += skb->len;
+				bp->netdev->stats.tx_bytes += tx_skb->skb_len;
+				queue->stats.tx_bytes += tx_skb->skb_len;
 				packets++;
 				bytes += skb->len;
 			}
@@ -2199,7 +2199,8 @@ static void macb_poll_controller(struct net_device *netdev)
 static unsigned int macb_tx_map(struct macb *bp,
 				struct macb_queue *queue,
 				struct sk_buff *skb,
-				unsigned int hdrlen)
+				unsigned int hdrlen,
+				unsigned int skb_len)
 {
 	unsigned int f, nr_frags = skb_shinfo(skb)->nr_frags;
 	unsigned int len, i, tx_head = queue->tx_head;
@@ -2284,6 +2285,7 @@ static unsigned int macb_tx_map(struct macb *bp,
 
 	/* This is the last buffer of the frame: save socket buffer */
 	tx_skb->skb = skb;
+	tx_skb->skb_len = skb_len;
 
 	/* Update TX ring: update buffer descriptors in reverse order
 	 * to avoid race condition
@@ -2476,7 +2478,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 	unsigned int desc_cnt, nr_frags, frag_size, f;
 	struct macb_queue *queue = &bp->queues[q];
 	netdev_tx_t ret = NETDEV_TX_OK;
-	unsigned int hdrlen;
+	unsigned int hdrlen, skb_len;
 	unsigned long flags;
 	bool is_lso;
 
@@ -2485,6 +2487,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 		return ret;
 	}
 
+	skb_len = skb->len;
 	if (macb_pad_and_fcs(&skb, netdev)) {
 		dev_kfree_skb_any(skb);
 		return ret;
@@ -2548,7 +2551,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
 	}
 
 	/* Map socket buffer for DMA transfer */
-	if (macb_tx_map(bp, queue, skb, hdrlen)) {
+	if (macb_tx_map(bp, queue, skb, hdrlen, skb_len)) {
 		dev_kfree_skb_any(skb);
 		goto unlock;
 	}
-- 
2.53.0

Re: [PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[Nicolai Buchwitz]

Replying to netdev's sashiko
https://netdev-ai.bots.linux.dev/sashiko/#/message/20260824134703.766708-2-nb%40tipi-net.de:

> --- Patch [1]: [PATCH net 1/2] net: macb: exclude software FCS from TX 
> byte statistics ---
> commit 24acee751440b1a44c9e16688554df0848ab621a
> Author: Nicolai Buchwitz <nb@tipi-net.de>
> 
>     net: macb: exclude software FCS from TX byte statistics
> 
>     Frames going through macb_pad_and_fcs() get padded and four FCS
>     bytes appended, and TX completion then accounts the grown skb->len.
>     tx_bytes is supposed to exclude the FCS, and frames padded by the
>     hardware are counted without the padding anyway, so these frames
>     show up too large in the statistics.
> 
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index 76ee4f5060335..623d072a271d0 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -1322,8 +1322,8 @@ static void macb_tx_error_task(struct work_struct *work)
> >  				bp->netdev->stats.tx_packets++;
> >  				queue->stats.tx_packets++;
> >  				packets++;
> > -				bp->netdev->stats.tx_bytes += skb->len;
> > -				queue->stats.tx_bytes += skb->len;
> > +				bp->netdev->stats.tx_bytes += tx_skb->skb_len;
> > +				queue->stats.tx_bytes += tx_skb->skb_len;
> >  				bytes += skb->len;
> >  			}
> >  		} else {
> 
> This is a pre-existing issue, but does macb_tx_error_task() leak BQL 
> bytes for
> dropped pending packets?
> 
> When recovering from a TX error, macb_tx_error_task() iterates through 
> the TX
> ring. For pending descriptors that did not successfully transmit, the 
> code calls
> macb_tx_unmap(bp, tx_skb, 0) which frees the SKB.
> 
> However, it appears to fail to accumulate the freed SKB's length into 
> the bytes
> accumulator. When netdev_tx_completed_queue() is subsequently called, 
> these
> leaked bytes are never reported to BQL, which could permanently retain 
> these
> charged bytes and stall the TX queue.

Looks like it. The dropped frames never get a completion credit, and DQL
only gets reset on close. This is independent of the statistics change,
so I would address it in a follow-up fix. Objections?

> This is another pre-existing issue, but does macb_tx_error_task() 
> modify the TX
> ring pointers without holding the appropriate lock?
> 
> macb_tx_error_task() acquires bp->lock and issues a non-blocking stop 
> to the
> queues via netif_tx_stop_all_queues(). It then manipulates the ring 
> pointers
> queue->tx_head and queue->tx_tail.
> 
> However, the fast-path macb_start_xmit() protects these identical 
> pointers
> using queue->tx_ptr_lock.
> 
> Since netif_tx_stop_all_queues() does not wait for an already-running
> macb_start_xmit() to complete, could macb_tx_error_task() zero out the 
> ring
> pointers while macb_start_xmit() is concurrently populating descriptors 
> on
> another CPU?

AFAIU yes, stopping the queues doesn't wait for a start_xmit that's
already running. Taking tx_ptr_lock there before bp->lock, in the same
order as start_xmit, should fix that. I would handle this together with 
the
BQL issue in a follow-up series.

> [ ... ]
> > @@ -2485,6 +2487,7 @@ static netdev_tx_t macb_start_xmit(struct sk_buff *skb,
> >  		return ret;
> >  	}
> >
> > +	skb_len = skb->len;
> > 	if (macb_pad_and_fcs(&skb, netdev)) {
> 
> This isn't a bug introduced by this patch, but does macb_pad_and_fcs() 
> append
> an FCS before verifying TX ring space, which could corrupt the SKB upon 
> retry?
> 
> [ ... ]
> 
> Because the network stack will subsequently retry transmitting this 
> identical
> SKB, and its length is now larger, will macb_pad_and_fcs() compute a 
> new CRC
> over the old FCS and append a second, duplicate FCS, eventually 
> corrupting the
> payload once tailroom is exhausted?

Yes, and the copy path looks worse: the stack can requeue an skb the 
driver already
consumed. I'll do the ring space check before touching the skb at all. 
After that
point the driver never returns NETDEV_TX_BUSY, so swapping the skb 
should be fine.

> This is also a pre-existing issue, but does macb_pad_and_fcs() modify 
> shared SKB
> data for cloned packets and break nonlinear SKB layouts?
> 
> If an SKB is cloned (e.g., multicast, TCP retransmissions) or nonlinear 
> (paged
> fragments), and its length is already >= ETH_ZLEN (60 bytes) so padlen 
> <= 0,
> and it has at least 4 bytes of tailroom, the code skips the 
> reallocation check:
> 
> 	if (padlen <= 0) {
> 		/* FCS could be appeded to tailroom. */
> 		if (tailroom >= ETH_FCS_LEN)
> 			goto add_fcs;
> 
> It then directly appends 4 bytes of FCS via skb_put_u8(). For cloned 
> SKBs, this
> modifies shared memory data. For nonlinear SKBs, skb_put() illegally 
> extends the
> linear part, which could trigger a panic in skb_panic().

The cloned part looks right. The nonlinear case IMHO can't trigger 
today:
skb_tailroom() is 0 for nonlinear skbs, so they go down 
skb_copy_expand(),
which linearizes them anyway.

I'll order the cloned check before the in-place FCS path.

Re: [PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[Paolo Abeni]

On 8/25/26 5:04 PM, Nicolai Buchwitz wrote:
> Replying to netdev's sashiko
> https://netdev-ai.bots.linux.dev/sashiko/#/message/20260824134703.766708-2-nb%40tipi-net.de:

It looks like you instead replied to the comments from:

https://sashiko.dev/#/patchset/20260824134703.766708-1-nb%40tipi-net.de

Note that our reference saskiko instance is actually the one you
pointed to above, even if the comments are at as slightly different
URL:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824134703.766708-1-nb%40tipi-net.de

Also note that pre-existing issue should (usually) not be addressed
in the same patchset (unless really bad/exploitable).

Finally I have some concerns WRT patch 2/2: it should bring a
significant performance regression, and the frames transmitted ATM are
still valid (even if 'downgraded' csum wise). It would probably be good
to provide some priv flag to enable/disable such behavior.

I guess some/most users would prefer send 1 frame out of 64K with csum
validation unexpectedly disabled than hit a huge slowdown.

/P

Re: [PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[Nicolai Buchwitz]

Hi Paolo

On 27.8.2026 10:52, Paolo Abeni wrote:
> On 8/25/26 5:04 PM, Nicolai Buchwitz wrote:
>> Replying to netdev's sashiko
>> https://netdev-ai.bots.linux.dev/sashiko/#/message/20260824134703.766708-2-nb%40tipi-net.de:
> 
> It looks like you instead replied to the comments from:
> 
> https://sashiko.dev/#/patchset/20260824134703.766708-1-nb%40tipi-net.de
> 
> Note that our reference saskiko instance is actually the one you
> pointed to above, even if the comments are at as slightly different
> URL:
> 
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824134703.766708-1-nb%40tipi-net.de

Thanks for the pointer. I'll have another look and check the findings.

Only partially related, but is there a good way to c&p the sashiko 
review already quoted, so
it is easier to respond inline via email? I saw the discussion about 
sending feedback via email
and also the note from Jakub in the 7.2 stats that the current false 
positive rate _might_
be stable enough to do this in the future, but until then this would 
help a lot!

> 
> Also note that pre-existing issue should (usually) not be addressed
> in the same patchset (unless really bad/exploitable).

I've bundled patch 1 because patch 2 makes it far more visible. Happy to 
split out if you'd rather.

> 
> Finally I have some concerns WRT patch 2/2: it should bring a
> significant performance regression, and the frames transmitted ATM are
> still valid (even if 'downgraded' csum wise). It would probably be good
> to provide some priv flag to enable/disable such behavior.

Agreed. I'm currently testing another approach that checks whether the 
checksum would be 0 and
only does the sw checksum calculation for those packets. Also, the 
testing from Alexander showed that
maybe not all revisions are affected. Théo can probably check Mobileye 
silicon so we can pin this down further.

> 
> I guess some/most users would prefer send 1 frame out of 64K with csum
> validation unexpectedly disabled than hit a huge slowdown.

Agreed, lets see if the different approach might save us this at all.

> 
> /P

Cheers
Nicolai

Re: [PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[Jakub Kicinski]

On Mon, 24 Aug 2026 15:47:02 +0200 Nicolai Buchwitz wrote:
> + * @skb_len: skb->len as handed over by the stack, before padding and
> + *           software FCS, only set for the last buffer of the frame

counting pad bytes to ETH_ZLEN as sent is perfectly legit,
it's a driver preference. Some don't because HW pads and
the driver doesn't want to bother adding a conditional.
But it's best if the driver bytes match wire bytes IMO.

Either way, you only need u8 or even a flag for this,
not a full u32?

Re: [PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[Nicolai Buchwitz]

Hi Jakub

On 27.8.2026 20:56, Jakub Kicinski wrote:
> On Mon, 24 Aug 2026 15:47:02 +0200 Nicolai Buchwitz wrote:
>> + * @skb_len: skb->len as handed over by the stack, before padding and
>> + *           software FCS, only set for the last buffer of the frame
> 
> counting pad bytes to ETH_ZLEN as sent is perfectly legit,
> it's a driver preference. Some don't because HW pads and
> the driver doesn't want to bother adding a conditional.
> But it's best if the driver bytes match wire bytes IMO.

Matches what I had in mind for my v2. So the driver bytes would match 
wire bytes minus FCS.

> 
> Either way, you only need u8 or even a flag for this,
> not a full u32?

Agreed. skb->len at completion should already do the trick (minus 
ETH_FCS_LEN).
So a flag would be sufficient.

Thanks,
Nicolai

Re: [PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[David Laight]

On Fri, 28 Aug 2026 10:20:24 +0200
Nicolai Buchwitz <nb@tipi-net.de> wrote:

> Hi Jakub
> 
> On 27.8.2026 20:56, Jakub Kicinski wrote:
> > On Mon, 24 Aug 2026 15:47:02 +0200 Nicolai Buchwitz wrote:  
> >> + * @skb_len: skb->len as handed over by the stack, before padding and
> >> + *           software FCS, only set for the last buffer of the frame  
> > 
> > counting pad bytes to ETH_ZLEN as sent is perfectly legit,
> > it's a driver preference. Some don't because HW pads and
> > the driver doesn't want to bother adding a conditional.
> > But it's best if the driver bytes match wire bytes IMO.  
> 
> Matches what I had in mind for my v2. So the driver bytes would match 
> wire bytes minus FCS.
> 
> > 
> > Either way, you only need u8 or even a flag for this,
> > not a full u32?  
> 
> Agreed. skb->len at completion should already do the trick (minus 
> ETH_FCS_LEN).
> So a flag would be sufficient.

If you make the 'flag' 0 or 4 it will save some maths (and maybe a branch)
in the completion code.

David

> 
> Thanks,
> Nicolai
> 

Re: [PATCH net 1/2] net: macb: exclude software FCS from TX byte statistics

[David Laight]

On Thu, 27 Aug 2026 11:56:01 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> On Mon, 24 Aug 2026 15:47:02 +0200 Nicolai Buchwitz wrote:
> > + * @skb_len: skb->len as handed over by the stack, before padding and
> > + *           software FCS, only set for the last buffer of the frame  
> 
> counting pad bytes to ETH_ZLEN as sent is perfectly legit,
> it's a driver preference. Some don't because HW pads and
> the driver doesn't want to bother adding a conditional.
> But it's best if the driver bytes match wire bytes IMO.
> 
> Either way, you only need u8 or even a flag for this,
> not a full u32?
> 

You could save a count to be subtracted from skb->len.
But isn't that also unrelated to the UDP checksum issue?

Note that the receiver will treat an IPv4 checksum of zero as valid.
For IPv6 it is a protocol error.

David

[PATCH net 2/2] net: macb: fix zero UDPv4 checksum on transmit

[Nicolai Buchwitz]

The GEM checksum engine writes its raw result into the UDP checksum
field, so a UDPv4 checksum that computes to zero goes out as 0x0000.
RFC 768 requires:

  If the computed checksum is zero, it is transmitted as all ones
  (the equivalent in one's complement arithmetic). An all zero
  transmitted checksum value means that the transmitter generated
  no checksum (for debugging or for higher level protocols that
  don't care).

Raspberry Pi confirmed in a simulation of the Cadence IP [1] that
the engine skips the final substitution exactly for UDP over IPv4.
UDPv6 and TCP come out correct. The IP changelog shows no related
change, so probably all GEM revisions have the same bug.

Clear the checksum offload features for UDPv4 frames in
macb_features_check(), the core then completes the checksum in
software. With ip_summed cleared, macb_pad_and_fcs() appends the
FCS and TX_NOCRC keeps the hardware off the frame. TCP and UDPv6
keep the offload.

One-step PTP sync packets stay on the hardware path, the MAC
rewrites their timestamp during transmit and would invalidate a
software checksum.

Link: https://github.com/raspberrypi/linux/issues/7550#issuecomment-5343018065 [1]
Fixes: 85ff3d87bf2e ("net/macb: add TX checksum offload feature")
Cc: Jonathan Bell <jonathan@raspberrypi.com>
Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
---
 drivers/net/ethernet/cadence/macb_main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 623d072a271d..f3548475f945 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -14,6 +14,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/etherdevice.h>
 #include <linux/firmware/xlnx-zynqmp.h>
+#include <linux/if_vlan.h>
 #include <linux/inetdevice.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
@@ -2373,6 +2374,16 @@ static netdev_features_t macb_features_check(struct sk_buff *skb,
 	unsigned int nr_frags, f;
 	unsigned int hdrlen;
 
+	/* The GEM skips the RFC 768 substitution of 0xffff for a zero
+	 * UDPv4 checksum, do those in software. One-step PTP sync frames
+	 * stay on hardware, the MAC rewrites their timestamp in flight.
+	 */
+	if (skb->ip_summed == CHECKSUM_PARTIAL &&
+	    vlan_get_protocol(skb) == htons(ETH_P_IP) &&
+	    skb->csum_offset == offsetof(struct udphdr, check) &&
+	    !ptp_one_step_sync(skb))
+		features &= ~NETIF_F_CSUM_MASK;
+
 	/* Validate LSO compatibility */
 
 	/* there is only one buffer or protocol is not UDP */
-- 
2.53.0

Re: [PATCH net 0/2] net: macb: fix zero UDPv4 checksum on transmit

[Alexander Dahl]

Hello Nicolai,

Am Mon, Aug 24, 2026 at 03:47:01PM +0200 schrieb Nicolai Buchwitz:
> When a UDPv4 checksum computes to zero, RFC 768 wants 0xffff on the
> wire, but the GEM sends the raw 0x0000, which receivers read as "no
> checksum". Raspberry Pi confirmed in a simulation of the Cadence IP
> that the engine skips the substitution for UDPv4. UDPv6 and TCP are
> fine, and the IP changelog suggests all GEM revisions could be affected.
> Reported and analyzed on the Raspberry Pi 5:
> https://github.com/raspberrypi/linux/issues/7550
> 
> Patch 2 does UDPv4 checksums in software and lets the existing pad
> and FCS path keep the hardware off the frame, UDPv6 and TCP keep
> the offload. That path miscounts tx_bytes by the software FCS, so
> patch 1 fixes the accounting first.
> 
> Tested on CM5. To reproduce, send bulk UDP to another host and capture
> zero checksums on the receiving side:
> 
>   iperf3 -c <host> -u -b 200M -t 30
>   tcpdump -ni <iface> 'src host <dut> and udp[6:2] == 0'
> 
> Without the fix a packet shows up every few seconds:
> 
>   12:46:20.002739 IP <dut>.36309 > <host>.5201: UDP, length 1448
>   12:46:23.773968 IP <dut>.36309 > <host>.5201: UDP, length 1448
>   12:46:40.227460 IP <dut>.36309 > <host>.5201: UDP, length 1448
> 
> With the fix the capture showed none.
> 
> Some numbers (udpgso_bench):
> - 18-byte UDP throughput dropped by ~ 8%
> - 256-byte UDP throughput dropped by ~ 10%
> - MTU-sized UDP and UDP GSO were unchanged at the 116 MiB/s link limit
> 
> Unfortunately I don't see a cheaper fix, the zero only shows up after
> computing the checksum, the MAC inserts it in flight, and AFAIU there is
> no register or descriptor bit for the substitution.
> 
> I'd appreciate testing on other silicon.

I tested without your patch on at91 sama5d2 (Cadence GEM rev
0x00020203), sam9x60 (Cadence MACB rev 0x0001010c), and sam9g20
(Cadence MACB rev 0x0001010c), and tried to reproduce the flaw (not
the fix).  Rewrote the Python reproducer suggested in that RPi github
issue in C, but iperf3 gave similar results:  tcpdump started as
suggested showed not a single wrong package!

However according to ethtool it seems at least the older MACB bases
hardware has no hardware checksumming here.  On sama5d2 (GEM, but no
Gigabit) I'm not sure if it is supposed to be done for UDP?  See:

    $ ethtool -k eth0 | grep sum
    rx-checksumming: on
    tx-checksumming: on
            tx-checksum-ipv4: off [fixed]
            tx-checksum-ip-generic: on
            tx-checksum-ipv6: off [fixed]
            tx-checksum-fcoe-crc: off [fixed]
            tx-checksum-sctp: off [fixed]
    tx-gre-csum-segmentation: off [fixed]
    tx-udp_tnl-csum-segmentation: off [fixed]
    tx-tunnel-remcsum-segmentation: off [fixed]
    esp-tx-csum-hw-offload: off [fixed]

On sam9x60 (MACB) there seems to be no hardware offloading at all:

    $ ethtool -k eth0 | grep sum
    rx-checksumming: off [fixed]
    tx-checksumming: off
            tx-checksum-ipv4: off [fixed]
            tx-checksum-ip-generic: off [fixed]
            tx-checksum-ipv6: off [fixed]
            tx-checksum-fcoe-crc: off [fixed]
            tx-checksum-sctp: off [fixed]
    tx-gre-csum-segmentation: off [fixed]
    tx-udp_tnl-csum-segmentation: off [fixed]
    tx-tunnel-remcsum-segmentation: off [fixed]
    esp-tx-csum-hw-offload: off [fixed]

How are those chips affected by your patch then?

> Nicolai Buchwitz (2):
>   net: macb: exclude software FCS from TX byte statistics
>   net: macb: fix zero UDPv4 checksum on transmit

That fix seems to be very generic and not specific to macb or gem nor
to any variant.  I'd suggest narrowing it down and testing on more
hardware.

Greets
Alex

> 
>  drivers/net/ethernet/cadence/macb.h      |  3 +++
>  drivers/net/ethernet/cadence/macb_main.c | 28 +++++++++++++++++++++-------
>  2 files changed, 24 insertions(+), 7 deletions(-)
> 
> -- 
> 2.53.0
>