[PATCH 1/5] qede: fix tunnel checksums offload flags

[PATCH 1/5] qede: fix tunnel checksums offload flags

[edwin.brossette]

From: Didier Pallard <didier.pallard@6wind.com>

In tunnel case, L3 bad checksum is properly setting
RTE_MBUF_F_RX_OUTER_IP_CKSUM_BAD while all other flags are set in inner
part of offload flags, this can cause both L4 flags BAD and GOOD to be set
in inner offloads when a tunnel packet is processed, changing these flags
to RTE_MBUF_F_RX_L4_CKSUM_NONE instead of GOOD/BAD values. This in turn can
cause upper layers to take incorrect decision on what to do with the
packet.

Remove IP_CKSUM_GOOD flag on outer IP layer, since there is currently no
way to indicate that this csum is good using DPDK offload flags.

Fixes: 81f8804992c9 ("net/qede: enhance Rx CPU utilization")
Fixes: 3d4bb4411683 ("net/qede: add fastpath support for VXLAN tunneling")
CC: stable@dpdk.org

Signed-off-by: Edwin Brossette <edwin.brossette@6wind.com>
Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/qede/qede_rxtx.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
index 25e28fd9f61b..c764e3d83763 100644
--- a/drivers/net/qede/qede_rxtx.c
+++ b/drivers/net/qede/qede_rxtx.c
@@ -1617,9 +1617,9 @@ qede_recv_pkts_regular(void *p_rxq, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 					    "L4 csum failed, flags = 0x%x",
 					    parse_flag);
 				rxq->rx_hw_errors++;
-				ol_flags |= RTE_MBUF_F_RX_L4_CKSUM_BAD;
+				ol_flags |= RTE_MBUF_F_RX_OUTER_L4_CKSUM_BAD;
 			} else {
-				ol_flags |= RTE_MBUF_F_RX_L4_CKSUM_GOOD;
+				ol_flags |= RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD;
 			}
 
 			if (unlikely(qede_check_tunn_csum_l3(parse_flag))) {
@@ -1628,8 +1628,6 @@ qede_recv_pkts_regular(void *p_rxq, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 					parse_flag);
 				rxq->rx_hw_errors++;
 				ol_flags |= RTE_MBUF_F_RX_OUTER_IP_CKSUM_BAD;
-			} else {
-				ol_flags |= RTE_MBUF_F_RX_IP_CKSUM_GOOD;
 			}
 
 			flags = fp_cqe->tunnel_pars_flags.flags;
@@ -1887,9 +1885,9 @@ qede_recv_pkts(void *p_rxq, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 					    "L4 csum failed, flags = 0x%x",
 					    parse_flag);
 				rxq->rx_hw_errors++;
-				ol_flags |= RTE_MBUF_F_RX_L4_CKSUM_BAD;
+				ol_flags |= RTE_MBUF_F_RX_OUTER_L4_CKSUM_BAD;
 			} else {
-				ol_flags |= RTE_MBUF_F_RX_L4_CKSUM_GOOD;
+				ol_flags |= RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD;
 			}
 
 			if (unlikely(qede_check_tunn_csum_l3(parse_flag))) {
@@ -1898,8 +1896,6 @@ qede_recv_pkts(void *p_rxq, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 					parse_flag);
 				  rxq->rx_hw_errors++;
 				ol_flags |= RTE_MBUF_F_RX_OUTER_IP_CKSUM_BAD;
-			} else {
-				ol_flags |= RTE_MBUF_F_RX_IP_CKSUM_GOOD;
 			}
 
 			if (tpa_start_flg)
-- 
2.35.0.4.g44a5d4affccf

[PATCH 2/5] net/qede: fix bad sanity check on Rx queue release

[edwin.brossette]

From: Edwin Brossette <edwin.brossette@6wind.com>

As per the rte_mbuf API: the driver is responsible of initializing all
the required fields. This is not done at qede alloc, meaning there can
be garbage data in mbufs memory, although this garbage data should be
overwritten when the mbufs are used. Since a sanity check is done when
freeing the queues, its possible some remaining garbage data causes a
panic when trying to release the queues if some mbufs are being
processed.

Use rte_pktmbuf_raw_free() instead of rte_pktmbuf_free() as the sanity
check is more relaxed.

Fixes: 2ea6f76aff40 ("qede: add core driver")
CC: stable@dpdk.org

Signed-off-by: Edwin Brossette <edwin.brossette@6wind.com>
Acked-by: Didier Pallard <didier.pallard@6wind.com>
---
 drivers/net/qede/qede_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
