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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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@intel.com>
> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com>
> Signed-off-by: Leyi Rong <leyi.rong@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.

> +		}
> +	}
> +}
> +
> +/**
> + * 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

> +			/* 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.

> +}
> +
>   /**
>    * 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.

>   }
>