[PATCH net] net: stmmac: fix rx checksum offload with extended descriptors

[PATCH net] net: stmmac: fix rx checksum offload with extended descriptors

[alex todadze via B4 Relay]

From: alex todadze <todadzealexi19@proton.me>

With extended descriptors, bit 0 of RDES0 is the Extended Status
Available flag. enh_desc_get_rx_status() still reads it as the payload
checksum error bit of the checksum offload table, so every IP frame that
has extended status is reported as csum_none and its checksum is
verified again in software.

When extended status is available, take the result from RDES4 instead.
A TCP, UDP or ICMP payload with no IP header error, no payload error and
no checksum bypass is a good frame. Everything else keeps the status
from RDES0. While at it, fix the IP payload type mask, which covered
bits 6:2 instead of 2:0 and had no users.

Tested on a Siflower SF19A2890 (GMAC 3.70) with the board as the iperf3
endpoint. TCP receive goes from 326 to 378 Mbit/s with one stream and
from 259 to 329 Mbit/s with four, and CPU use while receiving 200 Mbit/s
drops from 26.6% to 21.5%. Frames with a bad TCP, UDP or ICMP checksum,
over IPv4 and IPv6 and including fragmented UDP, are still dropped.

Fixes: c24602ef8664 ("stmmac: support extend descriptors")
Signed-off-by: alex todadze <todadzealexi19@proton.me>
---
 drivers/net/ethernet/stmicro/stmmac/descs.h       |  6 +++++-
 drivers/net/ethernet/stmicro/stmmac/enh_desc.c    | 26 +++++++++++++++++++++--
 drivers/net/ethernet/stmicro/stmmac/hwif.h        |  6 +++---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  9 +++++---
 4 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/descs.h b/drivers/net/ethernet/stmicro/stmmac/descs.h
index 846007cc2..0af819a98 100644
--- a/drivers/net/ethernet/stmicro/stmmac/descs.h
+++ b/drivers/net/ethernet/stmicro/stmmac/descs.h
@@ -116,7 +116,7 @@
 #define	ETDES1_BUFFER2_SIZE_MASK	GENMASK(28, 16)
 
 /* Extended Receive descriptor definitions */
-#define	ERDES4_IP_PAYLOAD_TYPE_MASK	GENMASK(6, 2)
+#define	ERDES4_IP_PAYLOAD_TYPE_MASK	GENMASK(2, 0)
 #define	ERDES4_IP_HDR_ERR		BIT(3)
 #define	ERDES4_IP_PAYLOAD_ERR		BIT(4)
 #define	ERDES4_IP_CSUM_BYPASSED		BIT(5)
@@ -147,6 +147,10 @@
 #define RDES_PTP_SIGNALING		0xa
 #define RDES_PTP_PKT_RESERVED_TYPE	0xf
 
+#define RDES_EXT_PAYLOAD_UDP		0x1
+#define RDES_EXT_PAYLOAD_TCP		0x2
+#define RDES_EXT_PAYLOAD_ICMP		0x3
+
 /* Basic descriptor structure for normal and alternate descriptors */
 struct dma_desc {
 	__le32 des0;
diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
index 051253601..7620246f6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
@@ -109,8 +109,26 @@ static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err)
 	return ret;
 }
 