index c764e3d83763..601fcb30b357 100644
--- a/drivers/net/qede/qede_rxtx.c
+++ b/drivers/net/qede/qede_rxtx.c
@@ -305,7 +305,7 @@ static void qede_rx_queue_release_mbufs(struct qede_rx_queue *rxq)
 	if (rxq->sw_rx_ring) {
 		for (i = 0; i < rxq->nb_rx_desc; i++) {
 			if (rxq->sw_rx_ring[i]) {
-				rte_pktmbuf_free(rxq->sw_rx_ring[i]);
+				rte_mbuf_raw_free(rxq->sw_rx_ring[i]);
 				rxq->sw_rx_ring[i] = NULL;
 			}
 		}
-- 
2.35.0.4.g44a5d4affccf

[PATCH 3/5] Revert "net/qede: fix maximum Rx packet length"

[edwin.brossette]

From: Edwin Brossette <edwin.brossette@6wind.com>

This reverts commit d8ded501e05ce879f27f0ed1df7721a88b737e25.

The maximum length for Rx packets computed in qede_rx_queue_setup()
takes Ethernet CRC into account. This is not consistent with the value
computed in qede_set_mtu(). RTE_ETHER_CRC_LEN should not be added to
max_rx_pktlen, as HW does not include CRC in received frames passed to
host.

The original commit tries to fix another bug with this inappropriate
patch: packets with size nearing MTU limit are being dropped. This is
not because CRC length is not being accounted for in Rx buff size, but
because of the flooring applied to it: the rx_buff size computed is
lower than expected because we try to align it.

This issue will be fixed in the following patch.

CC: stable@dpdk.org

Signed-off-by: Edwin Brossette <edwin.brossette@6wind.com>
Acked-by: Didier Pallard <didier.pallard@6wind.com>
---
 drivers/net/qede/qede_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
index 601fcb30b357..fe839a6ba844 100644
--- a/drivers/net/qede/qede_rxtx.c
+++ b/drivers/net/qede/qede_rxtx.c
@@ -235,7 +235,7 @@ qede_rx_queue_setup(struct rte_eth_dev *dev, uint16_t qid,
 		dev->data->rx_queues[qid] = NULL;
 	}
 
-	max_rx_pktlen = dev->data->mtu + RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
+	max_rx_pktlen = dev->data->mtu + RTE_ETHER_HDR_LEN;
 
 	/* Fix up RX buffer size */
 	bufsz = (uint16_t)rte_pktmbuf_data_room_size(mp) - RTE_PKTMBUF_HEADROOM;
-- 
2.35.0.4.g44a5d4affccf

[PATCH 4/5] net/qede: fix QEDE_ETH_OVERHEAD being counted twice in rx_buf_size

[edwin.brossette]

From: Edwin Brossette <edwin.brossette@6wind.com>

rx_buf_size is computed at 2 different places: in qede_rx_queue_setup()
and in qede_set_mtu().

In qede_rx_queue_setup(), it is initialized with mtu + RTE_ETHER_HDR_LEN
and QEDE_ETH_OVERHEAD is added to it in qede_calc_rx_buf_size().

In qede_set_mtu(), it is initialized with
mtu + RTE_ETHER_HDR_LEN + QEDE_ETH_OVERHEAD and QEDE_ETH_OVERHEAD is
added to it a second time in qede_calc_rx_buf_size().

This is inconsistent and wrong in the case of qede_set_mtu(). Initialize
this variable with mtu + QEDE_MAX_ETHER_HDR_LEN instead and stop adding
+ QEDE_ETH_OVERHEAD over and over again. This will factorize the code.

Fixes: 318d7da3122b ("net/qede: fix Rx buffer size calculation")
CC: stable@dpdk.org

Signed-off-by: Edwin Brossette <edwin.brossette@6wind.com>
Acked-by: Didier Pallard <didier.pallard@6wind.com>
---
 drivers/net/qede/qede_rxtx.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
index fe839a6ba844..ea77f09e18be 100644
--- a/drivers/net/qede/qede_rxtx.c
+++ b/drivers/net/qede/qede_rxtx.c
@@ -101,19 +101,14 @@ qede_calc_rx_buf_size(struct rte_eth_dev *dev, uint16_t mbufsz,
 		 * buffers can be used for single packet. So need to make sure
 		 * mbuf size is sufficient enough for this.
 		 */
-		if ((mbufsz * ETH_RX_MAX_BUFF_PER_PKT) <
-		     (max_frame_size + QEDE_ETH_OVERHEAD)) {
+		if ((mbufsz * ETH_RX_MAX_BUFF_PER_PKT) < max_frame_size) {
 			DP_ERR(edev, "mbuf %d size is not enough to hold max fragments (%d) for max rx packet length (%d)\n",
 			       mbufsz, ETH_RX_MAX_BUFF_PER_PKT, max_frame_size);
 			return -EINVAL;
 		}
-
-		rx_buf_size = RTE_MAX(mbufsz,
-				      (max_frame_size + QEDE_ETH_OVERHEAD) /
-				       ETH_RX_MAX_BUFF_PER_PKT);
-	} else {
-		rx_buf_size = max_frame_size + QEDE_ETH_OVERHEAD;
-	}
+		rx_buf_size = mbufsz;
+	} else
+		rx_buf_size = max_frame_size;
 
 	/* Align to cache-line size if needed */
 	return QEDE_FLOOR_TO_CACHE_LINE_SIZE(rx_buf_size);
@@ -235,14 +230,14 @@ qede_rx_queue_setup(struct rte_eth_dev *dev, uint16_t qid,
 		dev->data->rx_queues[qid] = NULL;
 	}
 
-	max_rx_pktlen = dev->data->mtu + RTE_ETHER_HDR_LEN;
+	max_rx_pktlen = dev->data->mtu + QEDE_MAX_ETHER_HDR_LEN;
 
 	/* Fix up RX buffer size */
 	bufsz = (uint16_t)rte_pktmbuf_data_room_size(mp) - RTE_PKTMBUF_HEADROOM;
 	/* cache align the mbuf size to simplify rx_buf_size calculation */
 	bufsz = QEDE_FLOOR_TO_CACHE_LINE_SIZE(bufsz);
 	if ((rxmode->offloads & RTE_ETH_RX_OFFLOAD_SCATTER)	||
-	    (max_rx_pktlen + QEDE_ETH_OVERHEAD) > bufsz) {
+	    max_rx_pktlen > bufsz) {
 		if (!dev->data->scattered_rx) {
 			DP_INFO(edev, "Forcing scatter-gather mode\n");
 			dev->data->scattered_rx = 1;
-- 
2.35.0.4.g44a5d4affccf

[PATCH 5/5] net/qede: fix rx_buf_size calculation

[edwin.brossette]

From: Edwin Brossette <edwin.brossette@6wind.com>

When the MTU configured is lower than maximum mbuf size (all packet data
can be stored in a single mbuf), then rx buffer size is configured with
MTU + some overhead. A flooring is applied to this value to align it,
meaning its actual value is going to be lower than expected.

This is a considerable design flaw, because a data packet fitting
exactly the MTU might not fit in the rx buffer. What is observed in this
case is that the nic splits the datagram in two segments, essentially
doing Rx scatter. However, the qede pmd does not expect gather scatter
in this case and does not use the required function: it uses
qede_recv_pkts_regular() which is not capable of handling this case.

Thus, we end up with malformed packet with m->nb_segs = 2 and
m->next = NULL. This means the last part of the data is missing.

CEIL max_rx_pktlen instead of FLOORing it. Also change the check on
max_rx_pktlen > bufsz in qede_rx_queue_setup(): if this ceiling would
make max_rx_pktlen exceed mbuf size, then force enable scatter-gather
mode.

Fixes: 318d7da3122b ("net/qede: fix Rx buffer size calculation")
CC: stable@dpdk.org

Signed-off-by: Edwin Brossette <edwin.brossette@6wind.com>
Acked-by: Didier Pallard <didier.pallard@6wind.com>
---
 drivers/net/qede/qede_rxtx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
index ea77f09e18be..2a6f1290ad4a 100644
--- a/drivers/net/qede/qede_rxtx.c
+++ b/drivers/net/qede/qede_rxtx.c
@@ -111,7 +111,7 @@ qede_calc_rx_buf_size(struct rte_eth_dev *dev, uint16_t mbufsz,
 		rx_buf_size = max_frame_size;
 
 	/* Align to cache-line size if needed */
-	return QEDE_FLOOR_TO_CACHE_LINE_SIZE(rx_buf_size);
+	return QEDE_CEIL_TO_CACHE_LINE_SIZE(rx_buf_size);
 }
 
 static struct qede_rx_queue *
@@ -237,7 +237,7 @@ qede_rx_queue_setup(struct rte_eth_dev *dev, uint16_t qid,
 	/* cache align the mbuf size to simplify rx_buf_size calculation */
 	bufsz = QEDE_FLOOR_TO_CACHE_LINE_SIZE(bufsz);
 	if ((rxmode->offloads & RTE_ETH_RX_OFFLOAD_SCATTER)	||
-	    max_rx_pktlen > bufsz) {
+	    QEDE_CEIL_TO_CACHE_LINE_SIZE(max_rx_pktlen) > bufsz) {
 		if (!dev->data->scattered_rx) {
 			DP_INFO(edev, "Forcing scatter-gather mode\n");
 			dev->data->scattered_rx = 1;
-- 
2.35.0.4.g44a5d4affccf

Re: [PATCH 2/5] net/qede: fix bad sanity check on Rx queue release

[Stephen Hemminger]

On Tue, 22 Apr 2025 17:51:40 +0200
edwin.brossette@6wind.com wrote:

> From: Edwin Brossette <edwin.brossette@6wind.com>
> 
> As per the rte_mbuf API: the driver is responsible of initializing all
> the required fields. This is not done at qede alloc, meaning there can
> be garbage data in mbufs memory, although this garbage data should be
> overwritten when the mbufs are used. Since a sanity check is done when
> freeing the queues, its possible some remaining garbage data causes a
> panic when trying to release the queues if some mbufs are being
> processed.
> 
> Use rte_pktmbuf_raw_free() instead of rte_pktmbuf_free() as the sanity
> check is more relaxed.
> 
> Fixes: 2ea6f76aff40 ("qede: add core driver")
> CC: stable@dpdk.org

Patch looks fine, but DPDK is trying to follow the inclusive naming
guidelines. The term "sanity check" is on the not allowed list.

I will reword the commit message.

https://inclusivenaming.org/word-lists/tier-2/sanity-check/