[PATCH net-next V5 0/2] devlink: add generic device max_sfs parameter

[PATCH net-next V5 0/2] devlink: add generic device max_sfs parameter

[Tariq Toukan]

Hi,

This series by Nikolay introduces a new generic devlink device
parameter, max_sfs, to control the number of light-weight NIC
subfunctions (SFs) that can be created on a device.

The first patch adds the generic devlink parameter and infrastructure
support.
The second patch implements support for the parameter in the mlx5
driver.

With this addition, users can enable or disable SF creation directly via
devlink, without relying on external vendor-specific tools.

Regards,
Tariq

Notes about a few items from a previous Sashiko review:
 > Should there be a validate callback analogous to
 > mlx5_devlink_total_vfs_validate() that reads the cap and rejects values
 > above the device-reported maximum, so the documented "device-specific
 > max" is actually enforced?

 N: I don't know of such cap, if there was I'd have added it.

 > mlx5_devlink_total_vfs_set() rejects with -EOPNOTSUPP and an extack
 > "SRIOV is not per PF on this device" when sriov_support or
 > per_pf_total_vf_supported is clear, but no equivalent
 > per_pf_num_sf_supported (or any SF-related) capability bit is added to
 > nv_global_pci_cap_bits or queried here.  On hardware that lacks the
 > feature, the user only sees the generic firmware error "Failed to
 > change ... global PCI configuration".

 N: I don't know of such bit, if there was such bit I'd have added it.

 > At this point, a successful write of per_pf_num_sf has already been
 > committed to non-volatile firmware storage.  If the subsequent
 > mlx5_nv_param_read_per_host_pf_conf() or the second mlx5_nv_param_write()
 > fails (for example, transient firmware/PCIe issue), is there a path that
 > rolls back per_pf_num_sf?
 >
 > Because these are permanent parameters that "require a reboot to take
 > effect", a half-applied state (e.g., per_pf_num_sf=1 but
 > pf_total_sf_en=0/total_sf=0) appears to persist across reboots until
 > the user issues another successful set.

 N: That is expected and in line with the rest of the code.

 > The commit message says max_sfs is to "control the total light-weight
 > NIC subfunctions"; the BAR-size side-effect is not mentioned, and any
 > previously configured log_sf_bar_size is overwritten on every
 > max_sfs set.  Should that behavior be documented or split out from the
 > count knob?

 N: It should be documented when it is split out, at present time - no.

 > Does this unconditionally overwrite any existing configuration for the
 > SubFunction BAR size?
 > When a user configures a custom log_sf_bar_size via firmware tools like
 > mlxconfig, and then later uses devlink to change max_sfs, it appears this
 > line discards any non-zero value already present in the configuration data
 > and forcibly replaces it with MLX5_DEFAULT_LOG_SF_BAR_SIZE.
 > Should this code preserve the existing log_sf_bar_size if it was already
 > configured?

 N: Yes, it does and it is expected and intentional.

V5:
- rebase again
- patch 02: always enable per_pf_num_sf when setting SFs > 0 because other
  PFs might be using SFs as well (sashiko)
- patch 02: use NL_SET_ERR_MSG_MOD in max_sfs set

V4:
https://lore.kernel.org/netdev/20260723082643.1870844-1-tariqt@nvidia.com/
- rebase due to build failure because of commit:
  d603517771d8 ("devlink: pass param values by pointer")
  fixed in patch 02
- added Alexander Lobakin's reviewed-by tags (please let me know
  if that is not ok, the change in patch 02 is minor and mechanical
  due to the commit above)

V3, all changes are in patch 02 (mlx5 implementation):
https://lore.kernel.org/netdev/b087dc20-7398-4ad3-9787-efef883d81b1@blackwall.org
- Cap max_sfs at U16_MAX using a validate callback (sashiko)
- Change the warning message to match the docs (sashiko)
- On get verify that per_pf_num_sf & pf_total_sf_en are set
  otherwise return 0 since it means SFs are not properly enabled (sashiko)
- Define the default log bar size and use it instead of raw value (sashiko)

