Re: [Intel-wired-lan] [PATCH iwl-net] ice: check for unregistering correct number of devlink params

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 5/21/24 23:26, Jacob Keller wrote:
> 
> 
> 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 <lukasz.czapnik@intel.com>
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
>> ---
>>   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.
> 

for current situation your proposed solution would be indeed more
elegant

> NACK. Please fix both the registration and unregistration to avoid this.
> 
> Thanks,
> Jake

please note that for general case (multiple conditional params), with
possibility of the need of a rollback (when we fail in the middle of
params register) current API is not optimal

I have already suggested @Mateusz to extend struct devlink_param by an
additional field, so the skipping will be performed by devlink code,
with drivers only setting the field. It's still not that convenient as
we have typically a global array of params without easy index<->feature
mapping. But other drivers will benefit too