diff mbox series

[iwl-net] ice: fix ice_parser_rt::bst_key array size

Message ID 20241211132745.112536-2-przemyslaw.kitszel@intel.com
State Changes Requested
Headers show
Series [iwl-net] ice: fix ice_parser_rt::bst_key array size | expand

Commit Message

Przemek Kitszel Dec. 11, 2024, 1:26 p.m. UTC 
Fix &ice_parser_rt::bst_key size. It was wrongly set to 10 instead of 20
in the initial impl commit (see Fixes tag). All usage code assumed it was
of size 20. That was also the initial size present up to v2 of the intro
series [2], but halved by v3 [3] refactor described as "Replace magic
hardcoded values with macros." The introducing series was so big that
some ugliness was unnoticed, same for bugs :/

ICE_BST_KEY_TCAM_SIZE and ICE_BST_TCAM_KEY_SIZE were differing by one.
There was tmp variable @j in the scope of edited function, but was not
used in all places. This ugliness is now gone.
I'm moving ice_parser_rt::pg_prio a few positions up, to fill up one of
the holes in order to compensate for the added 10 bytes to the ::bst_key,
resulting in the same size of the whole as prior to the fix, and miminal
changes in the offsets of the fields.

This fix obsoletes Ahmed's attempt at [1].

[1] https://lore.kernel.org/intel-wired-lan/20240823230847.172295-1-ahmed.zaki@intel.com
[2] https://lore.kernel.org/intel-wired-lan/20230605054641.2865142-13-junfeng.guo@intel.com
[3] https://lore.kernel.org/intel-wired-lan/20230817093442.2576997-13-junfeng.guo@intel.com

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/intel-wired-lan/b1fb6ff9-b69e-4026-9988-3c783d86c2e0@stanley.mountain
Fixes: 9a4c07aaa0f5 ("ice: add parser execution main loop")
CC: Ahmed Zaki <ahmed.zaki@intel.com>
Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_parser.h    |  6 ++----
 drivers/net/ethernet/intel/ice/ice_parser_rt.c | 12 +++++-------
 2 files changed, 7 insertions(+), 11 deletions(-)


base-commit: 51a00be6a0994da2ba6b4ace3b7a0d9373b4b25e

Comments

Simon Horman Dec. 12, 2024, 4:30 p.m. UTC | #1 
On Wed, Dec 11, 2024 at 02:26:36PM +0100, Przemek Kitszel wrote:
> Fix &ice_parser_rt::bst_key size. It was wrongly set to 10 instead of 20
> in the initial impl commit (see Fixes tag). All usage code assumed it was
> of size 20. That was also the initial size present up to v2 of the intro
> series [2], but halved by v3 [3] refactor described as "Replace magic
> hardcoded values with macros." The introducing series was so big that
> some ugliness was unnoticed, same for bugs :/
> 
> ICE_BST_KEY_TCAM_SIZE and ICE_BST_TCAM_KEY_SIZE were differing by one.
> There was tmp variable @j in the scope of edited function, but was not
> used in all places. This ugliness is now gone.
> I'm moving ice_parser_rt::pg_prio a few positions up, to fill up one of
> the holes in order to compensate for the added 10 bytes to the ::bst_key,
> resulting in the same size of the whole as prior to the fix, and miminal
> changes in the offsets of the fields.
> 
> This fix obsoletes Ahmed's attempt at [1].
> 
> [1] https://lore.kernel.org/intel-wired-lan/20240823230847.172295-1-ahmed.zaki@intel.com
> [2] https://lore.kernel.org/intel-wired-lan/20230605054641.2865142-13-junfeng.guo@intel.com
> [3] https://lore.kernel.org/intel-wired-lan/20230817093442.2576997-13-junfeng.guo@intel.com
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/intel-wired-lan/b1fb6ff9-b69e-4026-9988-3c783d86c2e0@stanley.mountain
> Fixes: 9a4c07aaa0f5 ("ice: add parser execution main loop")
> CC: Ahmed Zaki <ahmed.zaki@intel.com>
> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Hi Przemek,

