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

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

[Dave Ertman]

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)
-- 
2.44.0

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

[Jacob Keller]

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.

NACK. Please fix both the registration and unregistration to avoid this.

Thanks,
Jake

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

[Przemek Kitszel]

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

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

[Jacob Keller]

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.