[Intel-wired-lan] [PATCH iwl-next] ice: convert ice_add_prof() to bitmap

[Intel-wired-lan] [PATCH iwl-next] ice: convert ice_add_prof() to bitmap

[Przemek Kitszel]

From: Jesse Brandeburg <jesse.brandeburg@intel.com>

Previously the ice_add_prof() took an array of u8 and looped over it with
for_each_set_bit(), examining each 8 bit value as a bitmap.
This was just hard to understand and unnecessary, and was triggering
undefined behavior sanitizers with unaligned accesses within bitmap
fields (on our internal tools/builds). Since the @ptype being passed in
was already declared as a bitmap, refactor this to use native types with
the advantage of simplifying the code to use a single loop.

Co-developed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
CC: Jesse Brandeburg <jbrandeburg@cloudflare.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 .../net/ethernet/intel/ice/ice_flex_pipe.h    |  7 +-
 .../net/ethernet/intel/ice/ice_flex_pipe.c    | 78 +++++++------------
 drivers/net/ethernet/intel/ice/ice_flow.c     |  4 +-
 3 files changed, 34 insertions(+), 55 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.h b/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
index 28b0897adf32..ee5d9f9c9d53 100644
--- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
+++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
@@ -39,9 +39,10 @@ bool ice_hw_ptype_ena(struct ice_hw *hw, u16 ptype);
 
 /* XLT2/VSI group functions */
 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, bool fd_swap);
+ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id,
+	     unsigned long *ptypes, const struct ice_ptype_attributes *attr,
+	     u16 attr_cnt, 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_flex_pipe.c b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
index ed95072ca6e3..363ae79a3620 100644
--- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
+++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
@@ -3043,16 +3043,16 @@ ice_disable_fd_swap(struct ice_hw *hw, u8 prof_id)
  * the ID value used here.
  */
 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, bool fd_swap)
+ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id,
+	     unsigned long *ptypes, const struct ice_ptype_attributes *attr,
+	     u16 attr_cnt, 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);
 	struct ice_prof_map *prof;
-	u8 byte = 0;
-	u8 prof_id;
 	int status;
+	u8 prof_id;
+	u16 ptype;
 
 	bitmap_zero(ptgs_used, ICE_XLT1_CNT);
 
@@ -3102,57 +3102,35 @@ ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id, u8 ptypes[],
 	prof->context = 0;
 
 	/* build list of ptgs */
