[dpdk-dev] [PATCH 14/49] net/ice/base: refactor HW table init function

Stillwell Jr, Paul M paul.m.stillwell.jr at intel.com
Wed Jun 5 20:10:33 CEST 2019


> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin at redhat.com>
> Sent: Wednesday, June 5, 2019 3:35 AM
> To: Rong, Leyi <leyi.rong at intel.com>; Zhang, Qi Z <qi.z.zhang at intel.com>
> Cc: dev at dpdk.org; Sridhar, Vignesh <vignesh.sridhar at intel.com>; Stillwell Jr,
> Paul M <paul.m.stillwell.jr at intel.com>
> Subject: Re: [dpdk-dev] [PATCH 14/49] net/ice/base: refactor HW table init
> function
> 
> 
> 
> On 6/4/19 7:42 AM, Leyi Rong wrote:
> > 1. Separated the calls to initialize and allocate the HW XLT tables
> > from call to fill table. This is to allow the ice_init_hw_tbls call to
> > be made prior to package download so that all HW structures are
> > correctly initialized. This will avoid any invalid memory references
> > if package download fails on unloading the driver.
> > 2. Fill HW tables with package content after successful package download.
> > 3. Free HW table and flow profile allocations when unloading driver.
> > 4. Add flag in block structure to check if lists in block are
> > initialized. This is to avoid any NULL reference in releasing flow
> > profiles that may have been freed in previous calls to free tables.
> >
> > Signed-off-by: Vignesh Sridhar <vignesh.sridhar at intel.com>
> > Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr at intel.com>
> > Signed-off-by: Leyi Rong <leyi.rong at intel.com>
> > ---
> >   drivers/net/ice/base/ice_common.c    |   6 +-
> >   drivers/net/ice/base/ice_flex_pipe.c | 284 ++++++++++++++-------------
> >   drivers/net/ice/base/ice_flex_pipe.h |   1 +
> >   drivers/net/ice/base/ice_flex_type.h |   1 +
> >   4 files changed, 151 insertions(+), 141 deletions(-)
> >
> > diff --git a/drivers/net/ice/base/ice_common.c
> > b/drivers/net/ice/base/ice_common.c
> > index a0ab25aef..62c7fad0d 100644
> > --- a/drivers/net/ice/base/ice_common.c
> > +++ b/drivers/net/ice/base/ice_common.c
> > @@ -916,12 +916,13 @@ enum ice_status ice_init_hw(struct ice_hw *hw)
> >
> >   	ice_init_flex_flds(hw, ICE_RXDID_FLEX_NIC);
> >   	ice_init_flex_flds(hw, ICE_RXDID_FLEX_NIC_2);
> > -
> >   	/* Obtain counter base index which would be used by flow director
> */
> >   	status = ice_alloc_fd_res_cntr(hw, &hw->fd_ctr_base);
> >   	if (status)
> >   		goto err_unroll_fltr_mgmt_struct;
> > -
> > +	status = ice_init_hw_tbls(hw);
> > +	if (status)
> > +		goto err_unroll_fltr_mgmt_struct;
> >   	return ICE_SUCCESS;
> >
> >   err_unroll_fltr_mgmt_struct:
> > @@ -952,6 +953,7 @@ void ice_deinit_hw(struct ice_hw *hw)
> >   	ice_sched_cleanup_all(hw);
> >   	ice_sched_clear_agg(hw);
> >   	ice_free_seg(hw);
> > +	ice_free_hw_tbls(hw);
> >
> >   	if (hw->port_info) {
> >   		ice_free(hw, hw->port_info);
> > diff --git a/drivers/net/ice/base/ice_flex_pipe.c
> > b/drivers/net/ice/base/ice_flex_pipe.c
> > index 8f0b513f4..93e056853 100644
> > --- a/drivers/net/ice/base/ice_flex_pipe.c
> > +++ b/drivers/net/ice/base/ice_flex_pipe.c
> > @@ -1375,10 +1375,12 @@ enum ice_status ice_init_pkg(struct ice_hw
> > *hw, u8 *buf, u32 len)
> >
> >   	if (!status) {
> >   		hw->seg = seg;
> > -		/* on successful package download, update other required
> > -		 * registers to support the package
> > +		/* on successful package download update other required
> > +		 * registers to support the package and fill HW tables
> > +		 * with package content.
> >   		 */
> >   		ice_init_pkg_regs(hw);
> > +		ice_fill_blk_tbls(hw);
> >   	} else {
> >   		ice_debug(hw, ICE_DBG_INIT, "package load failed, %d\n",
> >   			  status);
> > @@ -2755,6 +2757,65 @@ static const u32
> ice_blk_sids[ICE_BLK_COUNT][ICE_SID_OFF_COUNT] = {
> >   	}
> >   };
> >
> > +/**
> > + * ice_init_sw_xlt1_db - init software XLT1 database from HW tables
> > + * @hw: pointer to the hardware structure
> > + * @blk: the HW block to initialize
> > + */
> > +static
> > +void ice_init_sw_xlt1_db(struct ice_hw *hw, enum ice_block blk) {
> > +	u16 pt;
> > +
> > +	for (pt = 0; pt < hw->blk[blk].xlt1.count; pt++) {
> > +		u8 ptg;
> > +
> > +		ptg = hw->blk[blk].xlt1.t[pt];
> > +		if (ptg != ICE_DEFAULT_PTG) {
> > +			ice_ptg_alloc_val(hw, blk, ptg);
> > +			ice_ptg_add_mv_ptype(hw, blk, pt, ptg);
> 
> ice_ptg_add_mv_ptype() can fail, error should be propagated.

You are correct that ice_ptg_add_mv_ptype() can fail, but it can never fail in this case. This function is called at init time when the HW has been loaded to a known good state and there is no chance it will fail this call.

> 
> > +		}
> > +	}
> > +}
> > +
> > +/**
> > + * ice_init_sw_xlt2_db - init software XLT2 database from HW tables
> > + * @hw: pointer to the hardware structure
> > + * @blk: the HW block to initialize
> > + */
> > +static void ice_init_sw_xlt2_db(struct ice_hw *hw, enum ice_block
> > +blk) {
> > +	u16 vsi;
> > +
> > +	for (vsi = 0; vsi < hw->blk[blk].xlt2.count; vsi++) {
> > +		u16 vsig;
> > +
> > +		vsig = hw->blk[blk].xlt2.t[vsi];
> > +		if (vsig) {
> > +			ice_vsig_alloc_val(hw, blk, vsig);
> > +			ice_vsig_add_mv_vsi(hw, blk, vsi, vsig);
> 
> Ditto

Same issue here as above.

> 
> > +			/* no changes at this time, since this has been
> > +			 * initialized from the original package
> > +			 */
> > +			hw->blk[blk].xlt2.vsis[vsi].changed = 0;
> > +		}
> > +	}
> > +}
> > +
> > +/**
> > + * ice_init_sw_db - init software database from HW tables
> > + * @hw: pointer to the hardware structure  */ static void
> > +ice_init_sw_db(struct ice_hw *hw) {
> > +	u16 i;
> > +
> > +	for (i = 0; i < ICE_BLK_COUNT; i++) {
> > +		ice_init_sw_xlt1_db(hw, (enum ice_block)i);
> > +		ice_init_sw_xlt2_db(hw, (enum ice_block)i);
> > +	}
> 
> And so this function should also propagate the error.

Same comment here.

> 
> > +}
> > +
> >   /**
> >    * ice_fill_tbl - Reads content of a single table type into database
> >    * @hw: pointer to the hardware structure @@ -2853,12 +2914,12 @@
> > static void ice_fill_tbl(struct ice_hw *hw, enum ice_block block_id, u32 sid)
> >   		case ICE_SID_FLD_VEC_PE:
> >   			es = (struct ice_sw_fv_section *)sect;
> >   			src = (u8 *)es->fv;
> > -			sect_len = LE16_TO_CPU(es->count) *
> > -				hw->blk[block_id].es.fvw *
> > +			sect_len = (u32)(LE16_TO_CPU(es->count) *
> > +					 hw->blk[block_id].es.fvw) *
> >   				sizeof(*hw->blk[block_id].es.t);
> >   			dst = (u8 *)hw->blk[block_id].es.t;
> > -			dst_len = hw->blk[block_id].es.count *
> > -				hw->blk[block_id].es.fvw *
> > +			dst_len = (u32)(hw->blk[block_id].es.count *
> > +					hw->blk[block_id].es.fvw) *
> >   				sizeof(*hw->blk[block_id].es.t);
> >   			break;
> >   		default:
> > @@ -2886,75 +2947,61 @@ static void ice_fill_tbl(struct ice_hw *hw, enum
> ice_block block_id, u32 sid)
> >   }
> >
> >   /**
> > - * ice_fill_blk_tbls - Read package content for tables of a block
> > + * ice_fill_blk_tbls - Read package context for tables
> >    * @hw: pointer to the hardware structure
> > - * @block_id: The block ID which contains the tables to be copied
> >    *
> >    * Reads the current package contents and populates the driver
> > - * database with the data it contains to allow for advanced driver
> > - * features.
> > - */
> > -static void ice_fill_blk_tbls(struct ice_hw *hw, enum ice_block
> > block_id) -{
> > -	ice_fill_tbl(hw, block_id, hw->blk[block_id].xlt1.sid);
> > -	ice_fill_tbl(hw, block_id, hw->blk[block_id].xlt2.sid);
> > -	ice_fill_tbl(hw, block_id, hw->blk[block_id].prof.sid);
> > -	ice_fill_tbl(hw, block_id, hw->blk[block_id].prof_redir.sid);
> > -	ice_fill_tbl(hw, block_id, hw->blk[block_id].es.sid);
> > -}
> > -
> > -/**
> > - * ice_free_flow_profs - free flow profile entries
> > - * @hw: pointer to the hardware structure
> > + * database with the data iteratively for all advanced feature
> > + * blocks. Assume that the Hw tables have been allocated.
> >    */
> > -static void ice_free_flow_profs(struct ice_hw *hw)
> > +void ice_fill_blk_tbls(struct ice_hw *hw)
> >   {
> >   	u8 i;
> >
> >   	for (i = 0; i < ICE_BLK_COUNT; i++) {
> > -		struct ice_flow_prof *p, *tmp;
> > -
> > -		if (!&hw->fl_profs[i])
> > -			continue;
> > -
> > -		/* This call is being made as part of resource deallocation
> > -		 * during unload. Lock acquire and release will not be
> > -		 * necessary here.
> > -		 */
> > -		LIST_FOR_EACH_ENTRY_SAFE(p, tmp, &hw->fl_profs[i],
> > -					 ice_flow_prof, l_entry) {
> > -			struct ice_flow_entry *e, *t;
> > -
> > -			LIST_FOR_EACH_ENTRY_SAFE(e, t, &p->entries,
> > -						 ice_flow_entry, l_entry)
> > -				ice_flow_rem_entry(hw,
> ICE_FLOW_ENTRY_HNDL(e));
> > -
> > -			LIST_DEL(&p->l_entry);
> > -			if (p->acts)
> > -				ice_free(hw, p->acts);
> > -			ice_free(hw, p);
> > -		}
> > +		enum ice_block blk_id = (enum ice_block)i;
> >
> > -		ice_destroy_lock(&hw->fl_profs_locks[i]);
> > +		ice_fill_tbl(hw, blk_id, hw->blk[blk_id].xlt1.sid);
> > +		ice_fill_tbl(hw, blk_id, hw->blk[blk_id].xlt2.sid);
> > +		ice_fill_tbl(hw, blk_id, hw->blk[blk_id].prof.sid);
> > +		ice_fill_tbl(hw, blk_id, hw->blk[blk_id].prof_redir.sid);
> > +		ice_fill_tbl(hw, blk_id, hw->blk[blk_id].es.sid);
> 
> >   	}
> > +
> > +	ice_init_sw_db(hw);
> 
> Propagate error once above is fixed.

Same comment here

> 
> >   }
> >



More information about the dev mailing list