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/[email protected]
> > > [2]
> > > https://lore.kernel.org/intel-wired-lan/[email protected]
> > > [3]
> > > https://lore.kernel.org/intel-wired-lan/[email protected]
> > >
> > > Reported-by: Dan Carpenter <[email protected]>
> > > Closes:
> > > https://lore.kernel.org/intel-wired-lan/[email protected]
> > > Fixes: 9a4c07aaa0f5 ("ice: add parser execution main loop")
> > > CC: Ahmed Zaki <[email protected]>
> > > Reviewed-by: Larysa Zaremba <[email protected]>
> > > Signed-off-by: Przemek Kitszel <[email protected]>
> >
> > 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
> > >
> > >
>