I agree that these changes are good.  But I wonder if it would be best to
only treat the update size of bst_key as a fix.

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_parser_rt.c b/drivers/net/ethernet/intel/ice/ice_parser_rt.c
> index dedf5e854e4b..d9c38ce27e4f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_parser_rt.c
> +++ b/drivers/net/ethernet/intel/ice/ice_parser_rt.c
> @@ -125,22 +125,20 @@ static void ice_bst_key_init(struct ice_parser_rt *rt,
>  	else
>  		key[idd] = imem->b_kb.prio;
>  
> -	idd = ICE_BST_KEY_TCAM_SIZE - 1;
> +	idd = ICE_BST_TCAM_KEY_SIZE - 2;
>  	for (i = idd; i >= 0; i--) {
>  		int j;
>  
>  		j = ho + idd - i;
>  		if (j < ICE_PARSER_MAX_PKT_LEN)
> -			key[i] = rt->pkt_buf[ho + idd - i];
> +			key[i] = rt->pkt_buf[j];
>  		else
>  			key[i] = 0;
>  	}
>  
> -	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "Generated Boost TCAM Key:\n");
> -	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "%02X %02X %02X %02X %02X %02X %02X %02X %02X %02X\n",
> -		  key[0], key[1], key[2], key[3], key[4],
> -		  key[5], key[6], key[7], key[8], key[9]);
> -	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "\n");
> +	ice_debug_array_w_prefix(rt->psr->hw, ICE_DBG_PARSER,
> +				 KBUILD_MODNAME "Generated Boost TCAM Key",

Should there be a delimeter between KBUILD_MODNAME and "Generated ..." ?
e.g.:

				 KBUILD_MODNAME ": Generated Boost TCAM Key",

> +				 key, ICE_BST_TCAM_KEY_SIZE);
>  }
>  
>  static u16 ice_bit_rev_u16(u16 v, int len)
> 
> base-commit: 51a00be6a0994da2ba6b4ace3b7a0d9373b4b25e
> -- 
> 2.46.0
> 
>
Przemek Kitszel Dec. 16, 2024, 5:58 a.m. UTC | #2 
On 12/12/24 17:30, Simon Horman wrote:
> On Wed, Dec 11, 2024 at 02:26:36PM +0100, Przemek Kitszel wrote:
>> Fix &ice_parser_rt::bst_key size. It was wrongly set to 10 instead of 20
>> in the initial impl commit (see Fixes tag). All usage code assumed it was
>> of size 20. That was also the initial size present up to v2 of the intro
>> series [2], but halved by v3 [3] refactor described as "Replace magic
>> hardcoded values with macros." The introducing series was so big that
>> some ugliness was unnoticed, same for bugs :/
>>
>> ICE_BST_KEY_TCAM_SIZE and ICE_BST_TCAM_KEY_SIZE were differing by one.
>> There was tmp variable @j in the scope of edited function, but was not
>> used in all places. This ugliness is now gone.
>> I'm moving ice_parser_rt::pg_prio a few positions up, to fill up one of
>> the holes in order to compensate for the added 10 bytes to the ::bst_key,
>> resulting in the same size of the whole as prior to the fix, and miminal
>> changes in the offsets of the fields.
>>
>> This fix obsoletes Ahmed's attempt at [1].
>>
>> [1] https://lore.kernel.org/intel-wired-lan/20240823230847.172295-1-ahmed.zaki@intel.com
>> [2] https://lore.kernel.org/intel-wired-lan/20230605054641.2865142-13-junfeng.guo@intel.com
>> [3] https://lore.kernel.org/intel-wired-lan/20230817093442.2576997-13-junfeng.guo@intel.com
>>
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/intel-wired-lan/b1fb6ff9-b69e-4026-9988-3c783d86c2e0@stanley.mountain
>> Fixes: 9a4c07aaa0f5 ("ice: add parser execution main loop")
>> CC: Ahmed Zaki <ahmed.zaki@intel.com>
>> Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> 
> Hi Przemek,
> 
> I agree that these changes are good.  But I wonder if it would be best to
> only treat the update size of bst_key as a fix.

I was also wondering, to the point that my first version was with
all 20 key bytes debug-printed in the exact way as it was prior to the
patch. Would be fine to just drop the printing part?

> 
> ...
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_parser_rt.c b/drivers/net/ethernet/intel/ice/ice_parser_rt.c
>> index dedf5e854e4b..d9c38ce27e4f 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_parser_rt.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_parser_rt.c
>> @@ -125,22 +125,20 @@ static void ice_bst_key_init(struct ice_parser_rt *rt,
>>   	else
>>   		key[idd] = imem->b_kb.prio;
>>   
>> -	idd = ICE_BST_KEY_TCAM_SIZE - 1;
>> +	idd = ICE_BST_TCAM_KEY_SIZE - 2;
>>   	for (i = idd; i >= 0; i--) {
>>   		int j;
>>   
>>   		j = ho + idd - i;
>>   		if (j < ICE_PARSER_MAX_PKT_LEN)
>> -			key[i] = rt->pkt_buf[ho + idd - i];
>> +			key[i] = rt->pkt_buf[j];
>>   		else
>>   			key[i] = 0;
>>   	}
>>   
>> -	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "Generated Boost TCAM Key:\n");
>> -	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "%02X %02X %02X %02X %02X %02X %02X %02X %02X %02X\n",
>> -		  key[0], key[1], key[2], key[3], key[4],
>> -		  key[5], key[6], key[7], key[8], key[9]);
>> -	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "\n");
>> +	ice_debug_array_w_prefix(rt->psr->hw, ICE_DBG_PARSER,
>> +				 KBUILD_MODNAME "Generated Boost TCAM Key",
> 
> Should there be a delimeter between KBUILD_MODNAME and "Generated ..." ?
> e.g.:

thank you :)

