| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZctT5kBdbjcxygQT@nanopsycho>
Date: Tue, 13 Feb 2024 12:35:02 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Wojciech Drewek <wojciech.drewek@...el.com>
Cc: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
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
Tue, Feb 13, 2024 at 11:19:49AM CET, wojciech.drewek@...el.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@...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/
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
>>>>
>>>>
Powered by blists - more mailing lists