Re: [Intel-wired-lan] [PATCH iwl-next v10 12/14] iavf: Implement checking DD desc field

[Alexander Lobakin <aleksander.lobakin@intel.com>]

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