> 
> 				 KBUILD_MODNAME ": Generated Boost TCAM Key",
> 
>> +				 key, ICE_BST_TCAM_KEY_SIZE);
>>   }
>>   
>>   static u16 ice_bit_rev_u16(u16 v, int len)
>>
>> base-commit: 51a00be6a0994da2ba6b4ace3b7a0d9373b4b25e
>> -- 
>> 2.46.0
>>
>>
Simon Horman Dec. 16, 2024, 5:05 p.m. UTC | #3 
On Mon, Dec 16, 2024 at 06:58:35AM +0100, Przemek Kitszel wrote:
> On 12/12/24 17:30, Simon Horman wrote:
> > On Wed, Dec 11, 2024 at 02:26:36PM +0100, Przemek Kitszel wrote:
> > > Fix &ice_parser_rt::bst_key size. It was wrongly set to 10 instead of 20
> > > in the initial impl commit (see Fixes tag). All usage code assumed it was
> > > of size 20. That was also the initial size present up to v2 of the intro
> > > series [2], but halved by v3 [3] refactor described as "Replace magic
> > > hardcoded values with macros." The introducing series was so big that
> > > some ugliness was unnoticed, same for bugs :/
> > > 
> > > ICE_BST_KEY_TCAM_SIZE and ICE_BST_TCAM_KEY_SIZE were differing by one.
> > > There was tmp variable @j in the scope of edited function, but was not
> > > used in all places. This ugliness is now gone.
> > > I'm moving ice_parser_rt::pg_prio a few positions up, to fill up one of
> > > the holes in order to compensate for the added 10 bytes to the ::bst_key,
> > > resulting in the same size of the whole as prior to the fix, and miminal
> > > changes in the offsets of the fields.
> > > 
> > > This fix obsoletes Ahmed's attempt at [1].
> > > 
> > > [1] https://lore.kernel.org/intel-wired-lan/20240823230847.172295-1-ahmed.zaki@intel.com
> > > [2] https://lore.kernel.org/intel-wired-lan/20230605054641.2865142-13-junfeng.guo@intel.com
> > > [3] https://lore.kernel.org/intel-wired-lan/20230817093442.2576997-13-junfeng.guo@intel.com
> > > 
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Closes: https://lore.kernel.org/intel-wired-lan/b1fb6ff9-b69e-4026-9988-3c783d86c2e0@stanley.mountain
> > > Fixes: 9a4c07aaa0f5 ("ice: add parser execution main loop")
> > > CC: Ahmed Zaki <ahmed.zaki@intel.com>
> > > Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> > 
> > Hi Przemek,
> > 
> > I agree that these changes are good.  But I wonder if it would be best to
> > only treat the update size of bst_key as a fix.
> 
> I was also wondering, to the point that my first version was with
> all 20 key bytes debug-printed in the exact way as it was prior to the
> patch. Would be fine to just drop the printing part?

Sorry, I didn't notice that the printing is updated to accommodate
the larger key. So on a second review, I think the patch is fine,
as long as a delimiter is added as noted in my previous email.

