[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZctI5v99LmGx285J@mev-dev>
Date: Tue, 13 Feb 2024 11:48:06 +0100
From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
Cc: Jiri Pirko <jiri@...nulli.us>, intel-wired-lan@...ts.osuosl.org,
netdev@...r.kernel.org, pawel.chmielewski@...el.com,
sridhar.samudrala@...el.com, jacob.e.keller@...el.com,
pio.raczynski@...il.com, konrad.knitter@...el.com,
marcin.szycik@...el.com, nex.sw.ncis.nat.hpm.dev@...el.com,
przemyslaw.kitszel@...el.com,
Jan Sokolowski <jan.sokolowski@...el.com>
Subject: Re: [iwl-next v1 6/7] ice: enable_rdma devlink param
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@...ux.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@...el.com>
> >>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> >>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.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
> >>>
> >>>
Powered by blists - more mailing lists