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

[Jacob Keller <jacob.e.keller@intel.com>]

On 5/22/2024 3:38 AM, Przemek Kitszel wrote:
> 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

This would be more useful. Alternatively, we could just ignore or warn
about errors on adding parameters and continue loading rather than
trying to unroll.