diff mbox series

[iwl-next,v10,12/14] iavf: Implement checking DD desc field

Message ID 20240821121539.374343-13-wojciech.drewek@intel.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series Add support for Rx timestamping for both ice and iavf drivers | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Wojciech Drewek Aug. 21, 2024, 12:15 p.m. UTC 
From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>

Rx timestamping introduced in PF driver caused the need of refactoring
the VF driver mechanism to check packet fields.

The function to check errors in descriptor has been removed and from
now only previously set struct fields are being checked. The field DD
(descriptor done) needs to be checked at the very beginning, before
extracting other fields.

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_txrx.c | 90 +++++++++++++++------
 drivers/net/ethernet/intel/iavf/iavf_txrx.h | 16 ----
 2 files changed, 65 insertions(+), 41 deletions(-)

Comments

Alexander Lobakin Aug. 22, 2024, 4:46 p.m. UTC | #1 
From: Wojciech Drewek <wojciech.drewek@intel.com>
Date: Wed, 21 Aug 2024 14:15:37 +0200

> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
> 
> Rx timestamping introduced in PF driver caused the need of refactoring
> the VF driver mechanism to check packet fields.

[...]

> +static bool iavf_is_descriptor_done(struct iavf_ring *rx_ring,

const

> +				    struct iavf_rx_desc *rx_desc)

Just pass qw1 here directly instead of @rx_desc.

