> -----Original Message-----
> From: Maxime Coquelin <maxime.coque...@redhat.com>
> Sent: Wednesday, June 5, 2019 3:35 AM
> To: Rong, Leyi <leyi.r...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>
> Cc: dev@dpdk.org; Sridhar, Vignesh <vignesh.srid...@intel.com>; Stillwell Jr,
> Paul M <paul.m.stillwell...@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.srid...@intel.com>
> > Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell...@intel.com>
> > Signed-off-by: Leyi Rong <leyi.r...@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
>
> > }
> >
