| Message ID | 20200131133905.42518-1-anthony.l.nguyen@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [S37,v2,01/15] ice: Fix DCB rebuild after reset | expand |
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of > Tony Nguyen > Sent: Friday, January 31, 2020 5:39 AM > To: intel-wired-lan@lists.osuosl.org > Subject: [Intel-wired-lan] [PATCH S37 v2 01/15] ice: Fix DCB rebuild after reset > > From: Dave Ertman <david.m.ertman@intel.com> > > The function ice_dcb_rebuild had some logic > flaws in it, and also didn't differentiate > between FW and SW modes needs. > > For FW flow, the willing setting was being > forced to OFF and left that way. Unwilling > in DCB FW mode is not a supported model. > > Leave the config alone and use the return value > from the set command to determine if setting the > config was successful. > > The SW DCB flow does not need to need to register > for MIB change events (as they are not used in > SW mode). > > Use !is_sw_lldp checks to only perform FW specific > task while in FW mode. > > Also adding a reapplication of the current DCB > config after a link event. Some NVMs are not > maintaining their DCB configs across link events. > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > --- > v2: > - Add missing mutex_unlock() in error path > --- > drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 83 +++++++++----------- > drivers/net/ethernet/intel/ice/ice_main.c | 1 + > 2 files changed, 36 insertions(+), 48 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > index 9401f2051293..8e96c722b065 100644 > --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > @@ -339,9 +339,9 @@ ice_dcb_need_recfg(struct ice_pf *pf, struct > ice_dcbx_cfg *old_cfg, > */ > void ice_dcb_rebuild(struct ice_pf *pf) > { > - struct ice_dcbx_cfg *local_dcbx_cfg, *desired_dcbx_cfg, *prev_cfg; > struct ice_aqc_port_ets_elem buf = { 0 }; > struct device *dev = ice_pf_to_dev(pf); > + struct ice_dcbx_cfg *err_cfg; > enum ice_status ret; > > ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL); > @@ -354,53 +354,25 @@ void ice_dcb_rebuild(struct ice_pf *pf) > if (!test_bit(ICE_FLAG_DCB_ENA, pf->flags)) > return; > > - local_dcbx_cfg = &pf->hw.port_info->local_dcbx_cfg; > - desired_dcbx_cfg = &pf->hw.port_info->desired_dcbx_cfg; > + mutex_lock(&pf->tc_mutex); > > - /* Save current willing state and force FW to unwilling */ > - local_dcbx_cfg->etscfg.willing = 0x0; > - local_dcbx_cfg->pfc.willing = 0x0; > - local_dcbx_cfg->app_mode = ICE_DCBX_APPS_NON_WILLING; > + if (!pf->hw.port_info->is_sw_lldp) > + ice_cfg_etsrec_defaults(pf->hw.port_info); > > - ice_cfg_etsrec_defaults(pf->hw.port_info); > ret = ice_set_dcb_cfg(pf->hw.port_info); > if (ret) { > - dev_err(dev, "Failed to set DCB to unwilling\n"); > + dev_err(dev, "Failed to set DCB config in rebuild\n"); > goto dcb_error; > } > > - /* Retrieve DCB config and ensure same as current in SW */ > - prev_cfg = kmemdup(local_dcbx_cfg, sizeof(*prev_cfg), GFP_KERNEL); > - if (!prev_cfg) > - goto dcb_error; > - > - ice_init_dcb(&pf->hw, true); > - if (pf->hw.port_info->dcbx_status == ICE_DCBX_STATUS_DIS) > - pf->hw.port_info->is_sw_lldp = true; > - else > - pf->hw.port_info->is_sw_lldp = false; > - > - if (ice_dcb_need_recfg(pf, prev_cfg, local_dcbx_cfg)) { > - /* difference in cfg detected - disable DCB till next MIB */ > - dev_err(dev, "Set local MIB not accurate\n"); > - kfree(prev_cfg); > - goto dcb_error; > + if (!pf->hw.port_info->is_sw_lldp) { > + ret = ice_cfg_lldp_mib_change(&pf->hw, true); > + if (ret && !pf->hw.port_info->is_sw_lldp) { > + dev_err(dev, "Failed to register for MIB changes\n"); > + goto dcb_error; > + } > } > > - /* fetched config congruent to previous configuration */ > - kfree(prev_cfg); > - > - /* Set the local desired config */ > - if (local_dcbx_cfg->dcbx_mode == ICE_DCBX_MODE_CEE) > - memcpy(local_dcbx_cfg, desired_dcbx_cfg, > - sizeof(*local_dcbx_cfg)); > - > - ice_cfg_etsrec_defaults(pf->hw.port_info); > - ret = ice_set_dcb_cfg(pf->hw.port_info); > - if (ret) { > - dev_err(dev, "Failed to set desired config\n"); > - goto dcb_error; > - } > dev_info(dev, "DCB restored after reset\n"); > ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL); > if (ret) { > @@ -408,26 +380,32 @@ void ice_dcb_rebuild(struct ice_pf *pf) > goto dcb_error; > } > > + mutex_unlock(&pf->tc_mutex); > + > return; > > dcb_error: > dev_err(dev, "Disabling DCB until new settings occur\n"); > - prev_cfg = kzalloc(sizeof(*prev_cfg), GFP_KERNEL); > - if (!prev_cfg) > + err_cfg = kzalloc(sizeof(*err_cfg), GFP_KERNEL); > + if (!err_cfg) { > + mutex_unlock(&pf->tc_mutex); > return; > + } > > - prev_cfg->etscfg.willing = true; > - prev_cfg->etscfg.tcbwtable[0] = ICE_TC_MAX_BW; > - prev_cfg->etscfg.tsatable[0] = ICE_IEEE_TSA_ETS; > - memcpy(&prev_cfg->etsrec, &prev_cfg->etscfg, sizeof(prev_cfg- > >etsrec)); > + err_cfg->etscfg.willing = true; > + err_cfg->etscfg.tcbwtable[0] = ICE_TC_MAX_BW; > + err_cfg->etscfg.tsatable[0] = ICE_IEEE_TSA_ETS; > + memcpy(&err_cfg->etsrec, &err_cfg->etscfg, sizeof(err_cfg->etsrec)); > /* Coverity warns the return code of ice_pf_dcb_cfg() is not checked > * here as is done for other calls to that function. That check is > * not necessary since this is in this function's error cleanup path. > * Suppress the Coverity warning with the following comment... > */ > /* coverity[check_return] */ > - ice_pf_dcb_cfg(pf, prev_cfg, false); > - kfree(prev_cfg); > + ice_pf_dcb_cfg(pf, err_cfg, false); > + kfree(err_cfg); > + > + mutex_unlock(&pf->tc_mutex); > } > > /** > @@ -842,6 +820,8 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf > *pf, > } > } > > + mutex_lock(&pf->tc_mutex); This lock is unlocked with most but not all return() function exit points. That needs fixing, and it would be better if there was a single function exit point where things like the locked mutex and locked rtnl are unrolled. > + > /* store the old configuration */ > tmp_dcbx_cfg = pf->hw.port_info->local_dcbx_cfg; > > @@ -852,20 +832,24 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf > *pf, > ret = ice_get_dcb_cfg(pf->hw.port_info); > if (ret) { > dev_err(dev, "Failed to get DCB config\n"); > + mutex_unlock(&pf->tc_mutex); > return; > } > > /* No change detected in DCBX configs */ > if (!memcmp(&tmp_dcbx_cfg, &pi->local_dcbx_cfg, > sizeof(tmp_dcbx_cfg))) { > dev_dbg(dev, "No change detected in DCBX configuration.\n"); > + mutex_unlock(&pf->tc_mutex); > return; > } > > need_reconfig = ice_dcb_need_recfg(pf, &tmp_dcbx_cfg, > &pi->local_dcbx_cfg); > ice_dcbnl_flush_apps(pf, &tmp_dcbx_cfg, &pi->local_dcbx_cfg); > - if (!need_reconfig) > + if (!need_reconfig) { > + mutex_unlock(&pf->tc_mutex); > return; > + } > > /* Enable DCB tagging only when more than one TC */ > if (ice_dcb_get_num_tc(&pi->local_dcbx_cfg) > 1) { > @@ -879,6 +863,7 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf > *pf, > pf_vsi = ice_get_main_vsi(pf); > if (!pf_vsi) { > dev_dbg(dev, "PF VSI doesn't exist\n"); > + mutex_unlock(&pf->tc_mutex); > return; > } > > @@ -889,6 +874,7 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf > *pf, > if (ret) { > dev_err(dev, "Query Port ETS failed\n"); > rtnl_unlock(); > + mutex_unlock(&pf->tc_mutex); > return; > } > > @@ -897,4 +883,5 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf > *pf, > > ice_ena_vsi(pf_vsi, true); > rtnl_unlock(); > + mutex_unlock(&pf->tc_mutex); > } > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c > b/drivers/net/ethernet/intel/ice/ice_main.c > index 6ce422789df7..d4bc6fd3321c 100644 > --- a/drivers/net/ethernet/intel/ice/ice_main.c > +++ b/drivers/net/ethernet/intel/ice/ice_main.c > @@ -844,6 +844,7 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info > *pi, bool link_up, > } > } > > + ice_dcb_rebuild(pf); > ice_vsi_link_event(vsi, link_up); > ice_print_link_msg(vsi, link_up); > > -- > 2.20.1 > > _______________________________________________ > Intel-wired-lan mailing list > Intel-wired-lan@osuosl.org > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
On Tue, 2020-02-04 at 09:17 -0800, Allan, Bruce W wrote: > > -----Original Message----- > > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On > > Behalf Of > > Tony Nguyen > > Sent: Friday, January 31, 2020 5:39 AM > > To: intel-wired-lan@lists.osuosl.org > > Subject: [Intel-wired-lan] [PATCH S37 v2 01/15] ice: Fix DCB > > rebuild after reset > > > > From: Dave Ertman <david.m.ertman@intel.com> > > > > The function ice_dcb_rebuild had some logic > > flaws in it, and also didn't differentiate > > between FW and SW modes needs. > > > > For FW flow, the willing setting was being > > forced to OFF and left that way. Unwilling > > in DCB FW mode is not a supported model. > > > > Leave the config alone and use the return value > > from the set command to determine if setting the > > config was successful. > > > > The SW DCB flow does not need to need to register > > for MIB change events (as they are not used in > > SW mode). > > > > Use !is_sw_lldp checks to only perform FW specific > > task while in FW mode. > > > > Also adding a reapplication of the current DCB > > config after a link event. Some NVMs are not > > maintaining their DCB configs across link events. > > > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > > --- > > v2: > > - Add missing mutex_unlock() in error path > > --- > > drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 83 +++++++++------- > > ---- > > drivers/net/ethernet/intel/ice/ice_main.c | 1 + > > 2 files changed, 36 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > > b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > > index 9401f2051293..8e96c722b065 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > > +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > > @@ -339,9 +339,9 @@ ice_dcb_need_recfg(struct ice_pf *pf, struct > > ice_dcbx_cfg *old_cfg, > > */ > > void ice_dcb_rebuild(struct ice_pf *pf) > > { > > - struct ice_dcbx_cfg *local_dcbx_cfg, *desired_dcbx_cfg, > > *prev_cfg; > > struct ice_aqc_port_ets_elem buf = { 0 }; > > struct device *dev = ice_pf_to_dev(pf); > > + struct ice_dcbx_cfg *err_cfg; > > enum ice_status ret; > > > > ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), > > NULL); > > @@ -354,53 +354,25 @@ void ice_dcb_rebuild(struct ice_pf *pf) > > if (!test_bit(ICE_FLAG_DCB_ENA, pf->flags)) > > return; > > > > - local_dcbx_cfg = &pf->hw.port_info->local_dcbx_cfg; > > - desired_dcbx_cfg = &pf->hw.port_info->desired_dcbx_cfg; > > + mutex_lock(&pf->tc_mutex); > > > > - /* Save current willing state and force FW to unwilling */ > > - local_dcbx_cfg->etscfg.willing = 0x0; > > - local_dcbx_cfg->pfc.willing = 0x0; > > - local_dcbx_cfg->app_mode = ICE_DCBX_APPS_NON_WILLING; > > + if (!pf->hw.port_info->is_sw_lldp) > > + ice_cfg_etsrec_defaults(pf->hw.port_info); > > > > - ice_cfg_etsrec_defaults(pf->hw.port_info); > > ret = ice_set_dcb_cfg(pf->hw.port_info); > > if (ret) { > > - dev_err(dev, "Failed to set DCB to unwilling\n"); > > + dev_err(dev, "Failed to set DCB config in rebuild\n"); > > goto dcb_error; > > } > > > > - /* Retrieve DCB config and ensure same as current in SW */ > > - prev_cfg = kmemdup(local_dcbx_cfg, sizeof(*prev_cfg), > > GFP_KERNEL); > > - if (!prev_cfg) > > - goto dcb_error; > > - > > - ice_init_dcb(&pf->hw, true); > > - if (pf->hw.port_info->dcbx_status == ICE_DCBX_STATUS_DIS) > > - pf->hw.port_info->is_sw_lldp = true; > > - else > > - pf->hw.port_info->is_sw_lldp = false; > > - > > - if (ice_dcb_need_recfg(pf, prev_cfg, local_dcbx_cfg)) { > > - /* difference in cfg detected - disable DCB till next > > MIB */ > > - dev_err(dev, "Set local MIB not accurate\n"); > > - kfree(prev_cfg); > > - goto dcb_error; > > + if (!pf->hw.port_info->is_sw_lldp) { > > + ret = ice_cfg_lldp_mib_change(&pf->hw, true); > > + if (ret && !pf->hw.port_info->is_sw_lldp) { > > + dev_err(dev, "Failed to register for MIB > > changes\n"); > > + goto dcb_error; > > + } > > } > > > > - /* fetched config congruent to previous configuration */ > > - kfree(prev_cfg); > > - > > - /* Set the local desired config */ > > - if (local_dcbx_cfg->dcbx_mode == ICE_DCBX_MODE_CEE) > > - memcpy(local_dcbx_cfg, desired_dcbx_cfg, > > - sizeof(*local_dcbx_cfg)); > > - > > - ice_cfg_etsrec_defaults(pf->hw.port_info); > > - ret = ice_set_dcb_cfg(pf->hw.port_info); > > - if (ret) { > > - dev_err(dev, "Failed to set desired config\n"); > > - goto dcb_error; > > - } > > dev_info(dev, "DCB restored after reset\n"); > > ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), > > NULL); > > if (ret) { > > @@ -408,26 +380,32 @@ void ice_dcb_rebuild(struct ice_pf *pf) > > goto dcb_error; > > } > > > > + mutex_unlock(&pf->tc_mutex); > > + > > return; > > > > dcb_error: > > dev_err(dev, "Disabling DCB until new settings occur\n"); > > - prev_cfg = kzalloc(sizeof(*prev_cfg), GFP_KERNEL); > > - if (!prev_cfg) > > + err_cfg = kzalloc(sizeof(*err_cfg), GFP_KERNEL); > > + if (!err_cfg) { > > + mutex_unlock(&pf->tc_mutex); > > return; > > + } > > > > - prev_cfg->etscfg.willing = true; > > - prev_cfg->etscfg.tcbwtable[0] = ICE_TC_MAX_BW; > > - prev_cfg->etscfg.tsatable[0] = ICE_IEEE_TSA_ETS; > > - memcpy(&prev_cfg->etsrec, &prev_cfg->etscfg, sizeof(prev_cfg- > > > etsrec)); > > > > + err_cfg->etscfg.willing = true; > > + err_cfg->etscfg.tcbwtable[0] = ICE_TC_MAX_BW; > > + err_cfg->etscfg.tsatable[0] = ICE_IEEE_TSA_ETS; > > + memcpy(&err_cfg->etsrec, &err_cfg->etscfg, sizeof(err_cfg- > > >etsrec)); > > /* Coverity warns the return code of ice_pf_dcb_cfg() is not > > checked > > * here as is done for other calls to that function. That check > > is > > * not necessary since this is in this function's error cleanup > > path. > > * Suppress the Coverity warning with the following comment... > > */ > > /* coverity[check_return] */ > > - ice_pf_dcb_cfg(pf, prev_cfg, false); > > - kfree(prev_cfg); > > + ice_pf_dcb_cfg(pf, err_cfg, false); > > + kfree(err_cfg); > > + > > + mutex_unlock(&pf->tc_mutex); > > } > > > > /** > > @@ -842,6 +820,8 @@ ice_dcb_process_lldp_set_mib_change(struct > > ice_pf > > *pf, > > } > > } > > > > + mutex_lock(&pf->tc_mutex); > > This lock is unlocked with most but not all return() function exit > points. That needs fixing, > and it would be better if there was a single function exit point > where things like the locked > mutex and locked rtnl are unrolled. Thanks for the feedback Bruce. I'll make those changes and send out a v3. -Tony
> -----Original Message----- > From: Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Sent: Tuesday, February 04, 2020 9:57 AM > To: Allan, Bruce W <bruce.w.allan@intel.com>; intel-wired- > lan@lists.osuosl.org > Cc: Ertman, David M <david.m.ertman@intel.com> > Subject: Re: [Intel-wired-lan] [PATCH S37 v2 01/15] ice: Fix DCB rebuild after > reset > > On Tue, 2020-02-04 at 09:17 -0800, Allan, Bruce W wrote: > > > -----Original Message----- > > > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On > > > Behalf Of > > > Tony Nguyen > > > Sent: Friday, January 31, 2020 5:39 AM > > > To: intel-wired-lan@lists.osuosl.org > > > Subject: [Intel-wired-lan] [PATCH S37 v2 01/15] ice: Fix DCB > > > rebuild after reset > > > > > > From: Dave Ertman <david.m.ertman@intel.com> > > > > > > The function ice_dcb_rebuild had some logic > > > flaws in it, and also didn't differentiate > > > between FW and SW modes needs. > > > > > > For FW flow, the willing setting was being > > > forced to OFF and left that way. Unwilling > > > in DCB FW mode is not a supported model. > > > > > > Leave the config alone and use the return value > > > from the set command to determine if setting the > > > config was successful. > > > > > > The SW DCB flow does not need to need to register > > > for MIB change events (as they are not used in > > > SW mode). > > > > > > Use !is_sw_lldp checks to only perform FW specific > > > task while in FW mode. > > > > > > Also adding a reapplication of the current DCB > > > config after a link event. Some NVMs are not > > > maintaining their DCB configs across link events. > > > > > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > > > --- > > > v2: > > > - Add missing mutex_unlock() in error path > > > --- > > > drivers/net/ethernet/intel/ice/ice_dcb_lib.c | 83 +++++++++------- > > > ---- > > > drivers/net/ethernet/intel/ice/ice_main.c | 1 + > > > 2 files changed, 36 insertions(+), 48 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > > > b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > > > index 9401f2051293..8e96c722b065 100644 > > > --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > > > +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c > > > @@ -339,9 +339,9 @@ ice_dcb_need_recfg(struct ice_pf *pf, struct > > > ice_dcbx_cfg *old_cfg, > > > */ > > > void ice_dcb_rebuild(struct ice_pf *pf) > > > { > > > - struct ice_dcbx_cfg *local_dcbx_cfg, *desired_dcbx_cfg, > > > *prev_cfg; > > > struct ice_aqc_port_ets_elem buf = { 0 }; > > > struct device *dev = ice_pf_to_dev(pf); > > > + struct ice_dcbx_cfg *err_cfg; > > > enum ice_status ret; > > > > > > ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), > > > NULL); > > > @@ -354,53 +354,25 @@ void ice_dcb_rebuild(struct ice_pf *pf) > > > if (!test_bit(ICE_FLAG_DCB_ENA, pf->flags)) > > > return; > > > > > > - local_dcbx_cfg = &pf->hw.port_info->local_dcbx_cfg; > > > - desired_dcbx_cfg = &pf->hw.port_info->desired_dcbx_cfg; > > > + mutex_lock(&pf->tc_mutex); > > > > > > - /* Save current willing state and force FW to unwilling */ > > > - local_dcbx_cfg->etscfg.willing = 0x0; > > > - local_dcbx_cfg->pfc.willing = 0x0; > > > - local_dcbx_cfg->app_mode = ICE_DCBX_APPS_NON_WILLING; > > > + if (!pf->hw.port_info->is_sw_lldp) > > > + ice_cfg_etsrec_defaults(pf->hw.port_info); > > > > > > - ice_cfg_etsrec_defaults(pf->hw.port_info); > > > ret = ice_set_dcb_cfg(pf->hw.port_info); > > > if (ret) { > > > - dev_err(dev, "Failed to set DCB to unwilling\n"); > > > + dev_err(dev, "Failed to set DCB config in rebuild\n"); > > > goto dcb_error; > > > } > > > > > > - /* Retrieve DCB config and ensure same as current in SW */ > > > - prev_cfg = kmemdup(local_dcbx_cfg, sizeof(*prev_cfg), > > > GFP_KERNEL); > > > - if (!prev_cfg) > > > - goto dcb_error; > > > - > > > - ice_init_dcb(&pf->hw, true); > > > - if (pf->hw.port_info->dcbx_status == ICE_DCBX_STATUS_DIS) > > > - pf->hw.port_info->is_sw_lldp = true; > > > - else > > > - pf->hw.port_info->is_sw_lldp = false; > > > - > > > - if (ice_dcb_need_recfg(pf, prev_cfg, local_dcbx_cfg)) { > > > - /* difference in cfg detected - disable DCB till next > > > MIB */ > > > - dev_err(dev, "Set local MIB not accurate\n"); > > > - kfree(prev_cfg); > > > - goto dcb_error; > > > + if (!pf->hw.port_info->is_sw_lldp) { > > > + ret = ice_cfg_lldp_mib_change(&pf->hw, true); > > > + if (ret && !pf->hw.port_info->is_sw_lldp) { > > > + dev_err(dev, "Failed to register for MIB > > > changes\n"); > > > + goto dcb_error; > > > + } > > > } > > > > > > - /* fetched config congruent to previous configuration */ > > > - kfree(prev_cfg); > > > - > > > - /* Set the local desired config */ > > > - if (local_dcbx_cfg->dcbx_mode == ICE_DCBX_MODE_CEE) > > > - memcpy(local_dcbx_cfg, desired_dcbx_cfg, > > > - sizeof(*local_dcbx_cfg)); > > > - > > > - ice_cfg_etsrec_defaults(pf->hw.port_info); > > > - ret = ice_set_dcb_cfg(pf->hw.port_info); > > > - if (ret) { > > > - dev_err(dev, "Failed to set desired config\n"); > > > - goto dcb_error; > > > - } > > > dev_info(dev, "DCB restored after reset\n"); > > > ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), > > > NULL); > > > if (ret) { > > > @@ -408,26 +380,32 @@ void ice_dcb_rebuild(struct ice_pf *pf) > > > goto dcb_error; > > > } > > > > > > + mutex_unlock(&pf->tc_mutex); > > > + > > > return; > > > > > > dcb_error: > > > dev_err(dev, "Disabling DCB until new settings occur\n"); > > > - prev_cfg = kzalloc(sizeof(*prev_cfg), GFP_KERNEL); > > > - if (!prev_cfg) > > > + err_cfg = kzalloc(sizeof(*err_cfg), GFP_KERNEL); > > > + if (!err_cfg) { > > > + mutex_unlock(&pf->tc_mutex); > > > return; > > > + } > > > > > > - prev_cfg->etscfg.willing = true; > > > - prev_cfg->etscfg.tcbwtable[0] = ICE_TC_MAX_BW; > > > - prev_cfg->etscfg.tsatable[0] = ICE_IEEE_TSA_ETS; > > > - memcpy(&prev_cfg->etsrec, &prev_cfg->etscfg, sizeof(prev_cfg- > > > > etsrec)); > > > > > > + err_cfg->etscfg.willing = true; > > > + err_cfg->etscfg.tcbwtable[0] = ICE_TC_MAX_BW; > > > + err_cfg->etscfg.tsatable[0] = ICE_IEEE_TSA_ETS; > > > + memcpy(&err_cfg->etsrec, &err_cfg->etscfg, sizeof(err_cfg- > > > >etsrec)); > > > /* Coverity warns the return code of ice_pf_dcb_cfg() is not > > > checked > > > * here as is done for other calls to that function. That check > > > is > > > * not necessary since this is in this function's error cleanup > > > path. > > > * Suppress the Coverity warning with the following comment... > > > */ > > > /* coverity[check_return] */ > > > - ice_pf_dcb_cfg(pf, prev_cfg, false); > > > - kfree(prev_cfg); > > > + ice_pf_dcb_cfg(pf, err_cfg, false); > > > + kfree(err_cfg); > > > + > > > + mutex_unlock(&pf->tc_mutex); > > > } > > > > > > /** > > > @@ -842,6 +820,8 @@ ice_dcb_process_lldp_set_mib_change(struct > > > ice_pf > > > *pf, > > > } > > > } > > > > > > + mutex_lock(&pf->tc_mutex); > > > > This lock is unlocked with most but not all return() function exit > > points. That needs fixing, > > and it would be better if there was a single function exit point > > where things like the locked > > mutex and locked rtnl are unrolled. > > Thanks for the feedback Bruce. I'll make those changes and send out a > v3. > > -Tony To clarify, I meant there is one return() function exit point _after_ the mutex is locked that does not unlock the mutex. The return()'s _before_ the mutex_lock() clearly do not need to unlock the mutex, and can be left as-is. Sorry for not making that clear. Bruce.
diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c index 9401f2051293..8e96c722b065 100644 --- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c @@ -339,9 +339,9 @@ ice_dcb_need_recfg(struct ice_pf *pf, struct ice_dcbx_cfg *old_cfg, */ void ice_dcb_rebuild(struct ice_pf *pf) { - struct ice_dcbx_cfg *local_dcbx_cfg, *desired_dcbx_cfg, *prev_cfg; struct ice_aqc_port_ets_elem buf = { 0 }; struct device *dev = ice_pf_to_dev(pf); + struct ice_dcbx_cfg *err_cfg; enum ice_status ret; ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL); @@ -354,53 +354,25 @@ void ice_dcb_rebuild(struct ice_pf *pf) if (!test_bit(ICE_FLAG_DCB_ENA, pf->flags)) return; - local_dcbx_cfg = &pf->hw.port_info->local_dcbx_cfg; - desired_dcbx_cfg = &pf->hw.port_info->desired_dcbx_cfg; + mutex_lock(&pf->tc_mutex); - /* Save current willing state and force FW to unwilling */ - local_dcbx_cfg->etscfg.willing = 0x0; - local_dcbx_cfg->pfc.willing = 0x0; - local_dcbx_cfg->app_mode = ICE_DCBX_APPS_NON_WILLING; + if (!pf->hw.port_info->is_sw_lldp) + ice_cfg_etsrec_defaults(pf->hw.port_info); - ice_cfg_etsrec_defaults(pf->hw.port_info); ret = ice_set_dcb_cfg(pf->hw.port_info); if (ret) { - dev_err(dev, "Failed to set DCB to unwilling\n"); + dev_err(dev, "Failed to set DCB config in rebuild\n"); goto dcb_error; } - /* Retrieve DCB config and ensure same as current in SW */ - prev_cfg = kmemdup(local_dcbx_cfg, sizeof(*prev_cfg), GFP_KERNEL); - if (!prev_cfg) - goto dcb_error; - - ice_init_dcb(&pf->hw, true); - if (pf->hw.port_info->dcbx_status == ICE_DCBX_STATUS_DIS) - pf->hw.port_info->is_sw_lldp = true; - else - pf->hw.port_info->is_sw_lldp = false; - - if (ice_dcb_need_recfg(pf, prev_cfg, local_dcbx_cfg)) { - /* difference in cfg detected - disable DCB till next MIB */ - dev_err(dev, "Set local MIB not accurate\n"); - kfree(prev_cfg); - goto dcb_error; + if (!pf->hw.port_info->is_sw_lldp) { + ret = ice_cfg_lldp_mib_change(&pf->hw, true); + if (ret && !pf->hw.port_info->is_sw_lldp) { + dev_err(dev, "Failed to register for MIB changes\n"); + goto dcb_error; + } } - /* fetched config congruent to previous configuration */ - kfree(prev_cfg); - - /* Set the local desired config */ - if (local_dcbx_cfg->dcbx_mode == ICE_DCBX_MODE_CEE) - memcpy(local_dcbx_cfg, desired_dcbx_cfg, - sizeof(*local_dcbx_cfg)); - - ice_cfg_etsrec_defaults(pf->hw.port_info); - ret = ice_set_dcb_cfg(pf->hw.port_info); - if (ret) { - dev_err(dev, "Failed to set desired config\n"); - goto dcb_error; - } dev_info(dev, "DCB restored after reset\n"); ret = ice_query_port_ets(pf->hw.port_info, &buf, sizeof(buf), NULL); if (ret) { @@ -408,26 +380,32 @@ void ice_dcb_rebuild(struct ice_pf *pf) goto dcb_error; } + mutex_unlock(&pf->tc_mutex); + return; dcb_error: dev_err(dev, "Disabling DCB until new settings occur\n"); - prev_cfg = kzalloc(sizeof(*prev_cfg), GFP_KERNEL); - if (!prev_cfg) + err_cfg = kzalloc(sizeof(*err_cfg), GFP_KERNEL); + if (!err_cfg) { + mutex_unlock(&pf->tc_mutex); return; + } - prev_cfg->etscfg.willing = true; - prev_cfg->etscfg.tcbwtable[0] = ICE_TC_MAX_BW; - prev_cfg->etscfg.tsatable[0] = ICE_IEEE_TSA_ETS; - memcpy(&prev_cfg->etsrec, &prev_cfg->etscfg, sizeof(prev_cfg->etsrec)); + err_cfg->etscfg.willing = true; + err_cfg->etscfg.tcbwtable[0] = ICE_TC_MAX_BW; + err_cfg->etscfg.tsatable[0] = ICE_IEEE_TSA_ETS; + memcpy(&err_cfg->etsrec, &err_cfg->etscfg, sizeof(err_cfg->etsrec)); /* Coverity warns the return code of ice_pf_dcb_cfg() is not checked * here as is done for other calls to that function. That check is * not necessary since this is in this function's error cleanup path. * Suppress the Coverity warning with the following comment... */ /* coverity[check_return] */ - ice_pf_dcb_cfg(pf, prev_cfg, false); - kfree(prev_cfg); + ice_pf_dcb_cfg(pf, err_cfg, false); + kfree(err_cfg); + + mutex_unlock(&pf->tc_mutex); } /** @@ -842,6 +820,8 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf, } } + mutex_lock(&pf->tc_mutex); + /* store the old configuration */ tmp_dcbx_cfg = pf->hw.port_info->local_dcbx_cfg; @@ -852,20 +832,24 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf, ret = ice_get_dcb_cfg(pf->hw.port_info); if (ret) { dev_err(dev, "Failed to get DCB config\n"); + mutex_unlock(&pf->tc_mutex); return; } /* No change detected in DCBX configs */ if (!memcmp(&tmp_dcbx_cfg, &pi->local_dcbx_cfg, sizeof(tmp_dcbx_cfg))) { dev_dbg(dev, "No change detected in DCBX configuration.\n"); + mutex_unlock(&pf->tc_mutex); return; } need_reconfig = ice_dcb_need_recfg(pf, &tmp_dcbx_cfg, &pi->local_dcbx_cfg); ice_dcbnl_flush_apps(pf, &tmp_dcbx_cfg, &pi->local_dcbx_cfg); - if (!need_reconfig) + if (!need_reconfig) { + mutex_unlock(&pf->tc_mutex); return; + } /* Enable DCB tagging only when more than one TC */ if (ice_dcb_get_num_tc(&pi->local_dcbx_cfg) > 1) { @@ -879,6 +863,7 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf, pf_vsi = ice_get_main_vsi(pf); if (!pf_vsi) { dev_dbg(dev, "PF VSI doesn't exist\n"); + mutex_unlock(&pf->tc_mutex); return; } @@ -889,6 +874,7 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf, if (ret) { dev_err(dev, "Query Port ETS failed\n"); rtnl_unlock(); + mutex_unlock(&pf->tc_mutex); return; } @@ -897,4 +883,5 @@ ice_dcb_process_lldp_set_mib_change(struct ice_pf *pf, ice_ena_vsi(pf_vsi, true); rtnl_unlock(); + mutex_unlock(&pf->tc_mutex); } diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c index 6ce422789df7..d4bc6fd3321c 100644 --- a/drivers/net/ethernet/intel/ice/ice_main.c +++ b/drivers/net/ethernet/intel/ice/ice_main.c @@ -844,6 +844,7 @@ ice_link_event(struct ice_pf *pf, struct ice_port_info *pi, bool link_up, } } + ice_dcb_rebuild(pf); ice_vsi_link_event(vsi, link_up); ice_print_link_msg(vsi, link_up);