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, [email protected]
>> 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 <[email protected]>
>>> Reviewed-by: Przemek Kitszel <[email protected]>
>>> Signed-off-by: Michal Swiatkowski <[email protected]>
>>> ---
>>> 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/[email protected]/
>
>>
>> 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
>>>
>>>