> +{
> +	__le64 qw1 = rx_desc->qw1;
> +
> +	if (rx_ring->rxdid == VIRTCHNL_RXDID_1_32B_BASE)

I would also pass `bool flex` here instead of @rx_ring.
At the beginning of iavf_clean_rx_irq(), do

	bool flex = rx_ring->rxdid == RXDID_FLEX

and then pass this @flex everywhere instead of checking for rx_ring->rxdid.

iavf_is_descriptor_done(u64 qw1, bool flex)
{
	if (flex)
		return le64_get_bits(qw1, FLEX_DD);
	else
		return le64_get_bits(qw1, LEGACY_DD);
}

That's it.

> +		return !!le64_get_bits(qw1, IAVF_RXD_LEGACY_DD_M);
> +
> +	return !!le64_get_bits(qw1, IAVF_RXD_FLEX_DD_M);
> +}
> +
>  static __le64 build_ctob(u32 td_cmd, u32 td_offset, unsigned int size,
>  			 u32 td_tag)
>  {
> @@ -1223,24 +1245,31 @@ iavf_extract_legacy_rx_fields(const struct iavf_ring *rx_ring,
>  			      const struct iavf_rx_desc *rx_desc, u32 *ptype)
>  {
>  	struct libeth_rqe_info fields = {};
> -	u64 qw0 = le64_to_cpu(rx_desc->qw0);
>  	u64 qw1 = le64_to_cpu(rx_desc->qw1);
> -	u64 qw2 = le64_to_cpu(rx_desc->qw2);
> -	bool l2tag1p;
> -	bool l2tag2p;
>  
> -	fields.eop = FIELD_GET(IAVF_RXD_LEGACY_EOP_M, qw1);
>  	fields.len = FIELD_GET(IAVF_RXD_LEGACY_LENGTH_M, qw1);
> -	fields.rxe = FIELD_GET(IAVF_RXD_LEGACY_RXE_M, qw1);
> -	*ptype = FIELD_GET(IAVF_RXD_LEGACY_PTYPE_M, qw1);
> -
> -	l2tag1p = FIELD_GET(IAVF_RXD_LEGACY_L2TAG1P_M, qw1);
> -	if (l2tag1p && (rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1))
> -		fields.vlan_tag = FIELD_GET(IAVF_RXD_LEGACY_L2TAG1_M, qw0);
> +	fields.eop = FIELD_GET(IAVF_RXD_LEGACY_EOP_M, qw1);
>  
> -	l2tag2p = FIELD_GET(IAVF_RXD_LEGACY_L2TAG2P_M, qw2);
> -	if (l2tag2p && (rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2))
> -		fields.vlan_tag = FIELD_GET(IAVF_RXD_LEGACY_L2TAG2_M, qw2);
> +	if (fields.eop) {
> +		bool l2tag1p, l2tag2p;
> +		u64 qw0 = le64_to_cpu(rx_desc->qw0);
> +		u64 qw2 = le64_to_cpu(rx_desc->qw2);
> +
> +		fields.rxe = FIELD_GET(IAVF_RXD_LEGACY_RXE_M, qw1);
> +		*ptype = FIELD_GET(IAVF_RXD_LEGACY_PTYPE_M, qw1);
> +
> +		l2tag1p = FIELD_GET(IAVF_RXD_LEGACY_L2TAG1P_M, qw1);
> +		if (l2tag1p &&
> +		    (rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1))
> +			fields.vlan_tag = FIELD_GET(IAVF_RXD_LEGACY_L2TAG1_M,
> +						    qw0);
> +
> +		l2tag2p = FIELD_GET(IAVF_RXD_LEGACY_L2TAG2P_M, qw2);
> +		if (l2tag2p &&
> +		    (rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2))
> +			fields.vlan_tag = FIELD_GET(IAVF_RXD_LEGACY_L2TAG2_M,
> +						    qw2);
> +	}

Just do that right where you introduce this function, this change is not
related to this patch.

Also, `if (!fields.eop) return fields` instead of this big if.

>  
>  	return fields;
>  }
> @@ -1266,21 +1295,29 @@ iavf_extract_flex_rx_fields(const struct iavf_ring *rx_ring,
>  	struct libeth_rqe_info fields = {};
>  	u64 qw0 = le64_to_cpu(rx_desc->qw0);
>  	u64 qw1 = le64_to_cpu(rx_desc->qw1);
> -	u64 qw2 = le64_to_cpu(rx_desc->qw2);
> -	bool l2tag1p, l2tag2p;
>  
>  	fields.len = FIELD_GET(IAVF_RXD_FLEX_PKT_LEN_M, qw0);
> -	fields.rxe = FIELD_GET(IAVF_RXD_FLEX_RXE_M, qw1);
>  	fields.eop = FIELD_GET(IAVF_RXD_FLEX_EOP_M, qw1);
> -	*ptype = FIELD_GET(IAVF_RXD_FLEX_PTYPE_M, qw0);
>  
> -	l2tag1p = FIELD_GET(IAVF_RXD_FLEX_L2TAG1P_M, qw1);
> -	if (l2tag1p && (rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1))
> -		fields.vlan_tag = FIELD_GET(IAVF_RXD_FLEX_L2TAG1_M, qw1);
> +	if (fields.len) {
> +		bool l2tag1p, l2tag2p;
> +		u64 qw2 = le64_to_cpu(rx_desc->qw2);
> +
> +		fields.rxe = FIELD_GET(IAVF_RXD_FLEX_RXE_M, qw1);
> +		*ptype = FIELD_GET(IAVF_RXD_FLEX_PTYPE_M, qw0);
>  
> -	l2tag2p = FIELD_GET(IAVF_RXD_FLEX_L2TAG2P_M, qw2);
> -	if (l2tag2p && (rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2))
> -		fields.vlan_tag = FIELD_GET(IAVF_RXD_FLEX_L2TAG2_2_M, qw2);
> +		l2tag1p = FIELD_GET(IAVF_RXD_FLEX_L2TAG1P_M, qw1);
> +		if (l2tag1p &&
> +		    (rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1))
> +			fields.vlan_tag = FIELD_GET(IAVF_RXD_FLEX_L2TAG1_M,
> +						    qw1);
> +
> +		l2tag2p = FIELD_GET(IAVF_RXD_FLEX_L2TAG2P_M, qw2);
> +		if (l2tag2p &&
> +		    (rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2))
> +			fields.vlan_tag = FIELD_GET(IAVF_RXD_FLEX_L2TAG2_2_M,
> +						    qw2);
> +	}

Same.

>  
>  	return fields;
>  }
> @@ -1335,7 +1372,10 @@ static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget)
>  		 */
>  		dma_rmb();
>  
> -		if (!iavf_test_staterr(rx_desc, IAVF_RXD_FLEX_DD_M))
> +		/* If DD field (descriptor done) is unset then other fields are
> +		 * not valid
> +		 */
> +		if (!iavf_is_descriptor_done(rx_ring, rx_desc))
>  			break;
>  
>  		fields = iavf_extract_rx_fields(rx_ring, rx_desc, &ptype);

