[PATCH net-next V2 0/2] devlink: add generic max_sfs parameter and mlx5 support

[PATCH net-next V2 0/2] devlink: add generic max_sfs parameter and mlx5 support

[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

V2:
- 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         | 83 ++++++++++++++++++-
 include/net/devlink.h                         |  4 +
 net/devlink/param.c                           |  5 ++
 5 files changed, 101 insertions(+), 4 deletions(-)


base-commit: 9bf93cb2e180a58d5984ba13daee95903ff4fc14
-- 
2.44.0

[PATCH net-next V2 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>
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 ea17756dcda6..29b8a9246fb6 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 bcd31de1f890..4ec455cfe7a4 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -546,6 +546,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,
@@ -619,6 +620,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 cf95268da5b0..523243e49d88 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 V2 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).

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

diff --git a/Documentation/networking/devlink/mlx5.rst b/Documentation/networking/devlink/mlx5.rst
index 4bba4d780a4a..f5e2dccafa5a 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 19bb620b7436..eff3a67e4ca0 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];
 
@@ -755,6 +759,76 @@ 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_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);
+	ctx->val.vu32 = MLX5_GET(nv_pf_pci_conf, data, total_sf);
+
+	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;
+
+	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);
+	MLX5_SET(nv_global_pci_conf, data, per_pf_num_sf, !!ctx->val.vu32);
+
+	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 ? 12 : 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(extack, "Modifying max_sfs requires a reboot");
+
+	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 +837,9 @@ 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, NULL),
 	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 V2 2/2] net/mlx5: implement max_sfs parameter

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/mlx5: implement max_sfs parameter

This patch adds a generic devlink permanent parameter named max_sfs to
the mlx5 driver so users can cap the number of light-weight NIC
subfunctions via devlink instead of vendor tooling. The setter rewrites
non-volatile firmware fields in nv_global_pci_conf and nv_pf_pci_conf,
and a reboot is required for the change to take effect.

A few questions on the implementation below.

> diff --git a/Documentation/networking/devlink/mlx5.rst b/Documentation/networking/devlink/mlx5.rst
> index 4bba4d780a4a..f5e2dccafa5a 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.

[Medium, Medium]
The documentation states the range is "between 0 and a device-specific
max", and the entry says max_sfs "Applies to each physical function (PF)
independently" with no "if the device supports it" qualifier like
total_vfs has.

Looking at the implementation in mlx5_devlink_max_sfs_set(), there is no
read of any nv_global_pci_cap field to obtain a device-reported maximum,
and the registration uses NULL for the validate callback:

  DEVLINK_PARAM_GENERIC(MAX_SFS, BIT(DEVLINK_PARAM_CMODE_PERMANENT),
                        mlx5_devlink_max_sfs_get,
                        mlx5_devlink_max_sfs_set, NULL),

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?

Should the doc also include the "if the device supports it" qualifier
to match how the setter behaves on devices without the relevant
capability?

> 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 19bb620b7436..eff3a67e4ca0 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];

[Medium]
The on-wire field total_sf is 16 bits wide, but the devlink parameter
is registered as DEVLINK_PARAM_TYPE_U32 (max_sfs uses
DEVLINK_PARAM_GENERIC_MAX_SFS_TYPE, which is u32) and the setter writes
ctx->val.vu32 directly via MLX5_SET(nv_pf_pci_conf, data, total_sf,
ctx->val.vu32).

MLX5_SET masks the value with __mlx5_mask(typ, fld) before storing, so
any user-supplied value above 0xffff is silently truncated.  For
example, "devlink dev param set ... name max_sfs value 70000" would
persistently store 4464.

Without a validate callback, is there anything that prevents this
silent truncation, and should the setter or a validate hook reject
values that do not fit in 16 bits?

> @@ -755,6 +759,76 @@ 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_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);
> +	ctx->val.vu32 = MLX5_GET(nv_pf_pci_conf, data, total_sf);
> +
> +	return 0;
> +}

[Medium]
Should max_sfs_get() mirror mlx5_devlink_total_vfs_get() and consult
nv_global_pci_conf.per_pf_num_sf before reporting nv_pf_pci_conf.total_sf?

The setter explicitly conditions the meaning of per-PF total_sf on
per_pf_num_sf:

  MLX5_SET(nv_global_pci_conf, data, per_pf_num_sf, !!ctx->val.vu32);
  ...
  MLX5_SET(nv_pf_pci_conf, data, total_sf, ctx->val.vu32);

But the getter reads total_sf unconditionally, so on a device whose
firmware was previously configured by another tool with per_pf_num_sf=0
and a stale per-host total_sf, "devlink dev param show" would return a
non-zero value that does not represent the active SF policy.  Is that
intentional?

> +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;
> +
> +	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);
> +	MLX5_SET(nv_global_pci_conf, data, per_pf_num_sf, !!ctx->val.vu32);
> +
> +	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;
> +	}

[Medium]
Should this path first read nv_global_pci_cap and reject the operation
when the device does not advertise SF NV configuration, the way
mlx5_devlink_total_vfs_set() does for SR-IOV?

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".

[Medium]
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.

> +
> +	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 ? 12 : 0);

[Low]
What is the meaning of the literal 12 here, and why is it correct on
all supported devices?

Elsewhere in the driver, the SF BAR length is computed from a
hardware-reported capability rather than a fixed value:

  drivers/net/ethernet/mellanox/mlx5/core/sf/dev/dev.c:
      sf_bar_length = 1 << (MLX5_CAP_GEN(dev, log_min_sf_size) + 12);

Should log_sf_bar_size be derived from MLX5_CAP_GEN(dev,
log_min_sf_size) (or another cap) rather than hardcoded?

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?

> +	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(extack, "Modifying max_sfs requires a reboot");
> +
> +	return 0;
> +}
-- 
pw-bot: cr