| Message ID | 20240710204015.124233-11-ahmed.zaki@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ice: iavf: add support for TC U32 filters on VFs | expand |
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
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.
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
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 --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;