* [PATCH net-next 1/1] ice: Simplify tracking status of RDMA support
@ 2022-01-05 0:04 Tony Nguyen
2022-01-05 7:19 ` Leon Romanovsky
0 siblings, 1 reply; 4+ messages in thread
From: Tony Nguyen @ 2022-01-05 0:04 UTC (permalink / raw)
To: davem, kuba
Cc: Dave Ertman, netdev, anthony.l.nguyen, linux-rdma, shiraz.saleem,
mustafa.ismail, Leszek Kaliszczuk
From: Dave Ertman <david.m.ertman@intel.com>
The status of support for RDMA is currently being tracked with two
separate status flags. This is unnecessary with the current state of
the driver.
Simplify status tracking down to a single flag.
Rename the helper function to denote the RDMA specific status and
universally use the helper function to test the status bit.
Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
Tested-by: Leszek Kaliszczuk <leszek.kaliszczuk@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
drivers/net/ethernet/intel/ice/ice.h | 3 ---
drivers/net/ethernet/intel/ice/ice_idc.c | 6 +++---
drivers/net/ethernet/intel/ice/ice_lib.c | 8 ++++----
drivers/net/ethernet/intel/ice/ice_lib.h | 2 +-
drivers/net/ethernet/intel/ice/ice_main.c | 13 +++++--------
5 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index 4e16d185077d..6f445cc3390f 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -468,7 +468,6 @@ enum ice_pf_flags {
ICE_FLAG_FD_ENA,
ICE_FLAG_PTP_SUPPORTED, /* PTP is supported by NVM */
ICE_FLAG_PTP, /* PTP is enabled by software */
- ICE_FLAG_AUX_ENA,
ICE_FLAG_ADV_FEATURES,
ICE_FLAG_TC_MQPRIO, /* support for Multi queue TC */
ICE_FLAG_CLS_FLOWER,
@@ -886,7 +885,6 @@ static inline void ice_set_rdma_cap(struct ice_pf *pf)
{
if (pf->hw.func_caps.common_cap.rdma && pf->num_rdma_msix) {
set_bit(ICE_FLAG_RDMA_ENA, pf->flags);
- set_bit(ICE_FLAG_AUX_ENA, pf->flags);
ice_plug_aux_dev(pf);
}
}
@@ -899,6 +897,5 @@ static inline void ice_clear_rdma_cap(struct ice_pf *pf)
{
ice_unplug_aux_dev(pf);
clear_bit(ICE_FLAG_RDMA_ENA, pf->flags);
- clear_bit(ICE_FLAG_AUX_ENA, pf->flags);
}
#endif /* _ICE_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
index fc3580167e7b..9493a38182f5 100644
--- a/drivers/net/ethernet/intel/ice/ice_idc.c
+++ b/drivers/net/ethernet/intel/ice/ice_idc.c
@@ -79,7 +79,7 @@ int ice_add_rdma_qset(struct ice_pf *pf, struct iidc_rdma_qset_params *qset)
dev = ice_pf_to_dev(pf);
- if (!test_bit(ICE_FLAG_RDMA_ENA, pf->flags))
+ if (!ice_is_rdma_ena(pf))
return -EINVAL;
vsi = ice_get_main_vsi(pf);
@@ -236,7 +236,7 @@ EXPORT_SYMBOL_GPL(ice_get_qos_params);
*/
static int ice_reserve_rdma_qvector(struct ice_pf *pf)
{
- if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
+ if (ice_is_rdma_ena(pf)) {
int index;
index = ice_get_res(pf, pf->irq_tracker, pf->num_rdma_msix,
@@ -274,7 +274,7 @@ int ice_plug_aux_dev(struct ice_pf *pf)
/* if this PF doesn't support a technology that requires auxiliary
* devices, then gracefully exit
*/
- if (!ice_is_aux_ena(pf))
+ if (!ice_is_rdma_ena(pf))
return 0;
iadev = kzalloc(sizeof(*iadev), GFP_KERNEL);
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 0c187cf04fcf..b1c164b8066c 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -732,14 +732,14 @@ bool ice_is_safe_mode(struct ice_pf *pf)
}
/**
- * ice_is_aux_ena
+ * ice_is_rdma_ena
* @pf: pointer to the PF struct
*
- * returns true if AUX devices/drivers are supported, false otherwise
+ * returns true if RDMA is currently supported, false otherwise
*/
-bool ice_is_aux_ena(struct ice_pf *pf)
+bool ice_is_rdma_ena(struct ice_pf *pf)
{
- return test_bit(ICE_FLAG_AUX_ENA, pf->flags);
+ return test_bit(ICE_FLAG_RDMA_ENA, pf->flags);
}
/**
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h b/drivers/net/ethernet/intel/ice/ice_lib.h
index b2ed189527d6..a2f54fbdc170 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.h
+++ b/drivers/net/ethernet/intel/ice/ice_lib.h
@@ -110,7 +110,7 @@ void ice_set_q_vector_intrl(struct ice_q_vector *q_vector);
int ice_vsi_cfg_mac_fltr(struct ice_vsi *vsi, const u8 *macaddr, bool set);
bool ice_is_safe_mode(struct ice_pf *pf);
-bool ice_is_aux_ena(struct ice_pf *pf);
+bool ice_is_rdma_ena(struct ice_pf *pf);
bool ice_is_dflt_vsi_in_use(struct ice_sw *sw);
bool ice_is_vsi_dflt_vsi(struct ice_sw *sw, struct ice_vsi *vsi);
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index e29176889c23..078eb588f41e 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -3653,11 +3653,8 @@ static void ice_set_pf_caps(struct ice_pf *pf)
struct ice_hw_func_caps *func_caps = &pf->hw.func_caps;
clear_bit(ICE_FLAG_RDMA_ENA, pf->flags);
- clear_bit(ICE_FLAG_AUX_ENA, pf->flags);
- if (func_caps->common_cap.rdma) {
+ if (func_caps->common_cap.rdma)
set_bit(ICE_FLAG_RDMA_ENA, pf->flags);
- set_bit(ICE_FLAG_AUX_ENA, pf->flags);
- }
clear_bit(ICE_FLAG_DCB_CAPABLE, pf->flags);
if (func_caps->common_cap.dcb)
set_bit(ICE_FLAG_DCB_CAPABLE, pf->flags);
@@ -3785,7 +3782,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
v_left -= needed;
/* reserve vectors for RDMA auxiliary driver */
- if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
+ if (ice_is_rdma_ena(pf)) {
needed = num_cpus + ICE_RDMA_NUM_AEQ_MSIX;
if (v_left < needed)
goto no_hw_vecs_left_err;
@@ -3826,7 +3823,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
int v_remain = v_actual - v_other;
int v_rdma = 0, v_min_rdma = 0;
- if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
+ if (ice_is_rdma_ena(pf)) {
/* Need at least 1 interrupt in addition to
* AEQ MSIX
*/
@@ -3860,7 +3857,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
dev_notice(dev, "Enabled %d MSI-X vectors for LAN traffic.\n",
pf->num_lan_msix);
- if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags))
+ if (ice_is_rdma_ena(pf))
dev_notice(dev, "Enabled %d MSI-X vectors for RDMA.\n",
pf->num_rdma_msix);
}
@@ -4688,7 +4685,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
/* ready to go, so clear down state bit */
clear_bit(ICE_DOWN, pf->state);
- if (ice_is_aux_ena(pf)) {
+ if (ice_is_rdma_ena(pf)) {
pf->aux_idx = ida_alloc(&ice_aux_ida, GFP_KERNEL);
if (pf->aux_idx < 0) {
dev_err(dev, "Failed to allocate device ID for AUX driver\n");
--
2.31.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net-next 1/1] ice: Simplify tracking status of RDMA support
2022-01-05 0:04 [PATCH net-next 1/1] ice: Simplify tracking status of RDMA support Tony Nguyen
@ 2022-01-05 7:19 ` Leon Romanovsky
2022-01-05 19:10 ` Ertman, David M
0 siblings, 1 reply; 4+ messages in thread
From: Leon Romanovsky @ 2022-01-05 7:19 UTC (permalink / raw)
To: Tony Nguyen
Cc: davem, kuba, Dave Ertman, netdev, linux-rdma, shiraz.saleem,
mustafa.ismail, Leszek Kaliszczuk
On Tue, Jan 04, 2022 at 04:04:56PM -0800, Tony Nguyen wrote:
> From: Dave Ertman <david.m.ertman@intel.com>
>
> The status of support for RDMA is currently being tracked with two
> separate status flags. This is unnecessary with the current state of
> the driver.
>
> Simplify status tracking down to a single flag.
>
> Rename the helper function to denote the RDMA specific status and
> universally use the helper function to test the status bit.
>
> Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> Tested-by: Leszek Kaliszczuk <leszek.kaliszczuk@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice.h | 3 ---
> drivers/net/ethernet/intel/ice/ice_idc.c | 6 +++---
> drivers/net/ethernet/intel/ice/ice_lib.c | 8 ++++----
> drivers/net/ethernet/intel/ice/ice_lib.h | 2 +-
> drivers/net/ethernet/intel/ice/ice_main.c | 13 +++++--------
> 5 files changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index 4e16d185077d..6f445cc3390f 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -468,7 +468,6 @@ enum ice_pf_flags {
> ICE_FLAG_FD_ENA,
> ICE_FLAG_PTP_SUPPORTED, /* PTP is supported by NVM */
> ICE_FLAG_PTP, /* PTP is enabled by software */
> - ICE_FLAG_AUX_ENA,
> ICE_FLAG_ADV_FEATURES,
> ICE_FLAG_TC_MQPRIO, /* support for Multi queue TC */
> ICE_FLAG_CLS_FLOWER,
> @@ -886,7 +885,6 @@ static inline void ice_set_rdma_cap(struct ice_pf *pf)
> {
> if (pf->hw.func_caps.common_cap.rdma && pf->num_rdma_msix) {
> set_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> - set_bit(ICE_FLAG_AUX_ENA, pf->flags);
> ice_plug_aux_dev(pf);
> }
> }
> @@ -899,6 +897,5 @@ static inline void ice_clear_rdma_cap(struct ice_pf *pf)
> {
> ice_unplug_aux_dev(pf);
> clear_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> - clear_bit(ICE_FLAG_AUX_ENA, pf->flags);
> }
> #endif /* _ICE_H_ */
> diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c b/drivers/net/ethernet/intel/ice/ice_idc.c
> index fc3580167e7b..9493a38182f5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_idc.c
> +++ b/drivers/net/ethernet/intel/ice/ice_idc.c
> @@ -79,7 +79,7 @@ int ice_add_rdma_qset(struct ice_pf *pf, struct iidc_rdma_qset_params *qset)
>
> dev = ice_pf_to_dev(pf);
>
> - if (!test_bit(ICE_FLAG_RDMA_ENA, pf->flags))
> + if (!ice_is_rdma_ena(pf))
> return -EINVAL;
>
> vsi = ice_get_main_vsi(pf);
> @@ -236,7 +236,7 @@ EXPORT_SYMBOL_GPL(ice_get_qos_params);
> */
> static int ice_reserve_rdma_qvector(struct ice_pf *pf)
> {
> - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
> + if (ice_is_rdma_ena(pf)) {
> int index;
>
> index = ice_get_res(pf, pf->irq_tracker, pf->num_rdma_msix,
> @@ -274,7 +274,7 @@ int ice_plug_aux_dev(struct ice_pf *pf)
> /* if this PF doesn't support a technology that requires auxiliary
> * devices, then gracefully exit
> */
> - if (!ice_is_aux_ena(pf))
> + if (!ice_is_rdma_ena(pf))
> return 0;
This check is redundant, you already checked it in ice_probe.
>
> iadev = kzalloc(sizeof(*iadev), GFP_KERNEL);
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index 0c187cf04fcf..b1c164b8066c 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -732,14 +732,14 @@ bool ice_is_safe_mode(struct ice_pf *pf)
> }
>
> /**
> - * ice_is_aux_ena
> + * ice_is_rdma_ena
> * @pf: pointer to the PF struct
> *
> - * returns true if AUX devices/drivers are supported, false otherwise
> + * returns true if RDMA is currently supported, false otherwise
> */
> -bool ice_is_aux_ena(struct ice_pf *pf)
> +bool ice_is_rdma_ena(struct ice_pf *pf)
> {
> - return test_bit(ICE_FLAG_AUX_ENA, pf->flags);
> + return test_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h b/drivers/net/ethernet/intel/ice/ice_lib.h
> index b2ed189527d6..a2f54fbdc170 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.h
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.h
> @@ -110,7 +110,7 @@ void ice_set_q_vector_intrl(struct ice_q_vector *q_vector);
> int ice_vsi_cfg_mac_fltr(struct ice_vsi *vsi, const u8 *macaddr, bool set);
>
> bool ice_is_safe_mode(struct ice_pf *pf);
> -bool ice_is_aux_ena(struct ice_pf *pf);
> +bool ice_is_rdma_ena(struct ice_pf *pf);
> bool ice_is_dflt_vsi_in_use(struct ice_sw *sw);
>
> bool ice_is_vsi_dflt_vsi(struct ice_sw *sw, struct ice_vsi *vsi);
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index e29176889c23..078eb588f41e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -3653,11 +3653,8 @@ static void ice_set_pf_caps(struct ice_pf *pf)
> struct ice_hw_func_caps *func_caps = &pf->hw.func_caps;
>
> clear_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> - clear_bit(ICE_FLAG_AUX_ENA, pf->flags);
> - if (func_caps->common_cap.rdma) {
> + if (func_caps->common_cap.rdma)
> set_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> - set_bit(ICE_FLAG_AUX_ENA, pf->flags);
> - }
> clear_bit(ICE_FLAG_DCB_CAPABLE, pf->flags);
> if (func_caps->common_cap.dcb)
> set_bit(ICE_FLAG_DCB_CAPABLE, pf->flags);
> @@ -3785,7 +3782,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
> v_left -= needed;
>
> /* reserve vectors for RDMA auxiliary driver */
> - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
> + if (ice_is_rdma_ena(pf)) {
> needed = num_cpus + ICE_RDMA_NUM_AEQ_MSIX;
> if (v_left < needed)
> goto no_hw_vecs_left_err;
> @@ -3826,7 +3823,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
> int v_remain = v_actual - v_other;
> int v_rdma = 0, v_min_rdma = 0;
>
> - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
> + if (ice_is_rdma_ena(pf)) {
> /* Need at least 1 interrupt in addition to
> * AEQ MSIX
> */
> @@ -3860,7 +3857,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
> dev_notice(dev, "Enabled %d MSI-X vectors for LAN traffic.\n",
> pf->num_lan_msix);
>
> - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags))
> + if (ice_is_rdma_ena(pf))
> dev_notice(dev, "Enabled %d MSI-X vectors for RDMA.\n",
> pf->num_rdma_msix);
> }
> @@ -4688,7 +4685,7 @@ ice_probe(struct pci_dev *pdev, const struct pci_device_id __always_unused *ent)
>
> /* ready to go, so clear down state bit */
> clear_bit(ICE_DOWN, pf->state);
Why don't you clear this bit after RDMA initialization?
> - if (ice_is_aux_ena(pf)) {
> + if (ice_is_rdma_ena(pf)) {
> pf->aux_idx = ida_alloc(&ice_aux_ida, GFP_KERNEL);
> if (pf->aux_idx < 0) {
> dev_err(dev, "Failed to allocate device ID for AUX driver\n");
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH net-next 1/1] ice: Simplify tracking status of RDMA support
2022-01-05 7:19 ` Leon Romanovsky
@ 2022-01-05 19:10 ` Ertman, David M
2022-01-06 6:56 ` Leon Romanovsky
0 siblings, 1 reply; 4+ messages in thread
From: Ertman, David M @ 2022-01-05 19:10 UTC (permalink / raw)
To: Leon Romanovsky, Nguyen, Anthony L
Cc: davem, kuba, netdev, linux-rdma, Saleem, Shiraz, Ismail, Mustafa,
Kaliszczuk, Leszek
> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Tuesday, January 4, 2022 11:19 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> Cc: davem@davemloft.net; kuba@kernel.org; Ertman, David M
> <david.m.ertman@intel.com>; netdev@vger.kernel.org; linux-
> rdma@vger.kernel.org; Saleem, Shiraz <shiraz.saleem@intel.com>; Ismail,
> Mustafa <mustafa.ismail@intel.com>; Kaliszczuk, Leszek
> <leszek.kaliszczuk@intel.com>
> Subject: Re: [PATCH net-next 1/1] ice: Simplify tracking status of RDMA
> support
>
> On Tue, Jan 04, 2022 at 04:04:56PM -0800, Tony Nguyen wrote:
> > From: Dave Ertman <david.m.ertman@intel.com>
> >
> > The status of support for RDMA is currently being tracked with two
> > separate status flags. This is unnecessary with the current state of
> > the driver.
> >
> > Simplify status tracking down to a single flag.
> >
> > Rename the helper function to denote the RDMA specific status and
> > universally use the helper function to test the status bit.
> >
> > Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> > Tested-by: Leszek Kaliszczuk <leszek.kaliszczuk@intel.com>
> > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > ---
> > drivers/net/ethernet/intel/ice/ice.h | 3 ---
> > drivers/net/ethernet/intel/ice/ice_idc.c | 6 +++---
> > drivers/net/ethernet/intel/ice/ice_lib.c | 8 ++++----
> > drivers/net/ethernet/intel/ice/ice_lib.h | 2 +-
> > drivers/net/ethernet/intel/ice/ice_main.c | 13 +++++--------
> > 5 files changed, 13 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice.h
> b/drivers/net/ethernet/intel/ice/ice.h
> > index 4e16d185077d..6f445cc3390f 100644
> > --- a/drivers/net/ethernet/intel/ice/ice.h
> > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > @@ -468,7 +468,6 @@ enum ice_pf_flags {
> > ICE_FLAG_FD_ENA,
> > ICE_FLAG_PTP_SUPPORTED, /* PTP is supported by NVM
> */
> > ICE_FLAG_PTP, /* PTP is enabled by software */
> > - ICE_FLAG_AUX_ENA,
> > ICE_FLAG_ADV_FEATURES,
> > ICE_FLAG_TC_MQPRIO, /* support for Multi queue TC
> */
> > ICE_FLAG_CLS_FLOWER,
> > @@ -886,7 +885,6 @@ static inline void ice_set_rdma_cap(struct ice_pf
> *pf)
> > {
> > if (pf->hw.func_caps.common_cap.rdma && pf->num_rdma_msix) {
> > set_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> > - set_bit(ICE_FLAG_AUX_ENA, pf->flags);
> > ice_plug_aux_dev(pf);
> > }
> > }
> > @@ -899,6 +897,5 @@ static inline void ice_clear_rdma_cap(struct ice_pf
> *pf)
> > {
> > ice_unplug_aux_dev(pf);
> > clear_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> > - clear_bit(ICE_FLAG_AUX_ENA, pf->flags);
> > }
> > #endif /* _ICE_H_ */
> > diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c
> b/drivers/net/ethernet/intel/ice/ice_idc.c
> > index fc3580167e7b..9493a38182f5 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_idc.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_idc.c
> > @@ -79,7 +79,7 @@ int ice_add_rdma_qset(struct ice_pf *pf, struct
> iidc_rdma_qset_params *qset)
> >
> > dev = ice_pf_to_dev(pf);
> >
> > - if (!test_bit(ICE_FLAG_RDMA_ENA, pf->flags))
> > + if (!ice_is_rdma_ena(pf))
> > return -EINVAL;
> >
> > vsi = ice_get_main_vsi(pf);
> > @@ -236,7 +236,7 @@ EXPORT_SYMBOL_GPL(ice_get_qos_params);
> > */
> > static int ice_reserve_rdma_qvector(struct ice_pf *pf)
> > {
> > - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
> > + if (ice_is_rdma_ena(pf)) {
> > int index;
> >
> > index = ice_get_res(pf, pf->irq_tracker, pf-
> >num_rdma_msix,
> > @@ -274,7 +274,7 @@ int ice_plug_aux_dev(struct ice_pf *pf)
> > /* if this PF doesn't support a technology that requires auxiliary
> > * devices, then gracefully exit
> > */
> > - if (!ice_is_aux_ena(pf))
> > + if (!ice_is_rdma_ena(pf))
> > return 0;
>
> This check is redundant, you already checked it in ice_probe.
>
This function is called from other places besides ice_probe (after a reset for instance).
This central check stops the creation of the auxiliary device if it has been determined if
RDMA functionality should not be allowed whenever ice_plug_aux_dev is called. The first
check in ice_probe stops even allocating the memory for the device if RDMA is not supported
at all.
> >
> > iadev = kzalloc(sizeof(*iadev), GFP_KERNEL);
> > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c
> b/drivers/net/ethernet/intel/ice/ice_lib.c
> > index 0c187cf04fcf..b1c164b8066c 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> > @@ -732,14 +732,14 @@ bool ice_is_safe_mode(struct ice_pf *pf)
> > }
> >
> > /**
> > - * ice_is_aux_ena
> > + * ice_is_rdma_ena
> > * @pf: pointer to the PF struct
> > *
> > - * returns true if AUX devices/drivers are supported, false otherwise
> > + * returns true if RDMA is currently supported, false otherwise
> > */
> > -bool ice_is_aux_ena(struct ice_pf *pf)
> > +bool ice_is_rdma_ena(struct ice_pf *pf)
> > {
> > - return test_bit(ICE_FLAG_AUX_ENA, pf->flags);
> > + return test_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> > }
> >
> > /**
> > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h
> b/drivers/net/ethernet/intel/ice/ice_lib.h
> > index b2ed189527d6..a2f54fbdc170 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_lib.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_lib.h
> > @@ -110,7 +110,7 @@ void ice_set_q_vector_intrl(struct ice_q_vector
> *q_vector);
> > int ice_vsi_cfg_mac_fltr(struct ice_vsi *vsi, const u8 *macaddr, bool set);
> >
> > bool ice_is_safe_mode(struct ice_pf *pf);
> > -bool ice_is_aux_ena(struct ice_pf *pf);
> > +bool ice_is_rdma_ena(struct ice_pf *pf);
> > bool ice_is_dflt_vsi_in_use(struct ice_sw *sw);
> >
> > bool ice_is_vsi_dflt_vsi(struct ice_sw *sw, struct ice_vsi *vsi);
> > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> b/drivers/net/ethernet/intel/ice/ice_main.c
> > index e29176889c23..078eb588f41e 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > @@ -3653,11 +3653,8 @@ static void ice_set_pf_caps(struct ice_pf *pf)
> > struct ice_hw_func_caps *func_caps = &pf->hw.func_caps;
> >
> > clear_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> > - clear_bit(ICE_FLAG_AUX_ENA, pf->flags);
> > - if (func_caps->common_cap.rdma) {
> > + if (func_caps->common_cap.rdma)
> > set_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> > - set_bit(ICE_FLAG_AUX_ENA, pf->flags);
> > - }
> > clear_bit(ICE_FLAG_DCB_CAPABLE, pf->flags);
> > if (func_caps->common_cap.dcb)
> > set_bit(ICE_FLAG_DCB_CAPABLE, pf->flags);
> > @@ -3785,7 +3782,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
> > v_left -= needed;
> >
> > /* reserve vectors for RDMA auxiliary driver */
> > - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
> > + if (ice_is_rdma_ena(pf)) {
> > needed = num_cpus + ICE_RDMA_NUM_AEQ_MSIX;
> > if (v_left < needed)
> > goto no_hw_vecs_left_err;
> > @@ -3826,7 +3823,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
> > int v_remain = v_actual - v_other;
> > int v_rdma = 0, v_min_rdma = 0;
> >
> > - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
> > + if (ice_is_rdma_ena(pf)) {
> > /* Need at least 1 interrupt in addition to
> > * AEQ MSIX
> > */
> > @@ -3860,7 +3857,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
> > dev_notice(dev, "Enabled %d MSI-X vectors for LAN
> traffic.\n",
> > pf->num_lan_msix);
> >
> > - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags))
> > + if (ice_is_rdma_ena(pf))
> > dev_notice(dev, "Enabled %d MSI-X vectors
> for RDMA.\n",
> > pf->num_rdma_msix);
> > }
> > @@ -4688,7 +4685,7 @@ ice_probe(struct pci_dev *pdev, const struct
> pci_device_id __always_unused *ent)
> >
> > /* ready to go, so clear down state bit */
> > clear_bit(ICE_DOWN, pf->state);
>
> Why don't you clear this bit after RDMA initialization?
We want the interface to be completely ready before we initialize RDMA
communication. Also, this bit is not relevant to the RDMA queues, so before
or after RDMA initialization doesn't matter since RDMA traffic doesn't go through
the LAN netdev anyway.
>
> > - if (ice_is_aux_ena(pf)) {
> > + if (ice_is_rdma_ena(pf)) {
> > pf->aux_idx = ida_alloc(&ice_aux_ida, GFP_KERNEL);
> > if (pf->aux_idx < 0) {
> > dev_err(dev, "Failed to allocate device ID for AUX
> driver\n");
> > --
> > 2.31.1
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net-next 1/1] ice: Simplify tracking status of RDMA support
2022-01-05 19:10 ` Ertman, David M
@ 2022-01-06 6:56 ` Leon Romanovsky
0 siblings, 0 replies; 4+ messages in thread
From: Leon Romanovsky @ 2022-01-06 6:56 UTC (permalink / raw)
To: Ertman, David M
Cc: Nguyen, Anthony L, davem, kuba, netdev, linux-rdma, Saleem,
Shiraz, Ismail, Mustafa, Kaliszczuk, Leszek
On Wed, Jan 05, 2022 at 07:10:24PM +0000, Ertman, David M wrote:
> > -----Original Message-----
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Tuesday, January 4, 2022 11:19 PM
> > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
> > Cc: davem@davemloft.net; kuba@kernel.org; Ertman, David M
> > <david.m.ertman@intel.com>; netdev@vger.kernel.org; linux-
> > rdma@vger.kernel.org; Saleem, Shiraz <shiraz.saleem@intel.com>; Ismail,
> > Mustafa <mustafa.ismail@intel.com>; Kaliszczuk, Leszek
> > <leszek.kaliszczuk@intel.com>
> > Subject: Re: [PATCH net-next 1/1] ice: Simplify tracking status of RDMA
> > support
> >
> > On Tue, Jan 04, 2022 at 04:04:56PM -0800, Tony Nguyen wrote:
> > > From: Dave Ertman <david.m.ertman@intel.com>
> > >
> > > The status of support for RDMA is currently being tracked with two
> > > separate status flags. This is unnecessary with the current state of
> > > the driver.
> > >
> > > Simplify status tracking down to a single flag.
> > >
> > > Rename the helper function to denote the RDMA specific status and
> > > universally use the helper function to test the status bit.
> > >
> > > Signed-off-by: Dave Ertman <david.m.ertman@intel.com>
> > > Tested-by: Leszek Kaliszczuk <leszek.kaliszczuk@intel.com>
> > > Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> > > ---
> > > drivers/net/ethernet/intel/ice/ice.h | 3 ---
> > > drivers/net/ethernet/intel/ice/ice_idc.c | 6 +++---
> > > drivers/net/ethernet/intel/ice/ice_lib.c | 8 ++++----
> > > drivers/net/ethernet/intel/ice/ice_lib.h | 2 +-
> > > drivers/net/ethernet/intel/ice/ice_main.c | 13 +++++--------
> > > 5 files changed, 13 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice.h
> > b/drivers/net/ethernet/intel/ice/ice.h
> > > index 4e16d185077d..6f445cc3390f 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > > @@ -468,7 +468,6 @@ enum ice_pf_flags {
> > > ICE_FLAG_FD_ENA,
> > > ICE_FLAG_PTP_SUPPORTED, /* PTP is supported by NVM
> > */
> > > ICE_FLAG_PTP, /* PTP is enabled by software */
> > > - ICE_FLAG_AUX_ENA,
> > > ICE_FLAG_ADV_FEATURES,
> > > ICE_FLAG_TC_MQPRIO, /* support for Multi queue TC
> > */
> > > ICE_FLAG_CLS_FLOWER,
> > > @@ -886,7 +885,6 @@ static inline void ice_set_rdma_cap(struct ice_pf
> > *pf)
> > > {
> > > if (pf->hw.func_caps.common_cap.rdma && pf->num_rdma_msix) {
> > > set_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> > > - set_bit(ICE_FLAG_AUX_ENA, pf->flags);
> > > ice_plug_aux_dev(pf);
> > > }
> > > }
> > > @@ -899,6 +897,5 @@ static inline void ice_clear_rdma_cap(struct ice_pf
> > *pf)
> > > {
> > > ice_unplug_aux_dev(pf);
> > > clear_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> > > - clear_bit(ICE_FLAG_AUX_ENA, pf->flags);
> > > }
> > > #endif /* _ICE_H_ */
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_idc.c
> > b/drivers/net/ethernet/intel/ice/ice_idc.c
> > > index fc3580167e7b..9493a38182f5 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_idc.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_idc.c
> > > @@ -79,7 +79,7 @@ int ice_add_rdma_qset(struct ice_pf *pf, struct
> > iidc_rdma_qset_params *qset)
> > >
> > > dev = ice_pf_to_dev(pf);
> > >
> > > - if (!test_bit(ICE_FLAG_RDMA_ENA, pf->flags))
> > > + if (!ice_is_rdma_ena(pf))
> > > return -EINVAL;
> > >
> > > vsi = ice_get_main_vsi(pf);
> > > @@ -236,7 +236,7 @@ EXPORT_SYMBOL_GPL(ice_get_qos_params);
> > > */
> > > static int ice_reserve_rdma_qvector(struct ice_pf *pf)
> > > {
> > > - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
> > > + if (ice_is_rdma_ena(pf)) {
> > > int index;
> > >
> > > index = ice_get_res(pf, pf->irq_tracker, pf-
> > >num_rdma_msix,
> > > @@ -274,7 +274,7 @@ int ice_plug_aux_dev(struct ice_pf *pf)
> > > /* if this PF doesn't support a technology that requires auxiliary
> > > * devices, then gracefully exit
> > > */
> > > - if (!ice_is_aux_ena(pf))
> > > + if (!ice_is_rdma_ena(pf))
> > > return 0;
> >
> > This check is redundant, you already checked it in ice_probe.
> >
>
> This function is called from other places besides ice_probe (after a reset for instance).
Right, and arguably the ice_rebuild() should have same check as in
ice_probe(). Other places shouldn't call to this ice_plug_aux_dev() if
RDMA is not supported. For example, you shouldn't present to the user
roce_en/iwarp_en devlink knobs if your device missing RDMA capability
bit. Maybe you did it, maybe not, I didn't check.
It is much easier to review if the function foo() is called only when it
is supported. It is also common kernel practice is that caller should check
and meet function requirements before such call.
>
> This central check stops the creation of the auxiliary device if it has been determined if
> RDMA functionality should not be allowed whenever ice_plug_aux_dev is called. The first
> check in ice_probe stops even allocating the memory for the device if RDMA is not supported
> at all.
I know.
>
> > >
> > > iadev = kzalloc(sizeof(*iadev), GFP_KERNEL);
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c
> > b/drivers/net/ethernet/intel/ice/ice_lib.c
> > > index 0c187cf04fcf..b1c164b8066c 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> > > @@ -732,14 +732,14 @@ bool ice_is_safe_mode(struct ice_pf *pf)
> > > }
> > >
> > > /**
> > > - * ice_is_aux_ena
> > > + * ice_is_rdma_ena
> > > * @pf: pointer to the PF struct
> > > *
> > > - * returns true if AUX devices/drivers are supported, false otherwise
> > > + * returns true if RDMA is currently supported, false otherwise
> > > */
> > > -bool ice_is_aux_ena(struct ice_pf *pf)
> > > +bool ice_is_rdma_ena(struct ice_pf *pf)
> > > {
> > > - return test_bit(ICE_FLAG_AUX_ENA, pf->flags);
> > > + return test_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> > > }
> > >
> > > /**
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.h
> > b/drivers/net/ethernet/intel/ice/ice_lib.h
> > > index b2ed189527d6..a2f54fbdc170 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_lib.h
> > > +++ b/drivers/net/ethernet/intel/ice/ice_lib.h
> > > @@ -110,7 +110,7 @@ void ice_set_q_vector_intrl(struct ice_q_vector
> > *q_vector);
> > > int ice_vsi_cfg_mac_fltr(struct ice_vsi *vsi, const u8 *macaddr, bool set);
> > >
> > > bool ice_is_safe_mode(struct ice_pf *pf);
> > > -bool ice_is_aux_ena(struct ice_pf *pf);
> > > +bool ice_is_rdma_ena(struct ice_pf *pf);
> > > bool ice_is_dflt_vsi_in_use(struct ice_sw *sw);
> > >
> > > bool ice_is_vsi_dflt_vsi(struct ice_sw *sw, struct ice_vsi *vsi);
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> > b/drivers/net/ethernet/intel/ice/ice_main.c
> > > index e29176889c23..078eb588f41e 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > > @@ -3653,11 +3653,8 @@ static void ice_set_pf_caps(struct ice_pf *pf)
> > > struct ice_hw_func_caps *func_caps = &pf->hw.func_caps;
> > >
> > > clear_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> > > - clear_bit(ICE_FLAG_AUX_ENA, pf->flags);
> > > - if (func_caps->common_cap.rdma) {
> > > + if (func_caps->common_cap.rdma)
> > > set_bit(ICE_FLAG_RDMA_ENA, pf->flags);
> > > - set_bit(ICE_FLAG_AUX_ENA, pf->flags);
> > > - }
> > > clear_bit(ICE_FLAG_DCB_CAPABLE, pf->flags);
> > > if (func_caps->common_cap.dcb)
> > > set_bit(ICE_FLAG_DCB_CAPABLE, pf->flags);
> > > @@ -3785,7 +3782,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
> > > v_left -= needed;
> > >
> > > /* reserve vectors for RDMA auxiliary driver */
> > > - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
> > > + if (ice_is_rdma_ena(pf)) {
> > > needed = num_cpus + ICE_RDMA_NUM_AEQ_MSIX;
> > > if (v_left < needed)
> > > goto no_hw_vecs_left_err;
> > > @@ -3826,7 +3823,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
> > > int v_remain = v_actual - v_other;
> > > int v_rdma = 0, v_min_rdma = 0;
> > >
> > > - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags)) {
> > > + if (ice_is_rdma_ena(pf)) {
> > > /* Need at least 1 interrupt in addition to
> > > * AEQ MSIX
> > > */
> > > @@ -3860,7 +3857,7 @@ static int ice_ena_msix_range(struct ice_pf *pf)
> > > dev_notice(dev, "Enabled %d MSI-X vectors for LAN
> > traffic.\n",
> > > pf->num_lan_msix);
> > >
> > > - if (test_bit(ICE_FLAG_RDMA_ENA, pf->flags))
> > > + if (ice_is_rdma_ena(pf))
> > > dev_notice(dev, "Enabled %d MSI-X vectors
> > for RDMA.\n",
> > > pf->num_rdma_msix);
> > > }
> > > @@ -4688,7 +4685,7 @@ ice_probe(struct pci_dev *pdev, const struct
> > pci_device_id __always_unused *ent)
> > >
> > > /* ready to go, so clear down state bit */
> > > clear_bit(ICE_DOWN, pf->state);
> >
> > Why don't you clear this bit after RDMA initialization?
>
> We want the interface to be completely ready before we initialize RDMA
> communication. Also, this bit is not relevant to the RDMA queues, so before
> or after RDMA initialization doesn't matter since RDMA traffic doesn't go through
> the LAN netdev anyway.
There is a difference between fully configured and enabled as this bbit
represents.
Anyway,
Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-06 6:56 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-05 0:04 [PATCH net-next 1/1] ice: Simplify tracking status of RDMA support Tony Nguyen
2022-01-05 7:19 ` Leon Romanovsky
2022-01-05 19:10 ` Ertman, David M
2022-01-06 6:56 ` Leon Romanovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.