[PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter

[PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter

[Tony Nguyen]

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

Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter

[David Laight]

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)

Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter

[Michal Swiatkowski]

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)
> 

Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter

[David Laight]

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

Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter

[Jakub Kicinski]

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.

Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter

[Michal Swiatkowski]

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.

Re: [PATCH net-next 2/9] ice: devlink PF MSI-X max and min parameter

[Michal Swiatkowski]

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