Thanks,
Olek
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
index cbcd6c7e675e..8f529cfaf7a8 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
@@ -9,6 +9,28 @@ 
 #include "iavf_trace.h"
 #include "iavf_prototype.h"
 
+/**
+ * iavf_is_descriptor_done - tests DD bit in Rx descriptor
+ * @rx_ring: the ring parameter to distinguish descriptor type (flex/legacy)
+ * @rx_desc: pointer to receive descriptor
+ *
+ * This function tests the descriptor done bit in specified descriptor. Because
+ * there are two types of descriptors (legacy and flex) the parameter rx_ring
+ * is used to distinguish.
+ *
+ * Return: true or false based on the state of DD bit in Rx descriptor.
+ */
+static bool iavf_is_descriptor_done(struct iavf_ring *rx_ring,
+				    struct iavf_rx_desc *rx_desc)
+{
+	__le64 qw1 = rx_desc->qw1;
+
+	if (rx_ring->rxdid == VIRTCHNL_RXDID_1_32B_BASE)
+		return !!le64_get_bits(qw1, IAVF_RXD_LEGACY_DD_M);
+
+	return !!le64_get_bits(qw1, IAVF_RXD_FLEX_DD_M);
+}
+
 static __le64 build_ctob(u32 td_cmd, u32 td_offset, unsigned int size,
 			 u32 td_tag)
 {
@@ -1223,24 +1245,31 @@  iavf_extract_legacy_rx_fields(const struct iavf_ring *rx_ring,
 			      const struct iavf_rx_desc *rx_desc, u32 *ptype)
 {
 	struct libeth_rqe_info fields = {};
-	u64 qw0 = le64_to_cpu(rx_desc->qw0);
 	u64 qw1 = le64_to_cpu(rx_desc->qw1);
-	u64 qw2 = le64_to_cpu(rx_desc->qw2);
-	bool l2tag1p;
-	bool l2tag2p;
 
-	fields.eop = FIELD_GET(IAVF_RXD_LEGACY_EOP_M, qw1);
 	fields.len = FIELD_GET(IAVF_RXD_LEGACY_LENGTH_M, qw1);
-	fields.rxe = FIELD_GET(IAVF_RXD_LEGACY_RXE_M, qw1);
-	*ptype = FIELD_GET(IAVF_RXD_LEGACY_PTYPE_M, qw1);
-
-	l2tag1p = FIELD_GET(IAVF_RXD_LEGACY_L2TAG1P_M, qw1);
-	if (l2tag1p && (rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1))
-		fields.vlan_tag = FIELD_GET(IAVF_RXD_LEGACY_L2TAG1_M, qw0);
+	fields.eop = FIELD_GET(IAVF_RXD_LEGACY_EOP_M, qw1);
 
-	l2tag2p = FIELD_GET(IAVF_RXD_LEGACY_L2TAG2P_M, qw2);
-	if (l2tag2p && (rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2))
-		fields.vlan_tag = FIELD_GET(IAVF_RXD_LEGACY_L2TAG2_M, qw2);
+	if (fields.eop) {
+		bool l2tag1p, l2tag2p;
+		u64 qw0 = le64_to_cpu(rx_desc->qw0);
+		u64 qw2 = le64_to_cpu(rx_desc->qw2);
+
+		fields.rxe = FIELD_GET(IAVF_RXD_LEGACY_RXE_M, qw1);
+		*ptype = FIELD_GET(IAVF_RXD_LEGACY_PTYPE_M, qw1);
+
+		l2tag1p = FIELD_GET(IAVF_RXD_LEGACY_L2TAG1P_M, qw1);
+		if (l2tag1p &&
+		    (rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1))
+			fields.vlan_tag = FIELD_GET(IAVF_RXD_LEGACY_L2TAG1_M,
+						    qw0);
+
+		l2tag2p = FIELD_GET(IAVF_RXD_LEGACY_L2TAG2P_M, qw2);
+		if (l2tag2p &&
+		    (rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2))
+			fields.vlan_tag = FIELD_GET(IAVF_RXD_LEGACY_L2TAG2_M,
+						    qw2);
+	}
 
 	return fields;
 }
@@ -1266,21 +1295,29 @@  iavf_extract_flex_rx_fields(const struct iavf_ring *rx_ring,
 	struct libeth_rqe_info fields = {};
 	u64 qw0 = le64_to_cpu(rx_desc->qw0);
 	u64 qw1 = le64_to_cpu(rx_desc->qw1);
-	u64 qw2 = le64_to_cpu(rx_desc->qw2);
-	bool l2tag1p, l2tag2p;
 
 	fields.len = FIELD_GET(IAVF_RXD_FLEX_PKT_LEN_M, qw0);
-	fields.rxe = FIELD_GET(IAVF_RXD_FLEX_RXE_M, qw1);
 	fields.eop = FIELD_GET(IAVF_RXD_FLEX_EOP_M, qw1);
-	*ptype = FIELD_GET(IAVF_RXD_FLEX_PTYPE_M, qw0);
 
-	l2tag1p = FIELD_GET(IAVF_RXD_FLEX_L2TAG1P_M, qw1);
-	if (l2tag1p && (rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1))
-		fields.vlan_tag = FIELD_GET(IAVF_RXD_FLEX_L2TAG1_M, qw1);
+	if (fields.len) {
+		bool l2tag1p, l2tag2p;
+		u64 qw2 = le64_to_cpu(rx_desc->qw2);
+
+		fields.rxe = FIELD_GET(IAVF_RXD_FLEX_RXE_M, qw1);
+		*ptype = FIELD_GET(IAVF_RXD_FLEX_PTYPE_M, qw0);
 
-	l2tag2p = FIELD_GET(IAVF_RXD_FLEX_L2TAG2P_M, qw2);
-	if (l2tag2p && (rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2))
-		fields.vlan_tag = FIELD_GET(IAVF_RXD_FLEX_L2TAG2_2_M, qw2);
+		l2tag1p = FIELD_GET(IAVF_RXD_FLEX_L2TAG1P_M, qw1);
+		if (l2tag1p &&
+		    (rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1))
+			fields.vlan_tag = FIELD_GET(IAVF_RXD_FLEX_L2TAG1_M,
+						    qw1);
+
+		l2tag2p = FIELD_GET(IAVF_RXD_FLEX_L2TAG2P_M, qw2);
+		if (l2tag2p &&
+		    (rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2))
+			fields.vlan_tag = FIELD_GET(IAVF_RXD_FLEX_L2TAG2_2_M,
+						    qw2);
+	}
 
 	return fields;
 }
@@ -1335,7 +1372,10 @@  static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget)
 		 */
 		dma_rmb();
 
