Skip to contentSkip to conversation

net/ice cast rx_flex_ring to proper type

PATCH In review Omar Munchaev 2 participants 2 messages Sep 25 Network drivers

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 net/ice cast rx_flex_ring to proper type

its look like, that some parts of fdir code was not updated with new generic ci_rx_flex_desc. This patch fixes it
Also im found out more verbose logs for ice_check_fdir_programming_status in linux driver implementation in function ice_vf_verify_rx_desc:
https://github.com/intel/ethernet-linux-ice/blob/e4a92b1e84e084bc599580d065020986e501b8ad/src/ice_virtchnl_fdir.c#L3513

---
 drivers/net/intel/ice/ice_rxtx.c | 46 +++++++++++++-------------------
 1 file changed, 19 insertions(+), 27 deletions(-)
drivers/net/intel/ice/ice_rxtx.c +19 -27
index b333444cbf..5ab58c8575 100644
@@ -2698,7 +2698,7 @@ ice_fdir_setup_rx_resources(struct ice_pf *pf)
 	}
 
 	/* Allocate RX hardware ring descriptors. */
-	ring_size = sizeof(union ice_32byte_rx_desc) * ICE_FDIR_NUM_RX_DESC;
+	ring_size = sizeof(union ci_rx_flex_desc) * ICE_FDIR_NUM_RX_DESC;
 	ring_size = RTE_ALIGN(ring_size, ICE_DMA_MEM_ALIGN);
 
 	rz = rte_eth_dma_zone_reserve(dev, "fdir_rx_ring",
@@ -2718,7 +2718,7 @@ ice_fdir_setup_rx_resources(struct ice_pf *pf)
 
 	rxq->rx_ring_phys_addr = rz->iova;
 	memset(rz->addr, 0, ICE_FDIR_NUM_RX_DESC *
-	       sizeof(union ice_32byte_rx_desc));
+	       sizeof(union ci_rx_flex_desc));
 	rxq->rx_flex_ring = (union ci_rx_flex_desc *)rz->addr;
 
 	/*
@@ -4464,40 +4464,33 @@ ice_set_default_ptype_table(struct rte_eth_dev *dev)
 static inline int
 ice_check_fdir_programming_status(struct ci_rx_queue *rxq)
 {
-	volatile union ice_32byte_rx_desc *rxdp;
-	uint64_t qword1;
-	uint32_t rx_status;
-	uint32_t error;
-	uint32_t id;
+	volatile union ci_rx_flex_desc *rxdp;
+	uint32_t stat_err, error, id;
 	int ret = -EAGAIN;
 
-	rxdp = (volatile union ice_32byte_rx_desc *)&rxq->rx_flex_ring[rxq->rx_tail];
-	qword1 = rte_le_to_cpu_64(rxdp->wb.qword1.status_error_len);
-	rx_status = (qword1 & ICE_RXD_QW1_STATUS_M)
-			>> ICE_RXD_QW1_STATUS_S;
+	rxdp = &rxq->rx_flex_ring[rxq->rx_tail];
+	stat_err = rte_le_to_cpu_16(rxdp->wb.status_error0);
 
-	if (rx_status & (1 << ICE_RX_DESC_STATUS_DD_S)) {
+	if (FIELD_GET(ICE_FXD_FLTR_WB_QW1_DD_M, stat_err) == ICE_FXD_FLTR_WB_QW1_DD_YES) {
 		ret = 0;
-		error = (qword1 & ICE_RX_PROG_STATUS_DESC_WB_QW1_FAIL_M) >>
-			ICE_RX_PROG_STATUS_DESC_WB_QW1_FAIL_S;
-		id = (qword1 & ICE_RX_PROG_STATUS_DESC_WB_QW1_PROGID_M) >>
-			ICE_RX_PROG_STATUS_DESC_WB_QW1_PROGID_S;
-		if (error) {
-			if (id == ICE_RX_PROG_STATUS_DESC_WB_QW1_PROG_ADD)
-				PMD_DRV_LOG(ERR, "Failed to add FDIR rule.");
-			else if (id == ICE_RX_PROG_STATUS_DESC_WB_QW1_PROG_DEL)
-				PMD_DRV_LOG(ERR, "Failed to remove FDIR rule.");
+		id = FIELD_GET(ICE_FXD_FLTR_WB_QW1_PROG_ID_M, stat_err);
+		error = FIELD_GET(ICE_FXD_FLTR_WB_QW1_FAIL_M, stat_err);
+		if (error == ICE_FXD_FLTR_WB_QW1_FAIL_YES) {
+			if (id == ICE_FXD_FLTR_WB_QW1_PROG_ADD)
+				PMD_DRV_LOG(ERR, "Failed to add FDIR rule due to no space in the table");
+			else
+				PMD_DRV_LOG(ERR, "Failed to remove FDIR rule, attempt to remove non-existent entry");
 			ret = -EINVAL;
 			goto err;
 		}
-		error = (qword1 & ICE_RX_PROG_STATUS_DESC_WB_QW1_FAIL_PROF_M) >>
-			ICE_RX_PROG_STATUS_DESC_WB_QW1_FAIL_PROF_S;
-		if (error) {
-			PMD_DRV_LOG(ERR, "Failed to create FDIR profile.");
+		error = FIELD_GET(ICE_FXD_FLTR_WB_QW1_FAIL_PROF_M, stat_err);
+		if (error == ICE_FXD_FLTR_WB_QW1_FAIL_PROF_YES) {
+			PMD_DRV_LOG(ERR, "Profile matching error");
 			ret = -EINVAL;
+			goto err;
 		}
 err:
-		rxdp->wb.qword1.status_error_len = 0;
+		rxdp->wb.status_error0 = 0;
 		rxq->rx_tail++;
 		if (unlikely(rxq->rx_tail == rxq->nb_rx_desc))
 			rxq->rx_tail = 0;
@@ -4506,7 +4499,6 @@ ice_check_fdir_programming_status(struct ci_rx_queue *rxq)
 		else
 			ICE_PCI_REG_WRITE(rxq->qrx_tail, rxq->rx_tail - 1);
 	}
-
 	return ret;
 }
 
Signature · 1 line
2.54.0
#1 lore raw
Quoted 8 lines · Omar Munchaev

its look like, that some parts of fdir code was not updated with new generic ci_rx_flex_desc. This patch fixes it
Also im found out more verbose logs for ice_check_fdir_programming_status in linux driver implementation in function ice_vf_verify_rx_desc:
https://github.com/intel/ethernet-linux-ice/blob/e4a92b1e84e084bc599580d065020986e501b8ad/src/ice_virtchnl_fdir.c#L3513

---

Hi Omar,

This looks like two commits jammed together into one. I would split up the ci_rx_flex_desc changes from the rest of the fix (the logging changes are technically separate but IMO they can go in with the macro changes). The flex desc changes in particular would probably also require a Fixes: tag and a Cc: stable if the source commit is old enough to be in stable releases.

For the macro changes, DPDK is a BSD-3 clause licensed project, so technically we cannot use kernel code as it's GPL-licensed. Please do not refer to kernel implementation directly.

I do however think it's worth rewriting the Rx status etc code with FIELD_GET macros the way you did as that makes the code a lot clearer, and better detailed logging is certainly appreciated.

This patch would also benefit from better commit message. Suggested rewording:

[patch 1: flex descriptor changes]

net/ice: fix missing type casts for flex descriptors

When ice driver was ported to use the common Rx descriptor format, some type casts were missed. Add them.

Fixes <sha of commit that introduced the issue>
Cc [email protected] [possibly - double check if this is needed]

---

[patch 2: macro/logging changes]

net/ice: use macros to get field values

Currently, in ice_check_fdir_programming_status, descriptor fields are accessed through direct bit mask manipulation and shifts. The ice driver has macros for this, so rewrite the function to use FIELD_GET macros instead.

Signature · 2 lines
Thanks,
Anatoly