Re: [Intel-wired-lan] [iwl-next v1 6/7] ice: enable_rdma devlink param

[Wojciech Drewek <wojciech.drewek@intel.com>]

On 13.02.2024 10:58, Michal Swiatkowski wrote:
> On Tue, Feb 13, 2024 at 10:07:17AM +0100, Jiri Pirko wrote:
>> Tue, Feb 13, 2024 at 08:35:08AM CET, michal.swiatkowski@linux.intel.com wrote:
>>> Implement enable_rdma devlink parameter to allow user to turn RDMA
>>> feature on and off.
>>>
>>> It is useful when there is no enough interrupts and user doesn't need
>>> RDMA feature.
>>>
>>> Reviewed-by: Jan Sokolowski <jan.sokolowski@intel.com>
>>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
>>> ---
>>> drivers/net/ethernet/intel/ice/ice_devlink.c | 32 ++++++++++++++++++--
>>> drivers/net/ethernet/intel/ice/ice_lib.c     |  8 ++++-
>>> drivers/net/ethernet/intel/ice/ice_main.c    | 18 +++++------
>>> 3 files changed, 45 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
>>> index b82ff9556a4b..4f048268db72 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_devlink.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
>>> @@ -1675,6 +1675,19 @@ ice_devlink_msix_min_pf_validate(struct devlink *devlink, u32 id,
>>> 	return 0;
>>> }
>>>
>>> +static int ice_devlink_enable_rdma_validate(struct devlink *devlink, u32 id,
>>> +					    union devlink_param_value val,
>>> +					    struct netlink_ext_ack *extack)
>>> +{
>>> +	struct ice_pf *pf = devlink_priv(devlink);
>>> +	bool new_state = val.vbool;
>>> +
>>> +	if (new_state && !test_bit(ICE_FLAG_RDMA_ENA, pf->flags))
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> static const struct devlink_param ice_devlink_params[] = {
>>> 	DEVLINK_PARAM_GENERIC(ENABLE_ROCE, BIT(DEVLINK_PARAM_CMODE_RUNTIME),
>>> 			      ice_devlink_enable_roce_get,
>>> @@ -1700,6 +1713,8 @@ static const struct devlink_param ice_devlink_params[] = {
>>> 			      ice_devlink_msix_min_pf_get,
>>> 			      ice_devlink_msix_min_pf_set,
>>> 			      ice_devlink_msix_min_pf_validate),
>>> +	DEVLINK_PARAM_GENERIC(ENABLE_RDMA, BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
>>> +			      NULL, NULL, ice_devlink_enable_rdma_validate),
>>> };
>>>
>>> static void ice_devlink_free(void *devlink_ptr)
>>> @@ -1776,9 +1791,22 @@ ice_devlink_set_switch_id(struct ice_pf *pf, struct netdev_phys_item_id *ppid)
>>> int ice_devlink_register_params(struct ice_pf *pf)
>>> {
>>> 	struct devlink *devlink = priv_to_devlink(pf);
>>> +	union devlink_param_value value;
>>> +	int err;
>>> +
>>> +	err = devlink_params_register(devlink, ice_devlink_params,
>>
>> Interesting, can't you just take the lock before this and call
>> devl_params_register()?
>>
> I mess up a locking here and also in subfunction patchset. I will follow
> you suggestion and take lock for whole init/cleanup. Thanks.
> 
>> Moreover, could you please fix your init/cleanup paths for hold devlink
>> instance lock the whole time?
>>
> You right, I will prepare patch for it.

I think my patch implements your suggestion Jiri:
https://lore.kernel.org/netdev/20240212211202.1051990-5-anthony.l.nguyen@intel.com/

> 
>>
>> pw-bot: cr
>>
>>
>>> +				      ARRAY_SIZE(ice_devlink_params));
>>> +	if (err)
>>> +		return err;
>>>
>>> -	return devlink_params_register(devlink, ice_devlink_params,
>>> -				       ARRAY_SIZE(ice_devlink_params));
>>> +	devl_lock(devlink);
>>> +	value.vbool = test_bit(ICE_FLAG_RDMA_ENA, pf->flags);
>>> +	devl_param_driverinit_value_set(devlink,
>>> +					DEVLINK_PARAM_GENERIC_ID_ENABLE_RDMA,
>>> +					value);
>>> +	devl_unlock(devlink);
>>> +
>>> +	return 0;
>>> }
>>>
>>> void ice_devlink_unregister_params(struct ice_pf *pf)
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
>>> index a30d2d2b51c1..4d5c3d699044 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>>> @@ -829,7 +829,13 @@ bool ice_is_safe_mode(struct ice_pf *pf)
>>>  */
>>> bool ice_is_rdma_ena(struct ice_pf *pf)
>>> {
>>> -	return test_bit(ICE_FLAG_RDMA_ENA, pf->flags);
>>> +	union devlink_param_value value;
>>> +	int err;
>>> +
>>> +	err = devl_param_driverinit_value_get(priv_to_devlink(pf),
>>> +					      DEVLINK_PARAM_GENERIC_ID_ENABLE_RDMA,
>>> +					      &value);
>>> +	return err ? false : value.vbool;
>>> }
>>>
>>> /**
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>>> index 824732f16112..4dd59d888ec7 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>>> @@ -5177,23 +5177,21 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>>> 	if (err)
>>> 		goto err_init;
>>>
>>> +	err = ice_init_devlink(pf);
>>> +	if (err)
>>> +		goto err_init_devlink;
>>> +
>>> 	devl_lock(priv_to_devlink(pf));
>>> 	err = ice_load(pf);
>>> 	devl_unlock(priv_to_devlink(pf));
>>> 	if (err)
>>> 		goto err_load;
>>>
>>> -	err = ice_init_devlink(pf);
>>> -	if (err)
>>> -		goto err_init_devlink;
>>> -
>>> 	return 0;
>>>
>>> -err_init_devlink:
>>> -	devl_lock(priv_to_devlink(pf));
>>> -	ice_unload(pf);
>>> -	devl_unlock(priv_to_devlink(pf));
>>> err_load:
>>> +	ice_deinit_devlink(pf);
>>> +err_init_devlink:
>>> 	ice_deinit(pf);
>>> err_init:
>>> 	pci_disable_device(pdev);
>>> @@ -5290,12 +5288,12 @@ static void ice_remove(struct pci_dev *pdev)
>>> 	if (!ice_is_safe_mode(pf))
>>> 		ice_remove_arfs(pf);
>>>
>>> -	ice_deinit_devlink(pf);
>>> -
>>> 	devl_lock(priv_to_devlink(pf));
>>> 	ice_unload(pf);
>>> 	devl_unlock(priv_to_devlink(pf));
>>>
>>> +	ice_deinit_devlink(pf);
>>> +
>>> 	ice_deinit(pf);
>>> 	ice_vsi_release_all(pf);
>>>
>>> -- 
>>> 2.42.0
>>>
>>>