-		if (!iavf_test_staterr(rx_desc, IAVF_RXD_FLEX_DD_M))
+		/* If DD field (descriptor done) is unset then other fields are
+		 * not valid
+		 */
+		if (!iavf_is_descriptor_done(rx_ring, rx_desc))
 			break;
 
 		fields = iavf_extract_rx_fields(rx_ring, rx_desc, &ptype);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.h b/drivers/net/ethernet/intel/iavf/iavf_txrx.h
index 59e7a58bc0f7..b72741f11338 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_txrx.h
+++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.h
@@ -80,22 +80,6 @@  enum iavf_dyn_idx_t {
 	BIT_ULL(IAVF_FILTER_PCTYPE_NONF_UNICAST_IPV6_UDP) | \
 	BIT_ULL(IAVF_FILTER_PCTYPE_NONF_MULTICAST_IPV6_UDP))
 
-/**
- * iavf_test_staterr - tests bits in Rx descriptor status and error fields
- * @rx_desc: pointer to receive descriptor (in le64 format)
- * @stat_err_bits: value to mask
- *
- * This function does some fast chicanery in order to return the
- * value of the mask which is really only used for boolean tests.
- * The status_error_len doesn't need to be shifted because it begins
- * at offset zero.
- */
-static inline bool iavf_test_staterr(struct iavf_rx_desc *rx_desc,
-				     const u64 stat_err_bits)
-{
-	return !!(rx_desc->qw1 & cpu_to_le64(stat_err_bits));
-}
-
 /* How many Rx Buffers do we bundle into one write to the hardware ? */
 #define IAVF_RX_INCREMENT(r, i) \
 	do {					\