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

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Tue, Feb 13, 2024 at 11:19:49AM +0100, Wojciech Drewek wrote:
> 
> 
> 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/
> 

Right, I based my patchset before your changes was applied to Tony's
tree. Thanks Wojtek.

So, in v2 there will be dev_version.

Thanks
Michal
> > 
> >>
> >> 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
> >>>
> >>>