| Message ID | 20240213073509.77622-7-michal.swiatkowski@linux.intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ice: managing MSI-X in driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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()? Moreover, could you please fix your init/cleanup paths for hold devlink instance lock the whole time? 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 > >
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. > > 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 > > > >
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 >>> >>>
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 > >>> > >>>
Tue, Feb 13, 2024 at 11:19:49AM CET, wojciech.drewek@intel.com 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/ This patch is based on the one in your link apparently. I think it is incomplete. Idk > >> >>> >>> 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 >>>> >>>>
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, + 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);