On 5/20/2024 3:20 PM, Dave Ertman wrote:
> On module load, the ice driver checks for the lack of a specific PF
> capabilty 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.
>
> Add a symmetrical check in the unload path so that the correct value is
> sent to the devlink unregister params call.
>
> Fixes: 109eb2917284 ("ice: Add tx_scheduling_layers devlink param")
> CC: Lukasz Czapnik <[email protected]>
> Reviewed-by: Przemek Kitszel <[email protected]>
> Signed-off-by: Dave Ertman <[email protected]>
> ---
> drivers/net/ethernet/intel/ice/devlink/devlink.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c
> b/drivers/net/ethernet/intel/ice/devlink/devlink.c
> index c4b69655cdf5..94ad78d2d11e 100644
> --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c
> +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c
> @@ -1477,8 +1477,14 @@ int ice_devlink_register_params(struct ice_pf *pf)
>
> void ice_devlink_unregister_params(struct ice_pf *pf)
> {
> + size_t params_size = ARRAY_SIZE(ice_devlink_params);
> + struct ice_hw *hw = &pf->hw;
> +
> + if (!hw->func_caps.common_cap.tx_sched_topo_comp_mode_en)
> + params_size--;
> +
> devl_params_unregister(priv_to_devlink(pf), ice_devlink_params,
> - ARRAY_SIZE(ice_devlink_params));
> + params_size);
> }
>
> #define ICE_DEVLINK_READ_BLK_SIZE (1024 * 1024)
What? This makes no sense. Just separate the Tx sched parameter from the
list and register it separately based on whether the flag is set. Then
only unregister it when the flag is set.
Messing with the parameter size list is brittle and requires us to be
extra careful that the Tx sched parameter is last.
NACK. Please fix both the registration and unregistration to avoid this.
Thanks,
Jake
