diff mbox series

[iwl-next,v3,10/13] ice: add method to disable FDIR SWAP option

Message ID 20240710204015.124233-11-ahmed.zaki@intel.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series ice: iavf: add support for TC U32 filters on VFs | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 816 this patch: 816
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 4 maintainers not CCed: edumazet@google.com przemyslaw.kitszel@intel.com pabeni@redhat.com kuba@kernel.org
netdev/build_clang success Errors and warnings before: 821 this patch: 821
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 823 this patch: 823
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 106 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 116 this patch: 115
netdev/source_inline success Was 0 now: 0

Commit Message

Ahmed Zaki July 10, 2024, 8:40 p.m. UTC 
From: Junfeng Guo <junfeng.guo@intel.com>

The SWAP Flag in the FDIR Programming Descriptor doesn't work properly,
it is always set and cannot be unset (hardware bug). Thus, add a method
to effectively disable the FDIR SWAP option by setting the FDSWAP instead
of FDINSET registers.

Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
---
 .../net/ethernet/intel/ice/ice_flex_pipe.c    | 52 ++++++++++++++++++-
 .../net/ethernet/intel/ice/ice_flex_pipe.h    |  4 +-
 drivers/net/ethernet/intel/ice/ice_flow.c     |  2 +-
 3 files changed, 54 insertions(+), 4 deletions(-)

Comments

Paul Menzel July 11, 2024, 4:59 a.m. UTC | #1 
Dear Ahmed, dear Junfeng,


Thank you for the patch.


Am 10.07.24 um 22:40 schrieb Ahmed Zaki:
> From: Junfeng Guo <junfeng.guo@intel.com>
> 
> The SWAP Flag in the FDIR Programming Descriptor doesn't work properly,
> it is always set and cannot be unset (hardware bug).

Please document the datasheet/errata.

> Thus, add a method
> to effectively disable the FDIR SWAP option by setting the FDSWAP instead
> of FDINSET registers.

Please paste the new debug messages.

> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> ---
>   .../net/ethernet/intel/ice/ice_flex_pipe.c    | 52 ++++++++++++++++++-
>   .../net/ethernet/intel/ice/ice_flex_pipe.h    |  4 +-
>   drivers/net/ethernet/intel/ice/ice_flow.c     |  2 +-
>   3 files changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
> index 20d5db88c99f..a750d7e1edd8 100644
> --- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
> +++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
> @@ -2981,6 +2981,51 @@ ice_add_prof_attrib(struct ice_prof_map *prof, u8 ptg, u16 ptype,
>   }
>   
>   /**
> + * ice_disable_fd_swap - set register appropriately to disable FD swap

Below you write SWAP all uppercase.

> + * @hw: pointer to the HW struct
> + * @prof_id: profile ID
> + *
> + * Return: Void.
> + */
> +static void
> +ice_disable_fd_swap(struct ice_hw *hw, u8 prof_id)
> +{
> +	u16 swap_val, i, fvw_num;

Try to use non-fixed-width types, where possible.

> +
> +	swap_val = ICE_SWAP_VALID;
> +	fvw_num = hw->blk[ICE_BLK_FD].es.fvw / ICE_FDIR_REG_SET_SIZE;
> +
> +	/* Since the SWAP Flag in the Programming Desc doesn't work,
> +	 * here add method to disable the SWAP Option via setting
> +	 * certain SWAP and INSET register sets.
> +	 */
> +	for (i = 0; i < fvw_num ; i++) {
> +		u32 raw_swap, raw_in;
> +		u8 j;

unsigned int

> +
> +		raw_swap = 0;
> +		raw_in = 0;
> +
> +		for (j = 0; j < ICE_FDIR_REG_SET_SIZE; j++) {
> +			raw_swap |= (swap_val++) << (j * BITS_PER_BYTE);
> +			raw_in |= ICE_INSET_DFLT << (j * BITS_PER_BYTE);
> +		}
> +
> +		/* write the FDIR swap register set */
> +		wr32(hw, GLQF_FDSWAP(prof_id, i), raw_swap);
> +
> +		ice_debug(hw, ICE_DBG_INIT, "swap wr(%d, %d): 0x%x = 0x%08x\n",
> +			  prof_id, i, GLQF_FDSWAP(prof_id, i), raw_swap);
> +
> +		/* write the FDIR inset register set */
> +		wr32(hw, GLQF_FDINSET(prof_id, i), raw_in);
> +
> +		ice_debug(hw, ICE_DBG_INIT, "inset wr(%d, %d): 0x%x = 0x%08x\n",
> +			  prof_id, i, GLQF_FDINSET(prof_id, i), raw_in);
> +	}
> +}
> +
> +/*
>    * ice_add_prof - add profile
>    * @hw: pointer to the HW struct
>    * @blk: hardware block
> @@ -2991,6 +3036,7 @@ ice_add_prof_attrib(struct ice_prof_map *prof, u8 ptg, u16 ptype,
>    * @es: extraction sequence (length of array is determined by the block)
>    * @masks: mask for extraction sequence
>    * @symm: symmetric setting for RSS profiles
> + * @fd_swap: enable/disable FDIR paired src/dst fields swap option
>    *
>    * This function registers a profile, which matches a set of PTYPES with a
>    * particular extraction sequence. While the hardware profile is allocated
> @@ -3000,7 +3046,7 @@ ice_add_prof_attrib(struct ice_prof_map *prof, u8 ptg, u16 ptype,
>   int
>   ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id, u8 ptypes[],
>   	     const struct ice_ptype_attributes *attr, u16 attr_cnt,
> -	     struct ice_fv_word *es, u16 *masks, bool symm)
> +	     struct ice_fv_word *es, u16 *masks, bool symm, bool fd_swap)
>   {
>   	u32 bytes = DIV_ROUND_UP(ICE_FLOW_PTYPE_MAX, BITS_PER_BYTE);
>   	DECLARE_BITMAP(ptgs_used, ICE_XLT1_CNT);
> @@ -3020,7 +3066,7 @@ ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id, u8 ptypes[],
>   		status = ice_alloc_prof_id(hw, blk, &prof_id);
>   		if (status)
>   			goto err_ice_add_prof;
> -		if (blk == ICE_BLK_FD) {
> +		if (blk == ICE_BLK_FD && fd_swap) {
>   			/* For Flow Director block, the extraction sequence may
>   			 * need to be altered in the case where there are paired
>   			 * fields that have no match. This is necessary because
> @@ -3031,6 +3077,8 @@ ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id, u8 ptypes[],
>   			status = ice_update_fd_swap(hw, prof_id, es);
>   			if (status)
>   				goto err_ice_add_prof;
> +		} else if (blk == ICE_BLK_FD) {
> +			ice_disable_fd_swap(hw, prof_id);
>   		}
>   		status = ice_update_prof_masking(hw, blk, prof_id, masks);
>   		if (status)
> diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.h b/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
> index b39d7cdc381f..7c66652dadd6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
> +++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
> @@ -6,6 +6,8 @@
>   
>   #include "ice_type.h"
>   
> +#define ICE_FDIR_REG_SET_SIZE	4
> +
>   int
>   ice_acquire_change_lock(struct ice_hw *hw, enum ice_aq_res_access_type access);
>   void ice_release_change_lock(struct ice_hw *hw);
> @@ -42,7 +44,7 @@ bool ice_hw_ptype_ena(struct ice_hw *hw, u16 ptype);
>   int
>   ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id, u8 ptypes[],
>   	     const struct ice_ptype_attributes *attr, u16 attr_cnt,
> -	     struct ice_fv_word *es, u16 *masks, bool symm);
> +	     struct ice_fv_word *es, u16 *masks, bool symm, bool fd_swap);
>   struct ice_prof_map *
>   ice_search_prof_id(struct ice_hw *hw, enum ice_block blk, u64 id);
>   int
> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> index fc2b58f56279..79106503194b 100644
> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> @@ -1400,7 +1400,7 @@ ice_flow_add_prof_sync(struct ice_hw *hw, enum ice_block blk,
>   	/* Add a HW profile for this flow profile */
>   	status = ice_add_prof(hw, blk, prof_id, (u8 *)params->ptypes,
>   			      params->attr, params->attr_cnt, params->es,
> -			      params->mask, symm);
> +			      params->mask, symm, true);
>   	if (status) {
>   		ice_debug(hw, ICE_DBG_FLOW, "Error adding a HW flow profile\n");
>   		goto out;


Kind regards,

Paul
Ahmed Zaki July 15, 2024, 2:23 p.m. UTC | #2 
On 2024-07-10 10:59 p.m., Paul Menzel wrote:
> Dear Ahmed, dear Junfeng,
> 
> 
> Thank you for the patch.
> 
> 
> Am 10.07.24 um 22:40 schrieb Ahmed Zaki:
>> From: Junfeng Guo <junfeng.guo@intel.com>
>>
>> The SWAP Flag in the FDIR Programming Descriptor doesn't work properly,
>> it is always set and cannot be unset (hardware bug).
> 
> Please document the datasheet/errata.

Unfortunately, I don't think this is in any docs or errata.

> 
>> Thus, add a method
>> to effectively disable the FDIR SWAP option by setting the FDSWAP instead
>> of FDINSET registers.
> 
> Please paste the new debug messages.

What debug messages? If you mean the ones logged by ice_debug() in this 
patch, please note fvw_num = 48 for the parser. So that's 96 lines of:

swap wr(%d, %d): 0x%x = 0x%08x
inset wr(%d, %d): 0x%x = 0x%08x

> 
>> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
>> Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
>> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
>> ---
>>   .../net/ethernet/intel/ice/ice_flex_pipe.c    | 52 ++++++++++++++++++-
>>   .../net/ethernet/intel/ice/ice_flex_pipe.h    |  4 +-
>>   drivers/net/ethernet/intel/ice/ice_flow.c     |  2 +-
>>   3 files changed, 54 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c 
>> b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
>> index 20d5db88c99f..a750d7e1edd8 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
>> @@ -2981,6 +2981,51 @@ ice_add_prof_attrib(struct ice_prof_map *prof, 
>> u8 ptg, u16 ptype,
>>   }
>>   /**
>> + * ice_disable_fd_swap - set register appropriately to disable FD swap
> 
> Below you write SWAP all uppercase.

Will fix.

> 
>> + * @hw: pointer to the HW struct
>> + * @prof_id: profile ID
>> + *
>> + * Return: Void.
>> + */
>> +static void
>> +ice_disable_fd_swap(struct ice_hw *hw, u8 prof_id)
>> +{
>> +    u16 swap_val, i, fvw_num;
> 
> Try to use non-fixed-width types, where possible.

Sure, will fix i and j here.


> 
>> +
>> +    swap_val = ICE_SWAP_VALID;
>> +    fvw_num = hw->blk[ICE_BLK_FD].es.fvw / ICE_FDIR_REG_SET_SIZE;
>> +
>> +    /* Since the SWAP Flag in the Programming Desc doesn't work,
>> +     * here add method to disable the SWAP Option via setting
>> +     * certain SWAP and INSET register sets.
>> +     */
>> +    for (i = 0; i < fvw_num ; i++) {
>> +        u32 raw_swap, raw_in;
>> +        u8 j;
> 
> unsigned int

Thanks.
Paul Menzel July 17, 2024, 9:55 a.m. UTC | #3 
Dear Ahmed,


Thank you for your reply.


Am 15.07.24 um 16:23 schrieb Ahmed Zaki:

> On 2024-07-10 10:59 p.m., Paul Menzel wrote:

>> Am 10.07.24 um 22:40 schrieb Ahmed Zaki:
>>> From: Junfeng Guo <junfeng.guo@intel.com>
>>>
>>> The SWAP Flag in the FDIR Programming Descriptor doesn't work properly,
>>> it is always set and cannot be unset (hardware bug).
>>
>> Please document the datasheet/errata.
> 
> Unfortunately, I don't think this is in any docs or errata.

Oh. How did you find out?

>>> Thus, add a method to effectively disable the FDIR SWAP option by
>>> setting the FDSWAP instead of FDINSET registers.
>>
>> Please paste the new debug messages.
> 
> What debug messages? If you mean the ones logged by ice_debug() in this 
> patch, please note fvw_num = 48 for the parser. So that's 96 lines of:
> 
> swap wr(%d, %d): 0x%x = 0x%08x
> inset wr(%d, %d): 0x%x = 0x%08x

Personally, I’d prefer an example line, but I know that it’s not common.

[…]


Kind regards,

Paul
Ahmed Zaki July 18, 2024, 4:54 p.m. UTC | #4 
On 2024-07-17 3:55 a.m., Paul Menzel wrote:
> Dear Ahmed,
> 
> 
> Thank you for your reply.
> 
> 
> Am 15.07.24 um 16:23 schrieb Ahmed Zaki:
> 
>> On 2024-07-10 10:59 p.m., Paul Menzel wrote:
> 
>>> Am 10.07.24 um 22:40 schrieb Ahmed Zaki:
>>>> From: Junfeng Guo <junfeng.guo@intel.com>
>>>>
>>>> The SWAP Flag in the FDIR Programming Descriptor doesn't work properly,
>>>> it is always set and cannot be unset (hardware bug).
>>>
>>> Please document the datasheet/errata.
>>
>> Unfortunately, I don't think this is in any docs or errata.
> 
> Oh. How did you find out?
> 

I believe that was discovered when changing the SWAP flag in the 
descriptor did not have any effect at all.

>>>> Thus, add a method to effectively disable the FDIR SWAP option by
>>>> setting the FDSWAP instead of FDINSET registers.
>>>

Ahmed
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
index 20d5db88c99f..a750d7e1edd8 100644
--- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
+++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
@@ -2981,6 +2981,51 @@  ice_add_prof_attrib(struct ice_prof_map *prof, u8 ptg, u16 ptype,
 }
 
 /**
+ * ice_disable_fd_swap - set register appropriately to disable FD swap
+ * @hw: pointer to the HW struct
+ * @prof_id: profile ID
+ *
+ * Return: Void.
+ */
+static void
+ice_disable_fd_swap(struct ice_hw *hw, u8 prof_id)
+{
+	u16 swap_val, i, fvw_num;
+
+	swap_val = ICE_SWAP_VALID;
+	fvw_num = hw->blk[ICE_BLK_FD].es.fvw / ICE_FDIR_REG_SET_SIZE;
+
+	/* Since the SWAP Flag in the Programming Desc doesn't work,
+	 * here add method to disable the SWAP Option via setting
+	 * certain SWAP and INSET register sets.
+	 */
+	for (i = 0; i < fvw_num ; i++) {
+		u32 raw_swap, raw_in;
+		u8 j;
+
+		raw_swap = 0;
+		raw_in = 0;
+
+		for (j = 0; j < ICE_FDIR_REG_SET_SIZE; j++) {
+			raw_swap |= (swap_val++) << (j * BITS_PER_BYTE);
+			raw_in |= ICE_INSET_DFLT << (j * BITS_PER_BYTE);
+		}
+
+		/* write the FDIR swap register set */
+		wr32(hw, GLQF_FDSWAP(prof_id, i), raw_swap);
+
+		ice_debug(hw, ICE_DBG_INIT, "swap wr(%d, %d): 0x%x = 0x%08x\n",
+			  prof_id, i, GLQF_FDSWAP(prof_id, i), raw_swap);
+
+		/* write the FDIR inset register set */
+		wr32(hw, GLQF_FDINSET(prof_id, i), raw_in);
+
+		ice_debug(hw, ICE_DBG_INIT, "inset wr(%d, %d): 0x%x = 0x%08x\n",
+			  prof_id, i, GLQF_FDINSET(prof_id, i), raw_in);
+	}
+}
+
+/*
  * ice_add_prof - add profile
  * @hw: pointer to the HW struct
  * @blk: hardware block
@@ -2991,6 +3036,7 @@  ice_add_prof_attrib(struct ice_prof_map *prof, u8 ptg, u16 ptype,
  * @es: extraction sequence (length of array is determined by the block)
  * @masks: mask for extraction sequence
  * @symm: symmetric setting for RSS profiles
+ * @fd_swap: enable/disable FDIR paired src/dst fields swap option
  *
  * This function registers a profile, which matches a set of PTYPES with a
  * particular extraction sequence. While the hardware profile is allocated
@@ -3000,7 +3046,7 @@  ice_add_prof_attrib(struct ice_prof_map *prof, u8 ptg, u16 ptype,
 int
 ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id, u8 ptypes[],
 	     const struct ice_ptype_attributes *attr, u16 attr_cnt,
-	     struct ice_fv_word *es, u16 *masks, bool symm)
+	     struct ice_fv_word *es, u16 *masks, bool symm, bool fd_swap)
 {
 	u32 bytes = DIV_ROUND_UP(ICE_FLOW_PTYPE_MAX, BITS_PER_BYTE);
 	DECLARE_BITMAP(ptgs_used, ICE_XLT1_CNT);
@@ -3020,7 +3066,7 @@  ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id, u8 ptypes[],
 		status = ice_alloc_prof_id(hw, blk, &prof_id);
 		if (status)
 			goto err_ice_add_prof;
-		if (blk == ICE_BLK_FD) {
+		if (blk == ICE_BLK_FD && fd_swap) {
 			/* For Flow Director block, the extraction sequence may
 			 * need to be altered in the case where there are paired
 			 * fields that have no match. This is necessary because
@@ -3031,6 +3077,8 @@  ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id, u8 ptypes[],
 			status = ice_update_fd_swap(hw, prof_id, es);
 			if (status)
 				goto err_ice_add_prof;
+		} else if (blk == ICE_BLK_FD) {
+			ice_disable_fd_swap(hw, prof_id);
 		}
 		status = ice_update_prof_masking(hw, blk, prof_id, masks);
 		if (status)
diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.h b/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
index b39d7cdc381f..7c66652dadd6 100644
--- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
+++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
@@ -6,6 +6,8 @@ 
 
 #include "ice_type.h"
 
+#define ICE_FDIR_REG_SET_SIZE	4
+
 int
 ice_acquire_change_lock(struct ice_hw *hw, enum ice_aq_res_access_type access);
 void ice_release_change_lock(struct ice_hw *hw);
@@ -42,7 +44,7 @@  bool ice_hw_ptype_ena(struct ice_hw *hw, u16 ptype);
 int
 ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id, u8 ptypes[],
 	     const struct ice_ptype_attributes *attr, u16 attr_cnt,
-	     struct ice_fv_word *es, u16 *masks, bool symm);
+	     struct ice_fv_word *es, u16 *masks, bool symm, bool fd_swap);
 struct ice_prof_map *
 ice_search_prof_id(struct ice_hw *hw, enum ice_block blk, u64 id);
 int
diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
index fc2b58f56279..79106503194b 100644
--- a/drivers/net/ethernet/intel/ice/ice_flow.c
+++ b/drivers/net/ethernet/intel/ice/ice_flow.c
@@ -1400,7 +1400,7 @@  ice_flow_add_prof_sync(struct ice_hw *hw, enum ice_block blk,
 	/* Add a HW profile for this flow profile */
 	status = ice_add_prof(hw, blk, prof_id, (u8 *)params->ptypes,
 			      params->attr, params->attr_cnt, params->es,
-			      params->mask, symm);
+			      params->mask, symm, true);
 	if (status) {
 		ice_debug(hw, ICE_DBG_FLOW, "Error adding a HW flow profile\n");
 		goto out;