| Message ID | 20240808072016.10321-2-michal.swiatkowski@linux.intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ice: managing MSI-X in driver | expand |
Thu, Aug 08, 2024 at 09:20:09AM CEST, michal.swiatkowski@linux.intel.com wrote: >Use generic devlink PF MSI-X parameter to allow user to change MSI-X >range. > >Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> >--- > .../net/ethernet/intel/ice/devlink/devlink.c | 56 ++++++++++++++++++- > drivers/net/ethernet/intel/ice/ice.h | 8 +++ > drivers/net/ethernet/intel/ice/ice_irq.c | 14 ++++- > 3 files changed, 76 insertions(+), 2 deletions(-) > >diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c >index 29a5f822cb8b..bdc22ea13e0f 100644 >--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c >+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c >@@ -1518,6 +1518,32 @@ static int ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, > return 0; > } > >+static int >+ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, >+ union devlink_param_value val, >+ struct netlink_ext_ack *extack) >+{ >+ if (val.vu16 > ICE_MAX_MSIX) { >+ NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); No reason to have "PF" in the text. Also, no reason to have "max MSI-X". That is the name of the param. >+ return -EINVAL; >+ } >+ >+ return 0; >+} >+ >+static int >+ice_devlink_msix_min_pf_validate(struct devlink *devlink, u32 id, >+ union devlink_param_value val, >+ struct netlink_ext_ack *extack) >+{ >+ if (val.vu16 <= ICE_MIN_MSIX) { >+ NL_SET_ERR_MSG_MOD(extack, "PF min MSI-X is too low"); Same comment as for max goes here. >+ return -EINVAL; >+ } >+ >+ return 0; >+} >+ > enum ice_param_id { > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, >@@ -1535,6 +1561,15 @@ static const struct devlink_param ice_dvl_rdma_params[] = { > ice_devlink_enable_iw_validate), > }; > >+static const struct devlink_param ice_dvl_msix_params[] = { >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MAX, >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), >+ NULL, NULL, ice_devlink_msix_max_pf_validate), >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MIN, >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), >+ NULL, NULL, ice_devlink_msix_min_pf_validate), >+}; >+ > static const struct devlink_param ice_dvl_sched_params[] = { > DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, > "tx_scheduling_layers", >@@ -1637,6 +1672,7 @@ void ice_devlink_unregister(struct ice_pf *pf) > int ice_devlink_register_params(struct ice_pf *pf) > { > struct devlink *devlink = priv_to_devlink(pf); >+ union devlink_param_value value; > struct ice_hw *hw = &pf->hw; > int status; > >@@ -1645,11 +1681,27 @@ int ice_devlink_register_params(struct ice_pf *pf) > if (status) > return status; > >+ status = devl_params_register(devlink, ice_dvl_msix_params, >+ ARRAY_SIZE(ice_dvl_msix_params)); >+ if (status) >+ return status; >+ > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) > status = devl_params_register(devlink, ice_dvl_sched_params, > ARRAY_SIZE(ice_dvl_sched_params)); >+ if (status) >+ return status; > >- return status; >+ value.vu16 = pf->msix.max; >+ devl_param_driverinit_value_set(devlink, >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, >+ value); >+ value.vu16 = pf->msix.min; >+ devl_param_driverinit_value_set(devlink, >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, >+ value); >+ >+ return 0; > } > > void ice_devlink_unregister_params(struct ice_pf *pf) >@@ -1659,6 +1711,8 @@ void ice_devlink_unregister_params(struct ice_pf *pf) > > devl_params_unregister(devlink, ice_dvl_rdma_params, > ARRAY_SIZE(ice_dvl_rdma_params)); >+ devl_params_unregister(devlink, ice_dvl_msix_params, >+ ARRAY_SIZE(ice_dvl_msix_params)); > > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) > devl_params_unregister(devlink, ice_dvl_sched_params, >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h >index d6f80da30dec..a67456057c77 100644 >--- a/drivers/net/ethernet/intel/ice/ice.h >+++ b/drivers/net/ethernet/intel/ice/ice.h >@@ -95,6 +95,7 @@ > #define ICE_MIN_LAN_TXRX_MSIX 1 > #define ICE_MIN_LAN_OICR_MSIX 1 > #define ICE_MIN_MSIX (ICE_MIN_LAN_TXRX_MSIX + ICE_MIN_LAN_OICR_MSIX) >+#define ICE_MAX_MSIX 256 > #define ICE_FDIR_MSIX 2 > #define ICE_RDMA_NUM_AEQ_MSIX 4 > #define ICE_MIN_RDMA_MSIX 2 >@@ -545,6 +546,12 @@ struct ice_agg_node { > u8 valid; > }; > >+struct ice_pf_msix { >+ u16 cur; >+ u16 min; >+ u16 max; >+}; >+ > struct ice_pf { > struct pci_dev *pdev; > struct ice_adapter *adapter; >@@ -615,6 +622,7 @@ struct ice_pf { > struct msi_map ll_ts_irq; /* LL_TS interrupt MSIX vector */ > u16 max_pf_txqs; /* Total Tx queues PF wide */ > u16 max_pf_rxqs; /* Total Rx queues PF wide */ >+ struct ice_pf_msix msix; > u16 num_lan_msix; /* Total MSIX vectors for base driver */ > u16 num_lan_tx; /* num LAN Tx queues setup */ > u16 num_lan_rx; /* num LAN Rx queues setup */ >diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c >index ad82ff7d1995..4e559fd6e49f 100644 >--- a/drivers/net/ethernet/intel/ice/ice_irq.c >+++ b/drivers/net/ethernet/intel/ice/ice_irq.c >@@ -252,7 +252,19 @@ void ice_clear_interrupt_scheme(struct ice_pf *pf) > int ice_init_interrupt_scheme(struct ice_pf *pf) > { > int total_vectors = pf->hw.func_caps.common_cap.num_msix_vectors; >- int vectors, max_vectors; >+ union devlink_param_value value; >+ int vectors, max_vectors, err; >+ >+ /* load default PF MSI-X range */ >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, >+ &value); If err is not 0, you have a bug in the driver. Perhaps it a about the time to make this return void and add some WARN_ONs inside the function? >+ pf->msix.min = err ? ICE_MIN_MSIX : value.vu16; >+ >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, >+ &value); >+ pf->msix.max = err ? total_vectors / 2 : value.vu16; > > vectors = ice_ena_msix_range(pf); > >-- >2.42.0 >
On Thu, Aug 08, 2024 at 05:34:35PM +0200, Jiri Pirko wrote: > Thu, Aug 08, 2024 at 09:20:09AM CEST, michal.swiatkowski@linux.intel.com wrote: > >Use generic devlink PF MSI-X parameter to allow user to change MSI-X > >range. > > > >Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > >--- > > .../net/ethernet/intel/ice/devlink/devlink.c | 56 ++++++++++++++++++- > > drivers/net/ethernet/intel/ice/ice.h | 8 +++ > > drivers/net/ethernet/intel/ice/ice_irq.c | 14 ++++- > > 3 files changed, 76 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c > >index 29a5f822cb8b..bdc22ea13e0f 100644 > >--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c > >+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c > >@@ -1518,6 +1518,32 @@ static int ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, > > return 0; > > } > > > >+static int > >+ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, > >+ union devlink_param_value val, > >+ struct netlink_ext_ack *extack) > >+{ > >+ if (val.vu16 > ICE_MAX_MSIX) { > >+ NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); > > No reason to have "PF" in the text. Also, no reason to have "max MSI-X". > That is the name of the param. > Ok, will change both, thanks. > > > >+ return -EINVAL; > >+ } > >+ > >+ return 0; > >+} > >+ > >+static int > >+ice_devlink_msix_min_pf_validate(struct devlink *devlink, u32 id, > >+ union devlink_param_value val, > >+ struct netlink_ext_ack *extack) > >+{ > >+ if (val.vu16 <= ICE_MIN_MSIX) { > >+ NL_SET_ERR_MSG_MOD(extack, "PF min MSI-X is too low"); > > Same comment as for max goes here. > > > >+ return -EINVAL; > >+ } > >+ > >+ return 0; > >+} > >+ > > enum ice_param_id { > > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > > ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, > >@@ -1535,6 +1561,15 @@ static const struct devlink_param ice_dvl_rdma_params[] = { > > ice_devlink_enable_iw_validate), > > }; > > > >+static const struct devlink_param ice_dvl_msix_params[] = { > >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MAX, > >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), > >+ NULL, NULL, ice_devlink_msix_max_pf_validate), > >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MIN, > >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), > >+ NULL, NULL, ice_devlink_msix_min_pf_validate), > >+}; > >+ > > static const struct devlink_param ice_dvl_sched_params[] = { > > DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, > > "tx_scheduling_layers", > >@@ -1637,6 +1672,7 @@ void ice_devlink_unregister(struct ice_pf *pf) > > int ice_devlink_register_params(struct ice_pf *pf) > > { > > struct devlink *devlink = priv_to_devlink(pf); > >+ union devlink_param_value value; > > struct ice_hw *hw = &pf->hw; > > int status; > > > >@@ -1645,11 +1681,27 @@ int ice_devlink_register_params(struct ice_pf *pf) > > if (status) > > return status; > > > >+ status = devl_params_register(devlink, ice_dvl_msix_params, > >+ ARRAY_SIZE(ice_dvl_msix_params)); > >+ if (status) > >+ return status; > >+ > > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) > > status = devl_params_register(devlink, ice_dvl_sched_params, > > ARRAY_SIZE(ice_dvl_sched_params)); > >+ if (status) > >+ return status; > > > >- return status; > >+ value.vu16 = pf->msix.max; > >+ devl_param_driverinit_value_set(devlink, > >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > >+ value); > >+ value.vu16 = pf->msix.min; > >+ devl_param_driverinit_value_set(devlink, > >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > >+ value); > >+ > >+ return 0; > > } > > > > void ice_devlink_unregister_params(struct ice_pf *pf) > >@@ -1659,6 +1711,8 @@ void ice_devlink_unregister_params(struct ice_pf *pf) > > > > devl_params_unregister(devlink, ice_dvl_rdma_params, > > ARRAY_SIZE(ice_dvl_rdma_params)); > >+ devl_params_unregister(devlink, ice_dvl_msix_params, > >+ ARRAY_SIZE(ice_dvl_msix_params)); > > > > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) > > devl_params_unregister(devlink, ice_dvl_sched_params, > >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h > >index d6f80da30dec..a67456057c77 100644 > >--- a/drivers/net/ethernet/intel/ice/ice.h > >+++ b/drivers/net/ethernet/intel/ice/ice.h > >@@ -95,6 +95,7 @@ > > #define ICE_MIN_LAN_TXRX_MSIX 1 > > #define ICE_MIN_LAN_OICR_MSIX 1 > > #define ICE_MIN_MSIX (ICE_MIN_LAN_TXRX_MSIX + ICE_MIN_LAN_OICR_MSIX) > >+#define ICE_MAX_MSIX 256 > > #define ICE_FDIR_MSIX 2 > > #define ICE_RDMA_NUM_AEQ_MSIX 4 > > #define ICE_MIN_RDMA_MSIX 2 > >@@ -545,6 +546,12 @@ struct ice_agg_node { > > u8 valid; > > }; > > > >+struct ice_pf_msix { > >+ u16 cur; > >+ u16 min; > >+ u16 max; > >+}; > >+ > > struct ice_pf { > > struct pci_dev *pdev; > > struct ice_adapter *adapter; > >@@ -615,6 +622,7 @@ struct ice_pf { > > struct msi_map ll_ts_irq; /* LL_TS interrupt MSIX vector */ > > u16 max_pf_txqs; /* Total Tx queues PF wide */ > > u16 max_pf_rxqs; /* Total Rx queues PF wide */ > >+ struct ice_pf_msix msix; > > u16 num_lan_msix; /* Total MSIX vectors for base driver */ > > u16 num_lan_tx; /* num LAN Tx queues setup */ > > u16 num_lan_rx; /* num LAN Rx queues setup */ > >diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c > >index ad82ff7d1995..4e559fd6e49f 100644 > >--- a/drivers/net/ethernet/intel/ice/ice_irq.c > >+++ b/drivers/net/ethernet/intel/ice/ice_irq.c > >@@ -252,7 +252,19 @@ void ice_clear_interrupt_scheme(struct ice_pf *pf) > > int ice_init_interrupt_scheme(struct ice_pf *pf) > > { > > int total_vectors = pf->hw.func_caps.common_cap.num_msix_vectors; > >- int vectors, max_vectors; > >+ union devlink_param_value value; > >+ int vectors, max_vectors, err; > >+ > >+ /* load default PF MSI-X range */ > >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), > >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > >+ &value); > > If err is not 0, you have a bug in the driver. Perhaps it a about the > time to make this return void and add some WARN_ONs inside the function? > err is not 0 when this param isn't found (not registered yet). It is a case when driver is probing, I want to have here default values and register it later. Instead of checking if it is probe context or reload context I am checking if param already exists. The param doesn't exist in probe, but exists in reload. > > >+ pf->msix.min = err ? ICE_MIN_MSIX : value.vu16; > >+ > >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), > >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > >+ &value); > >+ pf->msix.max = err ? total_vectors / 2 : value.vu16; > > > > vectors = ice_ena_msix_range(pf); > > > >-- > >2.42.0 > >
Dear Michal, Thank you for your patch. Am 09.08.24 um 07:13 schrieb Michal Swiatkowski: > On Thu, Aug 08, 2024 at 05:34:35PM +0200, Jiri Pirko wrote: >> Thu, Aug 08, 2024 at 09:20:09AM CEST, michal.swiatkowski@linux.intel.com wrote: >>> Use generic devlink PF MSI-X parameter to allow user to change MSI-X >>> range. >>> >>> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> >>> --- >>> .../net/ethernet/intel/ice/devlink/devlink.c | 56 ++++++++++++++++++- >>> drivers/net/ethernet/intel/ice/ice.h | 8 +++ >>> drivers/net/ethernet/intel/ice/ice_irq.c | 14 ++++- >>> 3 files changed, 76 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c >>> index 29a5f822cb8b..bdc22ea13e0f 100644 >>> --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c >>> +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c >>> @@ -1518,6 +1518,32 @@ static int ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, >>> return 0; >>> } >>> >>> +static int >>> +ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, >>> + union devlink_param_value val, >>> + struct netlink_ext_ack *extack) >>> +{ >>> + if (val.vu16 > ICE_MAX_MSIX) { >>> + NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); >> >> No reason to have "PF" in the text. Also, no reason to have "max MSI-X". >> That is the name of the param. > > Ok, will change both, thanks. Maybe also print both values in the error message? >>> + return -EINVAL; >>> + } >>> + >>> + return 0; >>> +} […] Kind regards, Paul
[Cc: -nex.sw.ncis.nat.hpm.dev@intel.com (550 #5.1.0 Address rejected.)] Am 09.08.24 um 07:18 schrieb Paul Menzel: > Dear Michal, > > > Thank you for your patch. > > Am 09.08.24 um 07:13 schrieb Michal Swiatkowski: >> On Thu, Aug 08, 2024 at 05:34:35PM +0200, Jiri Pirko wrote: >>> Thu, Aug 08, 2024 at 09:20:09AM CEST, >>> michal.swiatkowski@linux.intel.com wrote: >>>> Use generic devlink PF MSI-X parameter to allow user to change MSI-X >>>> range. >>>> >>>> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >>>> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> >>>> --- >>>> .../net/ethernet/intel/ice/devlink/devlink.c | 56 ++++++++++++++++++- >>>> drivers/net/ethernet/intel/ice/ice.h | 8 +++ >>>> drivers/net/ethernet/intel/ice/ice_irq.c | 14 ++++- >>>> 3 files changed, 76 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/ >>>> drivers/net/ethernet/intel/ice/devlink/devlink.c >>>> index 29a5f822cb8b..bdc22ea13e0f 100644 >>>> --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c >>>> +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c >>>> @@ -1518,6 +1518,32 @@ static int >>>> ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, >>>> return 0; >>>> } >>>> >>>> +static int >>>> +ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, >>>> + union devlink_param_value val, >>>> + struct netlink_ext_ack *extack) >>>> +{ >>>> + if (val.vu16 > ICE_MAX_MSIX) { >>>> + NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); >>> >>> No reason to have "PF" in the text. Also, no reason to have "max MSI-X". >>> That is the name of the param. >> >> Ok, will change both, thanks. > > Maybe also print both values in the error message? > >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return 0; >>>> +} > > […] > > > Kind regards, > > Paul
Fri, Aug 09, 2024 at 07:13:34AM CEST, michal.swiatkowski@linux.intel.com wrote: >On Thu, Aug 08, 2024 at 05:34:35PM +0200, Jiri Pirko wrote: >> Thu, Aug 08, 2024 at 09:20:09AM CEST, michal.swiatkowski@linux.intel.com wrote: >> >Use generic devlink PF MSI-X parameter to allow user to change MSI-X >> >range. >> > >> >Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >> >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> >> >--- >> > .../net/ethernet/intel/ice/devlink/devlink.c | 56 ++++++++++++++++++- >> > drivers/net/ethernet/intel/ice/ice.h | 8 +++ >> > drivers/net/ethernet/intel/ice/ice_irq.c | 14 ++++- >> > 3 files changed, 76 insertions(+), 2 deletions(-) >> > >> >diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c >> >index 29a5f822cb8b..bdc22ea13e0f 100644 >> >--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c >> >+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c >> >@@ -1518,6 +1518,32 @@ static int ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, >> > return 0; >> > } >> > >> >+static int >> >+ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, >> >+ union devlink_param_value val, >> >+ struct netlink_ext_ack *extack) >> >+{ >> >+ if (val.vu16 > ICE_MAX_MSIX) { >> >+ NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); >> >> No reason to have "PF" in the text. Also, no reason to have "max MSI-X". >> That is the name of the param. >> > >Ok, will change both, thanks. > >> >> >> >+ return -EINVAL; >> >+ } >> >+ >> >+ return 0; >> >+} >> >+ >> >+static int >> >+ice_devlink_msix_min_pf_validate(struct devlink *devlink, u32 id, >> >+ union devlink_param_value val, >> >+ struct netlink_ext_ack *extack) >> >+{ >> >+ if (val.vu16 <= ICE_MIN_MSIX) { >> >+ NL_SET_ERR_MSG_MOD(extack, "PF min MSI-X is too low"); >> >> Same comment as for max goes here. >> >> >> >+ return -EINVAL; >> >+ } >> >+ >> >+ return 0; >> >+} >> >+ >> > enum ice_param_id { >> > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, >> > ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, >> >@@ -1535,6 +1561,15 @@ static const struct devlink_param ice_dvl_rdma_params[] = { >> > ice_devlink_enable_iw_validate), >> > }; >> > >> >+static const struct devlink_param ice_dvl_msix_params[] = { >> >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MAX, >> >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), >> >+ NULL, NULL, ice_devlink_msix_max_pf_validate), >> >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MIN, >> >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), >> >+ NULL, NULL, ice_devlink_msix_min_pf_validate), >> >+}; >> >+ >> > static const struct devlink_param ice_dvl_sched_params[] = { >> > DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, >> > "tx_scheduling_layers", >> >@@ -1637,6 +1672,7 @@ void ice_devlink_unregister(struct ice_pf *pf) >> > int ice_devlink_register_params(struct ice_pf *pf) >> > { >> > struct devlink *devlink = priv_to_devlink(pf); >> >+ union devlink_param_value value; >> > struct ice_hw *hw = &pf->hw; >> > int status; >> > >> >@@ -1645,11 +1681,27 @@ int ice_devlink_register_params(struct ice_pf *pf) >> > if (status) >> > return status; >> > >> >+ status = devl_params_register(devlink, ice_dvl_msix_params, >> >+ ARRAY_SIZE(ice_dvl_msix_params)); >> >+ if (status) >> >+ return status; >> >+ >> > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) >> > status = devl_params_register(devlink, ice_dvl_sched_params, >> > ARRAY_SIZE(ice_dvl_sched_params)); >> >+ if (status) >> >+ return status; >> > >> >- return status; >> >+ value.vu16 = pf->msix.max; >> >+ devl_param_driverinit_value_set(devlink, >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, >> >+ value); >> >+ value.vu16 = pf->msix.min; >> >+ devl_param_driverinit_value_set(devlink, >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, >> >+ value); >> >+ >> >+ return 0; >> > } >> > >> > void ice_devlink_unregister_params(struct ice_pf *pf) >> >@@ -1659,6 +1711,8 @@ void ice_devlink_unregister_params(struct ice_pf *pf) >> > >> > devl_params_unregister(devlink, ice_dvl_rdma_params, >> > ARRAY_SIZE(ice_dvl_rdma_params)); >> >+ devl_params_unregister(devlink, ice_dvl_msix_params, >> >+ ARRAY_SIZE(ice_dvl_msix_params)); >> > >> > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) >> > devl_params_unregister(devlink, ice_dvl_sched_params, >> >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h >> >index d6f80da30dec..a67456057c77 100644 >> >--- a/drivers/net/ethernet/intel/ice/ice.h >> >+++ b/drivers/net/ethernet/intel/ice/ice.h >> >@@ -95,6 +95,7 @@ >> > #define ICE_MIN_LAN_TXRX_MSIX 1 >> > #define ICE_MIN_LAN_OICR_MSIX 1 >> > #define ICE_MIN_MSIX (ICE_MIN_LAN_TXRX_MSIX + ICE_MIN_LAN_OICR_MSIX) >> >+#define ICE_MAX_MSIX 256 >> > #define ICE_FDIR_MSIX 2 >> > #define ICE_RDMA_NUM_AEQ_MSIX 4 >> > #define ICE_MIN_RDMA_MSIX 2 >> >@@ -545,6 +546,12 @@ struct ice_agg_node { >> > u8 valid; >> > }; >> > >> >+struct ice_pf_msix { >> >+ u16 cur; >> >+ u16 min; >> >+ u16 max; >> >+}; >> >+ >> > struct ice_pf { >> > struct pci_dev *pdev; >> > struct ice_adapter *adapter; >> >@@ -615,6 +622,7 @@ struct ice_pf { >> > struct msi_map ll_ts_irq; /* LL_TS interrupt MSIX vector */ >> > u16 max_pf_txqs; /* Total Tx queues PF wide */ >> > u16 max_pf_rxqs; /* Total Rx queues PF wide */ >> >+ struct ice_pf_msix msix; >> > u16 num_lan_msix; /* Total MSIX vectors for base driver */ >> > u16 num_lan_tx; /* num LAN Tx queues setup */ >> > u16 num_lan_rx; /* num LAN Rx queues setup */ >> >diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c >> >index ad82ff7d1995..4e559fd6e49f 100644 >> >--- a/drivers/net/ethernet/intel/ice/ice_irq.c >> >+++ b/drivers/net/ethernet/intel/ice/ice_irq.c >> >@@ -252,7 +252,19 @@ void ice_clear_interrupt_scheme(struct ice_pf *pf) >> > int ice_init_interrupt_scheme(struct ice_pf *pf) >> > { >> > int total_vectors = pf->hw.func_caps.common_cap.num_msix_vectors; >> >- int vectors, max_vectors; >> >+ union devlink_param_value value; >> >+ int vectors, max_vectors, err; >> >+ >> >+ /* load default PF MSI-X range */ >> >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, >> >+ &value); >> >> If err is not 0, you have a bug in the driver. Perhaps it a about the >> time to make this return void and add some WARN_ONs inside the function? >> > >err is not 0 when this param isn't found (not registered yet). It is a >case when driver is probing, I want to have here default values and >register it later. Instead of checking if it is probe context or reload >context I am checking if param already exists. The param doesn't exist in >probe, but exists in reload. No, you have to make sure that you are using these values after they are set. Please fix. > >> >> >+ pf->msix.min = err ? ICE_MIN_MSIX : value.vu16; >> >+ >> >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, >> >+ &value); >> >+ pf->msix.max = err ? total_vectors / 2 : value.vu16; >> > >> > vectors = ice_ena_msix_range(pf); >> > >> >-- >> >2.42.0 >> >
Fri, Aug 09, 2024 at 07:18:38AM CEST, pmenzel@molgen.mpg.de wrote: >Dear Michal, > > >Thank you for your patch. > >Am 09.08.24 um 07:13 schrieb Michal Swiatkowski: >> On Thu, Aug 08, 2024 at 05:34:35PM +0200, Jiri Pirko wrote: >> > Thu, Aug 08, 2024 at 09:20:09AM CEST, michal.swiatkowski@linux.intel.com wrote: >> > > Use generic devlink PF MSI-X parameter to allow user to change MSI-X >> > > range. >> > > >> > > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >> > > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> >> > > --- >> > > .../net/ethernet/intel/ice/devlink/devlink.c | 56 ++++++++++++++++++- >> > > drivers/net/ethernet/intel/ice/ice.h | 8 +++ >> > > drivers/net/ethernet/intel/ice/ice_irq.c | 14 ++++- >> > > 3 files changed, 76 insertions(+), 2 deletions(-) >> > > >> > > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c >> > > index 29a5f822cb8b..bdc22ea13e0f 100644 >> > > --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c >> > > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c >> > > @@ -1518,6 +1518,32 @@ static int ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, >> > > return 0; >> > > } >> > > >> > > +static int >> > > +ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, >> > > + union devlink_param_value val, >> > > + struct netlink_ext_ack *extack) >> > > +{ >> > > + if (val.vu16 > ICE_MAX_MSIX) { >> > > + NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); >> > >> > No reason to have "PF" in the text. Also, no reason to have "max MSI-X". >> > That is the name of the param. >> >> Ok, will change both, thanks. > >Maybe also print both values in the error message? Why? The user is passing the value. Does not make any sense. > >> > > + return -EINVAL; >> > > + } >> > > + >> > > + return 0; >> > > +} > >[…] > > >Kind regards, > >Paul
On Fri, Aug 09, 2024 at 12:51:58PM +0200, Jiri Pirko wrote: > Fri, Aug 09, 2024 at 07:13:34AM CEST, michal.swiatkowski@linux.intel.com wrote: > >On Thu, Aug 08, 2024 at 05:34:35PM +0200, Jiri Pirko wrote: > >> Thu, Aug 08, 2024 at 09:20:09AM CEST, michal.swiatkowski@linux.intel.com wrote: > >> >Use generic devlink PF MSI-X parameter to allow user to change MSI-X > >> >range. > >> > > >> >Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > >> >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > >> >--- > >> > .../net/ethernet/intel/ice/devlink/devlink.c | 56 ++++++++++++++++++- > >> > drivers/net/ethernet/intel/ice/ice.h | 8 +++ > >> > drivers/net/ethernet/intel/ice/ice_irq.c | 14 ++++- > >> > 3 files changed, 76 insertions(+), 2 deletions(-) > >> > > >> >diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c > >> >index 29a5f822cb8b..bdc22ea13e0f 100644 > >> >--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c > >> >+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c > >> >@@ -1518,6 +1518,32 @@ static int ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, > >> > return 0; > >> > } > >> > > >> >+static int > >> >+ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, > >> >+ union devlink_param_value val, > >> >+ struct netlink_ext_ack *extack) > >> >+{ > >> >+ if (val.vu16 > ICE_MAX_MSIX) { > >> >+ NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); > >> > >> No reason to have "PF" in the text. Also, no reason to have "max MSI-X". > >> That is the name of the param. > >> > > > >Ok, will change both, thanks. > > > >> > >> > >> >+ return -EINVAL; > >> >+ } > >> >+ > >> >+ return 0; > >> >+} > >> >+ > >> >+static int > >> >+ice_devlink_msix_min_pf_validate(struct devlink *devlink, u32 id, > >> >+ union devlink_param_value val, > >> >+ struct netlink_ext_ack *extack) > >> >+{ > >> >+ if (val.vu16 <= ICE_MIN_MSIX) { > >> >+ NL_SET_ERR_MSG_MOD(extack, "PF min MSI-X is too low"); > >> > >> Same comment as for max goes here. > >> > >> > >> >+ return -EINVAL; > >> >+ } > >> >+ > >> >+ return 0; > >> >+} > >> >+ > >> > enum ice_param_id { > >> > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > >> > ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, > >> >@@ -1535,6 +1561,15 @@ static const struct devlink_param ice_dvl_rdma_params[] = { > >> > ice_devlink_enable_iw_validate), > >> > }; > >> > > >> >+static const struct devlink_param ice_dvl_msix_params[] = { > >> >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MAX, > >> >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), > >> >+ NULL, NULL, ice_devlink_msix_max_pf_validate), > >> >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MIN, > >> >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), > >> >+ NULL, NULL, ice_devlink_msix_min_pf_validate), > >> >+}; > >> >+ > >> > static const struct devlink_param ice_dvl_sched_params[] = { > >> > DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, > >> > "tx_scheduling_layers", > >> >@@ -1637,6 +1672,7 @@ void ice_devlink_unregister(struct ice_pf *pf) > >> > int ice_devlink_register_params(struct ice_pf *pf) > >> > { > >> > struct devlink *devlink = priv_to_devlink(pf); > >> >+ union devlink_param_value value; > >> > struct ice_hw *hw = &pf->hw; > >> > int status; > >> > > >> >@@ -1645,11 +1681,27 @@ int ice_devlink_register_params(struct ice_pf *pf) > >> > if (status) > >> > return status; > >> > > >> >+ status = devl_params_register(devlink, ice_dvl_msix_params, > >> >+ ARRAY_SIZE(ice_dvl_msix_params)); > >> >+ if (status) > >> >+ return status; > >> >+ > >> > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) > >> > status = devl_params_register(devlink, ice_dvl_sched_params, > >> > ARRAY_SIZE(ice_dvl_sched_params)); > >> >+ if (status) > >> >+ return status; > >> > > >> >- return status; > >> >+ value.vu16 = pf->msix.max; > >> >+ devl_param_driverinit_value_set(devlink, > >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > >> >+ value); > >> >+ value.vu16 = pf->msix.min; > >> >+ devl_param_driverinit_value_set(devlink, > >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > >> >+ value); > >> >+ > >> >+ return 0; > >> > } > >> > > >> > void ice_devlink_unregister_params(struct ice_pf *pf) > >> >@@ -1659,6 +1711,8 @@ void ice_devlink_unregister_params(struct ice_pf *pf) > >> > > >> > devl_params_unregister(devlink, ice_dvl_rdma_params, > >> > ARRAY_SIZE(ice_dvl_rdma_params)); > >> >+ devl_params_unregister(devlink, ice_dvl_msix_params, > >> >+ ARRAY_SIZE(ice_dvl_msix_params)); > >> > > >> > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) > >> > devl_params_unregister(devlink, ice_dvl_sched_params, > >> >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h > >> >index d6f80da30dec..a67456057c77 100644 > >> >--- a/drivers/net/ethernet/intel/ice/ice.h > >> >+++ b/drivers/net/ethernet/intel/ice/ice.h > >> >@@ -95,6 +95,7 @@ > >> > #define ICE_MIN_LAN_TXRX_MSIX 1 > >> > #define ICE_MIN_LAN_OICR_MSIX 1 > >> > #define ICE_MIN_MSIX (ICE_MIN_LAN_TXRX_MSIX + ICE_MIN_LAN_OICR_MSIX) > >> >+#define ICE_MAX_MSIX 256 > >> > #define ICE_FDIR_MSIX 2 > >> > #define ICE_RDMA_NUM_AEQ_MSIX 4 > >> > #define ICE_MIN_RDMA_MSIX 2 > >> >@@ -545,6 +546,12 @@ struct ice_agg_node { > >> > u8 valid; > >> > }; > >> > > >> >+struct ice_pf_msix { > >> >+ u16 cur; > >> >+ u16 min; > >> >+ u16 max; > >> >+}; > >> >+ > >> > struct ice_pf { > >> > struct pci_dev *pdev; > >> > struct ice_adapter *adapter; > >> >@@ -615,6 +622,7 @@ struct ice_pf { > >> > struct msi_map ll_ts_irq; /* LL_TS interrupt MSIX vector */ > >> > u16 max_pf_txqs; /* Total Tx queues PF wide */ > >> > u16 max_pf_rxqs; /* Total Rx queues PF wide */ > >> >+ struct ice_pf_msix msix; > >> > u16 num_lan_msix; /* Total MSIX vectors for base driver */ > >> > u16 num_lan_tx; /* num LAN Tx queues setup */ > >> > u16 num_lan_rx; /* num LAN Rx queues setup */ > >> >diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c > >> >index ad82ff7d1995..4e559fd6e49f 100644 > >> >--- a/drivers/net/ethernet/intel/ice/ice_irq.c > >> >+++ b/drivers/net/ethernet/intel/ice/ice_irq.c > >> >@@ -252,7 +252,19 @@ void ice_clear_interrupt_scheme(struct ice_pf *pf) > >> > int ice_init_interrupt_scheme(struct ice_pf *pf) > >> > { > >> > int total_vectors = pf->hw.func_caps.common_cap.num_msix_vectors; > >> >- int vectors, max_vectors; > >> >+ union devlink_param_value value; > >> >+ int vectors, max_vectors, err; > >> >+ > >> >+ /* load default PF MSI-X range */ > >> >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), > >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > >> >+ &value); > >> > >> If err is not 0, you have a bug in the driver. Perhaps it a about the > >> time to make this return void and add some WARN_ONs inside the function? > >> > > > >err is not 0 when this param isn't found (not registered yet). It is a > >case when driver is probing, I want to have here default values and > >register it later. Instead of checking if it is probe context or reload > >context I am checking if param already exists. The param doesn't exist in > >probe, but exists in reload. > > No, you have to make sure that you are using these values after they are > set. Please fix. > I am not using value if this function returns error. If function returns error default values are set. The function devl_param_driverinit_value_get() is already checking if parameter exists. Why do you want me to check it before calling this function? Do you mean that calling it with not registered parameters is a problem? I don't see why it can be a problem. > > > > >> > >> >+ pf->msix.min = err ? ICE_MIN_MSIX : value.vu16; > >> >+ > >> >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), > >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > >> >+ &value); > >> >+ pf->msix.max = err ? total_vectors / 2 : value.vu16; > >> > > >> > vectors = ice_ena_msix_range(pf); > >> > > >> >-- > >> >2.42.0 > >> >
Fri, Aug 09, 2024 at 01:05:00PM CEST, michal.swiatkowski@linux.intel.com wrote: >On Fri, Aug 09, 2024 at 12:51:58PM +0200, Jiri Pirko wrote: >> Fri, Aug 09, 2024 at 07:13:34AM CEST, michal.swiatkowski@linux.intel.com wrote: >> >On Thu, Aug 08, 2024 at 05:34:35PM +0200, Jiri Pirko wrote: >> >> Thu, Aug 08, 2024 at 09:20:09AM CEST, michal.swiatkowski@linux.intel.com wrote: >> >> >Use generic devlink PF MSI-X parameter to allow user to change MSI-X >> >> >range. >> >> > >> >> >Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >> >> >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> >> >> >--- >> >> > .../net/ethernet/intel/ice/devlink/devlink.c | 56 ++++++++++++++++++- >> >> > drivers/net/ethernet/intel/ice/ice.h | 8 +++ >> >> > drivers/net/ethernet/intel/ice/ice_irq.c | 14 ++++- >> >> > 3 files changed, 76 insertions(+), 2 deletions(-) >> >> > >> >> >diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c >> >> >index 29a5f822cb8b..bdc22ea13e0f 100644 >> >> >--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c >> >> >+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c >> >> >@@ -1518,6 +1518,32 @@ static int ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, >> >> > return 0; >> >> > } >> >> > >> >> >+static int >> >> >+ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, >> >> >+ union devlink_param_value val, >> >> >+ struct netlink_ext_ack *extack) >> >> >+{ >> >> >+ if (val.vu16 > ICE_MAX_MSIX) { >> >> >+ NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); >> >> >> >> No reason to have "PF" in the text. Also, no reason to have "max MSI-X". >> >> That is the name of the param. >> >> >> > >> >Ok, will change both, thanks. >> > >> >> >> >> >> >> >+ return -EINVAL; >> >> >+ } >> >> >+ >> >> >+ return 0; >> >> >+} >> >> >+ >> >> >+static int >> >> >+ice_devlink_msix_min_pf_validate(struct devlink *devlink, u32 id, >> >> >+ union devlink_param_value val, >> >> >+ struct netlink_ext_ack *extack) >> >> >+{ >> >> >+ if (val.vu16 <= ICE_MIN_MSIX) { >> >> >+ NL_SET_ERR_MSG_MOD(extack, "PF min MSI-X is too low"); >> >> >> >> Same comment as for max goes here. >> >> >> >> >> >> >+ return -EINVAL; >> >> >+ } >> >> >+ >> >> >+ return 0; >> >> >+} >> >> >+ >> >> > enum ice_param_id { >> >> > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, >> >> > ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, >> >> >@@ -1535,6 +1561,15 @@ static const struct devlink_param ice_dvl_rdma_params[] = { >> >> > ice_devlink_enable_iw_validate), >> >> > }; >> >> > >> >> >+static const struct devlink_param ice_dvl_msix_params[] = { >> >> >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MAX, >> >> >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), >> >> >+ NULL, NULL, ice_devlink_msix_max_pf_validate), >> >> >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MIN, >> >> >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), >> >> >+ NULL, NULL, ice_devlink_msix_min_pf_validate), >> >> >+}; >> >> >+ >> >> > static const struct devlink_param ice_dvl_sched_params[] = { >> >> > DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, >> >> > "tx_scheduling_layers", >> >> >@@ -1637,6 +1672,7 @@ void ice_devlink_unregister(struct ice_pf *pf) >> >> > int ice_devlink_register_params(struct ice_pf *pf) >> >> > { >> >> > struct devlink *devlink = priv_to_devlink(pf); >> >> >+ union devlink_param_value value; >> >> > struct ice_hw *hw = &pf->hw; >> >> > int status; >> >> > >> >> >@@ -1645,11 +1681,27 @@ int ice_devlink_register_params(struct ice_pf *pf) >> >> > if (status) >> >> > return status; >> >> > >> >> >+ status = devl_params_register(devlink, ice_dvl_msix_params, >> >> >+ ARRAY_SIZE(ice_dvl_msix_params)); >> >> >+ if (status) >> >> >+ return status; >> >> >+ >> >> > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) >> >> > status = devl_params_register(devlink, ice_dvl_sched_params, >> >> > ARRAY_SIZE(ice_dvl_sched_params)); >> >> >+ if (status) >> >> >+ return status; >> >> > >> >> >- return status; >> >> >+ value.vu16 = pf->msix.max; >> >> >+ devl_param_driverinit_value_set(devlink, >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, >> >> >+ value); >> >> >+ value.vu16 = pf->msix.min; >> >> >+ devl_param_driverinit_value_set(devlink, >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, >> >> >+ value); >> >> >+ >> >> >+ return 0; >> >> > } >> >> > >> >> > void ice_devlink_unregister_params(struct ice_pf *pf) >> >> >@@ -1659,6 +1711,8 @@ void ice_devlink_unregister_params(struct ice_pf *pf) >> >> > >> >> > devl_params_unregister(devlink, ice_dvl_rdma_params, >> >> > ARRAY_SIZE(ice_dvl_rdma_params)); >> >> >+ devl_params_unregister(devlink, ice_dvl_msix_params, >> >> >+ ARRAY_SIZE(ice_dvl_msix_params)); >> >> > >> >> > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) >> >> > devl_params_unregister(devlink, ice_dvl_sched_params, >> >> >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h >> >> >index d6f80da30dec..a67456057c77 100644 >> >> >--- a/drivers/net/ethernet/intel/ice/ice.h >> >> >+++ b/drivers/net/ethernet/intel/ice/ice.h >> >> >@@ -95,6 +95,7 @@ >> >> > #define ICE_MIN_LAN_TXRX_MSIX 1 >> >> > #define ICE_MIN_LAN_OICR_MSIX 1 >> >> > #define ICE_MIN_MSIX (ICE_MIN_LAN_TXRX_MSIX + ICE_MIN_LAN_OICR_MSIX) >> >> >+#define ICE_MAX_MSIX 256 >> >> > #define ICE_FDIR_MSIX 2 >> >> > #define ICE_RDMA_NUM_AEQ_MSIX 4 >> >> > #define ICE_MIN_RDMA_MSIX 2 >> >> >@@ -545,6 +546,12 @@ struct ice_agg_node { >> >> > u8 valid; >> >> > }; >> >> > >> >> >+struct ice_pf_msix { >> >> >+ u16 cur; >> >> >+ u16 min; >> >> >+ u16 max; >> >> >+}; >> >> >+ >> >> > struct ice_pf { >> >> > struct pci_dev *pdev; >> >> > struct ice_adapter *adapter; >> >> >@@ -615,6 +622,7 @@ struct ice_pf { >> >> > struct msi_map ll_ts_irq; /* LL_TS interrupt MSIX vector */ >> >> > u16 max_pf_txqs; /* Total Tx queues PF wide */ >> >> > u16 max_pf_rxqs; /* Total Rx queues PF wide */ >> >> >+ struct ice_pf_msix msix; >> >> > u16 num_lan_msix; /* Total MSIX vectors for base driver */ >> >> > u16 num_lan_tx; /* num LAN Tx queues setup */ >> >> > u16 num_lan_rx; /* num LAN Rx queues setup */ >> >> >diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c >> >> >index ad82ff7d1995..4e559fd6e49f 100644 >> >> >--- a/drivers/net/ethernet/intel/ice/ice_irq.c >> >> >+++ b/drivers/net/ethernet/intel/ice/ice_irq.c >> >> >@@ -252,7 +252,19 @@ void ice_clear_interrupt_scheme(struct ice_pf *pf) >> >> > int ice_init_interrupt_scheme(struct ice_pf *pf) >> >> > { >> >> > int total_vectors = pf->hw.func_caps.common_cap.num_msix_vectors; >> >> >- int vectors, max_vectors; >> >> >+ union devlink_param_value value; >> >> >+ int vectors, max_vectors, err; >> >> >+ >> >> >+ /* load default PF MSI-X range */ >> >> >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, >> >> >+ &value); >> >> >> >> If err is not 0, you have a bug in the driver. Perhaps it a about the >> >> time to make this return void and add some WARN_ONs inside the function? >> >> >> > >> >err is not 0 when this param isn't found (not registered yet). It is a >> >case when driver is probing, I want to have here default values and >> >register it later. Instead of checking if it is probe context or reload >> >context I am checking if param already exists. The param doesn't exist in >> >probe, but exists in reload. >> >> No, you have to make sure that you are using these values after they are >> set. Please fix. >> > >I am not using value if this function returns error. If function returns >error default values are set. The function >devl_param_driverinit_value_get() is already checking if parameter >exists. Why do you want me to check it before calling this function? Do >you mean that calling it with not registered parameters is a problem? I >don't see why it can be a problem. If you call this for non-existing parameter, your driver flow is wrong. That's my point. > >> >> > >> >> >> >> >+ pf->msix.min = err ? ICE_MIN_MSIX : value.vu16; >> >> >+ >> >> >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, >> >> >+ &value); >> >> >+ pf->msix.max = err ? total_vectors / 2 : value.vu16; >> >> > >> >> > vectors = ice_ena_msix_range(pf); >> >> > >> >> >-- >> >> >2.42.0 >> >> >
On Fri, Aug 09, 2024 at 01:29:29PM +0200, Jiri Pirko wrote: > Fri, Aug 09, 2024 at 01:05:00PM CEST, michal.swiatkowski@linux.intel.com wrote: > >On Fri, Aug 09, 2024 at 12:51:58PM +0200, Jiri Pirko wrote: > >> Fri, Aug 09, 2024 at 07:13:34AM CEST, michal.swiatkowski@linux.intel.com wrote: > >> >On Thu, Aug 08, 2024 at 05:34:35PM +0200, Jiri Pirko wrote: > >> >> Thu, Aug 08, 2024 at 09:20:09AM CEST, michal.swiatkowski@linux.intel.com wrote: > >> >> >Use generic devlink PF MSI-X parameter to allow user to change MSI-X > >> >> >range. > >> >> > > >> >> >Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > >> >> >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > >> >> >--- > >> >> > .../net/ethernet/intel/ice/devlink/devlink.c | 56 ++++++++++++++++++- > >> >> > drivers/net/ethernet/intel/ice/ice.h | 8 +++ > >> >> > drivers/net/ethernet/intel/ice/ice_irq.c | 14 ++++- > >> >> > 3 files changed, 76 insertions(+), 2 deletions(-) > >> >> > > >> >> >diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c > >> >> >index 29a5f822cb8b..bdc22ea13e0f 100644 > >> >> >--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c > >> >> >+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c > >> >> >@@ -1518,6 +1518,32 @@ static int ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, > >> >> > return 0; > >> >> > } > >> >> > > >> >> >+static int > >> >> >+ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, > >> >> >+ union devlink_param_value val, > >> >> >+ struct netlink_ext_ack *extack) > >> >> >+{ > >> >> >+ if (val.vu16 > ICE_MAX_MSIX) { > >> >> >+ NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); > >> >> > >> >> No reason to have "PF" in the text. Also, no reason to have "max MSI-X". > >> >> That is the name of the param. > >> >> > >> > > >> >Ok, will change both, thanks. > >> > > >> >> > >> >> > >> >> >+ return -EINVAL; > >> >> >+ } > >> >> >+ > >> >> >+ return 0; > >> >> >+} > >> >> >+ > >> >> >+static int > >> >> >+ice_devlink_msix_min_pf_validate(struct devlink *devlink, u32 id, > >> >> >+ union devlink_param_value val, > >> >> >+ struct netlink_ext_ack *extack) > >> >> >+{ > >> >> >+ if (val.vu16 <= ICE_MIN_MSIX) { > >> >> >+ NL_SET_ERR_MSG_MOD(extack, "PF min MSI-X is too low"); > >> >> > >> >> Same comment as for max goes here. > >> >> > >> >> > >> >> >+ return -EINVAL; > >> >> >+ } > >> >> >+ > >> >> >+ return 0; > >> >> >+} > >> >> >+ > >> >> > enum ice_param_id { > >> >> > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > >> >> > ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, > >> >> >@@ -1535,6 +1561,15 @@ static const struct devlink_param ice_dvl_rdma_params[] = { > >> >> > ice_devlink_enable_iw_validate), > >> >> > }; > >> >> > > >> >> >+static const struct devlink_param ice_dvl_msix_params[] = { > >> >> >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MAX, > >> >> >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), > >> >> >+ NULL, NULL, ice_devlink_msix_max_pf_validate), > >> >> >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MIN, > >> >> >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), > >> >> >+ NULL, NULL, ice_devlink_msix_min_pf_validate), > >> >> >+}; > >> >> >+ > >> >> > static const struct devlink_param ice_dvl_sched_params[] = { > >> >> > DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, > >> >> > "tx_scheduling_layers", > >> >> >@@ -1637,6 +1672,7 @@ void ice_devlink_unregister(struct ice_pf *pf) > >> >> > int ice_devlink_register_params(struct ice_pf *pf) > >> >> > { > >> >> > struct devlink *devlink = priv_to_devlink(pf); > >> >> >+ union devlink_param_value value; > >> >> > struct ice_hw *hw = &pf->hw; > >> >> > int status; > >> >> > > >> >> >@@ -1645,11 +1681,27 @@ int ice_devlink_register_params(struct ice_pf *pf) > >> >> > if (status) > >> >> > return status; > >> >> > > >> >> >+ status = devl_params_register(devlink, ice_dvl_msix_params, > >> >> >+ ARRAY_SIZE(ice_dvl_msix_params)); > >> >> >+ if (status) > >> >> >+ return status; > >> >> >+ > >> >> > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) > >> >> > status = devl_params_register(devlink, ice_dvl_sched_params, > >> >> > ARRAY_SIZE(ice_dvl_sched_params)); > >> >> >+ if (status) > >> >> >+ return status; > >> >> > > >> >> >- return status; > >> >> >+ value.vu16 = pf->msix.max; > >> >> >+ devl_param_driverinit_value_set(devlink, > >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > >> >> >+ value); > >> >> >+ value.vu16 = pf->msix.min; > >> >> >+ devl_param_driverinit_value_set(devlink, > >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > >> >> >+ value); > >> >> >+ > >> >> >+ return 0; > >> >> > } > >> >> > > >> >> > void ice_devlink_unregister_params(struct ice_pf *pf) > >> >> >@@ -1659,6 +1711,8 @@ void ice_devlink_unregister_params(struct ice_pf *pf) > >> >> > > >> >> > devl_params_unregister(devlink, ice_dvl_rdma_params, > >> >> > ARRAY_SIZE(ice_dvl_rdma_params)); > >> >> >+ devl_params_unregister(devlink, ice_dvl_msix_params, > >> >> >+ ARRAY_SIZE(ice_dvl_msix_params)); > >> >> > > >> >> > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) > >> >> > devl_params_unregister(devlink, ice_dvl_sched_params, > >> >> >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h > >> >> >index d6f80da30dec..a67456057c77 100644 > >> >> >--- a/drivers/net/ethernet/intel/ice/ice.h > >> >> >+++ b/drivers/net/ethernet/intel/ice/ice.h > >> >> >@@ -95,6 +95,7 @@ > >> >> > #define ICE_MIN_LAN_TXRX_MSIX 1 > >> >> > #define ICE_MIN_LAN_OICR_MSIX 1 > >> >> > #define ICE_MIN_MSIX (ICE_MIN_LAN_TXRX_MSIX + ICE_MIN_LAN_OICR_MSIX) > >> >> >+#define ICE_MAX_MSIX 256 > >> >> > #define ICE_FDIR_MSIX 2 > >> >> > #define ICE_RDMA_NUM_AEQ_MSIX 4 > >> >> > #define ICE_MIN_RDMA_MSIX 2 > >> >> >@@ -545,6 +546,12 @@ struct ice_agg_node { > >> >> > u8 valid; > >> >> > }; > >> >> > > >> >> >+struct ice_pf_msix { > >> >> >+ u16 cur; > >> >> >+ u16 min; > >> >> >+ u16 max; > >> >> >+}; > >> >> >+ > >> >> > struct ice_pf { > >> >> > struct pci_dev *pdev; > >> >> > struct ice_adapter *adapter; > >> >> >@@ -615,6 +622,7 @@ struct ice_pf { > >> >> > struct msi_map ll_ts_irq; /* LL_TS interrupt MSIX vector */ > >> >> > u16 max_pf_txqs; /* Total Tx queues PF wide */ > >> >> > u16 max_pf_rxqs; /* Total Rx queues PF wide */ > >> >> >+ struct ice_pf_msix msix; > >> >> > u16 num_lan_msix; /* Total MSIX vectors for base driver */ > >> >> > u16 num_lan_tx; /* num LAN Tx queues setup */ > >> >> > u16 num_lan_rx; /* num LAN Rx queues setup */ > >> >> >diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c > >> >> >index ad82ff7d1995..4e559fd6e49f 100644 > >> >> >--- a/drivers/net/ethernet/intel/ice/ice_irq.c > >> >> >+++ b/drivers/net/ethernet/intel/ice/ice_irq.c > >> >> >@@ -252,7 +252,19 @@ void ice_clear_interrupt_scheme(struct ice_pf *pf) > >> >> > int ice_init_interrupt_scheme(struct ice_pf *pf) > >> >> > { > >> >> > int total_vectors = pf->hw.func_caps.common_cap.num_msix_vectors; > >> >> >- int vectors, max_vectors; > >> >> >+ union devlink_param_value value; > >> >> >+ int vectors, max_vectors, err; > >> >> >+ > >> >> >+ /* load default PF MSI-X range */ > >> >> >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), > >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > >> >> >+ &value); > >> >> > >> >> If err is not 0, you have a bug in the driver. Perhaps it a about the > >> >> time to make this return void and add some WARN_ONs inside the function? > >> >> > >> > > >> >err is not 0 when this param isn't found (not registered yet). It is a > >> >case when driver is probing, I want to have here default values and > >> >register it later. Instead of checking if it is probe context or reload > >> >context I am checking if param already exists. The param doesn't exist in > >> >probe, but exists in reload. > >> > >> No, you have to make sure that you are using these values after they are > >> set. Please fix. > >> > > > >I am not using value if this function returns error. If function returns > >error default values are set. The function > >devl_param_driverinit_value_get() is already checking if parameter > >exists. Why do you want me to check it before calling this function? Do > >you mean that calling it with not registered parameters is a problem? I > >don't see why it can be a problem. > > If you call this for non-existing parameter, your driver flow is wrong. > That's my point. > > But this function is checking this scenerio (existing of parameter), why not to use it? The ice_init_interrupt_scheme is reused in probe and in reload. I don't think it is reasonable to have one for probe and one for reload. Simpler is to check if the context is probe or reload. Instead of checking sth else (I don't know, flag from upper layer, or flag set only in probe/reload) I am checking if parameters exsists. I don't think the flow is wrong here. > > > >> > >> > > >> >> > >> >> >+ pf->msix.min = err ? ICE_MIN_MSIX : value.vu16; > >> >> >+ > >> >> >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), > >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > >> >> >+ &value); > >> >> >+ pf->msix.max = err ? total_vectors / 2 : value.vu16; > >> >> > > >> >> > vectors = ice_ena_msix_range(pf); > >> >> > > >> >> >-- > >> >> >2.42.0 > >> >> >
Fri, Aug 09, 2024 at 01:39:42PM CEST, michal.swiatkowski@linux.intel.com wrote: >On Fri, Aug 09, 2024 at 01:29:29PM +0200, Jiri Pirko wrote: >> Fri, Aug 09, 2024 at 01:05:00PM CEST, michal.swiatkowski@linux.intel.com wrote: >> >On Fri, Aug 09, 2024 at 12:51:58PM +0200, Jiri Pirko wrote: >> >> Fri, Aug 09, 2024 at 07:13:34AM CEST, michal.swiatkowski@linux.intel.com wrote: >> >> >On Thu, Aug 08, 2024 at 05:34:35PM +0200, Jiri Pirko wrote: >> >> >> Thu, Aug 08, 2024 at 09:20:09AM CEST, michal.swiatkowski@linux.intel.com wrote: >> >> >> >Use generic devlink PF MSI-X parameter to allow user to change MSI-X >> >> >> >range. >> >> >> > >> >> >> >Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >> >> >> >Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> >> >> >> >--- >> >> >> > .../net/ethernet/intel/ice/devlink/devlink.c | 56 ++++++++++++++++++- >> >> >> > drivers/net/ethernet/intel/ice/ice.h | 8 +++ >> >> >> > drivers/net/ethernet/intel/ice/ice_irq.c | 14 ++++- >> >> >> > 3 files changed, 76 insertions(+), 2 deletions(-) >> >> >> > >> >> >> >diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c >> >> >> >index 29a5f822cb8b..bdc22ea13e0f 100644 >> >> >> >--- a/drivers/net/ethernet/intel/ice/devlink/devlink.c >> >> >> >+++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c >> >> >> >@@ -1518,6 +1518,32 @@ static int ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, >> >> >> > return 0; >> >> >> > } >> >> >> > >> >> >> >+static int >> >> >> >+ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, >> >> >> >+ union devlink_param_value val, >> >> >> >+ struct netlink_ext_ack *extack) >> >> >> >+{ >> >> >> >+ if (val.vu16 > ICE_MAX_MSIX) { >> >> >> >+ NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); >> >> >> >> >> >> No reason to have "PF" in the text. Also, no reason to have "max MSI-X". >> >> >> That is the name of the param. >> >> >> >> >> > >> >> >Ok, will change both, thanks. >> >> > >> >> >> >> >> >> >> >> >> >+ return -EINVAL; >> >> >> >+ } >> >> >> >+ >> >> >> >+ return 0; >> >> >> >+} >> >> >> >+ >> >> >> >+static int >> >> >> >+ice_devlink_msix_min_pf_validate(struct devlink *devlink, u32 id, >> >> >> >+ union devlink_param_value val, >> >> >> >+ struct netlink_ext_ack *extack) >> >> >> >+{ >> >> >> >+ if (val.vu16 <= ICE_MIN_MSIX) { >> >> >> >+ NL_SET_ERR_MSG_MOD(extack, "PF min MSI-X is too low"); >> >> >> >> >> >> Same comment as for max goes here. >> >> >> >> >> >> >> >> >> >+ return -EINVAL; >> >> >> >+ } >> >> >> >+ >> >> >> >+ return 0; >> >> >> >+} >> >> >> >+ >> >> >> > enum ice_param_id { >> >> >> > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, >> >> >> > ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, >> >> >> >@@ -1535,6 +1561,15 @@ static const struct devlink_param ice_dvl_rdma_params[] = { >> >> >> > ice_devlink_enable_iw_validate), >> >> >> > }; >> >> >> > >> >> >> >+static const struct devlink_param ice_dvl_msix_params[] = { >> >> >> >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MAX, >> >> >> >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), >> >> >> >+ NULL, NULL, ice_devlink_msix_max_pf_validate), >> >> >> >+ DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MIN, >> >> >> >+ BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), >> >> >> >+ NULL, NULL, ice_devlink_msix_min_pf_validate), >> >> >> >+}; >> >> >> >+ >> >> >> > static const struct devlink_param ice_dvl_sched_params[] = { >> >> >> > DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, >> >> >> > "tx_scheduling_layers", >> >> >> >@@ -1637,6 +1672,7 @@ void ice_devlink_unregister(struct ice_pf *pf) >> >> >> > int ice_devlink_register_params(struct ice_pf *pf) >> >> >> > { >> >> >> > struct devlink *devlink = priv_to_devlink(pf); >> >> >> >+ union devlink_param_value value; >> >> >> > struct ice_hw *hw = &pf->hw; >> >> >> > int status; >> >> >> > >> >> >> >@@ -1645,11 +1681,27 @@ int ice_devlink_register_params(struct ice_pf *pf) >> >> >> > if (status) >> >> >> > return status; >> >> >> > >> >> >> >+ status = devl_params_register(devlink, ice_dvl_msix_params, >> >> >> >+ ARRAY_SIZE(ice_dvl_msix_params)); >> >> >> >+ if (status) >> >> >> >+ return status; >> >> >> >+ >> >> >> > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) >> >> >> > status = devl_params_register(devlink, ice_dvl_sched_params, >> >> >> > ARRAY_SIZE(ice_dvl_sched_params)); >> >> >> >+ if (status) >> >> >> >+ return status; >> >> >> > >> >> >> >- return status; >> >> >> >+ value.vu16 = pf->msix.max; >> >> >> >+ devl_param_driverinit_value_set(devlink, >> >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, >> >> >> >+ value); >> >> >> >+ value.vu16 = pf->msix.min; >> >> >> >+ devl_param_driverinit_value_set(devlink, >> >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, >> >> >> >+ value); >> >> >> >+ >> >> >> >+ return 0; >> >> >> > } >> >> >> > >> >> >> > void ice_devlink_unregister_params(struct ice_pf *pf) >> >> >> >@@ -1659,6 +1711,8 @@ void ice_devlink_unregister_params(struct ice_pf *pf) >> >> >> > >> >> >> > devl_params_unregister(devlink, ice_dvl_rdma_params, >> >> >> > ARRAY_SIZE(ice_dvl_rdma_params)); >> >> >> >+ devl_params_unregister(devlink, ice_dvl_msix_params, >> >> >> >+ ARRAY_SIZE(ice_dvl_msix_params)); >> >> >> > >> >> >> > if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) >> >> >> > devl_params_unregister(devlink, ice_dvl_sched_params, >> >> >> >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h >> >> >> >index d6f80da30dec..a67456057c77 100644 >> >> >> >--- a/drivers/net/ethernet/intel/ice/ice.h >> >> >> >+++ b/drivers/net/ethernet/intel/ice/ice.h >> >> >> >@@ -95,6 +95,7 @@ >> >> >> > #define ICE_MIN_LAN_TXRX_MSIX 1 >> >> >> > #define ICE_MIN_LAN_OICR_MSIX 1 >> >> >> > #define ICE_MIN_MSIX (ICE_MIN_LAN_TXRX_MSIX + ICE_MIN_LAN_OICR_MSIX) >> >> >> >+#define ICE_MAX_MSIX 256 >> >> >> > #define ICE_FDIR_MSIX 2 >> >> >> > #define ICE_RDMA_NUM_AEQ_MSIX 4 >> >> >> > #define ICE_MIN_RDMA_MSIX 2 >> >> >> >@@ -545,6 +546,12 @@ struct ice_agg_node { >> >> >> > u8 valid; >> >> >> > }; >> >> >> > >> >> >> >+struct ice_pf_msix { >> >> >> >+ u16 cur; >> >> >> >+ u16 min; >> >> >> >+ u16 max; >> >> >> >+}; >> >> >> >+ >> >> >> > struct ice_pf { >> >> >> > struct pci_dev *pdev; >> >> >> > struct ice_adapter *adapter; >> >> >> >@@ -615,6 +622,7 @@ struct ice_pf { >> >> >> > struct msi_map ll_ts_irq; /* LL_TS interrupt MSIX vector */ >> >> >> > u16 max_pf_txqs; /* Total Tx queues PF wide */ >> >> >> > u16 max_pf_rxqs; /* Total Rx queues PF wide */ >> >> >> >+ struct ice_pf_msix msix; >> >> >> > u16 num_lan_msix; /* Total MSIX vectors for base driver */ >> >> >> > u16 num_lan_tx; /* num LAN Tx queues setup */ >> >> >> > u16 num_lan_rx; /* num LAN Rx queues setup */ >> >> >> >diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c >> >> >> >index ad82ff7d1995..4e559fd6e49f 100644 >> >> >> >--- a/drivers/net/ethernet/intel/ice/ice_irq.c >> >> >> >+++ b/drivers/net/ethernet/intel/ice/ice_irq.c >> >> >> >@@ -252,7 +252,19 @@ void ice_clear_interrupt_scheme(struct ice_pf *pf) >> >> >> > int ice_init_interrupt_scheme(struct ice_pf *pf) >> >> >> > { >> >> >> > int total_vectors = pf->hw.func_caps.common_cap.num_msix_vectors; >> >> >> >- int vectors, max_vectors; >> >> >> >+ union devlink_param_value value; >> >> >> >+ int vectors, max_vectors, err; >> >> >> >+ >> >> >> >+ /* load default PF MSI-X range */ >> >> >> >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), >> >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, >> >> >> >+ &value); >> >> >> >> >> >> If err is not 0, you have a bug in the driver. Perhaps it a about the >> >> >> time to make this return void and add some WARN_ONs inside the function? >> >> >> >> >> > >> >> >err is not 0 when this param isn't found (not registered yet). It is a >> >> >case when driver is probing, I want to have here default values and >> >> >register it later. Instead of checking if it is probe context or reload >> >> >context I am checking if param already exists. The param doesn't exist in >> >> >probe, but exists in reload. >> >> >> >> No, you have to make sure that you are using these values after they are >> >> set. Please fix. >> >> >> > >> >I am not using value if this function returns error. If function returns >> >error default values are set. The function >> >devl_param_driverinit_value_get() is already checking if parameter >> >exists. Why do you want me to check it before calling this function? Do >> >you mean that calling it with not registered parameters is a problem? I >> >don't see why it can be a problem. >> >> If you call this for non-existing parameter, your driver flow is wrong. >> That's my point. >> >> > >But this function is checking this scenerio (existing of parameter), why >not to use it? It's basically a sanity check. There should be WARN_ON, for sure. I will send the patch to add it. > >The ice_init_interrupt_scheme is reused in probe and in reload. I don't >think it is reasonable to have one for probe and one for reload. Simpler >is to check if the context is probe or reload. Instead of checking sth No, you should not care if you are in reload or probe, you code should behave the same. In other words, you should get the param in a phase it is ready for both probe and reload flows. >else (I don't know, flag from upper layer, or flag set only in >probe/reload) I am checking if parameters exsists. I don't think the >flow is wrong here. > >> > >> >> >> >> > >> >> >> >> >> >> >+ pf->msix.min = err ? ICE_MIN_MSIX : value.vu16; >> >> >> >+ >> >> >> >+ err = devl_param_driverinit_value_get(priv_to_devlink(pf), >> >> >> >+ DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, >> >> >> >+ &value); >> >> >> >+ pf->msix.max = err ? total_vectors / 2 : value.vu16; >> >> >> > >> >> >> > vectors = ice_ena_msix_range(pf); >> >> >> > >> >> >> >-- >> >> >> >2.42.0 >> >> >> >
diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c index 29a5f822cb8b..bdc22ea13e0f 100644 --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c @@ -1518,6 +1518,32 @@ static int ice_devlink_local_fwd_validate(struct devlink *devlink, u32 id, return 0; } +static int +ice_devlink_msix_max_pf_validate(struct devlink *devlink, u32 id, + union devlink_param_value val, + struct netlink_ext_ack *extack) +{ + if (val.vu16 > ICE_MAX_MSIX) { + NL_SET_ERR_MSG_MOD(extack, "PF max MSI-X is too high"); + return -EINVAL; + } + + return 0; +} + +static int +ice_devlink_msix_min_pf_validate(struct devlink *devlink, u32 id, + union devlink_param_value val, + struct netlink_ext_ack *extack) +{ + if (val.vu16 <= ICE_MIN_MSIX) { + NL_SET_ERR_MSG_MOD(extack, "PF min MSI-X is too low"); + return -EINVAL; + } + + return 0; +} + enum ice_param_id { ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, @@ -1535,6 +1561,15 @@ static const struct devlink_param ice_dvl_rdma_params[] = { ice_devlink_enable_iw_validate), }; +static const struct devlink_param ice_dvl_msix_params[] = { + DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MAX, + BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), + NULL, NULL, ice_devlink_msix_max_pf_validate), + DEVLINK_PARAM_GENERIC(MSIX_VEC_PER_PF_MIN, + BIT(DEVLINK_PARAM_CMODE_DRIVERINIT), + NULL, NULL, ice_devlink_msix_min_pf_validate), +}; + static const struct devlink_param ice_dvl_sched_params[] = { DEVLINK_PARAM_DRIVER(ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, "tx_scheduling_layers", @@ -1637,6 +1672,7 @@ void ice_devlink_unregister(struct ice_pf *pf) int ice_devlink_register_params(struct ice_pf *pf) { struct devlink *devlink = priv_to_devlink(pf); + union devlink_param_value value; struct ice_hw *hw = &pf->hw; int status; @@ -1645,11 +1681,27 @@ int ice_devlink_register_params(struct ice_pf *pf) if (status) return status; + status = devl_params_register(devlink, ice_dvl_msix_params, + ARRAY_SIZE(ice_dvl_msix_params)); + if (status) + return status; + if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) status = devl_params_register(devlink, ice_dvl_sched_params, ARRAY_SIZE(ice_dvl_sched_params)); + if (status) + return status; - return status; + value.vu16 = pf->msix.max; + devl_param_driverinit_value_set(devlink, + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, + value); + value.vu16 = pf->msix.min; + devl_param_driverinit_value_set(devlink, + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, + value); + + return 0; } void ice_devlink_unregister_params(struct ice_pf *pf) @@ -1659,6 +1711,8 @@ void ice_devlink_unregister_params(struct ice_pf *pf) devl_params_unregister(devlink, ice_dvl_rdma_params, ARRAY_SIZE(ice_dvl_rdma_params)); + devl_params_unregister(devlink, ice_dvl_msix_params, + ARRAY_SIZE(ice_dvl_msix_params)); if (hw->func_caps.common_cap.tx_sched_topo_comp_mode_en) devl_params_unregister(devlink, ice_dvl_sched_params, diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h index d6f80da30dec..a67456057c77 100644 --- a/drivers/net/ethernet/intel/ice/ice.h +++ b/drivers/net/ethernet/intel/ice/ice.h @@ -95,6 +95,7 @@ #define ICE_MIN_LAN_TXRX_MSIX 1 #define ICE_MIN_LAN_OICR_MSIX 1 #define ICE_MIN_MSIX (ICE_MIN_LAN_TXRX_MSIX + ICE_MIN_LAN_OICR_MSIX) +#define ICE_MAX_MSIX 256 #define ICE_FDIR_MSIX 2 #define ICE_RDMA_NUM_AEQ_MSIX 4 #define ICE_MIN_RDMA_MSIX 2 @@ -545,6 +546,12 @@ struct ice_agg_node { u8 valid; }; +struct ice_pf_msix { + u16 cur; + u16 min; + u16 max; +}; + struct ice_pf { struct pci_dev *pdev; struct ice_adapter *adapter; @@ -615,6 +622,7 @@ struct ice_pf { struct msi_map ll_ts_irq; /* LL_TS interrupt MSIX vector */ u16 max_pf_txqs; /* Total Tx queues PF wide */ u16 max_pf_rxqs; /* Total Rx queues PF wide */ + struct ice_pf_msix msix; u16 num_lan_msix; /* Total MSIX vectors for base driver */ u16 num_lan_tx; /* num LAN Tx queues setup */ u16 num_lan_rx; /* num LAN Rx queues setup */ diff --git a/drivers/net/ethernet/intel/ice/ice_irq.c b/drivers/net/ethernet/intel/ice/ice_irq.c index ad82ff7d1995..4e559fd6e49f 100644 --- a/drivers/net/ethernet/intel/ice/ice_irq.c +++ b/drivers/net/ethernet/intel/ice/ice_irq.c @@ -252,7 +252,19 @@ void ice_clear_interrupt_scheme(struct ice_pf *pf) int ice_init_interrupt_scheme(struct ice_pf *pf) { int total_vectors = pf->hw.func_caps.common_cap.num_msix_vectors; - int vectors, max_vectors; + union devlink_param_value value; + int vectors, max_vectors, err; + + /* load default PF MSI-X range */ + err = devl_param_driverinit_value_get(priv_to_devlink(pf), + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, + &value); + pf->msix.min = err ? ICE_MIN_MSIX : value.vu16; + + err = devl_param_driverinit_value_get(priv_to_devlink(pf), + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, + &value); + pf->msix.max = err ? total_vectors / 2 : value.vu16; vectors = ice_ena_msix_range(pf);