* [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter [not found] <20250203210940.328608-1-anthony.l.nguyen@intel.com> @ 2025-02-03 21:09 ` Tony Nguyen 2025-02-03 21:48 ` David Laight 2025-02-04 22:35 ` Jakub Kicinski 0 siblings, 2 replies; 7+ messages in thread From: Tony Nguyen @ 2025-02-03 21:09 UTC (permalink / raw) To: davem, kuba, pabeni, edumazet, andrew+netdev, netdev Cc: Michal Swiatkowski, anthony.l.nguyen, sridhar.samudrala, jacob.e.keller, pio.raczynski, konrad.knitter, marcin.szycik, nex.sw.ncis.nat.hpm.dev, przemyslaw.kitszel, jiri, horms, David.Laight, pmenzel, mschmidt, tatyana.e.nikolova, Jason Gunthorpe, Leon Romanovsky, linux-rdma, corbet, linux-doc From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> Use generic devlink PF MSI-X parameter to allow user to change MSI-X range. Add notes about this parameters into ice devlink documentation. Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> --- Documentation/networking/devlink/ice.rst | 11 +++ .../net/ethernet/intel/ice/devlink/devlink.c | 88 +++++++++++++++++++ drivers/net/ethernet/intel/ice/ice.h | 7 ++ drivers/net/ethernet/intel/ice/ice_irq.c | 7 ++ 4 files changed, 113 insertions(+) diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst index e3972d03cea0..792e9f8c846a 100644 --- a/Documentation/networking/devlink/ice.rst +++ b/Documentation/networking/devlink/ice.rst @@ -69,6 +69,17 @@ Parameters To verify that value has been set: $ devlink dev param show pci/0000:16:00.0 name tx_scheduling_layers + * - ``msix_vec_per_pf_max`` + - driverinit + - Set the max MSI-X that can be used by the PF, rest can be utilized for + SRIOV. The range is from min value set in msix_vec_per_pf_min to + 2k/number of ports. + * - ``msix_vec_per_pf_min`` + - driverinit + - Set the min MSI-X that will be used by the PF. This value inform how many + MSI-X will be allocated statically. The range is from 2 to value set + in msix_vec_per_pf_max. + .. list-table:: Driver specific parameters implemented :widths: 5 5 90 diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c index d116e2b10bce..c53baecf8a90 100644 --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c @@ -1202,6 +1202,25 @@ static int ice_devlink_set_parent(struct devlink_rate *devlink_rate, return status; } +static void ice_set_min_max_msix(struct ice_pf *pf) +{ + struct devlink *devlink = priv_to_devlink(pf); + union devlink_param_value val; + int err; + + err = devl_param_driverinit_value_get(devlink, + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, + &val); + if (!err) + pf->msix.min = val.vu32; + + err = devl_param_driverinit_value_get(devlink, + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, + &val); + if (!err) + pf->msix.max = val.vu32; +} + /** * ice_devlink_reinit_up - do reinit of the given PF * @pf: pointer to the PF struct @@ -1217,6 +1236,9 @@ static int ice_devlink_reinit_up(struct ice_pf *pf) return err; } + /* load MSI-X values */ + ice_set_min_max_msix(pf); + err = ice_init_dev(pf); if (err) goto unroll_hw_init; @@ -1530,6 +1552,37 @@ 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) +{ + struct ice_pf *pf = devlink_priv(devlink); + + if (val.vu32 > pf->hw.func_caps.common_cap.num_msix_vectors || + val.vu32 < pf->msix.min) { + NL_SET_ERR_MSG_MOD(extack, "Value is invalid"); + 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) +{ + struct ice_pf *pf = devlink_priv(devlink); + + if (val.vu32 < ICE_MIN_MSIX || val.vu32 > pf->msix.max) { + NL_SET_ERR_MSG_MOD(extack, "Value is invalid"); + 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, @@ -1547,6 +1600,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", @@ -1648,6 +1710,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; @@ -1656,10 +1719,33 @@ 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) + goto unregister_rdma_params; + 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) + goto unregister_msix_params; + + value.vu32 = pf->msix.max; + devl_param_driverinit_value_set(devlink, + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, + value); + value.vu32 = pf->msix.min; + devl_param_driverinit_value_set(devlink, + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, + value); + return 0; +unregister_msix_params: + devl_params_unregister(devlink, ice_dvl_msix_params, + ARRAY_SIZE(ice_dvl_msix_params)); +unregister_rdma_params: + devl_params_unregister(devlink, ice_dvl_rdma_params, + ARRAY_SIZE(ice_dvl_rdma_params)); return status; } @@ -1670,6 +1756,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 71e05d30f0fd..d041b04ff324 100644 --- a/drivers/net/ethernet/intel/ice/ice.h +++ b/drivers/net/ethernet/intel/ice/ice.h @@ -542,6 +542,12 @@ struct ice_agg_node { u8 valid; }; +struct ice_pf_msix { + u32 cur; + u32 min; + u32 max; +}; + struct ice_pf { struct pci_dev *pdev; struct ice_adapter *adapter; @@ -612,6 +618,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..0659b96b9b8c 100644 --- a/drivers/net/ethernet/intel/ice/ice_irq.c +++ b/drivers/net/ethernet/intel/ice/ice_irq.c @@ -254,6 +254,13 @@ 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; + /* load default PF MSI-X range */ + if (!pf->msix.min) + pf->msix.min = ICE_MIN_MSIX; + + if (!pf->msix.max) + pf->msix.max = total_vectors / 2; + vectors = ice_ena_msix_range(pf); if (vectors < 0) -- 2.47.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter 2025-02-03 21:09 ` [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter Tony Nguyen @ 2025-02-03 21:48 ` David Laight 2025-02-04 6:06 ` Michal Swiatkowski 2025-02-04 22:35 ` Jakub Kicinski 1 sibling, 1 reply; 7+ messages in thread From: David Laight @ 2025-02-03 21:48 UTC (permalink / raw) To: Tony Nguyen Cc: davem, kuba, pabeni, edumazet, andrew+netdev, netdev, Michal Swiatkowski, sridhar.samudrala, jacob.e.keller, pio.raczynski, konrad.knitter, marcin.szycik, nex.sw.ncis.nat.hpm.dev, przemyslaw.kitszel, jiri, horms, David.Laight, pmenzel, mschmidt, tatyana.e.nikolova, Jason Gunthorpe, Leon Romanovsky, linux-rdma, corbet, linux-doc On Mon, 3 Feb 2025 13:09:31 -0800 Tony Nguyen <anthony.l.nguyen@intel.com> wrote: > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > Use generic devlink PF MSI-X parameter to allow user to change MSI-X > range. > > Add notes about this parameters into ice devlink documentation. > > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > --- > Documentation/networking/devlink/ice.rst | 11 +++ > .../net/ethernet/intel/ice/devlink/devlink.c | 88 +++++++++++++++++++ > drivers/net/ethernet/intel/ice/ice.h | 7 ++ > drivers/net/ethernet/intel/ice/ice_irq.c | 7 ++ > 4 files changed, 113 insertions(+) > > diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst > index e3972d03cea0..792e9f8c846a 100644 > --- a/Documentation/networking/devlink/ice.rst > +++ b/Documentation/networking/devlink/ice.rst > @@ -69,6 +69,17 @@ Parameters > > To verify that value has been set: > $ devlink dev param show pci/0000:16:00.0 name tx_scheduling_layers > + * - ``msix_vec_per_pf_max`` > + - driverinit > + - Set the max MSI-X that can be used by the PF, rest can be utilized for > + SRIOV. The range is from min value set in msix_vec_per_pf_min to > + 2k/number of ports. > + * - ``msix_vec_per_pf_min`` > + - driverinit > + - Set the min MSI-X that will be used by the PF. This value inform how many > + MSI-X will be allocated statically. The range is from 2 to value set > + in msix_vec_per_pf_max. > + > .. list-table:: Driver specific parameters implemented > :widths: 5 5 90 > > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c > index d116e2b10bce..c53baecf8a90 100644 > --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c > @@ -1202,6 +1202,25 @@ static int ice_devlink_set_parent(struct devlink_rate *devlink_rate, > return status; > } > > +static void ice_set_min_max_msix(struct ice_pf *pf) > +{ > + struct devlink *devlink = priv_to_devlink(pf); > + union devlink_param_value val; > + int err; > + > + err = devl_param_driverinit_value_get(devlink, > + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > + &val); > + if (!err) > + pf->msix.min = val.vu32; > + > + err = devl_param_driverinit_value_get(devlink, > + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > + &val); > + if (!err) > + pf->msix.max = val.vu32; > +} > + > /** > * ice_devlink_reinit_up - do reinit of the given PF > * @pf: pointer to the PF struct > @@ -1217,6 +1236,9 @@ static int ice_devlink_reinit_up(struct ice_pf *pf) > return err; > } > > + /* load MSI-X values */ > + ice_set_min_max_msix(pf); > + > err = ice_init_dev(pf); > if (err) > goto unroll_hw_init; > @@ -1530,6 +1552,37 @@ 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) > +{ > + struct ice_pf *pf = devlink_priv(devlink); > + > + if (val.vu32 > pf->hw.func_caps.common_cap.num_msix_vectors || > + val.vu32 < pf->msix.min) { > + NL_SET_ERR_MSG_MOD(extack, "Value is invalid"); > + 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) > +{ > + struct ice_pf *pf = devlink_priv(devlink); > + > + if (val.vu32 < ICE_MIN_MSIX || val.vu32 > pf->msix.max) { > + NL_SET_ERR_MSG_MOD(extack, "Value is invalid"); > + return -EINVAL; > + } > + > + return 0; > +} Don't those checks make it difficult to set the min and max together? I think you need to create the new min/max pair and check they are valid together. Which probably requires one parameter with two values. David > + > enum ice_param_id { > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, > @@ -1547,6 +1600,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", > @@ -1648,6 +1710,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; > > @@ -1656,10 +1719,33 @@ 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) > + goto unregister_rdma_params; > + > 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) > + goto unregister_msix_params; > + > + value.vu32 = pf->msix.max; > + devl_param_driverinit_value_set(devlink, > + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > + value); > + value.vu32 = pf->msix.min; > + devl_param_driverinit_value_set(devlink, > + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > + value); > + return 0; > > +unregister_msix_params: > + devl_params_unregister(devlink, ice_dvl_msix_params, > + ARRAY_SIZE(ice_dvl_msix_params)); > +unregister_rdma_params: > + devl_params_unregister(devlink, ice_dvl_rdma_params, > + ARRAY_SIZE(ice_dvl_rdma_params)); > return status; > } > > @@ -1670,6 +1756,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 71e05d30f0fd..d041b04ff324 100644 > --- a/drivers/net/ethernet/intel/ice/ice.h > +++ b/drivers/net/ethernet/intel/ice/ice.h > @@ -542,6 +542,12 @@ struct ice_agg_node { > u8 valid; > }; > > +struct ice_pf_msix { > + u32 cur; > + u32 min; > + u32 max; > +}; > + > struct ice_pf { > struct pci_dev *pdev; > struct ice_adapter *adapter; > @@ -612,6 +618,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..0659b96b9b8c 100644 > --- a/drivers/net/ethernet/intel/ice/ice_irq.c > +++ b/drivers/net/ethernet/intel/ice/ice_irq.c > @@ -254,6 +254,13 @@ 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; > > + /* load default PF MSI-X range */ > + if (!pf->msix.min) > + pf->msix.min = ICE_MIN_MSIX; > + > + if (!pf->msix.max) > + pf->msix.max = total_vectors / 2; > + > vectors = ice_ena_msix_range(pf); > > if (vectors < 0) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter 2025-02-03 21:48 ` David Laight @ 2025-02-04 6:06 ` Michal Swiatkowski 2025-02-04 18:41 ` David Laight 0 siblings, 1 reply; 7+ messages in thread From: Michal Swiatkowski @ 2025-02-04 6:06 UTC (permalink / raw) To: David Laight Cc: Tony Nguyen, davem, kuba, pabeni, edumazet, andrew+netdev, netdev, sridhar.samudrala, jacob.e.keller, pio.raczynski, konrad.knitter, marcin.szycik, nex.sw.ncis.nat.hpm.dev, przemyslaw.kitszel, jiri, horms, David.Laight, pmenzel, mschmidt, tatyana.e.nikolova, Jason Gunthorpe, Leon Romanovsky, linux-rdma, corbet, linux-doc On Mon, Feb 03, 2025 at 09:48:08PM +0000, David Laight wrote: > On Mon, 3 Feb 2025 13:09:31 -0800 > Tony Nguyen <anthony.l.nguyen@intel.com> wrote: > > > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > > > Use generic devlink PF MSI-X parameter to allow user to change MSI-X > > range. > > > > Add notes about this parameters into ice devlink documentation. > > > > Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> > > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > --- > > Documentation/networking/devlink/ice.rst | 11 +++ > > .../net/ethernet/intel/ice/devlink/devlink.c | 88 +++++++++++++++++++ > > drivers/net/ethernet/intel/ice/ice.h | 7 ++ > > drivers/net/ethernet/intel/ice/ice_irq.c | 7 ++ > > 4 files changed, 113 insertions(+) > > > > diff --git a/Documentation/networking/devlink/ice.rst b/Documentation/networking/devlink/ice.rst > > index e3972d03cea0..792e9f8c846a 100644 > > --- a/Documentation/networking/devlink/ice.rst > > +++ b/Documentation/networking/devlink/ice.rst > > @@ -69,6 +69,17 @@ Parameters > > > > To verify that value has been set: > > $ devlink dev param show pci/0000:16:00.0 name tx_scheduling_layers > > + * - ``msix_vec_per_pf_max`` > > + - driverinit > > + - Set the max MSI-X that can be used by the PF, rest can be utilized for > > + SRIOV. The range is from min value set in msix_vec_per_pf_min to > > + 2k/number of ports. > > + * - ``msix_vec_per_pf_min`` > > + - driverinit > > + - Set the min MSI-X that will be used by the PF. This value inform how many > > + MSI-X will be allocated statically. The range is from 2 to value set > > + in msix_vec_per_pf_max. > > + > > .. list-table:: Driver specific parameters implemented > > :widths: 5 5 90 > > > > diff --git a/drivers/net/ethernet/intel/ice/devlink/devlink.c b/drivers/net/ethernet/intel/ice/devlink/devlink.c > > index d116e2b10bce..c53baecf8a90 100644 > > --- a/drivers/net/ethernet/intel/ice/devlink/devlink.c > > +++ b/drivers/net/ethernet/intel/ice/devlink/devlink.c > > @@ -1202,6 +1202,25 @@ static int ice_devlink_set_parent(struct devlink_rate *devlink_rate, > > return status; > > } > > > > +static void ice_set_min_max_msix(struct ice_pf *pf) > > +{ > > + struct devlink *devlink = priv_to_devlink(pf); > > + union devlink_param_value val; > > + int err; > > + > > + err = devl_param_driverinit_value_get(devlink, > > + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > > + &val); > > + if (!err) > > + pf->msix.min = val.vu32; > > + > > + err = devl_param_driverinit_value_get(devlink, > > + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > > + &val); > > + if (!err) > > + pf->msix.max = val.vu32; > > +} > > + > > /** > > * ice_devlink_reinit_up - do reinit of the given PF > > * @pf: pointer to the PF struct > > @@ -1217,6 +1236,9 @@ static int ice_devlink_reinit_up(struct ice_pf *pf) > > return err; > > } > > > > + /* load MSI-X values */ > > + ice_set_min_max_msix(pf); > > + > > err = ice_init_dev(pf); > > if (err) > > goto unroll_hw_init; > > @@ -1530,6 +1552,37 @@ 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) > > +{ > > + struct ice_pf *pf = devlink_priv(devlink); > > + > > + if (val.vu32 > pf->hw.func_caps.common_cap.num_msix_vectors || > > + val.vu32 < pf->msix.min) { > > + NL_SET_ERR_MSG_MOD(extack, "Value is invalid"); > > + 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) > > +{ > > + struct ice_pf *pf = devlink_priv(devlink); > > + > > + if (val.vu32 < ICE_MIN_MSIX || val.vu32 > pf->msix.max) { > > + NL_SET_ERR_MSG_MOD(extack, "Value is invalid"); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > Don't those checks make it difficult to set the min and max together? > I think you need to create the new min/max pair and check they are > valid together. > Which probably requires one parameter with two values. > I wanted to reuse exsisting parameter. The other user of it is bnxt driver. In it there is a separate check for min "max" and max "max". It is also problematic, because min can be set to value greater than max (here it can happen when setting together to specific values). I can do a follow up to this series and change this parameter as you suggested. What do you think? Thanks, Michal > David > > > + > > enum ice_param_id { > > ICE_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX, > > ICE_DEVLINK_PARAM_ID_TX_SCHED_LAYERS, > > @@ -1547,6 +1600,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", > > @@ -1648,6 +1710,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; > > > > @@ -1656,10 +1719,33 @@ 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) > > + goto unregister_rdma_params; > > + > > 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) > > + goto unregister_msix_params; > > + > > + value.vu32 = pf->msix.max; > > + devl_param_driverinit_value_set(devlink, > > + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MAX, > > + value); > > + value.vu32 = pf->msix.min; > > + devl_param_driverinit_value_set(devlink, > > + DEVLINK_PARAM_GENERIC_ID_MSIX_VEC_PER_PF_MIN, > > + value); > > + return 0; > > > > +unregister_msix_params: > > + devl_params_unregister(devlink, ice_dvl_msix_params, > > + ARRAY_SIZE(ice_dvl_msix_params)); > > +unregister_rdma_params: > > + devl_params_unregister(devlink, ice_dvl_rdma_params, > > + ARRAY_SIZE(ice_dvl_rdma_params)); > > return status; > > } > > > > @@ -1670,6 +1756,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 71e05d30f0fd..d041b04ff324 100644 > > --- a/drivers/net/ethernet/intel/ice/ice.h > > +++ b/drivers/net/ethernet/intel/ice/ice.h > > @@ -542,6 +542,12 @@ struct ice_agg_node { > > u8 valid; > > }; > > > > +struct ice_pf_msix { > > + u32 cur; > > + u32 min; > > + u32 max; > > +}; > > + > > struct ice_pf { > > struct pci_dev *pdev; > > struct ice_adapter *adapter; > > @@ -612,6 +618,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..0659b96b9b8c 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_irq.c > > +++ b/drivers/net/ethernet/intel/ice/ice_irq.c > > @@ -254,6 +254,13 @@ 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; > > > > + /* load default PF MSI-X range */ > > + if (!pf->msix.min) > > + pf->msix.min = ICE_MIN_MSIX; > > + > > + if (!pf->msix.max) > > + pf->msix.max = total_vectors / 2; > > + > > vectors = ice_ena_msix_range(pf); > > > > if (vectors < 0) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter 2025-02-04 6:06 ` Michal Swiatkowski @ 2025-02-04 18:41 ` David Laight 2025-02-05 7:40 ` Michal Swiatkowski 0 siblings, 1 reply; 7+ messages in thread From: David Laight @ 2025-02-04 18:41 UTC (permalink / raw) To: Michal Swiatkowski Cc: Tony Nguyen, davem, kuba, pabeni, edumazet, andrew+netdev, netdev, sridhar.samudrala, jacob.e.keller, pio.raczynski, konrad.knitter, marcin.szycik, nex.sw.ncis.nat.hpm.dev, przemyslaw.kitszel, jiri, horms, David.Laight, pmenzel, mschmidt, tatyana.e.nikolova, Jason Gunthorpe, Leon Romanovsky, linux-rdma, corbet, linux-doc On Tue, 4 Feb 2025 07:06:00 +0100 Michal Swiatkowski <michal.swiatkowski@linux.intel.com> wrote: > On Mon, Feb 03, 2025 at 09:48:08PM +0000, David Laight wrote: > > On Mon, 3 Feb 2025 13:09:31 -0800 > > Tony Nguyen <anthony.l.nguyen@intel.com> wrote: > > > > > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > > > > > Use generic devlink PF MSI-X parameter to allow user to change MSI-X > > > range. > > > > > > Add notes about this parameters into ice devlink documentation. .... > > Don't those checks make it difficult to set the min and max together? > > I think you need to create the new min/max pair and check they are > > valid together. > > Which probably requires one parameter with two values. > > > > I wanted to reuse exsisting parameter. The other user of it is bnxt > driver. In it there is a separate check for min "max" and max "max". > It is also problematic, because min can be set to value greater than > max (here it can happen when setting together to specific values). > I can do a follow up to this series and change this parameter as you > suggested. What do you think? Changing the way a parameter is used will break API compatibility. Perhaps you can get the generic parameter validation function to update a 'pending' copy, and then do the final min < max check after all the parameters have been processed before actually updating the live limits. The other option is just not to check whether min < max and just document which takes precedence (and not use clamp()). It may even be worth saving the 'live limits' as 'hi << 16 | lo' so that then can be accessed atomically (with READ/WRITE_ONCE) to avoid anything looking at the limits getting confused. (Although maybe that doesn't matter here?) David > > Thanks, > Michal ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter 2025-02-04 18:41 ` David Laight @ 2025-02-05 7:40 ` Michal Swiatkowski 0 siblings, 0 replies; 7+ messages in thread From: Michal Swiatkowski @ 2025-02-05 7:40 UTC (permalink / raw) To: David Laight Cc: Tony Nguyen, davem, kuba, pabeni, edumazet, andrew+netdev, netdev, sridhar.samudrala, jacob.e.keller, pio.raczynski, konrad.knitter, marcin.szycik, nex.sw.ncis.nat.hpm.dev, przemyslaw.kitszel, jiri, horms, David.Laight, pmenzel, mschmidt, tatyana.e.nikolova, Jason Gunthorpe, Leon Romanovsky, linux-rdma, corbet, linux-doc On Tue, Feb 04, 2025 at 06:41:21PM +0000, David Laight wrote: > On Tue, 4 Feb 2025 07:06:00 +0100 > Michal Swiatkowski <michal.swiatkowski@linux.intel.com> wrote: > > > On Mon, Feb 03, 2025 at 09:48:08PM +0000, David Laight wrote: > > > On Mon, 3 Feb 2025 13:09:31 -0800 > > > Tony Nguyen <anthony.l.nguyen@intel.com> wrote: > > > > > > > From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > > > > > > > Use generic devlink PF MSI-X parameter to allow user to change MSI-X > > > > range. > > > > > > > > Add notes about this parameters into ice devlink documentation. > .... > > > Don't those checks make it difficult to set the min and max together? > > > I think you need to create the new min/max pair and check they are > > > valid together. > > > Which probably requires one parameter with two values. > > > > > > > I wanted to reuse exsisting parameter. The other user of it is bnxt > > driver. In it there is a separate check for min "max" and max "max". > > It is also problematic, because min can be set to value greater than > > max (here it can happen when setting together to specific values). > > I can do a follow up to this series and change this parameter as you > > suggested. What do you think? > > Changing the way a parameter is used will break API compatibility. > Perhaps you can get the generic parameter validation function to > update a 'pending' copy, and then do the final min < max check after > all the parameters have been processed before actually updating > the live limits. > > The other option is just not to check whether min < max and just > document which takes precedence (and not use clamp()). > > It may even be worth saving the 'live limits' as 'hi << 16 | lo' so > that then can be accessed atomically (with READ/WRITE_ONCE) to avoid > anything looking at the limits getting confused. > (Although maybe that doesn't matter here?) > > David Right, I though it is better to have any additional validation for min > max cases, but it looks like it is more problematic. I can drop it to algin with the bnxt solution. Thanks, Michal > > > > > Thanks, > > Michal ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter 2025-02-03 21:09 ` [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter Tony Nguyen 2025-02-03 21:48 ` David Laight @ 2025-02-04 22:35 ` Jakub Kicinski 2025-02-05 5:46 ` Michal Swiatkowski 1 sibling, 1 reply; 7+ messages in thread From: Jakub Kicinski @ 2025-02-04 22:35 UTC (permalink / raw) To: Tony Nguyen Cc: davem, pabeni, edumazet, andrew+netdev, netdev, Michal Swiatkowski, sridhar.samudrala, jacob.e.keller, pio.raczynski, konrad.knitter, marcin.szycik, nex.sw.ncis.nat.hpm.dev, przemyslaw.kitszel, jiri, horms, David.Laight, pmenzel, mschmidt, tatyana.e.nikolova, Jason Gunthorpe, Leon Romanovsky, linux-rdma, corbet, linux-doc On Mon, 3 Feb 2025 13:09:31 -0800 Tony Nguyen wrote: > + if (val.vu32 > pf->hw.func_caps.common_cap.num_msix_vectors || > + val.vu32 < pf->msix.min) { > + NL_SET_ERR_MSG_MOD(extack, "Value is invalid"); > + return -EINVAL; > + if (val.vu32 < ICE_MIN_MSIX || val.vu32 > pf->msix.max) { > + NL_SET_ERR_MSG_MOD(extack, "Value is invalid"); > + return -EINVAL; Please follow up and either remove these extack messages, or make them more meaningful. The "value is invalid" is already expressed by EINVAL The suggestion to set the values at once or as "pending" is a distraction IMO. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter 2025-02-04 22:35 ` Jakub Kicinski @ 2025-02-05 5:46 ` Michal Swiatkowski 0 siblings, 0 replies; 7+ messages in thread From: Michal Swiatkowski @ 2025-02-05 5:46 UTC (permalink / raw) To: Jakub Kicinski Cc: Tony Nguyen, davem, pabeni, edumazet, andrew+netdev, netdev, sridhar.samudrala, jacob.e.keller, pio.raczynski, konrad.knitter, marcin.szycik, nex.sw.ncis.nat.hpm.dev, przemyslaw.kitszel, jiri, horms, David.Laight, pmenzel, mschmidt, tatyana.e.nikolova, Jason Gunthorpe, Leon Romanovsky, linux-rdma, corbet, linux-doc On Tue, Feb 04, 2025 at 02:35:17PM -0800, Jakub Kicinski wrote: > On Mon, 3 Feb 2025 13:09:31 -0800 Tony Nguyen wrote: > > + if (val.vu32 > pf->hw.func_caps.common_cap.num_msix_vectors || > > + val.vu32 < pf->msix.min) { > > + NL_SET_ERR_MSG_MOD(extack, "Value is invalid"); > > + return -EINVAL; > > > + if (val.vu32 < ICE_MIN_MSIX || val.vu32 > pf->msix.max) { > > + NL_SET_ERR_MSG_MOD(extack, "Value is invalid"); > > + return -EINVAL; > > Please follow up and either remove these extack messages, or make them > more meaningful. The "value is invalid" is already expressed by EINVAL Will be removed. > > The suggestion to set the values at once or as "pending" is a > distraction IMO. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-02-05 7:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250203210940.328608-1-anthony.l.nguyen@intel.com>
2025-02-03 21:09 ` [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter Tony Nguyen
2025-02-03 21:48 ` David Laight
2025-02-04 6:06 ` Michal Swiatkowski
2025-02-04 18:41 ` David Laight
2025-02-05 7:40 ` Michal Swiatkowski
2025-02-04 22:35 ` Jakub Kicinski
2025-02-05 5:46 ` Michal Swiatkowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).