-static void enh_desc_get_ext_status(struct stmmac_extra_stats *x,
-				    struct dma_extended_desc *p)
+static int enh_desc_ext_rx_csum(u32 rdes0, u32 rdes4, int status)
+{
+	u32 type = FIELD_GET(ERDES4_IP_PAYLOAD_TYPE_MASK, rdes4);
+
+	if (status != csum_none || !(rdes0 & RDES0_FRAME_TYPE) ||
+	    (rdes0 & RDES0_IPC_CSUM_ERROR))
+		return status;
+	if (!(rdes4 & (ERDES4_IPV4_PKT_RCVD | ERDES4_IPV6_PKT_RCVD)) ||
+	    (rdes4 & (ERDES4_IP_HDR_ERR | ERDES4_IP_PAYLOAD_ERR |
+		      ERDES4_IP_CSUM_BYPASSED)))
+		return status;
+	if (type != RDES_EXT_PAYLOAD_UDP && type != RDES_EXT_PAYLOAD_TCP &&
+	    type != RDES_EXT_PAYLOAD_ICMP)
+		return status;
+
+	return good_frame;
+}
+
+static int enh_desc_get_ext_status(struct stmmac_extra_stats *x,
+				   struct dma_extended_desc *p, int status)
 {
 	u32 rdes0 = le32_to_cpu(p->basic.des0);
 	u32 rdes4 = le32_to_cpu(p->des4);
@@ -170,7 +188,11 @@ static void enh_desc_get_ext_status(struct stmmac_extra_stats *x,
 			x->l4_filter_match++;
 		if (rdes4 & ERDES4_L3_L4_FILT_NO_MATCH_MASK)
 			x->l3_l4_filter_no_match++;
+
+		status = enh_desc_ext_rx_csum(rdes0, rdes4, status);
 	}
+
+	return status;
 }
 
 static int enh_desc_get_rx_status(struct stmmac_extra_stats *x,
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 5e2654c91..9925b789b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -70,8 +70,8 @@ struct stmmac_desc_ops {
 	/* Return the reception status looking at the RDES1 */
 	int (*rx_status)(struct stmmac_extra_stats *x,
 			 struct dma_desc *p);
-	void (*rx_extended_status)(struct stmmac_extra_stats *x,
-				   struct dma_extended_desc *p);
+	int (*rx_extended_status)(struct stmmac_extra_stats *x,
+				  struct dma_extended_desc *p, int status);
 	/* Set tx timestamp enable bit */
 	void (*enable_tx_timestamp) (struct dma_desc *p);
 	/* get tx timestamp status */
@@ -128,7 +128,7 @@ struct stmmac_desc_ops {
 #define stmmac_rx_status(__priv, __args...) \
 	stmmac_do_callback(__priv, desc, rx_status, __args)
 #define stmmac_rx_extended_status(__priv, __args...) \
-	stmmac_do_void_callback(__priv, desc, rx_extended_status, __args)
+	stmmac_do_callback(__priv, desc, rx_extended_status, __args)
 #define stmmac_enable_tx_timestamp(__priv, __args...) \
 	stmmac_do_void_callback(__priv, desc, enable_tx_timestamp, __args)
 #define stmmac_get_tx_timestamp_status(__priv, __args...) \
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index b837e8e27..efedd0504 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5605,8 +5605,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 			break;
 
 		if (priv->extend_desc)
-			stmmac_rx_extended_status(priv, &priv->xstats,
-						  rx_q->dma_erx + entry);
+			status = stmmac_rx_extended_status(priv, &priv->xstats,
+							   rx_q->dma_erx + entry,
+							   status);
 		if (unlikely(status == discard_frame)) {
 			xsk_buff_free(buf->xdp);
 			buf->xdp = NULL;
@@ -5788,7 +5789,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		prefetch(np);
 
 		if (priv->extend_desc)
-			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
+			status = stmmac_rx_extended_status(priv, &priv->xstats,
+							   rx_q->dma_erx + entry,
+							   status);
 		if (unlikely(status == discard_frame)) {
 			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
 			buf->page = NULL;

---
base-commit: 191aa41548b2d2b6af534a6d5cf84a552435c74c
change-id: 20261016-stmmac-rx-csum-636a33f857ea

Best regards,
--  
alex todadze <todadzealexi19@proton.me>

Re: [PATCH net] net: stmmac: fix rx checksum offload with extended descriptors

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 3 · Low: 0

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 fix leaves its own stated root cause in place.
- [Medium] enh_desc_ext_rx_csum() keeps csum_none whenever RDES0 bit 7
  (RDES0_IPC_CSUM_ERROR) is set (enh_desc.c:116-118).
- [Medium] In stmmac_rx() and stmmac_rx_zc(), stmmac_rx_status() reads
  des0 for the OWN check, and then enh_desc_get_ext_status() reads…

Pre-existing issues:
- [High] The AF_XDP zero-copy RX path ignores the checksum status from
  the descriptor.

--

net: stmmac: fix rx checksum offload with extended descriptors

This patch changes the extended RX status callback so that it returns a
status. enh_desc_get_ext_status() now uses RDES4 to upgrade csum_none
frames to good_frame when the hardware reports a TCP, UDP or ICMP payload
with no header, payload or bypass error. It also fixes
ERDES4_IP_PAYLOAD_TYPE_MASK, and stmmac_rx() and stmmac_rx_zc() now use
the returned status.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> index 0512536012251..7620246f6a441 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> @@ -109,8 +109,26 @@ static int enh_desc_coe_rdes0(int ipc_err, int type, int payload_err)
>  	return ret;
>  }
>  
> -static void enh_desc_get_ext_status(struct stmmac_extra_stats *x,
> -				    struct dma_extended_desc *p)
> +static int enh_desc_ext_rx_csum(u32 rdes0, u32 rdes4, int status)
> +{
> +	u32 type = FIELD_GET(ERDES4_IP_PAYLOAD_TYPE_MASK, rdes4);
> +
> +	if (status != csum_none || !(rdes0 & RDES0_FRAME_TYPE) ||
> +	    (rdes0 & RDES0_IPC_CSUM_ERROR))
> +		return status;

[Severity: Medium]
The commit message says RDES0 bit 0 is the Extended Status Available flag
when extended descriptors are used. enh_desc_get_rx_status() still passes
that bit to the COE table as payload_err:

    ret = enh_desc_coe_rdes0(!!(rdes0 & RDES0_IPC_CSUM_ERROR),
                             !!(rdes0 & RDES0_FRAME_TYPE),
                             !!(rdes0 & ERDES0_RX_MAC_ADDR));

This early return only corrects the csum_none rows. With RDES0_FRAME_TYPE
clear and ESA set, the same misread produces table entry 0x1. If bit 7 is
also set, for example timestamp available, it produces 0x3. For both,
enh_desc_coe_rdes0() returns discard_frame:

    else if (status == 0x1)
        ret = discard_frame;
    else if (status == 0x3)
        ret = discard_frame;

Can this still drop valid IEEE 802.3 length-field frames (LLC/SNAP, STP
BPDUs) that carry extended status? On discard_frame, stmmac_rx() and
stmmac_rx_zc() release the buffer. When hwts_rx_en is set, rx_errors is
not incremented either.

The tree doesn't show whether the GMAC sets ESA on 802.3 frames, so this
depends on hardware behaviour.

The commit message says "Everything else keeps the status from RDES0".
That seems to include these discarded frames. Would it be more complete
to stop passing ERDES0_RX_MAC_ADDR into enh_desc_coe_rdes0() for extended
descriptors, instead of overriding only the csum_none subset?

[Severity: Medium]
Is RDES0_IPC_CSUM_ERROR the right bit to test here? With extended
descriptors and advanced timestamping, enh_desc_get_rx_timestamp_status()
reads RDES0 bit 7 as Timestamp Available:

    if (ats) {
        struct dma_extended_desc *p = (struct dma_extended_desc *)desc;
        return (le32_to_cpu(p->basic.des0) & RDES0_IPC_CSUM_ERROR) >> 7;

stmmac_init_timestamping() turns on adv_ts in exactly this configuration:

    else if (priv->extend_desc && priv->dma_cap.atime_stamp)
        priv->adv_ts = 1;

With HW RX timestamping enabled (for example HWTSTAMP_FILTER_ALL), would
every timestamped frame take this early return and get its checksum
verified in software again? The ERDES4_IP_HDR_ERR check just below
already covers IP header errors.

> +	if (!(rdes4 & (ERDES4_IPV4_PKT_RCVD | ERDES4_IPV6_PKT_RCVD)) ||
> +	    (rdes4 & (ERDES4_IP_HDR_ERR | ERDES4_IP_PAYLOAD_ERR |
> +		      ERDES4_IP_CSUM_BYPASSED)))
> +		return status;

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b837e8e27a352..efedd050474ff 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5605,8 +5605,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  			break;
>  
>  		if (priv->extend_desc)
> -			stmmac_rx_extended_status(priv, &priv->xstats,
> -						  rx_q->dma_erx + entry);
> +			status = stmmac_rx_extended_status(priv, &priv->xstats,
> +							   rx_q->dma_erx + entry,
> +							   status);
>  		if (unlikely(status == discard_frame)) {

[Severity: High]
This isn't a bug introduced by this patch, but in stmmac_rx_zc() the
refined status stored here is only compared against discard_frame and
rx_not_ls. stmmac_dispatch_skb_zc() takes no status argument. When coe is
set, it marks every IP frame as verified:

    if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
        skb_checksum_none_assert(skb);
    else
        skb->ip_summed = CHECKSUM_UNNECESSARY;

stmmac_rx(), by contrast, also checks status & csum_none.

dwmac4_wrback_get_rx_status() reports RDES1_IP_HDR_ERROR and
RDES1_IP_PAYLOAD_ERROR as csum_none, not discard_frame. enh_desc also
reports payload errors, bypassed checksums and fragments as csum_none.

With an XDP_PASS verdict, can frames with a bad TCP or UDP checksum reach
the stack as CHECKSUM_UNNECESSARY through
stmmac_rx_zc()->stmmac_dispatch_skb_zc()?

The commit message says "Frames with a bad TCP, UDP or ICMP checksum, over
IPv4 and IPv6 and including fragmented UDP, are still dropped". That looks
true only for stmmac_rx().

>  			xsk_buff_free(buf->xdp);
>  			buf->xdp = NULL;
> @@ -5788,7 +5789,9 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		prefetch(np);
>  
>  		if (priv->extend_desc)
> -			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
> +			status = stmmac_rx_extended_status(priv, &priv->xstats,
> +							   rx_q->dma_erx + entry,
> +							   status);

[Severity: Medium]
Is a dma_rmb() needed between the OWN check in stmmac_rx_status() and this
read of the extended status? enh_desc_get_ext_status() reads des4
directly:

    u32 rdes4 = le32_to_cpu(p->des4);

stmmac_tx_clean() has the barrier for the matching TX case:

    /* Make sure descriptor fields are read after reading
     * the own bit.
     */
    dma_rmb();

Before this patch, a stale des4 only skewed the extra_stats counters. Now
enh_desc_ext_rx_csum() can upgrade csum_none to good_frame based on RDES4
alone, and stmmac_rx() then sets CHECKSUM_UNNECESSARY.

enh_desc_set_rx_owner() only sets RDES0_OWN, and nothing clears des4 on
refill. On a weakly ordered CPU, could des4 be loaded before the OWN bit
and return the previous frame's clean TCP status, even though the new
frame's RDES4 reports a payload error?

stmmac_rx_zc() has the same gap.

>  		if (unlikely(status == discard_frame)) {
>  			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
>  			buf->page = NULL;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-stmmac-rx-csum-v1-1-986342ec40df%40proton.me