> 
> > 
> > ...
> > 
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_parser_rt.c b/drivers/net/ethernet/intel/ice/ice_parser_rt.c
> > > index dedf5e854e4b..d9c38ce27e4f 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_parser_rt.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_parser_rt.c
> > > @@ -125,22 +125,20 @@ static void ice_bst_key_init(struct ice_parser_rt *rt,
> > >   	else
> > >   		key[idd] = imem->b_kb.prio;
> > > -	idd = ICE_BST_KEY_TCAM_SIZE - 1;
> > > +	idd = ICE_BST_TCAM_KEY_SIZE - 2;
> > >   	for (i = idd; i >= 0; i--) {
> > >   		int j;
> > >   		j = ho + idd - i;
> > >   		if (j < ICE_PARSER_MAX_PKT_LEN)
> > > -			key[i] = rt->pkt_buf[ho + idd - i];
> > > +			key[i] = rt->pkt_buf[j];
> > >   		else
> > >   			key[i] = 0;
> > >   	}
> > > -	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "Generated Boost TCAM Key:\n");
> > > -	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "%02X %02X %02X %02X %02X %02X %02X %02X %02X %02X\n",
> > > -		  key[0], key[1], key[2], key[3], key[4],
> > > -		  key[5], key[6], key[7], key[8], key[9]);
> > > -	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "\n");
> > > +	ice_debug_array_w_prefix(rt->psr->hw, ICE_DBG_PARSER,
> > > +				 KBUILD_MODNAME "Generated Boost TCAM Key",
> > 
> > Should there be a delimeter between KBUILD_MODNAME and "Generated ..." ?
> > e.g.:
> 
> thank you :)
> 
> > 
> > 				 KBUILD_MODNAME ": Generated Boost TCAM Key",
> > 
> > > +				 key, ICE_BST_TCAM_KEY_SIZE);
> > >   }
> > >   static u16 ice_bit_rev_u16(u16 v, int len)
> > > 
> > > base-commit: 51a00be6a0994da2ba6b4ace3b7a0d9373b4b25e
> > > -- 
> > > 2.46.0
> > > 
> > > 
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_parser.h b/drivers/net/ethernet/intel/ice/ice_parser.h
index 6509d807627c..4f56d53d56b9 100644
--- a/drivers/net/ethernet/intel/ice/ice_parser.h
+++ b/drivers/net/ethernet/intel/ice/ice_parser.h
@@ -257,7 +257,6 @@  ice_pg_nm_cam_match(struct ice_pg_nm_cam_item *table, int size,
 /*** ICE_SID_RXPARSER_BOOST_TCAM and ICE_SID_LBL_RXPARSER_TMEM sections ***/
 #define ICE_BST_TCAM_TABLE_SIZE		256
 #define ICE_BST_TCAM_KEY_SIZE		20
-#define ICE_BST_KEY_TCAM_SIZE		19
 
 /* Boost TCAM item */
 struct ice_bst_tcam_item {
@@ -401,7 +400,6 @@  u16 ice_xlt_kb_flag_get(struct ice_xlt_kb *kb, u64 pkt_flag);
 #define ICE_PARSER_GPR_NUM	128
 #define ICE_PARSER_FLG_NUM	64
 #define ICE_PARSER_ERR_NUM	16
-#define ICE_BST_KEY_SIZE	10
 #define ICE_MARKER_ID_SIZE	9
 #define ICE_MARKER_MAX_SIZE	\
 		(ICE_MARKER_ID_SIZE * BITS_PER_BYTE - 1)
@@ -431,13 +429,13 @@  struct ice_parser_rt {
 	u8 pkt_buf[ICE_PARSER_MAX_PKT_LEN + ICE_PARSER_PKT_REV];
 	u16 pkt_len;
 	u16 po;
-	u8 bst_key[ICE_BST_KEY_SIZE];
+	u8 bst_key[ICE_BST_TCAM_KEY_SIZE];
 	struct ice_pg_cam_key pg_key;
+	u8 pg_prio;
 	struct ice_alu *alu0;
 	struct ice_alu *alu1;
 	struct ice_alu *alu2;
 	struct ice_pg_cam_action *action;
-	u8 pg_prio;
 	struct ice_gpr_pu pu;
 	u8 markers[ICE_MARKER_ID_SIZE];
 	bool protocols[ICE_PO_PAIR_SIZE];
diff --git a/drivers/net/ethernet/intel/ice/ice_parser_rt.c b/drivers/net/ethernet/intel/ice/ice_parser_rt.c
index dedf5e854e4b..d9c38ce27e4f 100644
--- a/drivers/net/ethernet/intel/ice/ice_parser_rt.c
+++ b/drivers/net/ethernet/intel/ice/ice_parser_rt.c
@@ -125,22 +125,20 @@  static void ice_bst_key_init(struct ice_parser_rt *rt,
 	else
 		key[idd] = imem->b_kb.prio;
 
-	idd = ICE_BST_KEY_TCAM_SIZE - 1;
+	idd = ICE_BST_TCAM_KEY_SIZE - 2;
 	for (i = idd; i >= 0; i--) {
 		int j;
 
 		j = ho + idd - i;
 		if (j < ICE_PARSER_MAX_PKT_LEN)
-			key[i] = rt->pkt_buf[ho + idd - i];
+			key[i] = rt->pkt_buf[j];
 		else
 			key[i] = 0;
 	}
 
-	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "Generated Boost TCAM Key:\n");
-	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "%02X %02X %02X %02X %02X %02X %02X %02X %02X %02X\n",
-		  key[0], key[1], key[2], key[3], key[4],
-		  key[5], key[6], key[7], key[8], key[9]);
-	ice_debug(rt->psr->hw, ICE_DBG_PARSER, "\n");
+	ice_debug_array_w_prefix(rt->psr->hw, ICE_DBG_PARSER,
+				 KBUILD_MODNAME "Generated Boost TCAM Key",
+				 key, ICE_BST_TCAM_KEY_SIZE);
 }
 
 static u16 ice_bit_rev_u16(u16 v, int len)