V2:
https://lore.kernel.org/netdev/20260519200436.353249-1-tariqt@nvidia.com/
- Add missing ` (Aleksandr Loktionov).
- Add review tag to patch 1.

V1:
https://lore.kernel.org/all/20260517112700.343575-1-tariqt@nvidia.com/

Nikolay Aleksandrov (2):
  devlink: add generic device max_sfs parameter
  net/mlx5: implement max_sfs parameter

 .../networking/devlink/devlink-params.rst     |   6 +
 Documentation/networking/devlink/mlx5.rst     |   7 +-
 .../mellanox/mlx5/core/lib/nv_param.c         | 125 +++++++++++++++++-
 include/net/devlink.h                         |   4 +
 net/devlink/param.c                           |   5 +
 5 files changed, 143 insertions(+), 4 deletions(-)


base-commit: b0057c68df711bf6a62033c072ac61c4f9d3cbc1
-- 
2.44.0

[PATCH net-next V5 1/2] devlink: add generic device max_sfs parameter

[Tariq Toukan]

From: Nikolay Aleksandrov <nikolay@nvidia.com>

Add a new generic devlink device parameter (max_sfs) to control if and
how many light-weight NIC subfunctions can be created. Subfunctions are
a light-weight network functions backed by an underlying PCI function.
Their lifecycle can already be managed by devlink, but currently users
cannot enable them in the device. They can be enabled/disabled only via
external vendor tools. This parameter allows subfunctions to be enabled
(>0) or disabled (0) via devlink. A subsequent patch will add support
for max_sfs to the mlx5 driver.

Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com>
Reviewed-by: David Ahern <dsahern@kernel.org>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 Documentation/networking/devlink/devlink-params.rst | 6 ++++++
 include/net/devlink.h                               | 4 ++++
 net/devlink/param.c                                 | 5 +++++
 3 files changed, 15 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-params.rst b/Documentation/networking/devlink/devlink-params.rst
index ca19ee3e63c8..13eb8aa84c38 100644
--- a/Documentation/networking/devlink/devlink-params.rst
+++ b/Documentation/networking/devlink/devlink-params.rst
@@ -165,3 +165,9 @@ own name.
      - u32
      - Controls the maximum number of MAC address filters that can be assigned
        to a Virtual Function (VF).
+   * - ``max_sfs``
+     - u32
+     - The maximum number of subfunctions which can be created on the device.
+       Modifying this parameter may require a device restart and PCI bus
+       rescanning because the BAR layout may change. A value of 0 disables
+       subfunction creation.
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 4830aba4087a..7abd23376319 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -554,6 +554,7 @@ enum devlink_param_generic_id {
 	DEVLINK_PARAM_GENERIC_ID_TOTAL_VFS,
 	DEVLINK_PARAM_GENERIC_ID_NUM_DOORBELLS,
 	DEVLINK_PARAM_GENERIC_ID_MAX_MAC_PER_VF,
+	DEVLINK_PARAM_GENERIC_ID_MAX_SFS,
 
 	/* add new param generic ids above here*/
 	__DEVLINK_PARAM_GENERIC_ID_MAX,
@@ -627,6 +628,9 @@ enum devlink_param_generic_id {
 #define DEVLINK_PARAM_GENERIC_MAX_MAC_PER_VF_NAME "max_mac_per_vf"
 #define DEVLINK_PARAM_GENERIC_MAX_MAC_PER_VF_TYPE DEVLINK_PARAM_TYPE_U32
 
+#define DEVLINK_PARAM_GENERIC_MAX_SFS_NAME "max_sfs"
+#define DEVLINK_PARAM_GENERIC_MAX_SFS_TYPE DEVLINK_PARAM_TYPE_U32
+
 #define DEVLINK_PARAM_GENERIC(_id, _cmodes, _get, _set, _validate)	\
 {									\
 	.id = DEVLINK_PARAM_GENERIC_ID_##_id,				\
diff --git a/net/devlink/param.c b/net/devlink/param.c
index 1cc562a6ebfd..8ca0f3ed646c 100644
--- a/net/devlink/param.c
+++ b/net/devlink/param.c
@@ -117,6 +117,11 @@ static const struct devlink_param devlink_param_generic[] = {
 		.name = DEVLINK_PARAM_GENERIC_MAX_MAC_PER_VF_NAME,
 		.type = DEVLINK_PARAM_GENERIC_MAX_MAC_PER_VF_TYPE,
 	},
+	{
+		.id = DEVLINK_PARAM_GENERIC_ID_MAX_SFS,
+		.name = DEVLINK_PARAM_GENERIC_MAX_SFS_NAME,
+		.type = DEVLINK_PARAM_GENERIC_MAX_SFS_TYPE,
+	},
 };
 
 static int devlink_param_generic_verify(const struct devlink_param *param)
-- 
2.44.0

[PATCH net-next V5 2/2] net/mlx5: implement max_sfs parameter

[Tariq Toukan]

From: Nikolay Aleksandrov <nikolay@nvidia.com>

Implement max_sfs generic parameter to allow users to control the total
light-weight NIC subfunctions that can be created using devlink instead
of external vendor tools. A value of 0 will effectively disable creation
of new subfunction devices. A warning is sent to user-space via extack
(returning extack without error code is interpreted as a warning by
user-space tools). The maximum value is capped at U16_MAX.

Signed-off-by: Nikolay Aleksandrov <nikolay@nvidia.com>
Reviewed-by: David Ahern <dsahern@kernel.org>
Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 Documentation/networking/devlink/mlx5.rst     |   7 +-
 .../mellanox/mlx5/core/lib/nv_param.c         | 125 +++++++++++++++++-
 2 files changed, 128 insertions(+), 4 deletions(-)

diff --git a/Documentation/networking/devlink/mlx5.rst b/Documentation/networking/devlink/mlx5.rst
index cf1dffa67669..9421c89951a0 100644
--- a/Documentation/networking/devlink/mlx5.rst
+++ b/Documentation/networking/devlink/mlx5.rst
@@ -45,8 +45,13 @@ Parameters
      - The range is between 1 and a device-specific max.
      - Applies to each physical function (PF) independently, if the device
        supports it. Otherwise, it applies symmetrically to all PFs.
+   * - ``max_sfs``
+     - permanent
+     - The range is between 0 and a device-specific max.
+     - Applies to each physical function (PF) independently.
 
-Note: permanent parameters such as ``enable_sriov`` and ``total_vfs`` require FW reset to take effect
+Note: permanent parameters such as ``enable_sriov``, ``total_vfs`` and ``max_sfs``
+      require FW reset to take effect
 
 .. code-block:: bash
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c
index 4a7275e8b62e..72cc8da5f119 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c
@@ -68,7 +68,9 @@ struct mlx5_ifc_mnvda_reg_bits {
 
 struct mlx5_ifc_nv_global_pci_conf_bits {
 	u8         sriov_valid[0x1];
-	u8         reserved_at_1[0x10];
+	u8         reserved_at_1[0xa];
+	u8         per_pf_num_sf[0x1];
+	u8         reserved_at_c[0x5];
 	u8         per_pf_total_vf[0x1];
 	u8         reserved_at_12[0xe];
 
@@ -93,9 +95,11 @@ struct mlx5_ifc_nv_global_pci_cap_bits {
 };
 
 struct mlx5_ifc_nv_pf_pci_conf_bits {
-	u8         reserved_at_0[0x9];
+	u8         log_sf_bar_size[0x8];
+	u8         pf_total_sf_en[0x1];
 	u8         pf_total_vf_en[0x1];
-	u8         reserved_at_a[0x16];
+	u8         reserved_at_a[0x6];
+	u8         total_sf[0x10];
 
 	u8         reserved_at_20[0x20];
 
@@ -158,6 +162,8 @@ struct mlx5_ifc_nv_sw_accelerate_conf_bits {
 #define MLX5_GET_CFG_HDR_LEN(_mnvda_ptr) \
 	MLX5_GET(mnvda_reg, _mnvda_ptr, configuration_item_header.length)
 
+#define MLX5_DEFAULT_LOG_SF_BAR_SIZE 12
+
 static int mlx5_nv_param_read(struct mlx5_core_dev *dev, void *mnvda,
 			      size_t len)
 {
@@ -755,6 +761,115 @@ static int mlx5_devlink_total_vfs_validate(struct devlink *devlink, u32 id,
 	return 0;
 }
 
+static int mlx5_devlink_max_sfs_get(struct devlink *devlink, u32 id,
+				    struct devlink_param_gset_ctx *ctx,
+				    struct netlink_ext_ack *extack)
+{
+	struct mlx5_core_dev *dev = devlink_priv(devlink);
+	u32 mnvda[MLX5_ST_SZ_DW(mnvda_reg)] = {};
+	void *data;
+	int err;
+
+	err = mlx5_nv_param_read_global_pci_conf(dev, mnvda, sizeof(mnvda));
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Failed to read global PCI configuration");
+		return err;
+	}
+
+	data = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
+	if (!MLX5_GET(nv_global_pci_conf, data, per_pf_num_sf)) {
+		ctx->val.vu32 = 0;
+		return 0;
+	}
+
+	memset(mnvda, 0, sizeof(mnvda));
+	err = mlx5_nv_param_read_per_host_pf_conf(dev, mnvda, sizeof(mnvda));
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to read PF configuration");
+		return err;
+	}
+
+	data = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
+	if (MLX5_GET(nv_pf_pci_conf, data, pf_total_sf_en))
+		ctx->val.vu32 = MLX5_GET(nv_pf_pci_conf, data, total_sf);
+	else
+		ctx->val.vu32 = 0;
+
+	return 0;
+}
+
+static int mlx5_devlink_max_sfs_validate(struct devlink *devlink, u32 id,
+					 union devlink_param_value *val,
+					 struct netlink_ext_ack *extack)
+{
+	if (val->vu32 > U16_MAX) {
+		NL_SET_ERR_MSG_FMT_MOD(extack,
+				       "Max SFs allowed value is %u", U16_MAX);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int mlx5_devlink_max_sfs_set(struct devlink *devlink, u32 id,
+				    struct devlink_param_gset_ctx *ctx,
+				    struct netlink_ext_ack *extack)
+{
+	struct mlx5_core_dev *dev = devlink_priv(devlink);
+	u32 mnvda[MLX5_ST_SZ_DW(mnvda_reg)] = {};
+	void *data;
+	int err;
+
+	/* we don't explicitly disable per_pf_num_sf when max_sfs is 0 because
+	 * another PF may be using SFs
+	 */
+	if (ctx->val.vu32) {
+		err = mlx5_nv_param_read_global_pci_conf(dev, mnvda,
+							 sizeof(mnvda));
+		if (err) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Failed to read global PCI configuration");
+			return err;
+		}
+
+		data = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
+		/* always enable per_pf_num_sf because another PF may use SFs */
+		MLX5_SET(nv_global_pci_conf, data, per_pf_num_sf, 1);
+
+		err = mlx5_nv_param_write(dev, mnvda, sizeof(mnvda));
+		if (err) {
+			NL_SET_ERR_MSG_MOD(extack,
+					   "Failed to change per_pf_num_sf global PCI configuration");
+			return err;
+		}
+		memset(mnvda, 0, sizeof(mnvda));
+	}
+
+	err = mlx5_nv_param_read_per_host_pf_conf(dev, mnvda, sizeof(mnvda));
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack, "Failed to read PF configuration");
+		return err;
+	}
+
+	data = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
+	MLX5_SET(nv_pf_pci_conf, data, log_sf_bar_size,
+		 ctx->val.vu32 ? MLX5_DEFAULT_LOG_SF_BAR_SIZE : 0);
+	MLX5_SET(nv_pf_pci_conf, data, pf_total_sf_en, !!ctx->val.vu32);
+	MLX5_SET(nv_pf_pci_conf, data, total_sf, ctx->val.vu32);
+
+	err = mlx5_nv_param_write(dev, mnvda, sizeof(mnvda));
+	if (err) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Failed to change PF PCI configuration");
+		return err;
+	}
+	NL_SET_ERR_MSG_MOD(extack,
+			   "Modifying max_sfs requires a FW reset and PCI bus rescan");
+
+	return 0;
+}
+
 static const struct devlink_param mlx5_nv_param_devlink_params[] = {
 	DEVLINK_PARAM_GENERIC(ENABLE_SRIOV, BIT(DEVLINK_PARAM_CMODE_PERMANENT),
 			      mlx5_devlink_enable_sriov_get,
@@ -763,6 +878,10 @@ static const struct devlink_param mlx5_nv_param_devlink_params[] = {
 			      mlx5_devlink_total_vfs_get,
 			      mlx5_devlink_total_vfs_set,
 			      mlx5_devlink_total_vfs_validate),
+	DEVLINK_PARAM_GENERIC(MAX_SFS, BIT(DEVLINK_PARAM_CMODE_PERMANENT),
+			      mlx5_devlink_max_sfs_get,
+			      mlx5_devlink_max_sfs_set,
+			      mlx5_devlink_max_sfs_validate),
 	DEVLINK_PARAM_DRIVER(MLX5_DEVLINK_PARAM_ID_CQE_COMPRESSION_TYPE,
 			     "cqe_compress_type", DEVLINK_PARAM_TYPE_STRING,
 			     BIT(DEVLINK_PARAM_CMODE_PERMANENT),
-- 
2.44.0

Re: [PATCH net-next V5 2/2] net/mlx5: implement max_sfs parameter

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "second one looks legit at least (about the max value)?"
---
net/mlx5: implement max_sfs parameter

This adds the generic devlink permanent parameter max_sfs to mlx5, so the
number of light-weight subfunctions a PF may create can be set through
devlink instead of vendor tooling.  The setter enables the global
per_pf_num_sf bit and then programs the per-PF SF fields in the MNVDA
non-volatile configuration.

A few questions on the new code below.

> diff --git a/Documentation/networking/devlink/mlx5.rst b/Documentation/networking/devlink/mlx5.rst
> index cf1dffa676696..9421c89951a05 100644
> --- a/Documentation/networking/devlink/mlx5.rst
> +++ b/Documentation/networking/devlink/mlx5.rst
> @@ -45,8 +45,13 @@ Parameters
>       - The range is between 1 and a device-specific max.
>       - Applies to each physical function (PF) independently, if the device
>         supports it. Otherwise, it applies symmetrically to all PFs.
> +   * - ``max_sfs``
> +     - permanent
> +     - The range is between 0 and a device-specific max.
> +     - Applies to each physical function (PF) independently.
>  
> -Note: permanent parameters such as ``enable_sriov`` and ``total_vfs`` require FW reset to take effect
> +Note: permanent parameters such as ``enable_sriov``, ``total_vfs`` and ``max_sfs``
> +      require FW reset to take effect
>  
>  .. code-block:: bash
>  
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c
> index 4a7275e8b62e3..72cc8da5f1193 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/nv_param.c
> @@ -68,7 +68,9 @@ struct mlx5_ifc_mnvda_reg_bits {
>  
>  struct mlx5_ifc_nv_global_pci_conf_bits {
>  	u8         sriov_valid[0x1];
> -	u8         reserved_at_1[0x10];
> +	u8         reserved_at_1[0xa];
> +	u8         per_pf_num_sf[0x1];
> +	u8         reserved_at_c[0x5];
>  	u8         per_pf_total_vf[0x1];
>  	u8         reserved_at_12[0xe];
>  
> @@ -93,9 +95,11 @@ struct mlx5_ifc_nv_global_pci_cap_bits {
>  };
>  
>  struct mlx5_ifc_nv_pf_pci_conf_bits {
> -	u8         reserved_at_0[0x9];
> +	u8         log_sf_bar_size[0x8];
> +	u8         pf_total_sf_en[0x1];
>  	u8         pf_total_vf_en[0x1];
> -	u8         reserved_at_a[0x16];
> +	u8         reserved_at_a[0x6];
> +	u8         total_sf[0x10];
>  
>  	u8         reserved_at_20[0x20];
>  
> @@ -158,6 +162,8 @@ struct mlx5_ifc_nv_sw_accelerate_conf_bits {
>  #define MLX5_GET_CFG_HDR_LEN(_mnvda_ptr) \
>  	MLX5_GET(mnvda_reg, _mnvda_ptr, configuration_item_header.length)
>  
> +#define MLX5_DEFAULT_LOG_SF_BAR_SIZE 12
> +
>  static int mlx5_nv_param_read(struct mlx5_core_dev *dev, void *mnvda,
>  			      size_t len)
>  {
> @@ -755,6 +761,115 @@ static int mlx5_devlink_total_vfs_validate(struct devlink *devlink, u32 id,
>  	return 0;
>  }
>  
> +static int mlx5_devlink_max_sfs_get(struct devlink *devlink, u32 id,
> +				    struct devlink_param_gset_ctx *ctx,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct mlx5_core_dev *dev = devlink_priv(devlink);
> +	u32 mnvda[MLX5_ST_SZ_DW(mnvda_reg)] = {};
> +	void *data;
> +	int err;
> +
> +	err = mlx5_nv_param_read_global_pci_conf(dev, mnvda, sizeof(mnvda));
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Failed to read global PCI configuration");
> +		return err;
> +	}

[Severity: Low]

Can this extack message show up as a warning on a perfectly successful
param dump?  mlx5_nv_param_read() maps every mlx5_core_access_reg()
failure to -EOPNOTSUPP:

	mlx5_core_warn(dev, "Failed to read mnvda reg: ...");
	return -EOPNOTSUPP;

and the dump path in net/devlink/param.c swallows that code without
clearing cb->extack:

devlink_nl_param_get_dump_one()
	if (err == -EOPNOTSUPP) {
		err = 0;
	} else if (err) {
		state->idx = param_id;
		break;
	}

netlink_dump_done() then attaches extack->_msg to the NLMSG_DONE even
when the reported error is 0, so on firmware that does not implement the
global PCI configuration item every "devlink dev param show" would print
"mlx5_core: Failed to read global PCI configuration".

The same applies to the "Failed to read PF configuration" message below.
The pre-existing mlx5_devlink_enable_sriov_get() and
mlx5_devlink_total_vfs_get() return the error without touching extack.

> +
> +	data = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
> +	if (!MLX5_GET(nv_global_pci_conf, data, per_pf_num_sf)) {
> +		ctx->val.vu32 = 0;
> +		return 0;
> +	}
> +
> +	memset(mnvda, 0, sizeof(mnvda));
> +	err = mlx5_nv_param_read_per_host_pf_conf(dev, mnvda, sizeof(mnvda));
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to read PF configuration");
> +		return err;
> +	}
> +
> +	data = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
> +	if (MLX5_GET(nv_pf_pci_conf, data, pf_total_sf_en))
> +		ctx->val.vu32 = MLX5_GET(nv_pf_pci_conf, data, total_sf);
> +	else
> +		ctx->val.vu32 = 0;
> +
> +	return 0;
> +}
> +
> +static int mlx5_devlink_max_sfs_validate(struct devlink *devlink, u32 id,
> +					 union devlink_param_value *val,
> +					 struct netlink_ext_ack *extack)
> +{
> +	if (val->vu32 > U16_MAX) {
> +		NL_SET_ERR_MSG_FMT_MOD(extack,
> +				       "Max SFs allowed value is %u", U16_MAX);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[Severity: Low]

The documentation hunk in this patch states:

     - The range is between 0 and a device-specific max.

but mlx5_devlink_max_sfs_validate() only rejects values above U16_MAX,
which is the width of nv_pf_pci_conf.total_sf[0x10] rather than any
device property, and neither the getter nor the setter reads
nv_global_pci_cap for an SF limit.  Compare
mlx5_devlink_total_vfs_validate(), which reports the real bound:

	max = MLX5_GET(nv_global_pci_cap, data, max_vfs_per_pf);
	if (val->vu16 > max) {
		NL_SET_ERR_MSG_FMT_MOD(extack,
				       "Max allowed by device is %u", max);

Relatedly, mlx5_devlink_max_sfs_set() below sets the global
per_pf_num_sf bit with no capability check, while
mlx5_devlink_total_vfs_set() and mlx5_devlink_enable_sriov_set() gate the
equivalent per-PF mode switch on
nv_global_pci_cap.per_pf_total_vf_supported and return -EOPNOTSUPP
otherwise.  No per_pf_num_sf_supported bit is added to
mlx5_ifc_nv_global_pci_cap_bits here.

Should a value beyond the device's actual SF maximum, or a device without
per-PF SF support, fail with something more specific than the generic
"Failed to change PF PCI configuration" that comes from the firmware
write?

> +
> +static int mlx5_devlink_max_sfs_set(struct devlink *devlink, u32 id,
> +				    struct devlink_param_gset_ctx *ctx,
> +				    struct netlink_ext_ack *extack)
> +{
> +	struct mlx5_core_dev *dev = devlink_priv(devlink);
> +	u32 mnvda[MLX5_ST_SZ_DW(mnvda_reg)] = {};
> +	void *data;
> +	int err;
> +
> +	/* we don't explicitly disable per_pf_num_sf when max_sfs is 0 because
> +	 * another PF may be using SFs
> +	 */
> +	if (ctx->val.vu32) {
> +		err = mlx5_nv_param_read_global_pci_conf(dev, mnvda,
> +							 sizeof(mnvda));
> +		if (err) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Failed to read global PCI configuration");
> +			return err;
> +		}

[Severity: Medium]

With value 0 this whole block is skipped, so on a device that is still in
global SF mode (per_pf_num_sf == 0) only the per-PF fields
pf_total_sf_en, total_sf and log_sf_bar_size are written.

mlx5_devlink_max_sfs_get() treats exactly those per-PF fields as
meaningless while per_pf_num_sf is clear:

	if (!MLX5_GET(nv_global_pci_conf, data, per_pf_num_sf)) {
		ctx->val.vu32 = 0;
		return 0;
	}

If the per-PF fields are inert in that mode, can

	devlink dev param set pci/<bdf> name max_sfs value 0 cmode permanent

actually stop SF creation, given that the SF budget keeps coming from the
global configuration?

The call still returns 0 with the "requires a FW reset" extack, and a
later param show reports 0 through the same early return, so user space
sees the disable as successful.  The commit message says:

    A value of 0 will effectively disable creation of new subfunction
    devices.

For comparison, mlx5_devlink_total_vfs_get() reports the global
total_vfs value in the equivalent situation instead of 0.

> +
> +		data = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
> +		/* always enable per_pf_num_sf because another PF may use SFs */
> +		MLX5_SET(nv_global_pci_conf, data, per_pf_num_sf, 1);

[Severity: Medium]

per_pf_num_sf lives in the device-global item, not in the per-host-PF
one:

mlx5_nv_param_read_global_pci_conf()
	MLX5_SET_CFG_ITEM_TYPE(global, mnvda, type_class, 0);
	MLX5_SET_CFG_ITEM_TYPE(global, mnvda, parameter_index,
			       MLX5_CLASS_0_CTRL_ID_NV_GLOBAL_PCI_CONF);

so flipping it selects, for the whole device, whether the SF quota comes
from the global configuration or from each PF's own total_sf, while only
this PF's pf_total_sf_en/total_sf are programmed below.

What happens on a sibling PF whose per-PF total_sf was never programmed -
after the FW reset, does its SF capacity get derived from an unprogrammed
(zero) field and SF creation start failing there?

The comment above acknowledges the cross-PF coupling for the disable
direction; the extack at the end only warns about the FW reset and PCI
rescan, not about SF provisioning changing for other functions.

> +
> +		err = mlx5_nv_param_write(dev, mnvda, sizeof(mnvda));
> +		if (err) {
> +			NL_SET_ERR_MSG_MOD(extack,
> +					   "Failed to change per_pf_num_sf global PCI configuration");
> +			return err;
> +		}
> +		memset(mnvda, 0, sizeof(mnvda));
> +	}

[Severity: Medium]

This read-modify-write touches the device-global nv_global_pci_conf item,
but mlx5_nv_param_register_dl_params() registers these params on each
PF's own devlink instance:

	if (!mlx5_core_is_pf(devlink_priv(devlink)))
		return 0;

	return devl_params_register(devlink, mlx5_nv_param_devlink_params,
				    ARRAY_SIZE(mlx5_nv_param_devlink_params));

so the devlink instance lock is per-PF and cannot serialize accesses to a
shared item.  Can two PFs of the same device lose each other's update?

PF0: mlx5_devlink_max_sfs_set()
       mlx5_nv_param_read_global_pci_conf()   /* reads the shared item */

PF1: mlx5_devlink_total_vfs_set()
       mlx5_nv_param_read_global_pci_conf()
       MLX5_SET(nv_global_pci_conf, data, sriov_valid, 1);
       MLX5_SET(nv_global_pci_conf, data, per_pf_total_vf, 1);
       mlx5_nv_param_write()                  /* commits */

PF0:   MLX5_SET(nv_global_pci_conf, data, per_pf_num_sf, 1);
       mlx5_nv_param_write()                  /* stale copy, clears the
                                                 bits PF1 just wrote */

Both commands report success plus the FW-reset warning, so neither
administrator learns the persistent configuration is not what was asked
for.  The same pattern already exists for enable_sriov and total_vfs, but
this adds a new writer of the shared item.

> +
> +	err = mlx5_nv_param_read_per_host_pf_conf(dev, mnvda, sizeof(mnvda));
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack, "Failed to read PF configuration");
> +		return err;
> +	}

[Severity: Medium]

The two non-volatile writes are not atomic and the first one is never
rolled back.  If this read fails (mlx5_nv_param_read() returns
-EOPNOTSUPP for any mlx5_core_access_reg() failure), or the
mlx5_nv_param_write() below fails on a busy or restricted firmware, the
function returns an error to user space while per_pf_num_sf stays
committed in the NV configuration and this PF's
pf_total_sf_en/total_sf were never programmed.

Since that state survives reboot, is the device left in per-PF SF mode
with an effective budget of 0 after the FW reset even though the devlink
operation reported failure?

Would programming the per-host-PF item first, or restoring the
per_pf_num_sf value that was just read on the failure paths, keep the two
steps balanced?

> +
> +	data = MLX5_ADDR_OF(mnvda_reg, mnvda, configuration_item_data);
> +	MLX5_SET(nv_pf_pci_conf, data, log_sf_bar_size,
> +		 ctx->val.vu32 ? MLX5_DEFAULT_LOG_SF_BAR_SIZE : 0);

[Severity: Medium]

log_sf_bar_size is a separate persistent knob - the size of each SF's
BAR/doorbell window, the same quantity the device reports as
mlx5_ifc_sf_partition_bits.log_sf_bar_size - and max_sfs does not claim
to control it.

The read above preserves whatever the device is currently programmed
with, and this overwrites it with the hardcoded
MLX5_DEFAULT_LOG_SF_BAR_SIZE (12), or with 0 on the disable path.

Should setting max_sfs also reset the SF BAR window size?  Neither the
commit message nor the documentation hunk mentions it, and 12 is not
validated against any device capability or against the BAR space implied
by total_sf, which may be up to 65535.

> +	MLX5_SET(nv_pf_pci_conf, data, pf_total_sf_en, !!ctx->val.vu32);
> +	MLX5_SET(nv_pf_pci_conf, data, total_sf, ctx->val.vu32);
> +
> +	err = mlx5_nv_param_write(dev, mnvda, sizeof(mnvda));
> +	if (err) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "Failed to change PF PCI configuration");
> +		return err;
> +	}
> +	NL_SET_ERR_MSG_MOD(extack,
> +			   "Modifying max_sfs requires a FW reset and PCI bus rescan");
> +
> +	return 0;
> +}
> +

[ ... ]