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, [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]/
>
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
> >>>
> >>>
