[PATCH net-next] net/mlx5: remove the recent devlink params

[PATCH net-next] net/mlx5: remove the recent devlink params

[Jakub Kicinski]

revert commit 46ae40b94d88 ("net/mlx5: Let user configure io_eq_size param")
revert commit a6cb08daa3b4 ("net/mlx5: Let user configure event_eq_size param")
revert commit 554604061979 ("net/mlx5: Let user configure max_macs param")

The EQE parameters are applicable to more drivers, they should
be configured via standard API, probably ethtool. Example of
another driver needing something similar:

https://lore.kernel.org/all/1633454136-14679-3-git-send-email-sbhatta@marvell.com/

The last param for "max_macs" is probably fine but the documentation
is severely lacking. The meaning and implications for changing the
param need to be stated.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>

LocalWords:  EQE param
---
 Documentation/networking/devlink/mlx5.rst     | 20 -----
 .../net/ethernet/mellanox/mlx5/core/Makefile  |  2 +-
 .../net/ethernet/mellanox/mlx5/core/devlink.c | 69 ----------------
 .../net/ethernet/mellanox/mlx5/core/devlink.h | 12 ---
 .../ethernet/mellanox/mlx5/core/devlink_res.c | 80 -------------------
 drivers/net/ethernet/mellanox/mlx5/core/eq.c  |  5 +-
 .../net/ethernet/mellanox/mlx5/core/main.c    | 21 -----
 include/linux/mlx5/driver.h                   |  4 +
 include/linux/mlx5/eq.h                       |  1 +
 include/linux/mlx5/mlx5_ifc.h                 |  2 +-
 10 files changed, 9 insertions(+), 207 deletions(-)
 delete mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink_res.c

diff --git a/Documentation/networking/devlink/mlx5.rst b/Documentation/networking/devlink/mlx5.rst
index d467e770906e..4e4b97f7971a 100644
--- a/Documentation/networking/devlink/mlx5.rst
+++ b/Documentation/networking/devlink/mlx5.rst
@@ -14,12 +14,8 @@ Parameters
 
    * - Name
      - Mode
-     - Validation
    * - ``enable_roce``
      - driverinit
-   * - ``max_macs``
-     - driverinit
-     - The range is between 1 and 2^31. Only power of 2 values are supported.
 
 The ``mlx5`` driver also implements the following driver-specific
 parameters.
@@ -50,22 +46,6 @@ parameters.
 
 The ``mlx5`` driver supports reloading via ``DEVLINK_CMD_RELOAD``
 
-Resources
-=========
-
-.. list-table:: Driver-specific resources implemented
-   :widths: 5 5 5 85
-
-   * - Name
-     - Description
-   * - ``comp_eq_size``
-     - Control the size of I/O completion EQs.
-       * The default value is 1024, and the range is between 64 and 4096.
-   * - ``event_eq_size``
-     - Control the size of the asynchronous control events EQ.
-       * The default value is 4096, and the range is between 64 and 4096.
-
-
 Info versions
 =============
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index 79c15ee62cde..bdb271b604d9 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -16,7 +16,7 @@ mlx5_core-y :=	main.o cmd.o debugfs.o fw.o eq.o uar.o pagealloc.o \
 		transobj.o vport.o sriov.o fs_cmd.o fs_core.o pci_irq.o \
 		fs_counters.o fs_ft_pool.o rl.o lag/lag.o dev.o events.o wq.o lib/gid.o \
 		lib/devcom.o lib/pci_vsc.o lib/dm.o lib/fs_ttc.o diag/fs_tracepoint.o \
-		diag/fw_tracer.o diag/crdump.o devlink.o devlink_res.o diag/rsc_dump.o \
+		diag/fw_tracer.o diag/crdump.o devlink.o diag/rsc_dump.o \
 		fw_reset.o qos.o lib/tout.o
 
 #
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
index fc78c745ead1..1c98652b244a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
@@ -752,68 +752,6 @@ static void mlx5_devlink_auxdev_params_unregister(struct devlink *devlink)
 	mlx5_devlink_eth_param_unregister(devlink);
 }
 
