| Message ID | 20240523210424.2883960-1-david.m.ertman@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [iwl-net,v2] ice: check for unregistering correct number of devlink params | expand |
On 5/23/2024 2:04 PM, Dave Ertman wrote: > On module load, the ice driver checks for the lack of a specific PF > capability to determine if it should reduce the number of devlink params > to register. One situation when this test returns true is when the > driver loads in safe mode. The same check is not present on the unload > path when devlink params are unregistered. This results in the driver > triggering a WARN_ON in the kernel devlink code. > > The current check and code path uses a reduction in the number of elements > reported in the list of params. This is fragile and not good for future > maintaining. > > Change the parameters to be held in two lists, one always registered and > one dependent on the check. > > Add a symmetrical check in the unload path so that the correct parameters > are unregistered as well. > > Fixes: 109eb2917284 ("ice: Add tx_scheduling_layers devlink param") > CC: Lukasz Czapnik <lukasz.czapnik@intel.com> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > > --- > Changes in v2: break params into two list > clean up variable usage > --- Thanks for cleaning this up! Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Dave Ertman > Sent: Friday, May 24, 2024 2:34 AM > To: intel-wired-lan@lists.osuosl.org > Cc: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Czapnik, Lukasz <lukasz.czapnik@intel.com> > Subject: [Intel-wired-lan] [PATCH iwl-net v2] ice: check for unregistering correct number of devlink params > > On module load, the ice driver checks for the lack of a specific PF capability to determine if it should reduce the number of devlink params to register. One situation when this test returns true is when the driver loads in safe mode. The same check is not present on the unload path when devlink params are unregistered. This results in the driver triggering a WARN_ON in the kernel devlink code. > > The current check and code path uses a reduction in the number of elements reported in the list of params. This is fragile and not good for future maintaining. > > Change the parameters to be held in two lists, one always registered and one dependent on the check. > > Add a symmetrical check in the unload path so that the correct parameters are unregistered as well. > > Fixes: 109eb2917284 ("ice: Add tx_scheduling_layers devlink param") > CC: Lukasz Czapnik <lukasz.czapnik@intel.com> > Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> > Signed-off-by: Dave Ertman <david.m.ertman@intel.com> > > --- > Changes in v2: break params into two list clean up variable usage > --- > .../net/ethernet/intel/ice/devlink/devlink.c | 31 +++++++++++++------ > 1 file changed, 22 insertions(+), 9 deletions(-) > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c index c4b69655cdf5..704e9ad5144e 100644 --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c @@ -1388,7 +1388,7 @@ enum ice_param_id { ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, }; -static const struct devlink_param ice_devlink_params[] = { +static const struct devlink_param ice_dvl_rdma_params[] = { DEVLINK_PARAM_GENERIC(ENABLE_ROCE, BIT(DEVLINK_PARAM_CMODE_RUNTIME), ice_devlink_enable_roce_get, ice_devlink_enable_roce_set, @@ -1397,6 +1397,9 @@ static const struct devlink_param ice_devlink_params[] = { ice_devlink_enable_iw_get, ice_devlink_enable_iw_set, ice_devlink_enable_iw_validate), +}; + +static const struct devlink_param ice_dvl_sched_params[] = { DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, "tx_scheduling_layers", DEVLINK_PARAM_TYPE_U8, @@ -1464,21 +1467,31 @@ int ice_devlink_register_params(struct ice_pf *pf) { struct devlink *devlink = priv_to_devlink(pf); struct ice_hw *hw = &pf->hw; - size_t params_size; + int status; - params_size = ARRAY_SIZE(ice_devlink_params); + status = devl_params_register(devlink, ice_dvl_rdma_params, + ARRAY_SIZE(ice_dvl_rdma_params)); + if (status) + return status; - if (!hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) - params_size--; + if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) + status = devl_params_register(devlink, ice_dvl_sched_params, + ARRAY_SIZE(ice_dvl_sched_params)); - return devl_params_register(devlink, ice_devlink_params, - params_size); + return status; } void ice_devlink_unregister_params(struct ice_pf *pf) { - devl_params_unregister(priv_to_devlink(pf), ice_devlink_params, - ARRAY_SIZE(ice_devlink_params)); + struct devlink *devlink = priv_to_devlink(pf); + struct ice_hw *hw = &pf->hw; + + devl_params_unregister(devlink, ice_dvl_rdma_params, + ARRAY_SIZE(ice_dvl_rdma_params)); + + if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) + devl_params_unregister(devlink, ice_dvl_sched_params, + ARRAY_SIZE(ice_dvl_sched_params)); } #define ICE_DEVLINK_READ_BLK_SIZE (1024 * 1024)
On module load, the ice driver checks for the lack of a specific PF capability to determine if it should reduce the number of devlink params to register. One situation when this test returns true is when the driver loads in safe mode. The same check is not present on the unload path when devlink params are unregistered. This results in the driver triggering a WARN_ON in the kernel devlink code. The current check and code path uses a reduction in the number of elements reported in the list of params. This is fragile and not good for future maintaining. Change the parameters to be held in two lists, one always registered and one dependent on the check. Add a symmetrical check in the unload path so that the correct parameters are unregistered as well. Fixes: 109eb2917284 ("ice: Add tx_scheduling_layers devlink param") CC: Lukasz Czapnik <lukasz.czapnik@intel.com> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> Signed-off-by: Dave Ertman <david.m.ertman@intel.com> --- Changes in v2: break params into two list clean up variable usage --- .../net/ethernet/intel/ice/devlink/devlink.c | 31 +++++++++++++------ 1 file changed, 22 insertions(+), 9 deletions(-)