-	while (bytes && prof->ptg_cnt < ICE_MAX_PTG_PER_PROFILE) {
-		u8 bit;
+	for_each_set_bit(ptype, ptypes, ICE_FLOW_PTYPE_MAX) {
+		u8 ptg;
 
-		if (!ptypes[byte]) {
-			bytes--;
-			byte++;
+		/* The package should place all ptypes in a non-zero
+		 * PTG, so the following call should never fail.
+		 */
+		if (ice_ptg_find_ptype(hw, blk, ptype, &ptg))
 			continue;
-		}
 
-		/* Examine 8 bits per byte */
-		for_each_set_bit(bit, (unsigned long *)&ptypes[byte],
-				 BITS_PER_BYTE) {
-			u16 ptype;
-			u8 ptg;
-
-			ptype = byte * BITS_PER_BYTE + bit;
-
-			/* The package should place all ptypes in a non-zero
-			 * PTG, so the following call should never fail.
-			 */
-			if (ice_ptg_find_ptype(hw, blk, ptype, &ptg))
-				continue;
+		/* If PTG is already added, skip and continue */
+		if (test_bit(ptg, ptgs_used))
+			continue;
 
-			/* If PTG is already added, skip and continue */
-			if (test_bit(ptg, ptgs_used))
-				continue;
+		set_bit(ptg, ptgs_used);
+		/* Check to see there are any attributes for this ptype, and
+		 * add them if found.
+		 */
+		status = ice_add_prof_attrib(prof, ptg, ptype, attr, attr_cnt);
+		if (status == -ENOSPC)
+			break;
+		if (status) {
+			/* This is simple a ptype/PTG with no attribute */
+			prof->ptg[prof->ptg_cnt] = ptg;
+			prof->attr[prof->ptg_cnt].flags = 0;
+			prof->attr[prof->ptg_cnt].mask = 0;
 
-			__set_bit(ptg, ptgs_used);
-			/* Check to see there are any attributes for
-			 * this PTYPE, and add them if found.
-			 */
-			status = ice_add_prof_attrib(prof, ptg, ptype,
-						     attr, attr_cnt);
-			if (status == -ENOSPC)
+			if (++prof->ptg_cnt >= ICE_MAX_PTG_PER_PROFILE)
 				break;
-			if (status) {
-				/* This is simple a PTYPE/PTG with no
-				 * attribute
-				 */
-				prof->ptg[prof->ptg_cnt] = ptg;
-				prof->attr[prof->ptg_cnt].flags = 0;
-				prof->attr[prof->ptg_cnt].mask = 0;
-
-				if (++prof->ptg_cnt >=
-				    ICE_MAX_PTG_PER_PROFILE)
-					break;
-			}
 		}
-
-		bytes--;
-		byte++;
 	}
 
 	list_add(&prof->list, &hw->blk[blk].es.prof_map);
diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
index d97b751052f2..c63e43b8b110 100644
--- a/drivers/net/ethernet/intel/ice/ice_flow.c
+++ b/drivers/net/ethernet/intel/ice/ice_flow.c
@@ -1421,7 +1421,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,
+	status = ice_add_prof(hw, blk, prof_id, params->ptypes,
 			      params->attr, params->attr_cnt, params->es,
 			      params->mask, symm, true);
 	if (status) {
@@ -1617,7 +1617,7 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
 		break;
 	}
 
-	status = ice_add_prof(hw, blk, id, (u8 *)prof->ptypes,
+	status = ice_add_prof(hw, blk, id, prof->ptypes,
 			      params->attr, params->attr_cnt,
 			      params->es, params->mask, false, false);
 	if (status)
-- 
2.46.0

Re: [Intel-wired-lan] [PATCH iwl-next] ice: convert ice_add_prof() to bitmap

[Paul Menzel]

Dear Przemek, dear Jesse,


Thank you for the patch.

Am 18.06.25 um 13:28 schrieb Przemek Kitszel:
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> Previously the ice_add_prof() took an array of u8 and looped over it with
> for_each_set_bit(), examining each 8 bit value as a bitmap.
> This was just hard to understand and unnecessary, and was triggering
> undefined behavior sanitizers with unaligned accesses within bitmap
> fields (on our internal tools/builds). Since the @ptype being passed in
> was already declared as a bitmap, refactor this to use native types with
> the advantage of simplifying the code to use a single loop.

Any tests to verify no regressions are introduced?

> Co-developed-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> CC: Jesse Brandeburg <jbrandeburg@cloudflare.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>   .../net/ethernet/intel/ice/ice_flex_pipe.h    |  7 +-
>   .../net/ethernet/intel/ice/ice_flex_pipe.c    | 78 +++++++------------
>   drivers/net/ethernet/intel/ice/ice_flow.c     |  4 +-
>   3 files changed, 34 insertions(+), 55 deletions(-)

More removed lines than added ones is always a good diff stat.

> diff --git a/drivers/net/ethernet/intel/ice/ice_flex_pipe.h b/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
> index 28b0897adf32..ee5d9f9c9d53 100644
> --- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
> +++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.h
> @@ -39,9 +39,10 @@ bool ice_hw_ptype_ena(struct ice_hw *hw, u16 ptype);
>   
>   /* XLT2/VSI group functions */
>   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, bool fd_swap);
> +ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id,
> +	     unsigned long *ptypes, const struct ice_ptype_attributes *attr,
> +	     u16 attr_cnt, 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_flex_pipe.c b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
> index ed95072ca6e3..363ae79a3620 100644
> --- a/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
> +++ b/drivers/net/ethernet/intel/ice/ice_flex_pipe.c
> @@ -3043,16 +3043,16 @@ ice_disable_fd_swap(struct ice_hw *hw, u8 prof_id)
>    * the ID value used here.
>    */
>   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, bool fd_swap)
> +ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id,
> +	     unsigned long *ptypes, const struct ice_ptype_attributes *attr,
> +	     u16 attr_cnt, 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);
>   	struct ice_prof_map *prof;
> -	u8 byte = 0;
> -	u8 prof_id;
>   	int status;
> +	u8 prof_id;
> +	u16 ptype;
>   
>   	bitmap_zero(ptgs_used, ICE_XLT1_CNT);
>   
> @@ -3102,57 +3102,35 @@ ice_add_prof(struct ice_hw *hw, enum ice_block blk, u64 id, u8 ptypes[],
>   	prof->context = 0;
>   
>   	/* build list of ptgs */
> -	while (bytes && prof->ptg_cnt < ICE_MAX_PTG_PER_PROFILE) {
> -		u8 bit;
> +	for_each_set_bit(ptype, ptypes, ICE_FLOW_PTYPE_MAX) {
> +		u8 ptg;
>   
> -		if (!ptypes[byte]) {
> -			bytes--;
> -			byte++;
> +		/* The package should place all ptypes in a non-zero
> +		 * PTG, so the following call should never fail.
> +		 */
> +		if (ice_ptg_find_ptype(hw, blk, ptype, &ptg))
>   			continue;
> -		}
>   
> -		/* Examine 8 bits per byte */
> -		for_each_set_bit(bit, (unsigned long *)&ptypes[byte],
> -				 BITS_PER_BYTE) {
> -			u16 ptype;
> -			u8 ptg;
> -
> -			ptype = byte * BITS_PER_BYTE + bit;
> -
> -			/* The package should place all ptypes in a non-zero
> -			 * PTG, so the following call should never fail.
> -			 */
> -			if (ice_ptg_find_ptype(hw, blk, ptype, &ptg))
> -				continue;
> +		/* If PTG is already added, skip and continue */
> +		if (test_bit(ptg, ptgs_used))
> +			continue;
>   
> -			/* If PTG is already added, skip and continue */
> -			if (test_bit(ptg, ptgs_used))
> -				continue;
> +		set_bit(ptg, ptgs_used);
> +		/* Check to see there are any attributes for this ptype, and
> +		 * add them if found.
> +		 */
> +		status = ice_add_prof_attrib(prof, ptg, ptype, attr, attr_cnt);
> +		if (status == -ENOSPC)
> +			break;
> +		if (status) {
> +			/* This is simple a ptype/PTG with no attribute */
> +			prof->ptg[prof->ptg_cnt] = ptg;
> +			prof->attr[prof->ptg_cnt].flags = 0;
> +			prof->attr[prof->ptg_cnt].mask = 0;
>   
> -			__set_bit(ptg, ptgs_used);
> -			/* Check to see there are any attributes for
> -			 * this PTYPE, and add them if found.
> -			 */
> -			status = ice_add_prof_attrib(prof, ptg, ptype,
> -						     attr, attr_cnt);
> -			if (status == -ENOSPC)
> +			if (++prof->ptg_cnt >= ICE_MAX_PTG_PER_PROFILE)
>   				break;
> -			if (status) {
> -				/* This is simple a PTYPE/PTG with no
> -				 * attribute
> -				 */
> -				prof->ptg[prof->ptg_cnt] = ptg;
> -				prof->attr[prof->ptg_cnt].flags = 0;
> -				prof->attr[prof->ptg_cnt].mask = 0;
> -
> -				if (++prof->ptg_cnt >=
> -				    ICE_MAX_PTG_PER_PROFILE)
> -					break;
> -			}
>   		}
> -
> -		bytes--;
> -		byte++;
>   	}
>   
>   	list_add(&prof->list, &hw->blk[blk].es.prof_map);
> diff --git a/drivers/net/ethernet/intel/ice/ice_flow.c b/drivers/net/ethernet/intel/ice/ice_flow.c
> index d97b751052f2..c63e43b8b110 100644
> --- a/drivers/net/ethernet/intel/ice/ice_flow.c
> +++ b/drivers/net/ethernet/intel/ice/ice_flow.c
> @@ -1421,7 +1421,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,
> +	status = ice_add_prof(hw, blk, prof_id, params->ptypes,
>   			      params->attr, params->attr_cnt, params->es,
>   			      params->mask, symm, true);
>   	if (status) {
> @@ -1617,7 +1617,7 @@ ice_flow_set_parser_prof(struct ice_hw *hw, u16 dest_vsi, u16 fdir_vsi,
>   		break;
>   	}
>   
> -	status = ice_add_prof(hw, blk, id, (u8 *)prof->ptypes,
> +	status = ice_add_prof(hw, blk, id, prof->ptypes,
>   			      params->attr, params->attr_cnt,
>   			      params->es, params->mask, false, false);
>   	if (status)

Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH iwl-next] ice: convert ice_add_prof() to bitmap

[Przemek Kitszel]

On 6/18/25 13:54, Paul Menzel wrote:
> Dear Przemek, dear Jesse,
> 
> 
> Thank you for the patch.
> 
> Am 18.06.25 um 13:28 schrieb Przemek Kitszel:
>> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
>>
>> Previously the ice_add_prof() took an array of u8 and looped over it with
>> for_each_set_bit(), examining each 8 bit value as a bitmap.
>> This was just hard to understand and unnecessary, and was triggering
>> undefined behavior sanitizers with unaligned accesses within bitmap
>> fields (on our internal tools/builds). Since the @ptype being passed in
>> was already declared as a bitmap, refactor this to use native types with
>> the advantage of simplifying the code to use a single loop.
> 
> Any tests to verify no regressions are introduced?

Hi, nothing specific, and nothing that could not be inferred from the
commit message, if our VAL would ask me, the reply would be: please test
against FDIR regressions :)

Alexandr has this patch included in a bigger pile of work, which passed
many tests, this is also a form we have present in our OOT driver for
about 3 years by now.

[...]

> Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>

Thank you!

> 
> 
> Kind regards,
> 
> Paul

Re: [Intel-wired-lan] [PATCH iwl-next] ice: convert ice_add_prof() to bitmap

[Rinitha, SX]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Przemek Kitszel
> Sent: 18 June 2025 16:59
> To: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: netdev@vger.kernel.org; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Brandeburg, Jesse <jbrandeburg@cloudflare.com>
> Subject: [Intel-wired-lan] [PATCH iwl-next] ice: convert ice_add_prof() to bitmap
>
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
>
> Previously the ice_add_prof() took an array of u8 and looped over it with for_each_set_bit(), examining each 8 bit value as a bitmap.
>This was just hard to understand and unnecessary, and was triggering undefined behavior sanitizers with unaligned accesses within bitmap fields (on our internal tools/builds). Since the @ptype being passed in was already declared as a bitmap, refactor this to use native types with the advantage of simplifying the code to use a single loop.
>
> Co-developed-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> CC: Jesse Brandeburg <jbrandeburg@cloudflare.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
> .../net/ethernet/intel/ice/ice_flex_pipe.h    |  7 +-
> .../net/ethernet/intel/ice/ice_flex_pipe.c    | 78 +++++++------------
> drivers/net/ethernet/intel/ice/ice_flow.c     |  4 +-
> 3 files changed, 34 insertions(+), 55 deletions(-)
>

Tested-by: Rinitha S <sx.rinitha@intel.com> (A Contingent worker at Intel)