Re: [iwl-next v1 1/7] ice: devlink PF MSI-X max and min parameter

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Tue, Feb 13, 2024 at 12:31:59PM +0100, Jiri Pirko wrote:
> Tue, Feb 13, 2024 at 11:01:08AM CET, michal.swiatkowski@linux.intel.com wrote:
> >On Tue, Feb 13, 2024 at 10:03:05AM +0100, Jiri Pirko wrote:
> >> Tue, Feb 13, 2024 at 08:35:03AM CET, 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>
> >> >---
> >> > drivers/net/ethernet/intel/ice/ice.h         |  8 ++
> >> > drivers/net/ethernet/intel/ice/ice_devlink.c | 82 ++++++++++++++++++++
> >> > drivers/net/ethernet/intel/ice/ice_irq.c     |  6 ++
> >> > 3 files changed, 96 insertions(+)
> >> >
> >> >diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> >> >index c4127d5f2be3..24085f3c0966 100644
> >> >--- a/drivers/net/ethernet/intel/ice/ice.h
> >> >+++ b/drivers/net/ethernet/intel/ice/ice.h
> >> >@@ -94,6 +94,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
> >> >@@ -535,6 +536,12 @@ struct ice_agg_node {
> >> > 	u8 valid;
> >> > };
> >> > 
> >> >+struct ice_pf_msix {
> >> >+	u16 cur;
> >> >+	u16 min;
> >> >+	u16 max;
> >> >+};
> >> >+
> >> > struct ice_pf {
> >> > 	struct pci_dev *pdev;
> >> > 
> >> >@@ -604,6 +611,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_devlink.c b/drivers/net/ethernet/intel/ice/ice_devlink.c
> >> >index cc717175178b..b82ff9556a4b 100644
> >> >--- a/drivers/net/ethernet/intel/ice/ice_devlink.c
> >> >+++ b/drivers/net/ethernet/intel/ice/ice_devlink.c
> >> >@@ -1603,6 +1603,78 @@ enum ice_param_id {
> >> > 	ICE_DEVLINK_PARAM_ID_LOOPBACK,
> >> > };
> >> > 
> >> >+static int
> >> >+ice_devlink_msix_max_pf_get(struct devlink *devlink, u32 id,
> >> >+			    struct devlink_param_gset_ctx *ctx)
> >> >+{
> >> >+	struct ice_pf *pf = devlink_priv(devlink);
> >> >+
> >> >+	ctx->val.vu16 = pf->msix.max;
> >> >+
> >> >+	return 0;
> >> >+}
> >> >+
> >> >+static int
> >> >+ice_devlink_msix_max_pf_set(struct devlink *devlink, u32 id,
> >> >+			    struct devlink_param_gset_ctx *ctx)
> >> >+{
> >> >+	struct ice_pf *pf = devlink_priv(devlink);
> >> >+	u16 max = ctx->val.vu16;
> >> >+
> >> >+	pf->msix.max = max;
> >> 
> >> What's permanent about this exactly?
> >> 
> >
> >I want to store the value here after driver reinit. Isn't it enough to
> >use this parameter type? Which one should be used for this purpose?
> 
> Documentation/networking/devlink/devlink-params.rst say:
> 
> .. list-table:: Possible configuration modes
>    :widths: 5 90
> 
>    * - Name
>      - Description
>    * - ``runtime``
>      - set while the driver is running, and takes effect immediately. No
>        reset is required.
>    * - ``driverinit``
>      - applied while the driver initializes. Requires the user to restart
>        the driver using the ``devlink`` reload command.
>    * - ``permanent``
>      - written to the device's non-volatile memory. A hard reset is required
>        for it to take effect.
> 
> 
> [...]
Thanks for pointing it, I changed the idea during developing it (at
first I wanted to store it in NVM) and forgot to change the type.

I will go with driverinit param.