-static int mlx5_devlink_max_uc_list_validate(struct devlink *devlink, u32 id,
-					     union devlink_param_value val,
-					     struct netlink_ext_ack *extack)
-{
-	struct mlx5_core_dev *dev = devlink_priv(devlink);
-
-	/* At least one unicast mac is needed */
-	if (val.vu32 == 0) {
-		NL_SET_ERR_MSG_MOD(extack, "max_macs value must be greater than 0");
-		return -EINVAL;
-	}
-	/* Check if its power of 2 or not */
-	if (!is_power_of_2(val.vu32)) {
-		NL_SET_ERR_MSG_MOD(extack,
-				   "Only power of 2 values are supported for max_macs");
-		return -EOPNOTSUPP;
-	}
-
-	if (ilog2(val.vu32) >
-	    MLX5_CAP_GEN_MAX(dev, log_max_current_uc_list)) {
-		NL_SET_ERR_MSG_MOD(extack, "max_macs value is out of the supported range");
-		return -EOPNOTSUPP;
-	}
-
-	return 0;
-}
-
-static const struct devlink_param max_uc_list_param =
-	DEVLINK_PARAM_GENERIC(MAX_MACS, BIT(DEVLINK_PARAM_CMODE_DRIVERINIT),
-			      NULL, NULL, mlx5_devlink_max_uc_list_validate);
-
-static int mlx5_devlink_max_uc_list_param_register(struct devlink *devlink)
-{
-	struct mlx5_core_dev *dev = devlink_priv(devlink);
-	union devlink_param_value value;
-	int err;
-
-	if (!MLX5_CAP_GEN(dev, log_max_current_uc_list_wr_supported))
-		return 0;
-
-	err = devlink_param_register(devlink, &max_uc_list_param);
-	if (err)
-		return err;
-
-	value.vu32 = 1 << MLX5_CAP_GEN(dev, log_max_current_uc_list);
-	devlink_param_driverinit_value_set(devlink,
-					   DEVLINK_PARAM_GENERIC_ID_MAX_MACS,
-					   value);
-	return 0;
-}
-
-static void
-mlx5_devlink_max_uc_list_param_unregister(struct devlink *devlink)
-{
-	struct mlx5_core_dev *dev = devlink_priv(devlink);
-
-	if (!MLX5_CAP_GEN(dev, log_max_current_uc_list_wr_supported))
-		return;
-
-	devlink_param_unregister(devlink, &max_uc_list_param);
-}
-
 #define MLX5_TRAP_DROP(_id, _group_id)					\
 	DEVLINK_TRAP_GENERIC(DROP, DROP, _id,				\
 			     DEVLINK_TRAP_GROUP_GENERIC_ID_##_group_id, \
@@ -877,17 +815,11 @@ int mlx5_devlink_register(struct devlink *devlink)
 	if (err)
 		goto traps_reg_err;
 
-	err = mlx5_devlink_max_uc_list_param_register(devlink);
-	if (err)
-		goto uc_list_reg_err;
-
 	if (!mlx5_core_is_mp_slave(dev))
 		devlink_set_features(devlink, DEVLINK_F_RELOAD);
 
 	return 0;
 
-uc_list_reg_err:
-	mlx5_devlink_traps_unregister(devlink);
 traps_reg_err:
 	mlx5_devlink_auxdev_params_unregister(devlink);
 auxdev_reg_err:
@@ -898,7 +830,6 @@ int mlx5_devlink_register(struct devlink *devlink)
 
 void mlx5_devlink_unregister(struct devlink *devlink)
 {
-	mlx5_devlink_max_uc_list_param_unregister(devlink);
 	mlx5_devlink_traps_unregister(devlink);
 	mlx5_devlink_auxdev_params_unregister(devlink);
 	devlink_params_unregister(devlink, mlx5_devlink_params,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.h b/drivers/net/ethernet/mellanox/mlx5/core/devlink.h
index 674415fd0b3a..30bf4882779b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.h
@@ -6,14 +6,6 @@
 
 #include <net/devlink.h>
 
-enum mlx5_devlink_resource_id {
-	MLX5_DL_RES_COMP_EQ = 1,
-	MLX5_DL_RES_ASYNC_EQ,
-
-	__MLX5_ID_RES_MAX,
-	MLX5_ID_RES_MAX = __MLX5_ID_RES_MAX - 1,
-};
-
 enum mlx5_devlink_param_id {
 	MLX5_DEVLINK_PARAM_ID_BASE = DEVLINK_PARAM_GENERIC_ID_MAX,
 	MLX5_DEVLINK_PARAM_ID_FLOW_STEERING_MODE,
@@ -39,10 +31,6 @@ int mlx5_devlink_trap_get_num_active(struct mlx5_core_dev *dev);
 int mlx5_devlink_traps_get_action(struct mlx5_core_dev *dev, int trap_id,
 				  enum devlink_trap_action *action);
 
-void mlx5_devlink_res_register(struct mlx5_core_dev *dev);
-void mlx5_devlink_res_unregister(struct mlx5_core_dev *dev);
-size_t mlx5_devlink_res_size(struct mlx5_core_dev *dev, enum mlx5_devlink_resource_id id);
-
 struct devlink *mlx5_devlink_alloc(struct device *dev);
 void mlx5_devlink_free(struct devlink *devlink);
 int mlx5_devlink_register(struct devlink *devlink);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink_res.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink_res.c
deleted file mode 100644
index 549d23745942..000000000000
--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink_res.c
+++ /dev/null
@@ -1,80 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
-/* Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. */
-
-#include "devlink.h"
-#include "mlx5_core.h"
-
-enum {
-	MLX5_EQ_MIN_SIZE = 64,
-	MLX5_EQ_MAX_SIZE = 4096,
-	MLX5_NUM_ASYNC_EQE = 4096,
-	MLX5_COMP_EQ_SIZE = 1024,
-};
-
-static int comp_eq_res_register(struct mlx5_core_dev *dev)
-{
-	struct devlink_resource_size_params comp_eq_size;
-	struct devlink *devlink = priv_to_devlink(dev);
-
-	devlink_resource_size_params_init(&comp_eq_size, MLX5_EQ_MIN_SIZE,
-					  MLX5_EQ_MAX_SIZE, 1, DEVLINK_RESOURCE_UNIT_ENTRY);
-	return devlink_resource_register(devlink, "io_eq_size", MLX5_COMP_EQ_SIZE,
-					 MLX5_DL_RES_COMP_EQ,
-					 DEVLINK_RESOURCE_ID_PARENT_TOP,
-					 &comp_eq_size);
-}
-
-static int async_eq_resource_register(struct mlx5_core_dev *dev)
-{
-	struct devlink_resource_size_params async_eq_size;
-	struct devlink *devlink = priv_to_devlink(dev);
-
-	devlink_resource_size_params_init(&async_eq_size, MLX5_EQ_MIN_SIZE,
-					  MLX5_EQ_MAX_SIZE, 1, DEVLINK_RESOURCE_UNIT_ENTRY);
-	return devlink_resource_register(devlink, "event_eq_size",
-					 MLX5_NUM_ASYNC_EQE, MLX5_DL_RES_ASYNC_EQ,
-					 DEVLINK_RESOURCE_ID_PARENT_TOP,
-					 &async_eq_size);
-}
-
-void mlx5_devlink_res_register(struct mlx5_core_dev *dev)
-{
-	int err;
-
-	err = comp_eq_res_register(dev);
-	if (err)
-		goto err_msg;
-
-	err = async_eq_resource_register(dev);
-	if (err)
-		goto err;
-	return;
-err:
-	devlink_resources_unregister(priv_to_devlink(dev), NULL);
-err_msg:
-	mlx5_core_err(dev, "Failed to register resources, err = %d\n", err);
-}
-
-void mlx5_devlink_res_unregister(struct mlx5_core_dev *dev)
-{
-	devlink_resources_unregister(priv_to_devlink(dev), NULL);
-}
-
-static const size_t default_vals[MLX5_ID_RES_MAX + 1] = {
-	[MLX5_DL_RES_COMP_EQ] = MLX5_COMP_EQ_SIZE,
-	[MLX5_DL_RES_ASYNC_EQ] = MLX5_NUM_ASYNC_EQE,
-};
-
-size_t mlx5_devlink_res_size(struct mlx5_core_dev *dev, enum mlx5_devlink_resource_id id)
-{
-	struct devlink *devlink = priv_to_devlink(dev);
-	u64 size;
-	int err;
-
-	err = devlink_resource_size_get(devlink, id, &size);
-	if (!err)
-		return size;
-	mlx5_core_err(dev, "Failed to get param. using default. err = %d, id = %u\n",
-		      err, id);
-	return default_vals[id];
-}
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eq.c b/drivers/net/ethernet/mellanox/mlx5/core/eq.c
index 31e69067284b..792e0d6aa861 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eq.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eq.c
@@ -19,7 +19,6 @@
 #include "lib/clock.h"
 #include "diag/fw_tracer.h"
 #include "mlx5_irq.h"
-#include "devlink.h"
 
 enum {
 	MLX5_EQE_OWNER_INIT_VAL	= 0x1,
@@ -647,7 +646,7 @@ static int create_async_eqs(struct mlx5_core_dev *dev)
 
 	param = (struct mlx5_eq_param) {
 		.irq_index = MLX5_IRQ_EQ_CTRL,
-		.nent = mlx5_devlink_res_size(dev, MLX5_DL_RES_ASYNC_EQ),
+		.nent = MLX5_NUM_ASYNC_EQE,
 	};
 
 	gather_async_events_mask(dev, param.mask);
@@ -808,7 +807,7 @@ static int create_comp_eqs(struct mlx5_core_dev *dev)
 
 	INIT_LIST_HEAD(&table->comp_eqs_list);
 	ncomp_eqs = table->num_comp_eqs;
-	nent = mlx5_devlink_res_size(dev, MLX5_DL_RES_COMP_EQ);
+	nent = MLX5_COMP_EQ_SIZE;
 	for (i = 0; i < ncomp_eqs; i++) {
 		struct mlx5_eq_param param = {};
 		int vecidx = i;
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 079ee9e8da10..f8446395163a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -484,23 +484,10 @@ static int handle_hca_cap_odp(struct mlx5_core_dev *dev, void *set_ctx)
 	return set_caps(dev, set_ctx, MLX5_SET_HCA_CAP_OP_MOD_ODP);
 }
 
-static int max_uc_list_get_devlink_param(struct mlx5_core_dev *dev)
-{
-	struct devlink *devlink = priv_to_devlink(dev);
-	union devlink_param_value val;
-	int err;
-
-	err = devlink_param_driverinit_value_get(devlink,
-						 DEVLINK_PARAM_GENERIC_ID_MAX_MACS,
-						 &val);
-	return err ? 0 : val.vu32;
-}
-
 static int handle_hca_cap(struct mlx5_core_dev *dev, void *set_ctx)
 {
 	struct mlx5_profile *prof = &dev->profile;
 	void *set_hca_cap;
-	u32 max_uc_list;
 	int err;
 
 	err = mlx5_core_get_caps(dev, MLX5_CAP_GENERAL);
@@ -574,11 +561,6 @@ static int handle_hca_cap(struct mlx5_core_dev *dev, void *set_ctx)
 	if (MLX5_CAP_GEN(dev, roce_rw_supported))
 		MLX5_SET(cmd_hca_cap, set_hca_cap, roce, mlx5_is_roce_init_enabled(dev));
 
-	max_uc_list = max_uc_list_get_devlink_param(dev);
-	if (max_uc_list)
-		MLX5_SET(cmd_hca_cap, set_hca_cap, log_max_current_uc_list,
-			 ilog2(max_uc_list));
-
 	return set_caps(dev, set_ctx, MLX5_SET_HCA_CAP_OP_MOD_GENERAL_DEVICE);
 }
 
@@ -940,8 +922,6 @@ static int mlx5_init_once(struct mlx5_core_dev *dev)
 	dev->hv_vhca = mlx5_hv_vhca_create(dev);
 	dev->rsc_dump = mlx5_rsc_dump_create(dev);
 
-	mlx5_devlink_res_register(dev);
-
 	return 0;
 
 err_sf_table_cleanup:
@@ -977,7 +957,6 @@ static int mlx5_init_once(struct mlx5_core_dev *dev)
 
 static void mlx5_cleanup_once(struct mlx5_core_dev *dev)
 {
-	mlx5_devlink_res_unregister(dev);
 	mlx5_rsc_dump_destroy(dev);
 	mlx5_hv_vhca_destroy(dev->hv_vhca);
 	mlx5_fw_tracer_destroy(dev->tracer);
diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index 47c07f95bbe1..f617dfbcd9fd 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h
@@ -797,6 +797,10 @@ struct mlx5_db {
 	int			index;
 };
 
+enum {
+	MLX5_COMP_EQ_SIZE = 1024,
+};
+
 enum {
 	MLX5_PTYS_IB = 1 << 0,
 	MLX5_PTYS_EN = 1 << 2,
diff --git a/include/linux/mlx5/eq.h b/include/linux/mlx5/eq.h
index 11161e427608..ea3ff5a8ced3 100644
--- a/include/linux/mlx5/eq.h
+++ b/include/linux/mlx5/eq.h
@@ -5,6 +5,7 @@
 #define MLX5_CORE_EQ_H
 
 #define MLX5_NUM_CMD_EQE   (32)
+#define MLX5_NUM_ASYNC_EQE (0x1000)
 #define MLX5_NUM_SPARE_EQE (0x80)
 
 struct mlx5_eq;
diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 97465d00de9d..746381eccccf 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -1603,7 +1603,7 @@ struct mlx5_ifc_cmd_hca_cap_bits {
 
 	u8         ext_stride_num_range[0x1];
 	u8         roce_rw_supported[0x1];
-	u8         log_max_current_uc_list_wr_supported[0x1];
+	u8         reserved_at_3a2[0x1];
 	u8         log_max_stride_sz_rq[0x5];
 	u8         reserved_at_3a8[0x3];
 	u8         log_min_stride_sz_rq[0x5];
-- 
2.31.1

RE: [PATCH net-next] net/mlx5: remove the recent devlink params

[Parav Pandit]

Hi Jakub,

> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Tuesday, October 26, 2021 9:00 PM
> To: davem@davemloft.net
> Cc: Saeed Mahameed <saeedm@nvidia.com>; netdev@vger.kernel.org; Leon
> Romanovsky <leonro@nvidia.com>; Jakub Kicinski <kuba@kernel.org>
> Subject: [PATCH net-next] net/mlx5: remove the recent devlink params
> 
> revert commit 46ae40b94d88 ("net/mlx5: Let user configure io_eq_size
> param") revert commit a6cb08daa3b4 ("net/mlx5: Let user configure
> event_eq_size param") revert commit 554604061979 ("net/mlx5: Let user
> configure max_macs param")
> 
> The EQE parameters are applicable to more drivers, they should be configured
> via standard API, probably ethtool. Example of another driver needing
> something similar:

ethool is not a good choice for following reasons.

1. EQs of the mlx5 core devlink instance is used by multiple auxiliary devices, namely net, rdma, vdpa.
2. And sometime netdevice doesn't even exists to operate via ethtool (but devlink instance exist to serve other devices).
3. ethtool doesn't have notion set the config and apply (like devlink reload)
Such reload operation is useful when user wants to configure more than one parameter and initialize the device only once.
Otherwise dynamically changing parameter results in multiple device re-init sequence that increases device setup time.

Should we define the devlink resources in the devlink layer, instead of driver?
So that multiple drivers can make use of them without redefinition?

Re: [PATCH net-next] net/mlx5: remove the recent devlink params

[Leon Romanovsky]

On Tue, Oct 26, 2021 at 08:29:39AM -0700, Jakub Kicinski wrote:
> revert commit 46ae40b94d88 ("net/mlx5: Let user configure io_eq_size param")
> revert commit a6cb08daa3b4 ("net/mlx5: Let user configure event_eq_size param")
> revert commit 554604061979 ("net/mlx5: Let user configure max_macs param")
> 
> The EQE parameters are applicable to more drivers, they should
> be configured via standard API, probably ethtool. Example of
> another driver needing something similar:
> 
> https://lore.kernel.org/all/1633454136-14679-3-git-send-email-sbhatta@marvell.com/
> 
> The last param for "max_macs" is probably fine but the documentation
> is severely lacking. The meaning and implications for changing the
> param need to be stated.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> 
> LocalWords:  EQE param

Your emacs config sneaked out.

Thanks

Re: [PATCH net-next] net/mlx5: remove the recent devlink params

[Jakub Kicinski]

On Tue, 26 Oct 2021 15:53:17 +0000 Parav Pandit wrote:
> Hi Jakub,
> 
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Tuesday, October 26, 2021 9:00 PM
> > To: davem@davemloft.net
> > Cc: Saeed Mahameed <saeedm@nvidia.com>; netdev@vger.kernel.org; Leon
> > Romanovsky <leonro@nvidia.com>; Jakub Kicinski <kuba@kernel.org>
> > Subject: [PATCH net-next] net/mlx5: remove the recent devlink params
> > 
> > revert commit 46ae40b94d88 ("net/mlx5: Let user configure io_eq_size
> > param") revert commit a6cb08daa3b4 ("net/mlx5: Let user configure
> > event_eq_size param") revert commit 554604061979 ("net/mlx5: Let user
> > configure max_macs param")
> > 
> > The EQE parameters are applicable to more drivers, they should be configured
> > via standard API, probably ethtool. Example of another driver needing
> > something similar:  
> 
> ethool is not a good choice for following reasons.
> 
> 1. EQs of the mlx5 core devlink instance is used by multiple auxiliary devices, namely net, rdma, vdpa.
> 2. And sometime netdevice doesn't even exists to operate via ethtool (but devlink instance exist to serve other devices).
> 3. ethtool doesn't have notion set the config and apply (like devlink reload)
> Such reload operation is useful when user wants to configure more than one parameter and initialize the device only once.
> Otherwise dynamically changing parameter results in multiple device re-init sequence that increases device setup time.

Sure these are good points. OTOH devlink doesn't have any notion of
queues, IRQ vectors etc today so it doesn't really fit there, either.

> Should we define the devlink resources in the devlink layer, instead of driver?

I'm not sure how the EQE fits into the existing devlink objects.
params are not much of an API, it's a garbage bag.

> So that multiple drivers can make use of them without redefinition?

Yes, please.

Re: [PATCH net-next] net/mlx5: remove the recent devlink params

[Jakub Kicinski]

On Tue, 26 Oct 2021 19:45:10 +0300 Leon Romanovsky wrote:
> On Tue, Oct 26, 2021 at 08:29:39AM -0700, Jakub Kicinski wrote:
> > revert commit 46ae40b94d88 ("net/mlx5: Let user configure io_eq_size param")
> > revert commit a6cb08daa3b4 ("net/mlx5: Let user configure event_eq_size param")
> > revert commit 554604061979 ("net/mlx5: Let user configure max_macs param")
> > 
> > The EQE parameters are applicable to more drivers, they should
> > be configured via standard API, probably ethtool. Example of
> > another driver needing something similar:
> > 
> > https://lore.kernel.org/all/1633454136-14679-3-git-send-email-sbhatta@marvell.com/
> > 
> > The last param for "max_macs" is probably fine but the documentation
> > is severely lacking. The meaning and implications for changing the
> > param need to be stated.
> > 
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > 
> > LocalWords:  EQE param  
> 
> Your emacs config sneaked out.

I know, sorry, removed when applying.

Re: [PATCH net-next] net/mlx5: remove the recent devlink params

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 26 Oct 2021 08:29:39 -0700 you wrote:
> revert commit 46ae40b94d88 ("net/mlx5: Let user configure io_eq_size param")
> revert commit a6cb08daa3b4 ("net/mlx5: Let user configure event_eq_size param")
> revert commit 554604061979 ("net/mlx5: Let user configure max_macs param")
> 
> The EQE parameters are applicable to more drivers, they should
> be configured via standard API, probably ethtool. Example of
> another driver needing something similar:
> 
> [...]

Here is the summary with links:
  - [net-next] net/mlx5: remove the recent devlink params
    https://git.kernel.org/netdev/net-next/c/6b3671746a8a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next] net/mlx5: remove the recent devlink params

[Saeed Mahameed]

On Tue, 2021-10-26 at 09:47 -0700, Jakub Kicinski wrote:
> On Tue, 26 Oct 2021 15:53:17 +0000 Parav Pandit wrote:
> > Hi Jakub,
> > 
> > > From: Jakub Kicinski <kuba@kernel.org>
> > > Sent: Tuesday, October 26, 2021 9:00 PM
> > > To: davem@davemloft.net
> > > Cc: Saeed Mahameed <saeedm@nvidia.com>; netdev@vger.kernel.org;
> > > Leon
> > > Romanovsky <leonro@nvidia.com>; Jakub Kicinski <kuba@kernel.org>
> > > Subject: [PATCH net-next] net/mlx5: remove the recent devlink
> > > params
> > > 
> > > revert commit 46ae40b94d88 ("net/mlx5: Let user configure
> > > io_eq_size
> > > param") revert commit a6cb08daa3b4 ("net/mlx5: Let user configure
> > > event_eq_size param") revert commit 554604061979 ("net/mlx5: Let
> > > user
> > > configure max_macs param")
> > > 
> > > The EQE parameters are applicable to more drivers, they should be
> > > configured
> > > via standard API, probably ethtool. Example of another driver
> > > needing
> > > something similar:  
> > 
> > ethool is not a good choice for following reasons.
> > 
> > 1. EQs of the mlx5 core devlink instance is used by multiple
> > auxiliary devices, namely net, rdma, vdpa.
> > 2. And sometime netdevice doesn't even exists to operate via
> > ethtool (but devlink instance exist to serve other devices).
> > 3. ethtool doesn't have notion set the config and apply (like
> > devlink reload)
> > Such reload operation is useful when user wants to configure more
> > than one parameter and initialize the device only once.
> > Otherwise dynamically changing parameter results in multiple device
> > re-init sequence that increases device setup time.
> 
> Sure these are good points. OTOH devlink doesn't have any notion of
> queues, IRQ vectors etc today so it doesn't really fit there, either.
> 

The point is, mlx5 has different architecture compared to other
vendors, EQs, IRQs are considered shared resources of upper layer
protocols/interfaces.

ethtool is netdev specific. 
EQ concept might not apply to all vendors even with last gen HW.

netdev/ethtool shouldn't be controlling concrete/arch specific
resources. better if we kept ethtool abstract (queues, rings, and
ethernet specific ..)


> > Should we define the devlink resources in the devlink layer,
> > instead of driver?
> 
> I'm not sure how the EQE fits into the existing devlink objects.
> params are not much of an API, it's a garbage bag.
> 

devlink is good place, it is what we decide it should be, a garbage bag
or a standard place for all generic networking device related knobs.

> > So that multiple drivers can make use of them without redefinition?
> 
> Yes, please.

let's send a RFC, CC Jiri of course and other vendors who might be
interested according to Jakub's grep (huawei/hinic, ibm/ehea,
microsoft/mana, qlogic/qed) ..