[PATCH net-next 0/5] devlink: embed driver's priv data callback param into devlink_resource

[PATCH net-next 0/5] devlink: embed driver's priv data callback param into devlink_resource

[Przemek Kitszel]

(Patch 1)
Convert dsa to use devl_* variants of devlink resource related
calls, so we could remove devlink_* variants in next 2 patches.

(Patches 2,3)
Remove some unused functions that would otherwise need an update.

(Patch 4, the main one)
Then extend devlink resource to embed driver's priv data callback,
instead just storing a pointer (so drivers could put more context for
similar resource getters, to handle them via simple single function
instead of dumb duplication).

(Patch 5)
Make use of the new possibility from patch 4, I've picked the most
repetitive case.

Motivation: current API was to distracting for me to focus on adding my
new resources :)

I'm fine with it going through mlxsw or just netdev tree.

Przemek Kitszel (5):
  net: dsa: replace devlink resource registration calls by devl_
    variants
  devlink: remove unused devlink_resource_occ_get_register() and
    _unregister()
  devlink: remove unused devlink_resource_register()
  devlink: embed driver's priv data callback param into devlink_resource
  mlxsw: spectrum_kvdl: combine devlink resource occupation getters

 .../net/ethernet/mellanox/mlxsw/spectrum.h    |  5 +
 include/net/devlink.h                         | 18 +---
 .../ethernet/mellanox/mlx5/core/sf/hw_table.c |  5 +-
 drivers/net/ethernet/mellanox/mlxsw/core.c    |  5 +-
 .../net/ethernet/mellanox/mlxsw/spectrum.c    | 19 ++--
 .../ethernet/mellanox/mlxsw/spectrum1_kvdl.c  | 80 +++++++--------
 .../ethernet/mellanox/mlxsw/spectrum_cnt.c    |  9 +-
 .../mellanox/mlxsw/spectrum_policer.c         |  6 +-
 .../mellanox/mlxsw/spectrum_port_range.c      |  2 +-
 .../ethernet/mellanox/mlxsw/spectrum_router.c |  4 +-
 .../ethernet/mellanox/mlxsw/spectrum_span.c   |  3 +-
 drivers/net/netdevsim/dev.c                   | 14 +--
 drivers/net/netdevsim/fib.c                   | 10 +-
 net/devlink/resource.c                        | 97 +++----------------
 net/dsa/devlink.c                             | 23 +++--
 15 files changed, 115 insertions(+), 185 deletions(-)


base-commit: 10a6545f0bdcbb920c6a8a033fe342111d204915
-- 
2.39.3

[PATCH net-next 1/5] net: dsa: replace devlink resource registration calls by devl_ variants

[Przemek Kitszel]

Replace devlink_resource_register(), devlink_resource_occ_get_register(),
and devlink_resource_occ_get_unregister() calls by respective devl_*
variants. Mentioned functions have no direct users in any drivers, and are
going to be removed in subsequent patches.

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 net/dsa/devlink.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c
index 0aac887d0098..f41f9fc2194e 100644
--- a/net/dsa/devlink.c
+++ b/net/dsa/devlink.c
@@ -229,10 +229,15 @@ int dsa_devlink_resource_register(struct dsa_switch *ds,
 				  u64 parent_resource_id,
 				  const struct devlink_resource_size_params *size_params)
 {
-	return devlink_resource_register(ds->devlink, resource_name,
-					 resource_size, resource_id,
-					 parent_resource_id,
-					 size_params);
+	int ret;
+
+	devl_lock(ds->devlink);
+	ret = devl_resource_register(ds->devlink, resource_name, resource_size,
+				     resource_id, parent_resource_id,
+				     size_params);
+	devl_unlock(ds->devlink);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(dsa_devlink_resource_register);
 
@@ -247,15 +252,19 @@ void dsa_devlink_resource_occ_get_register(struct dsa_switch *ds,
 					   devlink_resource_occ_get_t *occ_get,
 					   void *occ_get_priv)
 {
-	return devlink_resource_occ_get_register(ds->devlink, resource_id,
-						 occ_get, occ_get_priv);
+	devl_lock(ds->devlink);
+	devl_resource_occ_get_register(ds->devlink, resource_id, occ_get,
+				       occ_get_priv);
+	devl_unlock(ds->devlink);
 }
 EXPORT_SYMBOL_GPL(dsa_devlink_resource_occ_get_register);
 
 void dsa_devlink_resource_occ_get_unregister(struct dsa_switch *ds,
 					     u64 resource_id)
 {
-	devlink_resource_occ_get_unregister(ds->devlink, resource_id);
+	devl_lock(ds->devlink);
+	devl_resource_occ_get_unregister(ds->devlink, resource_id);
+	devl_unlock(ds->devlink);
 }
 EXPORT_SYMBOL_GPL(dsa_devlink_resource_occ_get_unregister);
 
-- 
2.39.3

[PATCH net-next 2/5] devlink: remove unused devlink_resource_occ_get_register() and _unregister()

[Przemek Kitszel]

Remove not used devlink_resource_occ_get_register() and
devlink_resource_occ_get_unregister() functions; current devlink resource
users are fine with devl_ variants of the two.

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 include/net/devlink.h  |  7 -------
 net/devlink/resource.c | 39 ---------------------------------------
 2 files changed, 46 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index db5eff6cb60f..fdd6a0f9891d 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1797,15 +1797,8 @@ void devl_resource_occ_get_register(struct devlink *devlink,
 				    u64 resource_id,
 				    devlink_resource_occ_get_t *occ_get,
 				    void *occ_get_priv);
-void devlink_resource_occ_get_register(struct devlink *devlink,
-				       u64 resource_id,
-				       devlink_resource_occ_get_t *occ_get,
-				       void *occ_get_priv);
 void devl_resource_occ_get_unregister(struct devlink *devlink,
 				      u64 resource_id);
-
-void devlink_resource_occ_get_unregister(struct devlink *devlink,
-					 u64 resource_id);
 int devl_params_register(struct devlink *devlink,
 			 const struct devlink_param *params,
 			 size_t params_count);
diff --git a/net/devlink/resource.c b/net/devlink/resource.c
index 594c8aeb3bfa..6ae4b2080399 100644
--- a/net/devlink/resource.c
+++ b/net/devlink/resource.c
@@ -516,28 +516,6 @@ void devl_resource_occ_get_register(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devl_resource_occ_get_register);
 
-/**
- *	devlink_resource_occ_get_register - register occupancy getter
- *
- *	@devlink: devlink
- *	@resource_id: resource id
- *	@occ_get: occupancy getter callback
- *	@occ_get_priv: occupancy getter callback priv
- *
- *	Context: Takes and release devlink->lock <mutex>.
- */
-void devlink_resource_occ_get_register(struct devlink *devlink,
-				       u64 resource_id,
-				       devlink_resource_occ_get_t *occ_get,
-				       void *occ_get_priv)
-{
-	devl_lock(devlink);
-	devl_resource_occ_get_register(devlink, resource_id,
-				       occ_get, occ_get_priv);
-	devl_unlock(devlink);
-}
-EXPORT_SYMBOL_GPL(devlink_resource_occ_get_register);
-
 /**
  * devl_resource_occ_get_unregister - unregister occupancy getter
  *
@@ -560,20 +538,3 @@ void devl_resource_occ_get_unregister(struct devlink *devlink,
 	resource->occ_get_priv = NULL;
 }
 EXPORT_SYMBOL_GPL(devl_resource_occ_get_unregister);
-
-/**
- *	devlink_resource_occ_get_unregister - unregister occupancy getter
- *
- *	@devlink: devlink
- *	@resource_id: resource id
- *
- *	Context: Takes and release devlink->lock <mutex>.
- */
-void devlink_resource_occ_get_unregister(struct devlink *devlink,
-					 u64 resource_id)
-{
-	devl_lock(devlink);
-	devl_resource_occ_get_unregister(devlink, resource_id);
-	devl_unlock(devlink);
-}
-EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
-- 
2.39.3

[PATCH net-next 3/5] devlink: remove unused devlink_resource_register()

[Przemek Kitszel]

Remove unused devlink_resource_register(); all the drivers use
devl_resource_register() variant instead.

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 include/net/devlink.h  |  6 ------
 net/devlink/resource.c | 33 ---------------------------------
 2 files changed, 39 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index fdd6a0f9891d..fbb9a2668e24 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1779,12 +1779,6 @@ int devl_resource_register(struct devlink *devlink,
 			   u64 resource_id,
 			   u64 parent_resource_id,
 			   const struct devlink_resource_size_params *size_params);
-int devlink_resource_register(struct devlink *devlink,
-			      const char *resource_name,
-			      u64 resource_size,
-			      u64 resource_id,
-			      u64 parent_resource_id,
-			      const struct devlink_resource_size_params *size_params);
 void devl_resources_unregister(struct devlink *devlink);
 void devlink_resources_unregister(struct devlink *devlink);
 int devl_resource_size_get(struct devlink *devlink,
diff --git a/net/devlink/resource.c b/net/devlink/resource.c
index 6ae4b2080399..15efa9f49461 100644
--- a/net/devlink/resource.c
+++ b/net/devlink/resource.c
@@ -384,39 +384,6 @@ int devl_resource_register(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devl_resource_register);
 
-/**
- *	devlink_resource_register - devlink resource register
- *
- *	@devlink: devlink
- *	@resource_name: resource's name
- *	@resource_size: resource's size
- *	@resource_id: resource's id
- *	@parent_resource_id: resource's parent id
- *	@size_params: size parameters
- *
- *	Generic resources should reuse the same names across drivers.
- *	Please see the generic resources list at:
- *	Documentation/networking/devlink/devlink-resource.rst
- *
- *	Context: Takes and release devlink->lock <mutex>.
- */
-int devlink_resource_register(struct devlink *devlink,
-			      const char *resource_name,
-			      u64 resource_size,
-			      u64 resource_id,
-			      u64 parent_resource_id,
-			      const struct devlink_resource_size_params *size_params)
-{
-	int err;
-
-	devl_lock(devlink);
-	err = devl_resource_register(devlink, resource_name, resource_size,
-				     resource_id, parent_resource_id, size_params);
-	devl_unlock(devlink);
-	return err;
-}
-EXPORT_SYMBOL_GPL(devlink_resource_register);
-
 static void devlink_resource_unregister(struct devlink *devlink,
 					struct devlink_resource *resource)
 {
-- 
2.39.3

[PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource

[Przemek Kitszel]

Extend devlink resource by flex array to store drivers priv data, in
current usages it just replaces occupation getter's priv callback pointer.
Coupling lifetime of the resource and its getters metadata (don't confuse
with simple pointer to PF) is generally a good idea, and makes driver code
nicer.

Next commit will show how to makes use of it to combine related occ
getters into one - "mlxsw: spectrum_kvdl: combine devlink resource
occupation getters".

Note that we pass resource size at both resource register and occupation
getter register times, to avoid situation when developer forgets to
extend the former call when changing the latter one.

Note that it's compile tested only, I will exercise new interface also by
Intel's ice driver later.

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 include/net/devlink.h                         |  5 ++--
 .../ethernet/mellanox/mlx5/core/sf/hw_table.c |  5 ++--
 drivers/net/ethernet/mellanox/mlxsw/core.c    |  5 ++--
 .../net/ethernet/mellanox/mlxsw/spectrum.c    | 18 ++++++-------
 .../ethernet/mellanox/mlxsw/spectrum1_kvdl.c  | 14 +++++------
 .../ethernet/mellanox/mlxsw/spectrum_cnt.c    |  9 ++++---
 .../mellanox/mlxsw/spectrum_policer.c         |  6 ++---
 .../mellanox/mlxsw/spectrum_port_range.c      |  2 +-
 .../ethernet/mellanox/mlxsw/spectrum_router.c |  4 +--
 .../ethernet/mellanox/mlxsw/spectrum_span.c   |  3 ++-
 drivers/net/netdevsim/dev.c                   | 14 +++++------
 drivers/net/netdevsim/fib.c                   | 10 ++++----
 net/devlink/resource.c                        | 25 ++++++++++++-------
 net/dsa/devlink.c                             |  4 +--
 14 files changed, 68 insertions(+), 56 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index fbb9a2668e24..48e009c7b90c 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1778,7 +1778,8 @@ int devl_resource_register(struct devlink *devlink,
 			   u64 resource_size,
 			   u64 resource_id,
 			   u64 parent_resource_id,
-			   const struct devlink_resource_size_params *size_params);
+			   const struct devlink_resource_size_params *size_params,
+			   size_t priv_data_size);
 void devl_resources_unregister(struct devlink *devlink);
 void devlink_resources_unregister(struct devlink *devlink);
 int devl_resource_size_get(struct devlink *devlink,
@@ -1790,7 +1791,7 @@ int devl_dpipe_table_resource_set(struct devlink *devlink,
 void devl_resource_occ_get_register(struct devlink *devlink,
 				    u64 resource_id,
 				    devlink_resource_occ_get_t *occ_get,
-				    void *occ_get_priv);
+				    void *occ_get_priv, size_t occ_priv_size);
 void devl_resource_occ_get_unregister(struct devlink *devlink,
 				      u64 resource_id);
 int devl_params_register(struct devlink *devlink,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
index 1f613320fe07..f8b574687a41 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
@@ -259,15 +259,16 @@ static int mlx5_sf_hw_table_res_register(struct mlx5_core_dev *dev, u16 max_fn,
 	devlink_resource_size_params_init(&size_params, max_fn, max_fn, 1,
 					  DEVLINK_RESOURCE_UNIT_ENTRY);
 	err = devl_resource_register(devlink, "max_local_SFs", max_fn, MLX5_DL_RES_MAX_LOCAL_SFS,
-				     DEVLINK_RESOURCE_ID_PARENT_TOP, &size_params);
+				     DEVLINK_RESOURCE_ID_PARENT_TOP,
+				     &size_params, sizeof(void *));
 	if (err)
 		return err;
 
 	devlink_resource_size_params_init(&size_params, max_ext_fn, max_ext_fn, 1,
 					  DEVLINK_RESOURCE_UNIT_ENTRY);
 	return devl_resource_register(devlink, "max_external_SFs", max_ext_fn,
 				      MLX5_DL_RES_MAX_EXTERNAL_SFS, DEVLINK_RESOURCE_ID_PARENT_TOP,
-				      &size_params);
+				      &size_params, sizeof(void *));
 }
 
 int mlx5_sf_hw_table_init(struct mlx5_core_dev *dev)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 4a79c0d7e7ad..81d14ccfb949 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -134,7 +134,7 @@ static int mlxsw_core_resources_ports_register(struct mlxsw_core *mlxsw_core)
 				      DEVLINK_RESOURCE_GENERIC_NAME_PORTS,
 				      max_ports, MLXSW_CORE_RESOURCE_PORTS,
 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
-				      &ports_num_params);
+				      &ports_num_params, sizeof(void *));
 }
 
 static int mlxsw_ports_init(struct mlxsw_core *mlxsw_core, bool reload)
@@ -161,7 +161,8 @@ static int mlxsw_ports_init(struct mlxsw_core *mlxsw_core, bool reload)
 	}
 	atomic_set(&mlxsw_core->active_ports_count, 0);
 	devl_resource_occ_get_register(devlink, MLXSW_CORE_RESOURCE_PORTS,
-				       mlxsw_ports_occ_get, mlxsw_core);
+				       mlxsw_ports_occ_get, &mlxsw_core,
+				       sizeof(mlxsw_core));
 
 	return 0;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index f064789f3240..2730ae3d8fe6 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3660,16 +3660,16 @@ static int mlxsw_sp1_resources_kvd_register(struct mlxsw_core *mlxsw_core)
 	err = devl_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD,
 				     kvd_size, MLXSW_SP_RESOURCE_KVD,
 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
-				     &kvd_size_params);
+				     &kvd_size_params, sizeof(void *));
 	if (err)
 		return err;
 
 	linear_size = profile->kvd_linear_size;
 	err = devl_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD_LINEAR,
 				     linear_size,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
 				     MLXSW_SP_RESOURCE_KVD,
-				     &linear_size_params);
+				     &linear_size_params, sizeof(void *));
 	if (err)
 		return err;
 
@@ -3686,16 +3686,16 @@ static int mlxsw_sp1_resources_kvd_register(struct mlxsw_core *mlxsw_core)
 				     double_size,
 				     MLXSW_SP_RESOURCE_KVD_HASH_DOUBLE,
 				     MLXSW_SP_RESOURCE_KVD,
-				     &hash_double_size_params);
+				     &hash_double_size_params, sizeof(void *));
 	if (err)
 		return err;
 
 	single_size = kvd_size - double_size - linear_size;
 	err = devl_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD_HASH_SINGLE,
 				     single_size,
 				     MLXSW_SP_RESOURCE_KVD_HASH_SINGLE,
 				     MLXSW_SP_RESOURCE_KVD,
-				     &hash_single_size_params);
+				     &hash_single_size_params, sizeof(void *));
 	if (err)
 		return err;
 
@@ -3719,7 +3719,7 @@ static int mlxsw_sp2_resources_kvd_register(struct mlxsw_core *mlxsw_core)
 	return devl_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD,
 				      kvd_size, MLXSW_SP_RESOURCE_KVD,
 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
-				      &kvd_size_params);
+				      &kvd_size_params, sizeof(void *));
 }
 
 static int mlxsw_sp_resources_span_register(struct mlxsw_core *mlxsw_core)
@@ -3738,7 +3738,7 @@ static int mlxsw_sp_resources_span_register(struct mlxsw_core *mlxsw_core)
 	return devl_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_SPAN,
 				      max_span, MLXSW_SP_RESOURCE_SPAN,
 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
-				      &span_size_params);
+				      &span_size_params, sizeof(void *));
 }
 
 static int
@@ -3762,7 +3762,7 @@ mlxsw_sp_resources_rif_mac_profile_register(struct mlxsw_core *mlxsw_core)
 				      max_rif_mac_profiles,
 				      MLXSW_SP_RESOURCE_RIF_MAC_PROFILES,
 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
-				      &size_params);
+				      &size_params, sizeof(void *));
 }
 
 static int mlxsw_sp_resources_rifs_register(struct mlxsw_core *mlxsw_core)
@@ -3781,7 +3781,7 @@ static int mlxsw_sp_resources_rifs_register(struct mlxsw_core *mlxsw_core)
 	return devl_resource_register(devlink, "rifs", max_rifs,
 				      MLXSW_SP_RESOURCE_RIFS,
 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
-				      &size_params);
+				      &size_params, sizeof(void *));
 }
 
 static int
@@ -3801,7 +3801,7 @@ mlxsw_sp_resources_port_range_register(struct mlxsw_core *mlxsw_core)
 	return devl_resource_register(devlink, "port_range_registers", max,
 				      MLXSW_SP_RESOURCE_PORT_RANGE_REGISTERS,
 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
-				      &size_params);
+				      &size_params, sizeof(void *));
 }
 
 static int mlxsw_sp1_resources_register(struct mlxsw_core *mlxsw_core)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
index 1e3fc989393c..ee5f12746371 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
@@ -341,19 +341,19 @@ static int mlxsw_sp1_kvdl_init(struct mlxsw_sp *mlxsw_sp, void *priv)
 	devl_resource_occ_get_register(devlink,
 				       MLXSW_SP_RESOURCE_KVD_LINEAR,
 				       mlxsw_sp1_kvdl_occ_get,
-				       kvdl);
+				       &kvdl, sizeof(kvdl));
 	devl_resource_occ_get_register(devlink,
 				       MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
 				       mlxsw_sp1_kvdl_single_occ_get,
-				       kvdl);
+				       &kvdl, sizeof(kvdl));
 	devl_resource_occ_get_register(devlink,
 				       MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
 				       mlxsw_sp1_kvdl_chunks_occ_get,
-				       kvdl);
+				       &kvdl, sizeof(kvdl));
 	devl_resource_occ_get_register(devlink,
 				       MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
 				       mlxsw_sp1_kvdl_large_chunks_occ_get,
-				       kvdl);
+				       &kvdl, sizeof(kvdl));
 	return 0;
 }
 
@@ -400,7 +400,7 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
 				     MLXSW_SP1_KVDL_SINGLE_SIZE,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
-				     &size_params);
+				     &size_params, sizeof(void *));
 	if (err)
 		return err;
 
@@ -411,7 +411,7 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
 				     MLXSW_SP1_KVDL_CHUNKS_SIZE,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
-				     &size_params);
+				     &size_params, sizeof(void *));
 	if (err)
 		return err;
 
@@ -422,6 +422,6 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
 				     MLXSW_SP1_KVDL_LARGE_CHUNKS_SIZE,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
-				     &size_params);
+				     &size_params, sizeof(void *));
 	return err;
 }
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_cnt.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_cnt.c
index 50e591420bd9..bf6a9623bccb 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_cnt.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_cnt.c
@@ -76,7 +76,7 @@ static int mlxsw_sp_counter_sub_pools_init(struct mlxsw_sp *mlxsw_sp)
 		devl_resource_occ_get_register(devlink,
 					       sub_pool->resource_id,
 					       mlxsw_sp_counter_sub_pool_occ_get,
-					       sub_pool);
+					       &sub_pool, sizeof(sub_pool));
 
 		sub_pool->base_index = base_index;
 		base_index += sub_pool->size;
@@ -140,7 +140,8 @@ int mlxsw_sp_counter_pool_init(struct mlxsw_sp *mlxsw_sp)
 	if (err)
 		goto err_pool_resource_size_get;
 	devl_resource_occ_get_register(devlink, MLXSW_SP_RESOURCE_COUNTERS,
-				       mlxsw_sp_counter_pool_occ_get, pool);
+				       mlxsw_sp_counter_pool_occ_get, &pool,
+				       sizeof(pool));
 
 	pool->usage = bitmap_zalloc(pool->pool_size, GFP_KERNEL);
 	if (!pool->usage) {
@@ -267,7 +268,7 @@ int mlxsw_sp_counter_resources_register(struct mlxsw_core *mlxsw_core)
 				     pool_size,
 				     MLXSW_SP_RESOURCE_COUNTERS,
 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
-				     &size_params);
+				     &size_params, sizeof(void *));
 	if (err)
 		return err;
 
@@ -292,7 +293,7 @@ int mlxsw_sp_counter_resources_register(struct mlxsw_core *mlxsw_core)
 					     sub_pool_size,
 					     sub_pool->resource_id,
 					     MLXSW_SP_RESOURCE_COUNTERS,
-					     &size_params);
+					     &size_params, sizeof(void *));
 		if (err)
 			return err;
 		total_bank_config += sub_pool->bank_count;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_policer.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_policer.c
index 22ebb207ce4d..e3b3d998d60f 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_policer.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_policer.c
@@ -97,7 +97,7 @@ mlxsw_sp_policer_single_rate_family_init(struct mlxsw_sp_policer_family *family)
 	devl_resource_occ_get_register(devlink,
 				       MLXSW_SP_RESOURCE_SINGLE_RATE_POLICERS,
 				       mlxsw_sp_policer_single_rate_occ_get,
-				       family);
+				       &family, sizeof(family));
 
 	return 0;
 }
@@ -423,7 +423,7 @@ int mlxsw_sp_policer_resources_register(struct mlxsw_core *mlxsw_core)
 				     global_policers,
 				     MLXSW_SP_RESOURCE_GLOBAL_POLICERS,
 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
-				     &size_params);
+				     &size_params, sizeof(void *));
 	if (err)
 		return err;
 
@@ -434,7 +434,7 @@ int mlxsw_sp_policer_resources_register(struct mlxsw_core *mlxsw_core)
 				     single_rate_policers,
 				     MLXSW_SP_RESOURCE_SINGLE_RATE_POLICERS,
 				     MLXSW_SP_RESOURCE_GLOBAL_POLICERS,
-				     &size_params);
+				     &size_params, sizeof(void *));
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_port_range.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_port_range.c
index 2d193de12be6..52271eb93797 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_port_range.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_port_range.c
@@ -183,7 +183,7 @@ int mlxsw_sp_port_range_init(struct mlxsw_sp *mlxsw_sp)
 	devl_resource_occ_get_register(priv_to_devlink(core),
 				       MLXSW_SP_RESOURCE_PORT_RANGE_REGISTERS,
 				       mlxsw_sp_port_range_reg_occ_get,
-				       pr_core);
+				       &pr_core, sizeof(pr_core));
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
index 800dfb64ec83..52eeea33a00f 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
@@ -11132,11 +11132,11 @@ static int mlxsw_sp_rifs_init(struct mlxsw_sp *mlxsw_sp)
 	devl_resource_occ_get_register(devlink,
 				       MLXSW_SP_RESOURCE_RIF_MAC_PROFILES,
 				       mlxsw_sp_rif_mac_profiles_occ_get,
-				       mlxsw_sp);
+				       &mlxsw_sp, sizeof(mlxsw_sp));
 	devl_resource_occ_get_register(devlink,
 				       MLXSW_SP_RESOURCE_RIFS,
 				       mlxsw_sp_rifs_occ_get,
-				       mlxsw_sp);
+				       &mlxsw_sp, sizeof(mlxsw_sp));
 
 	return 0;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
index 4b5fd71c897d..2d6a0ad06f19 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
@@ -107,7 +107,8 @@ int mlxsw_sp_span_init(struct mlxsw_sp *mlxsw_sp)
 		goto err_init;
 
 	devl_resource_occ_get_register(devlink, MLXSW_SP_RESOURCE_SPAN,
-				       mlxsw_sp_span_occ_get, mlxsw_sp);
+				       mlxsw_sp_span_occ_get, &mlxsw_sp,
+				       sizeof(mlxsw_sp));
 	INIT_WORK(&span->work, mlxsw_sp_span_respin_work);
 
 	return 0;
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 92a7a36b93ac..707be92dbc65 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -439,59 +439,59 @@ static int nsim_dev_resources_register(struct devlink *devlink)
 	err = devl_resource_register(devlink, "IPv4", (u64)-1,
 				     NSIM_RESOURCE_IPV4,
 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
-				     &params);
+				     &params, 0);
 	if (err) {
 		pr_err("Failed to register IPv4 top resource\n");
 		goto err_out;
 	}
 
 	err = devl_resource_register(devlink, "fib", (u64)-1,
 				     NSIM_RESOURCE_IPV4_FIB,
-				     NSIM_RESOURCE_IPV4, &params);
+				     NSIM_RESOURCE_IPV4, &params, 0);
 	if (err) {
 		pr_err("Failed to register IPv4 FIB resource\n");
 		goto err_out;
 	}
 
 	err = devl_resource_register(devlink, "fib-rules", (u64)-1,
 				     NSIM_RESOURCE_IPV4_FIB_RULES,
-				     NSIM_RESOURCE_IPV4, &params);
+				     NSIM_RESOURCE_IPV4, &params, 0);
 	if (err) {
 		pr_err("Failed to register IPv4 FIB rules resource\n");
 		goto err_out;
 	}
 
 	/* Resources for IPv6 */
 	err = devl_resource_register(devlink, "IPv6", (u64)-1,
 				     NSIM_RESOURCE_IPV6,
 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
-				     &params);
+				     &params, 0);
 	if (err) {
 		pr_err("Failed to register IPv6 top resource\n");
 		goto err_out;
 	}
 
 	err = devl_resource_register(devlink, "fib", (u64)-1,
 				     NSIM_RESOURCE_IPV6_FIB,
-				     NSIM_RESOURCE_IPV6, &params);
+				     NSIM_RESOURCE_IPV6, &params, 0);
 	if (err) {
 		pr_err("Failed to register IPv6 FIB resource\n");
 		goto err_out;
 	}
 
 	err = devl_resource_register(devlink, "fib-rules", (u64)-1,
 				     NSIM_RESOURCE_IPV6_FIB_RULES,
-				     NSIM_RESOURCE_IPV6, &params);
+				     NSIM_RESOURCE_IPV6, &params, 0);
 	if (err) {
 		pr_err("Failed to register IPv6 FIB rules resource\n");
 		goto err_out;
 	}
 
 	/* Resources for nexthops */
 	err = devl_resource_register(devlink, "nexthops", (u64)-1,
 				     NSIM_RESOURCE_NEXTHOPS,
 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
-				     &params);
+				     &params, 0);
 	if (err) {
 		pr_err("Failed to register NEXTHOPS resource\n");
 		goto err_out;
diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
index a1f91ff8ec56..799e75cef13a 100644
--- a/drivers/net/netdevsim/fib.c
+++ b/drivers/net/netdevsim/fib.c
@@ -1602,23 +1602,23 @@ struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
 	devl_resource_occ_get_register(devlink,
 				       NSIM_RESOURCE_IPV4_FIB,
 				       nsim_fib_ipv4_resource_occ_get,
-				       data);
+				       &data, sizeof(data));
 	devl_resource_occ_get_register(devlink,
 				       NSIM_RESOURCE_IPV4_FIB_RULES,
 				       nsim_fib_ipv4_rules_res_occ_get,
-				       data);
+				       &data, sizeof(data));
 	devl_resource_occ_get_register(devlink,
 				       NSIM_RESOURCE_IPV6_FIB,
 				       nsim_fib_ipv6_resource_occ_get,
-				       data);
+				       &data, sizeof(data));
 	devl_resource_occ_get_register(devlink,
 				       NSIM_RESOURCE_IPV6_FIB_RULES,
 				       nsim_fib_ipv6_rules_res_occ_get,
-				       data);
+				       &data, sizeof(data));
 	devl_resource_occ_get_register(devlink,
 				       NSIM_RESOURCE_NEXTHOPS,
 				       nsim_fib_nexthops_res_occ_get,
-				       data);
+				       &data, sizeof(data));
 	return data;
 
 err_nexthop_nb_unregister:
diff --git a/net/devlink/resource.c b/net/devlink/resource.c
index 15efa9f49461..71feaa963ebe 100644
--- a/net/devlink/resource.c
+++ b/net/devlink/resource.c
@@ -19,7 +19,8 @@
  * @list: parent list
  * @resource_list: list of child resources
  * @occ_get: occupancy getter callback
- * @occ_get_priv: occupancy getter callback priv
+ * @priv_size: @priv data size
+ * @priv: priv data allocated for the driver, passed to occupancy callbacks
  */
 struct devlink_resource {
 	const char *name;
@@ -32,7 +33,8 @@ struct devlink_resource {
 	struct list_head list;
 	struct list_head resource_list;
 	devlink_resource_occ_get_t *occ_get;
-	void *occ_get_priv;
+	size_t priv_size;
+	u8 priv[] __counted_by(priv_size);
 };
 
 static struct devlink_resource *
@@ -158,7 +160,7 @@ static int devlink_resource_occ_put(struct devlink_resource *resource,
 	if (!resource->occ_get)
 		return 0;
 	return nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_OCC,
-				 resource->occ_get(resource->occ_get_priv),
+				 resource->occ_get(resource->priv),
 				 DEVLINK_ATTR_PAD);
 }
 
@@ -326,6 +328,7 @@ int devlink_resources_validate(struct devlink *devlink,
  * @resource_id: resource's id
  * @parent_resource_id: resource's parent id
  * @size_params: size parameters
+ * @priv_data_size: sizeof of priv data member of the resource
  *
  * Generic resources should reuse the same names across drivers.
  * Please see the generic resources list at:
@@ -336,7 +339,8 @@ int devl_resource_register(struct devlink *devlink,
 			   u64 resource_size,
 			   u64 resource_id,
 			   u64 parent_resource_id,
-			   const struct devlink_resource_size_params *size_params)
+			   const struct devlink_resource_size_params *size_params,
+			   size_t priv_data_size)
 {
 	struct devlink_resource *resource;
 	struct list_head *resource_list;
@@ -350,9 +354,11 @@ int devl_resource_register(struct devlink *devlink,
 	if (resource)
 		return -EINVAL;
 
-	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
+	resource = kzalloc(struct_size(resource, priv, priv_data_size),
+			   GFP_KERNEL);
 	if (!resource)
 		return -ENOMEM;
+	resource->priv_size = priv_data_size;
 
 	if (top_hierarchy) {
 		resource_list = &devlink->resource_list;
@@ -463,23 +469,25 @@ EXPORT_SYMBOL_GPL(devl_resource_size_get);
  * @resource_id: resource id
  * @occ_get: occupancy getter callback
  * @occ_get_priv: occupancy getter callback priv
+ * @occ_priv_size: 
  */
 void devl_resource_occ_get_register(struct devlink *devlink,
 				    u64 resource_id,
 				    devlink_resource_occ_get_t *occ_get,
-				    void *occ_get_priv)
+				    void *occ_get_priv, size_t occ_priv_size)
 {
 	struct devlink_resource *resource;
 
 	lockdep_assert_held(&devlink->lock);
 
 	resource = devlink_resource_find(devlink, NULL, resource_id);
-	if (WARN_ON(!resource))
+	if (WARN_ON(!resource || occ_priv_size > resource->priv_size))
 		return;
 	WARN_ON(resource->occ_get);
 
 	resource->occ_get = occ_get;
-	resource->occ_get_priv = occ_get_priv;
+	/* put driver provided data into resource priv memory */
+	memcpy(resource->priv, occ_get_priv, occ_priv_size);
 }
 EXPORT_SYMBOL_GPL(devl_resource_occ_get_register);
 
@@ -502,6 +510,5 @@ void devl_resource_occ_get_unregister(struct devlink *devlink,
 	WARN_ON(!resource->occ_get);
 
 	resource->occ_get = NULL;
-	resource->occ_get_priv = NULL;
 }
 EXPORT_SYMBOL_GPL(devl_resource_occ_get_unregister);
diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c
index f41f9fc2194e..29adf2d47540 100644
--- a/net/dsa/devlink.c
+++ b/net/dsa/devlink.c
@@ -234,7 +234,7 @@ int dsa_devlink_resource_register(struct dsa_switch *ds,
 	devl_lock(ds->devlink);
 	ret = devl_resource_register(ds->devlink, resource_name, resource_size,
 				     resource_id, parent_resource_id,
-				     size_params);
+				     size_params, sizeof(void *));
 	devl_unlock(ds->devlink);
 
 	return ret;
@@ -254,7 +254,7 @@ void dsa_devlink_resource_occ_get_register(struct dsa_switch *ds,
 {
 	devl_lock(ds->devlink);
 	devl_resource_occ_get_register(ds->devlink, resource_id, occ_get,
-				       occ_get_priv);
+				       &occ_get_priv, sizeof(occ_get_priv));
 	devl_unlock(ds->devlink);
 }
 EXPORT_SYMBOL_GPL(dsa_devlink_resource_occ_get_register);
-- 
2.39.3

[PATCH net-next 5/5] mlxsw: spectrum_kvdl: combine devlink resource occupation getters

[Przemek Kitszel]

Combine spectrum1 kvdl devlink resource occupation getters into one.

Thanks to previous commit of the series we could easily embed more than
just a single pointer into devlink resource.

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 .../net/ethernet/mellanox/mlxsw/spectrum.h    |  5 ++
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  3 +-
 .../ethernet/mellanox/mlxsw/spectrum1_kvdl.c  | 80 ++++++++-----------
 3 files changed, 41 insertions(+), 47 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index 8d3c61287696..91fe5fffa675 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -836,6 +836,11 @@ int mlxsw_sp_kvdl_alloc_count_query(struct mlxsw_sp *mlxsw_sp,
 				    unsigned int *p_alloc_count);
 
 /* spectrum1_kvdl.c */
+struct mlxsw_sp1_kvdl_occ_ctx {
+	struct mlxsw_sp1_kvdl *kvdl;
+	int first_part_id;
+	bool count_all_parts;
+};
 extern const struct mlxsw_sp_kvdl_ops mlxsw_sp1_kvdl_ops;
 int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core);
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 2730ae3d8fe6..3bda2b2d16f9 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3669,7 +3669,8 @@ static int mlxsw_sp1_resources_kvd_register(struct mlxsw_core *mlxsw_core)
 				     linear_size,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
 				     MLXSW_SP_RESOURCE_KVD,
-				     &linear_size_params, sizeof(void *));
+				     &linear_size_params,
+				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
index ee5f12746371..a8bf052adf31 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
@@ -292,68 +292,53 @@ static u64 mlxsw_sp1_kvdl_part_occ(struct mlxsw_sp1_kvdl_part *part)
 
 static u64 mlxsw_sp1_kvdl_occ_get(void *priv)
 {
-	const struct mlxsw_sp1_kvdl *kvdl = priv;
+	struct mlxsw_sp1_kvdl_occ_ctx *ctx = priv;
+	bool cnt_all = ctx->count_all_parts;
+	int beg, end;
 	u64 occ = 0;
-	int i;
 
-	for (i = 0; i < MLXSW_SP1_KVDL_PARTS_INFO_LEN; i++)
-		occ += mlxsw_sp1_kvdl_part_occ(kvdl->parts[i]);
+	beg = cnt_all ? 0 : ctx->first_part_id,
+	end = cnt_all ? MLXSW_SP1_KVDL_PARTS_INFO_LEN : beg + 1;
+	for (int i = beg; i < end; i++)
+		occ += mlxsw_sp1_kvdl_part_occ(ctx->kvdl->parts[i]);
 
 	return occ;
 }
 
-static u64 mlxsw_sp1_kvdl_single_occ_get(void *priv)
-{
-	const struct mlxsw_sp1_kvdl *kvdl = priv;
-	struct mlxsw_sp1_kvdl_part *part;
-
-	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_SINGLE];
-	return mlxsw_sp1_kvdl_part_occ(part);
-}
-
-static u64 mlxsw_sp1_kvdl_chunks_occ_get(void *priv)
-{
-	const struct mlxsw_sp1_kvdl *kvdl = priv;
-	struct mlxsw_sp1_kvdl_part *part;
-
-	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_CHUNKS];
-	return mlxsw_sp1_kvdl_part_occ(part);
-}
-
-static u64 mlxsw_sp1_kvdl_large_chunks_occ_get(void *priv)
-{
-	const struct mlxsw_sp1_kvdl *kvdl = priv;
-	struct mlxsw_sp1_kvdl_part *part;
-
-	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_LARGE_CHUNKS];
-	return mlxsw_sp1_kvdl_part_occ(part);
-}
-
 static int mlxsw_sp1_kvdl_init(struct mlxsw_sp *mlxsw_sp, void *priv)
 {
 	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
-	struct mlxsw_sp1_kvdl *kvdl = priv;
+	struct mlxsw_sp1_kvdl_occ_ctx ctx = { priv };
 	int err;
 
-	err = mlxsw_sp1_kvdl_parts_init(mlxsw_sp, kvdl);
+	err = mlxsw_sp1_kvdl_parts_init(mlxsw_sp, ctx.kvdl);
 	if (err)
 		return err;
-	devl_resource_occ_get_register(devlink,
-				       MLXSW_SP_RESOURCE_KVD_LINEAR,
-				       mlxsw_sp1_kvdl_occ_get,
-				       &kvdl, sizeof(kvdl));
+
+	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_SINGLE;
 	devl_resource_occ_get_register(devlink,
 				       MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
-				       mlxsw_sp1_kvdl_single_occ_get,
-				       &kvdl, sizeof(kvdl));
+				       mlxsw_sp1_kvdl_occ_get,
+				       &ctx, sizeof(ctx));
+
+	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_CHUNKS;
 	devl_resource_occ_get_register(devlink,
 				       MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
-				       mlxsw_sp1_kvdl_chunks_occ_get,
-				       &kvdl, sizeof(kvdl));
+				       mlxsw_sp1_kvdl_occ_get,
+				       &ctx, sizeof(ctx));
+
+	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_LARGE_CHUNKS;
 	devl_resource_occ_get_register(devlink,
 				       MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
-				       mlxsw_sp1_kvdl_large_chunks_occ_get,
-				       &kvdl, sizeof(kvdl));
+				       mlxsw_sp1_kvdl_occ_get,
+				       &ctx, sizeof(ctx));
+
+	ctx.count_all_parts = true;
+	devl_resource_occ_get_register(devlink,
+				       MLXSW_SP_RESOURCE_KVD_LINEAR,
+				       mlxsw_sp1_kvdl_occ_get,
+				       &ctx, sizeof(ctx));
+
 	return 0;
 }
 
@@ -400,7 +385,8 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
 				     MLXSW_SP1_KVDL_SINGLE_SIZE,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
-				     &size_params, sizeof(void *));
+				     &size_params,
+				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
 	if (err)
 		return err;
 
@@ -411,7 +397,8 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
 				     MLXSW_SP1_KVDL_CHUNKS_SIZE,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
-				     &size_params, sizeof(void *));
+				     &size_params,
+				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
 	if (err)
 		return err;
 
@@ -422,6 +409,7 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
 				     MLXSW_SP1_KVDL_LARGE_CHUNKS_SIZE,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
-				     &size_params, sizeof(void *));
+				     &size_params,
+				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
 	return err;
 }
-- 
2.39.3

Re: [PATCH net-next 5/5] mlxsw: spectrum_kvdl: combine devlink resource occupation getters

[Jiri Pirko]

Tue, Aug 06, 2024 at 04:33:07PM CEST, przemyslaw.kitszel@intel.com wrote:
>Combine spectrum1 kvdl devlink resource occupation getters into one.
>
>Thanks to previous commit of the series we could easily embed more than
>just a single pointer into devlink resource.
>
>Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
>Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>---
> .../net/ethernet/mellanox/mlxsw/spectrum.h    |  5 ++
> .../net/ethernet/mellanox/mlxsw/spectrum.c    |  3 +-
> .../ethernet/mellanox/mlxsw/spectrum1_kvdl.c  | 80 ++++++++-----------
> 3 files changed, 41 insertions(+), 47 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>index 8d3c61287696..91fe5fffa675 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>@@ -836,6 +836,11 @@ int mlxsw_sp_kvdl_alloc_count_query(struct mlxsw_sp *mlxsw_sp,
> 				    unsigned int *p_alloc_count);
> 
> /* spectrum1_kvdl.c */
>+struct mlxsw_sp1_kvdl_occ_ctx {
>+	struct mlxsw_sp1_kvdl *kvdl;
>+	int first_part_id;
>+	bool count_all_parts;
>+};
> extern const struct mlxsw_sp_kvdl_ops mlxsw_sp1_kvdl_ops;
> int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core);
> 
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>index 2730ae3d8fe6..3bda2b2d16f9 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>@@ -3669,7 +3669,8 @@ static int mlxsw_sp1_resources_kvd_register(struct mlxsw_core *mlxsw_core)
> 				     linear_size,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
> 				     MLXSW_SP_RESOURCE_KVD,
>-				     &linear_size_params, sizeof(void *));
>+				     &linear_size_params,
>+				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
> 	if (err)
> 		return err;
> 
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>index ee5f12746371..a8bf052adf31 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>@@ -292,68 +292,53 @@ static u64 mlxsw_sp1_kvdl_part_occ(struct mlxsw_sp1_kvdl_part *part)
> 
> static u64 mlxsw_sp1_kvdl_occ_get(void *priv)
> {
>-	const struct mlxsw_sp1_kvdl *kvdl = priv;
>+	struct mlxsw_sp1_kvdl_occ_ctx *ctx = priv;
>+	bool cnt_all = ctx->count_all_parts;
>+	int beg, end;
> 	u64 occ = 0;
>-	int i;
> 
>-	for (i = 0; i < MLXSW_SP1_KVDL_PARTS_INFO_LEN; i++)
>-		occ += mlxsw_sp1_kvdl_part_occ(kvdl->parts[i]);
>+	beg = cnt_all ? 0 : ctx->first_part_id,
>+	end = cnt_all ? MLXSW_SP1_KVDL_PARTS_INFO_LEN : beg + 1;
>+	for (int i = beg; i < end; i++)
>+		occ += mlxsw_sp1_kvdl_part_occ(ctx->kvdl->parts[i]);
> 
> 	return occ;

I don't see the benefit, this just makes code harder to read.


> }
> 
>-static u64 mlxsw_sp1_kvdl_single_occ_get(void *priv)
>-{
>-	const struct mlxsw_sp1_kvdl *kvdl = priv;
>-	struct mlxsw_sp1_kvdl_part *part;
>-
>-	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_SINGLE];
>-	return mlxsw_sp1_kvdl_part_occ(part);
>-}
>-
>-static u64 mlxsw_sp1_kvdl_chunks_occ_get(void *priv)
>-{
>-	const struct mlxsw_sp1_kvdl *kvdl = priv;
>-	struct mlxsw_sp1_kvdl_part *part;
>-
>-	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_CHUNKS];
>-	return mlxsw_sp1_kvdl_part_occ(part);
>-}
>-
>-static u64 mlxsw_sp1_kvdl_large_chunks_occ_get(void *priv)
>-{
>-	const struct mlxsw_sp1_kvdl *kvdl = priv;
>-	struct mlxsw_sp1_kvdl_part *part;
>-
>-	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_LARGE_CHUNKS];
>-	return mlxsw_sp1_kvdl_part_occ(part);
>-}
>-
> static int mlxsw_sp1_kvdl_init(struct mlxsw_sp *mlxsw_sp, void *priv)
> {
> 	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
>-	struct mlxsw_sp1_kvdl *kvdl = priv;
>+	struct mlxsw_sp1_kvdl_occ_ctx ctx = { priv };
> 	int err;
> 
>-	err = mlxsw_sp1_kvdl_parts_init(mlxsw_sp, kvdl);
>+	err = mlxsw_sp1_kvdl_parts_init(mlxsw_sp, ctx.kvdl);
> 	if (err)
> 		return err;
>-	devl_resource_occ_get_register(devlink,
>-				       MLXSW_SP_RESOURCE_KVD_LINEAR,
>-				       mlxsw_sp1_kvdl_occ_get,
>-				       &kvdl, sizeof(kvdl));
>+
>+	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_SINGLE;
> 	devl_resource_occ_get_register(devlink,
> 				       MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
>-				       mlxsw_sp1_kvdl_single_occ_get,
>-				       &kvdl, sizeof(kvdl));
>+				       mlxsw_sp1_kvdl_occ_get,
>+				       &ctx, sizeof(ctx));
>+
>+	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_CHUNKS;
> 	devl_resource_occ_get_register(devlink,
> 				       MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
>-				       mlxsw_sp1_kvdl_chunks_occ_get,
>-				       &kvdl, sizeof(kvdl));
>+				       mlxsw_sp1_kvdl_occ_get,
>+				       &ctx, sizeof(ctx));
>+
>+	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_LARGE_CHUNKS;
> 	devl_resource_occ_get_register(devlink,
> 				       MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
>-				       mlxsw_sp1_kvdl_large_chunks_occ_get,
>-				       &kvdl, sizeof(kvdl));
>+				       mlxsw_sp1_kvdl_occ_get,
>+				       &ctx, sizeof(ctx));
>+
>+	ctx.count_all_parts = true;
>+	devl_resource_occ_get_register(devlink,
>+				       MLXSW_SP_RESOURCE_KVD_LINEAR,
>+				       mlxsw_sp1_kvdl_occ_get,
>+				       &ctx, sizeof(ctx));
>+
> 	return 0;
> }
> 
>@@ -400,7 +385,8 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
> 				     MLXSW_SP1_KVDL_SINGLE_SIZE,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>-				     &size_params, sizeof(void *));
>+				     &size_params,
>+				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
> 	if (err)
> 		return err;
> 
>@@ -411,7 +397,8 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
> 				     MLXSW_SP1_KVDL_CHUNKS_SIZE,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>-				     &size_params, sizeof(void *));
>+				     &size_params,
>+				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
> 	if (err)
> 		return err;
> 
>@@ -422,6 +409,7 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
> 				     MLXSW_SP1_KVDL_LARGE_CHUNKS_SIZE,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>-				     &size_params, sizeof(void *));
>+				     &size_params,
>+				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
> 	return err;
> }
>-- 
>2.39.3
>

Re: [PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource

[Jiri Pirko]

Tue, Aug 06, 2024 at 04:33:06PM CEST, przemyslaw.kitszel@intel.com wrote:
>Extend devlink resource by flex array to store drivers priv data, in
>current usages it just replaces occupation getter's priv callback pointer.
>Coupling lifetime of the resource and its getters metadata (don't confuse
>with simple pointer to PF) is generally a good idea, and makes driver code
>nicer.
>
>Next commit will show how to makes use of it to combine related occ
>getters into one - "mlxsw: spectrum_kvdl: combine devlink resource
>occupation getters".
>
>Note that we pass resource size at both resource register and occupation
>getter register times, to avoid situation when developer forgets to
>extend the former call when changing the latter one.
>
>Note that it's compile tested only, I will exercise new interface also by
>Intel's ice driver later.
>
>Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
>Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>---
> include/net/devlink.h                         |  5 ++--
> .../ethernet/mellanox/mlx5/core/sf/hw_table.c |  5 ++--
> drivers/net/ethernet/mellanox/mlxsw/core.c    |  5 ++--
> .../net/ethernet/mellanox/mlxsw/spectrum.c    | 18 ++++++-------
> .../ethernet/mellanox/mlxsw/spectrum1_kvdl.c  | 14 +++++------
> .../ethernet/mellanox/mlxsw/spectrum_cnt.c    |  9 ++++---
> .../mellanox/mlxsw/spectrum_policer.c         |  6 ++---
> .../mellanox/mlxsw/spectrum_port_range.c      |  2 +-
> .../ethernet/mellanox/mlxsw/spectrum_router.c |  4 +--
> .../ethernet/mellanox/mlxsw/spectrum_span.c   |  3 ++-
> drivers/net/netdevsim/dev.c                   | 14 +++++------
> drivers/net/netdevsim/fib.c                   | 10 ++++----
> net/devlink/resource.c                        | 25 ++++++++++++-------
> net/dsa/devlink.c                             |  4 +--
> 14 files changed, 68 insertions(+), 56 deletions(-)
>
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index fbb9a2668e24..48e009c7b90c 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -1778,7 +1778,8 @@ int devl_resource_register(struct devlink *devlink,
> 			   u64 resource_size,
> 			   u64 resource_id,
> 			   u64 parent_resource_id,
>-			   const struct devlink_resource_size_params *size_params);
>+			   const struct devlink_resource_size_params *size_params,
>+			   size_t priv_data_size);
> void devl_resources_unregister(struct devlink *devlink);
> void devlink_resources_unregister(struct devlink *devlink);
> int devl_resource_size_get(struct devlink *devlink,
>@@ -1790,7 +1791,7 @@ int devl_dpipe_table_resource_set(struct devlink *devlink,
> void devl_resource_occ_get_register(struct devlink *devlink,
> 				    u64 resource_id,
> 				    devlink_resource_occ_get_t *occ_get,
>-				    void *occ_get_priv);
>+				    void *occ_get_priv, size_t occ_priv_size);
> void devl_resource_occ_get_unregister(struct devlink *devlink,
> 				      u64 resource_id);
> int devl_params_register(struct devlink *devlink,
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c b/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
>index 1f613320fe07..f8b574687a41 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/sf/hw_table.c
>@@ -259,15 +259,16 @@ static int mlx5_sf_hw_table_res_register(struct mlx5_core_dev *dev, u16 max_fn,
> 	devlink_resource_size_params_init(&size_params, max_fn, max_fn, 1,
> 					  DEVLINK_RESOURCE_UNIT_ENTRY);
> 	err = devl_resource_register(devlink, "max_local_SFs", max_fn, MLX5_DL_RES_MAX_LOCAL_SFS,
>-				     DEVLINK_RESOURCE_ID_PARENT_TOP, &size_params);
>+				     DEVLINK_RESOURCE_ID_PARENT_TOP,
>+				     &size_params, sizeof(void *));
> 	if (err)
> 		return err;
> 
> 	devlink_resource_size_params_init(&size_params, max_ext_fn, max_ext_fn, 1,
> 					  DEVLINK_RESOURCE_UNIT_ENTRY);
> 	return devl_resource_register(devlink, "max_external_SFs", max_ext_fn,
> 				      MLX5_DL_RES_MAX_EXTERNAL_SFS, DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				      &size_params);
>+				      &size_params, sizeof(void *));
> }
> 
> int mlx5_sf_hw_table_init(struct mlx5_core_dev *dev)
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
>index 4a79c0d7e7ad..81d14ccfb949 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
>@@ -134,7 +134,7 @@ static int mlxsw_core_resources_ports_register(struct mlxsw_core *mlxsw_core)
> 				      DEVLINK_RESOURCE_GENERIC_NAME_PORTS,
> 				      max_ports, MLXSW_CORE_RESOURCE_PORTS,
> 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				      &ports_num_params);
>+				      &ports_num_params, sizeof(void *));
> }
> 
> static int mlxsw_ports_init(struct mlxsw_core *mlxsw_core, bool reload)
>@@ -161,7 +161,8 @@ static int mlxsw_ports_init(struct mlxsw_core *mlxsw_core, bool reload)
> 	}
> 	atomic_set(&mlxsw_core->active_ports_count, 0);
> 	devl_resource_occ_get_register(devlink, MLXSW_CORE_RESOURCE_PORTS,
>-				       mlxsw_ports_occ_get, mlxsw_core);
>+				       mlxsw_ports_occ_get, &mlxsw_core,
>+				       sizeof(mlxsw_core));
> 
> 	return 0;
> 
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>index f064789f3240..2730ae3d8fe6 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>@@ -3660,16 +3660,16 @@ static int mlxsw_sp1_resources_kvd_register(struct mlxsw_core *mlxsw_core)
> 	err = devl_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD,
> 				     kvd_size, MLXSW_SP_RESOURCE_KVD,
> 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				     &kvd_size_params);
>+				     &kvd_size_params, sizeof(void *));
> 	if (err)
> 		return err;
> 
> 	linear_size = profile->kvd_linear_size;
> 	err = devl_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD_LINEAR,
> 				     linear_size,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
> 				     MLXSW_SP_RESOURCE_KVD,
>-				     &linear_size_params);
>+				     &linear_size_params, sizeof(void *));
> 	if (err)
> 		return err;
> 
>@@ -3686,16 +3686,16 @@ static int mlxsw_sp1_resources_kvd_register(struct mlxsw_core *mlxsw_core)
> 				     double_size,
> 				     MLXSW_SP_RESOURCE_KVD_HASH_DOUBLE,
> 				     MLXSW_SP_RESOURCE_KVD,
>-				     &hash_double_size_params);
>+				     &hash_double_size_params, sizeof(void *));
> 	if (err)
> 		return err;
> 
> 	single_size = kvd_size - double_size - linear_size;
> 	err = devl_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD_HASH_SINGLE,
> 				     single_size,
> 				     MLXSW_SP_RESOURCE_KVD_HASH_SINGLE,
> 				     MLXSW_SP_RESOURCE_KVD,
>-				     &hash_single_size_params);
>+				     &hash_single_size_params, sizeof(void *));
> 	if (err)
> 		return err;
> 
>@@ -3719,7 +3719,7 @@ static int mlxsw_sp2_resources_kvd_register(struct mlxsw_core *mlxsw_core)
> 	return devl_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD,
> 				      kvd_size, MLXSW_SP_RESOURCE_KVD,
> 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				      &kvd_size_params);
>+				      &kvd_size_params, sizeof(void *));
> }
> 
> static int mlxsw_sp_resources_span_register(struct mlxsw_core *mlxsw_core)
>@@ -3738,7 +3738,7 @@ static int mlxsw_sp_resources_span_register(struct mlxsw_core *mlxsw_core)
> 	return devl_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_SPAN,
> 				      max_span, MLXSW_SP_RESOURCE_SPAN,
> 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				      &span_size_params);
>+				      &span_size_params, sizeof(void *));
> }
> 
> static int
>@@ -3762,7 +3762,7 @@ mlxsw_sp_resources_rif_mac_profile_register(struct mlxsw_core *mlxsw_core)
> 				      max_rif_mac_profiles,
> 				      MLXSW_SP_RESOURCE_RIF_MAC_PROFILES,
> 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				      &size_params);
>+				      &size_params, sizeof(void *));
> }
> 
> static int mlxsw_sp_resources_rifs_register(struct mlxsw_core *mlxsw_core)
>@@ -3781,7 +3781,7 @@ static int mlxsw_sp_resources_rifs_register(struct mlxsw_core *mlxsw_core)
> 	return devl_resource_register(devlink, "rifs", max_rifs,
> 				      MLXSW_SP_RESOURCE_RIFS,
> 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				      &size_params);
>+				      &size_params, sizeof(void *));
> }
> 
> static int
>@@ -3801,7 +3801,7 @@ mlxsw_sp_resources_port_range_register(struct mlxsw_core *mlxsw_core)
> 	return devl_resource_register(devlink, "port_range_registers", max,
> 				      MLXSW_SP_RESOURCE_PORT_RANGE_REGISTERS,
> 				      DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				      &size_params);
>+				      &size_params, sizeof(void *));
> }
> 
> static int mlxsw_sp1_resources_register(struct mlxsw_core *mlxsw_core)
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>index 1e3fc989393c..ee5f12746371 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>@@ -341,19 +341,19 @@ static int mlxsw_sp1_kvdl_init(struct mlxsw_sp *mlxsw_sp, void *priv)
> 	devl_resource_occ_get_register(devlink,
> 				       MLXSW_SP_RESOURCE_KVD_LINEAR,
> 				       mlxsw_sp1_kvdl_occ_get,
>-				       kvdl);
>+				       &kvdl, sizeof(kvdl));
> 	devl_resource_occ_get_register(devlink,
> 				       MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
> 				       mlxsw_sp1_kvdl_single_occ_get,
>-				       kvdl);
>+				       &kvdl, sizeof(kvdl));
> 	devl_resource_occ_get_register(devlink,
> 				       MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
> 				       mlxsw_sp1_kvdl_chunks_occ_get,
>-				       kvdl);
>+				       &kvdl, sizeof(kvdl));
> 	devl_resource_occ_get_register(devlink,
> 				       MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
> 				       mlxsw_sp1_kvdl_large_chunks_occ_get,
>-				       kvdl);
>+				       &kvdl, sizeof(kvdl));
> 	return 0;
> }
> 
>@@ -400,7 +400,7 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
> 				     MLXSW_SP1_KVDL_SINGLE_SIZE,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>-				     &size_params);
>+				     &size_params, sizeof(void *));
> 	if (err)
> 		return err;
> 
>@@ -411,7 +411,7 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
> 				     MLXSW_SP1_KVDL_CHUNKS_SIZE,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>-				     &size_params);
>+				     &size_params, sizeof(void *));
> 	if (err)
> 		return err;
> 
>@@ -422,6 +422,6 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
> 				     MLXSW_SP1_KVDL_LARGE_CHUNKS_SIZE,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>-				     &size_params);
>+				     &size_params, sizeof(void *));
> 	return err;
> }
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_cnt.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_cnt.c
>index 50e591420bd9..bf6a9623bccb 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_cnt.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_cnt.c
>@@ -76,7 +76,7 @@ static int mlxsw_sp_counter_sub_pools_init(struct mlxsw_sp *mlxsw_sp)
> 		devl_resource_occ_get_register(devlink,
> 					       sub_pool->resource_id,
> 					       mlxsw_sp_counter_sub_pool_occ_get,
>-					       sub_pool);
>+					       &sub_pool, sizeof(sub_pool));
> 
> 		sub_pool->base_index = base_index;
> 		base_index += sub_pool->size;
>@@ -140,7 +140,8 @@ int mlxsw_sp_counter_pool_init(struct mlxsw_sp *mlxsw_sp)
> 	if (err)
> 		goto err_pool_resource_size_get;
> 	devl_resource_occ_get_register(devlink, MLXSW_SP_RESOURCE_COUNTERS,
>-				       mlxsw_sp_counter_pool_occ_get, pool);
>+				       mlxsw_sp_counter_pool_occ_get, &pool,
>+				       sizeof(pool));
> 
> 	pool->usage = bitmap_zalloc(pool->pool_size, GFP_KERNEL);
> 	if (!pool->usage) {
>@@ -267,7 +268,7 @@ int mlxsw_sp_counter_resources_register(struct mlxsw_core *mlxsw_core)
> 				     pool_size,
> 				     MLXSW_SP_RESOURCE_COUNTERS,
> 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				     &size_params);
>+				     &size_params, sizeof(void *));
> 	if (err)
> 		return err;
> 
>@@ -292,7 +293,7 @@ int mlxsw_sp_counter_resources_register(struct mlxsw_core *mlxsw_core)
> 					     sub_pool_size,
> 					     sub_pool->resource_id,
> 					     MLXSW_SP_RESOURCE_COUNTERS,
>-					     &size_params);
>+					     &size_params, sizeof(void *));
> 		if (err)
> 			return err;
> 		total_bank_config += sub_pool->bank_count;
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_policer.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_policer.c
>index 22ebb207ce4d..e3b3d998d60f 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_policer.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_policer.c
>@@ -97,7 +97,7 @@ mlxsw_sp_policer_single_rate_family_init(struct mlxsw_sp_policer_family *family)
> 	devl_resource_occ_get_register(devlink,
> 				       MLXSW_SP_RESOURCE_SINGLE_RATE_POLICERS,
> 				       mlxsw_sp_policer_single_rate_occ_get,
>-				       family);
>+				       &family, sizeof(family));
> 
> 	return 0;
> }
>@@ -423,7 +423,7 @@ int mlxsw_sp_policer_resources_register(struct mlxsw_core *mlxsw_core)
> 				     global_policers,
> 				     MLXSW_SP_RESOURCE_GLOBAL_POLICERS,
> 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				     &size_params);
>+				     &size_params, sizeof(void *));
> 	if (err)
> 		return err;
> 
>@@ -434,7 +434,7 @@ int mlxsw_sp_policer_resources_register(struct mlxsw_core *mlxsw_core)
> 				     single_rate_policers,
> 				     MLXSW_SP_RESOURCE_SINGLE_RATE_POLICERS,
> 				     MLXSW_SP_RESOURCE_GLOBAL_POLICERS,
>-				     &size_params);
>+				     &size_params, sizeof(void *));
> 	if (err)
> 		return err;
> 
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_port_range.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_port_range.c
>index 2d193de12be6..52271eb93797 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_port_range.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_port_range.c
>@@ -183,7 +183,7 @@ int mlxsw_sp_port_range_init(struct mlxsw_sp *mlxsw_sp)
> 	devl_resource_occ_get_register(priv_to_devlink(core),
> 				       MLXSW_SP_RESOURCE_PORT_RANGE_REGISTERS,
> 				       mlxsw_sp_port_range_reg_occ_get,
>-				       pr_core);
>+				       &pr_core, sizeof(pr_core));
> 
> 	return 0;
> }
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>index 800dfb64ec83..52eeea33a00f 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c
>@@ -11132,11 +11132,11 @@ static int mlxsw_sp_rifs_init(struct mlxsw_sp *mlxsw_sp)
> 	devl_resource_occ_get_register(devlink,
> 				       MLXSW_SP_RESOURCE_RIF_MAC_PROFILES,
> 				       mlxsw_sp_rif_mac_profiles_occ_get,
>-				       mlxsw_sp);
>+				       &mlxsw_sp, sizeof(mlxsw_sp));
> 	devl_resource_occ_get_register(devlink,
> 				       MLXSW_SP_RESOURCE_RIFS,
> 				       mlxsw_sp_rifs_occ_get,
>-				       mlxsw_sp);
>+				       &mlxsw_sp, sizeof(mlxsw_sp));
> 
> 	return 0;
> 
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
>index 4b5fd71c897d..2d6a0ad06f19 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_span.c
>@@ -107,7 +107,8 @@ int mlxsw_sp_span_init(struct mlxsw_sp *mlxsw_sp)
> 		goto err_init;
> 
> 	devl_resource_occ_get_register(devlink, MLXSW_SP_RESOURCE_SPAN,
>-				       mlxsw_sp_span_occ_get, mlxsw_sp);
>+				       mlxsw_sp_span_occ_get, &mlxsw_sp,
>+				       sizeof(mlxsw_sp));
> 	INIT_WORK(&span->work, mlxsw_sp_span_respin_work);
> 
> 	return 0;
>diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>index 92a7a36b93ac..707be92dbc65 100644
>--- a/drivers/net/netdevsim/dev.c
>+++ b/drivers/net/netdevsim/dev.c
>@@ -439,59 +439,59 @@ static int nsim_dev_resources_register(struct devlink *devlink)
> 	err = devl_resource_register(devlink, "IPv4", (u64)-1,
> 				     NSIM_RESOURCE_IPV4,
> 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				     &params);
>+				     &params, 0);
> 	if (err) {
> 		pr_err("Failed to register IPv4 top resource\n");
> 		goto err_out;
> 	}
> 
> 	err = devl_resource_register(devlink, "fib", (u64)-1,
> 				     NSIM_RESOURCE_IPV4_FIB,
>-				     NSIM_RESOURCE_IPV4, &params);
>+				     NSIM_RESOURCE_IPV4, &params, 0);
> 	if (err) {
> 		pr_err("Failed to register IPv4 FIB resource\n");
> 		goto err_out;
> 	}
> 
> 	err = devl_resource_register(devlink, "fib-rules", (u64)-1,
> 				     NSIM_RESOURCE_IPV4_FIB_RULES,
>-				     NSIM_RESOURCE_IPV4, &params);
>+				     NSIM_RESOURCE_IPV4, &params, 0);
> 	if (err) {
> 		pr_err("Failed to register IPv4 FIB rules resource\n");
> 		goto err_out;
> 	}
> 
> 	/* Resources for IPv6 */
> 	err = devl_resource_register(devlink, "IPv6", (u64)-1,
> 				     NSIM_RESOURCE_IPV6,
> 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				     &params);
>+				     &params, 0);
> 	if (err) {
> 		pr_err("Failed to register IPv6 top resource\n");
> 		goto err_out;
> 	}
> 
> 	err = devl_resource_register(devlink, "fib", (u64)-1,
> 				     NSIM_RESOURCE_IPV6_FIB,
>-				     NSIM_RESOURCE_IPV6, &params);
>+				     NSIM_RESOURCE_IPV6, &params, 0);
> 	if (err) {
> 		pr_err("Failed to register IPv6 FIB resource\n");
> 		goto err_out;
> 	}
> 
> 	err = devl_resource_register(devlink, "fib-rules", (u64)-1,
> 				     NSIM_RESOURCE_IPV6_FIB_RULES,
>-				     NSIM_RESOURCE_IPV6, &params);
>+				     NSIM_RESOURCE_IPV6, &params, 0);
> 	if (err) {
> 		pr_err("Failed to register IPv6 FIB rules resource\n");
> 		goto err_out;
> 	}
> 
> 	/* Resources for nexthops */
> 	err = devl_resource_register(devlink, "nexthops", (u64)-1,
> 				     NSIM_RESOURCE_NEXTHOPS,
> 				     DEVLINK_RESOURCE_ID_PARENT_TOP,
>-				     &params);
>+				     &params, 0);
> 	if (err) {
> 		pr_err("Failed to register NEXTHOPS resource\n");
> 		goto err_out;
>diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
>index a1f91ff8ec56..799e75cef13a 100644
>--- a/drivers/net/netdevsim/fib.c
>+++ b/drivers/net/netdevsim/fib.c
>@@ -1602,23 +1602,23 @@ struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
> 	devl_resource_occ_get_register(devlink,
> 				       NSIM_RESOURCE_IPV4_FIB,
> 				       nsim_fib_ipv4_resource_occ_get,
>-				       data);
>+				       &data, sizeof(data));
> 	devl_resource_occ_get_register(devlink,
> 				       NSIM_RESOURCE_IPV4_FIB_RULES,
> 				       nsim_fib_ipv4_rules_res_occ_get,
>-				       data);
>+				       &data, sizeof(data));
> 	devl_resource_occ_get_register(devlink,
> 				       NSIM_RESOURCE_IPV6_FIB,
> 				       nsim_fib_ipv6_resource_occ_get,
>-				       data);
>+				       &data, sizeof(data));
> 	devl_resource_occ_get_register(devlink,
> 				       NSIM_RESOURCE_IPV6_FIB_RULES,
> 				       nsim_fib_ipv6_rules_res_occ_get,
>-				       data);
>+				       &data, sizeof(data));
> 	devl_resource_occ_get_register(devlink,
> 				       NSIM_RESOURCE_NEXTHOPS,
> 				       nsim_fib_nexthops_res_occ_get,
>-				       data);
>+				       &data, sizeof(data));
> 	return data;
> 
> err_nexthop_nb_unregister:
>diff --git a/net/devlink/resource.c b/net/devlink/resource.c
>index 15efa9f49461..71feaa963ebe 100644
>--- a/net/devlink/resource.c
>+++ b/net/devlink/resource.c
>@@ -19,7 +19,8 @@
>  * @list: parent list
>  * @resource_list: list of child resources
>  * @occ_get: occupancy getter callback
>- * @occ_get_priv: occupancy getter callback priv
>+ * @priv_size: @priv data size
>+ * @priv: priv data allocated for the driver, passed to occupancy callbacks
>  */
> struct devlink_resource {
> 	const char *name;
>@@ -32,7 +33,8 @@ struct devlink_resource {
> 	struct list_head list;
> 	struct list_head resource_list;
> 	devlink_resource_occ_get_t *occ_get;
>-	void *occ_get_priv;
>+	size_t priv_size;
>+	u8 priv[] __counted_by(priv_size);
> };
> 
> static struct devlink_resource *
>@@ -158,7 +160,7 @@ static int devlink_resource_occ_put(struct devlink_resource *resource,
> 	if (!resource->occ_get)
> 		return 0;
> 	return nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_OCC,
>-				 resource->occ_get(resource->occ_get_priv),
>+				 resource->occ_get(resource->priv),
> 				 DEVLINK_ATTR_PAD);
> }
> 
>@@ -326,6 +328,7 @@ int devlink_resources_validate(struct devlink *devlink,
>  * @resource_id: resource's id
>  * @parent_resource_id: resource's parent id
>  * @size_params: size parameters
>+ * @priv_data_size: sizeof of priv data member of the resource
>  *
>  * Generic resources should reuse the same names across drivers.
>  * Please see the generic resources list at:
>@@ -336,7 +339,8 @@ int devl_resource_register(struct devlink *devlink,
> 			   u64 resource_size,
> 			   u64 resource_id,
> 			   u64 parent_resource_id,
>-			   const struct devlink_resource_size_params *size_params)
>+			   const struct devlink_resource_size_params *size_params,
>+			   size_t priv_data_size)
> {
> 	struct devlink_resource *resource;
> 	struct list_head *resource_list;
>@@ -350,9 +354,11 @@ int devl_resource_register(struct devlink *devlink,
> 	if (resource)
> 		return -EINVAL;
> 
>-	resource = kzalloc(sizeof(*resource), GFP_KERNEL);
>+	resource = kzalloc(struct_size(resource, priv, priv_data_size),
>+			   GFP_KERNEL);
> 	if (!resource)
> 		return -ENOMEM;
>+	resource->priv_size = priv_data_size;
> 
> 	if (top_hierarchy) {
> 		resource_list = &devlink->resource_list;
>@@ -463,23 +469,25 @@ EXPORT_SYMBOL_GPL(devl_resource_size_get);
>  * @resource_id: resource id
>  * @occ_get: occupancy getter callback
>  * @occ_get_priv: occupancy getter callback priv
>+ * @occ_priv_size: 
>  */
> void devl_resource_occ_get_register(struct devlink *devlink,
> 				    u64 resource_id,
> 				    devlink_resource_occ_get_t *occ_get,
>-				    void *occ_get_priv)
>+				    void *occ_get_priv, size_t occ_priv_size)
> {
> 	struct devlink_resource *resource;
> 
> 	lockdep_assert_held(&devlink->lock);
> 
> 	resource = devlink_resource_find(devlink, NULL, resource_id);
>-	if (WARN_ON(!resource))
>+	if (WARN_ON(!resource || occ_priv_size > resource->priv_size))

Very odd. You allocate a mem in devl_resource_register() and here you
copy data to it. Why the void pointer is not enough for you? You can
easily alloc struct in the driver and pass a pointer to it.

This is quite weird. Please don't.


> 		return;
> 	WARN_ON(resource->occ_get);
> 
> 	resource->occ_get = occ_get;
>-	resource->occ_get_priv = occ_get_priv;
>+	/* put driver provided data into resource priv memory */
>+	memcpy(resource->priv, occ_get_priv, occ_priv_size);
> }
> EXPORT_SYMBOL_GPL(devl_resource_occ_get_register);
> 
>@@ -502,6 +510,5 @@ void devl_resource_occ_get_unregister(struct devlink *devlink,
> 	WARN_ON(!resource->occ_get);
> 
> 	resource->occ_get = NULL;
>-	resource->occ_get_priv = NULL;
> }
> EXPORT_SYMBOL_GPL(devl_resource_occ_get_unregister);
>diff --git a/net/dsa/devlink.c b/net/dsa/devlink.c
>index f41f9fc2194e..29adf2d47540 100644
>--- a/net/dsa/devlink.c
>+++ b/net/dsa/devlink.c
>@@ -234,7 +234,7 @@ int dsa_devlink_resource_register(struct dsa_switch *ds,
> 	devl_lock(ds->devlink);
> 	ret = devl_resource_register(ds->devlink, resource_name, resource_size,
> 				     resource_id, parent_resource_id,
>-				     size_params);
>+				     size_params, sizeof(void *));
> 	devl_unlock(ds->devlink);
> 
> 	return ret;
>@@ -254,7 +254,7 @@ void dsa_devlink_resource_occ_get_register(struct dsa_switch *ds,
> {
> 	devl_lock(ds->devlink);
> 	devl_resource_occ_get_register(ds->devlink, resource_id, occ_get,
>-				       occ_get_priv);
>+				       &occ_get_priv, sizeof(occ_get_priv));
> 	devl_unlock(ds->devlink);
> }
> EXPORT_SYMBOL_GPL(dsa_devlink_resource_occ_get_register);
>-- 
>2.39.3
>

Re: [PATCH net-next 0/5] devlink: embed driver's priv data callback param into devlink_resource

[Jiri Pirko]

Tue, Aug 06, 2024 at 04:33:02PM CEST, przemyslaw.kitszel@intel.com wrote:
>(Patch 1)
>Convert dsa to use devl_* variants of devlink resource related
>calls, so we could remove devlink_* variants in next 2 patches.
>
>(Patches 2,3)
>Remove some unused functions that would otherwise need an update.
>
>(Patch 4, the main one)
>Then extend devlink resource to embed driver's priv data callback,
>instead just storing a pointer (so drivers could put more context for
>similar resource getters, to handle them via simple single function
>instead of dumb duplication).
>
>(Patch 5)
>Make use of the new possibility from patch 4, I've picked the most
>repetitive case.
>
>Motivation: current API was to distracting for me to focus on adding my
>new resources :)

Hmm, I don't really understand how this justifies. It just makes code
harder to follow and introduces oddities, like passing sizeof(void *).
Please don't.

>
>I'm fine with it going through mlxsw or just netdev tree.
>
>Przemek Kitszel (5):
>  net: dsa: replace devlink resource registration calls by devl_
>    variants
>  devlink: remove unused devlink_resource_occ_get_register() and
>    _unregister()
>  devlink: remove unused devlink_resource_register()
>  devlink: embed driver's priv data callback param into devlink_resource
>  mlxsw: spectrum_kvdl: combine devlink resource occupation getters
>
> .../net/ethernet/mellanox/mlxsw/spectrum.h    |  5 +
> include/net/devlink.h                         | 18 +---
> .../ethernet/mellanox/mlx5/core/sf/hw_table.c |  5 +-
> drivers/net/ethernet/mellanox/mlxsw/core.c    |  5 +-
> .../net/ethernet/mellanox/mlxsw/spectrum.c    | 19 ++--
> .../ethernet/mellanox/mlxsw/spectrum1_kvdl.c  | 80 +++++++--------
> .../ethernet/mellanox/mlxsw/spectrum_cnt.c    |  9 +-
> .../mellanox/mlxsw/spectrum_policer.c         |  6 +-
> .../mellanox/mlxsw/spectrum_port_range.c      |  2 +-
> .../ethernet/mellanox/mlxsw/spectrum_router.c |  4 +-
> .../ethernet/mellanox/mlxsw/spectrum_span.c   |  3 +-
> drivers/net/netdevsim/dev.c                   | 14 +--
> drivers/net/netdevsim/fib.c                   | 10 +-
> net/devlink/resource.c                        | 97 +++----------------
> net/dsa/devlink.c                             | 23 +++--
> 15 files changed, 115 insertions(+), 185 deletions(-)
>
>
>base-commit: 10a6545f0bdcbb920c6a8a033fe342111d204915
>-- 
>2.39.3
>

Re: [PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource

[Jakub Kicinski]

On Wed, 7 Aug 2024 08:49:57 +0200 Jiri Pirko wrote:
> > 	lockdep_assert_held(&devlink->lock);
> > 
> > 	resource = devlink_resource_find(devlink, NULL, resource_id);
> >-	if (WARN_ON(!resource))
> >+	if (WARN_ON(!resource || occ_priv_size > resource->priv_size))  
> 
> Very odd. You allocate a mem in devl_resource_register() and here you
> copy data to it. Why the void pointer is not enough for you? You can
> easily alloc struct in the driver and pass a pointer to it.
> 
> This is quite weird. Please don't.

The patch is a bit of a half measure, true.

Could you shed more light on the design choices for the resource API,
tho? Why the tying of objects by driver-defined IDs? It looks like 
the callback for getting resources occupancy is "added" later once 
the resource is registered? Is this some legacy of the old locking
scheme? It's quite unusual.

Re: [PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource

[Jiri Pirko]

Fri, Aug 09, 2024 at 04:41:50AM CEST, kuba@kernel.org wrote:
>On Wed, 7 Aug 2024 08:49:57 +0200 Jiri Pirko wrote:
>> > 	lockdep_assert_held(&devlink->lock);
>> > 
>> > 	resource = devlink_resource_find(devlink, NULL, resource_id);
>> >-	if (WARN_ON(!resource))
>> >+	if (WARN_ON(!resource || occ_priv_size > resource->priv_size))  
>> 
>> Very odd. You allocate a mem in devl_resource_register() and here you
>> copy data to it. Why the void pointer is not enough for you? You can
>> easily alloc struct in the driver and pass a pointer to it.
>> 
>> This is quite weird. Please don't.
>
>The patch is a bit of a half measure, true.
>
>Could you shed more light on the design choices for the resource API,
>tho? Why the tying of objects by driver-defined IDs? It looks like 

The ids are exposed all the way down to the user. They are the same
across the reboots and allow user to use the same scripts. Similar to
port index for example.


>the callback for getting resources occupancy is "added" later once 
>the resource is registered? Is this some legacy of the old locking
>scheme? It's quite unusual.

It's been some while since I reviewed this, but afaik the reason is that
the occupancy was not possible to obtain during reload, yet the resource
itself stayed during reload. This is now not a problem, since
devlink->lock protects it. I don't see why occupancy getter cannot be
put during resource register, you are correct.

Re: [PATCH net-next 5/5] mlxsw: spectrum_kvdl: combine devlink resource occupation getters

[Przemek Kitszel]

On 8/7/24 08:47, Jiri Pirko wrote:
> Tue, Aug 06, 2024 at 04:33:07PM CEST, przemyslaw.kitszel@intel.com wrote:
>> Combine spectrum1 kvdl devlink resource occupation getters into one.
>>
>> Thanks to previous commit of the series we could easily embed more than
>> just a single pointer into devlink resource.
>>
>> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
>> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> ---
>> .../net/ethernet/mellanox/mlxsw/spectrum.h    |  5 ++
>> .../net/ethernet/mellanox/mlxsw/spectrum.c    |  3 +-
>> .../ethernet/mellanox/mlxsw/spectrum1_kvdl.c  | 80 ++++++++-----------
>> 3 files changed, 41 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> index 8d3c61287696..91fe5fffa675 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> @@ -836,6 +836,11 @@ int mlxsw_sp_kvdl_alloc_count_query(struct mlxsw_sp *mlxsw_sp,
>> 				    unsigned int *p_alloc_count);
>>
>> /* spectrum1_kvdl.c */
>> +struct mlxsw_sp1_kvdl_occ_ctx {
>> +	struct mlxsw_sp1_kvdl *kvdl;
>> +	int first_part_id;
>> +	bool count_all_parts;
>> +};
>> extern const struct mlxsw_sp_kvdl_ops mlxsw_sp1_kvdl_ops;
>> int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core);
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> index 2730ae3d8fe6..3bda2b2d16f9 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> @@ -3669,7 +3669,8 @@ static int mlxsw_sp1_resources_kvd_register(struct mlxsw_core *mlxsw_core)
>> 				     linear_size,
>> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>> 				     MLXSW_SP_RESOURCE_KVD,
>> -				     &linear_size_params, sizeof(void *));
>> +				     &linear_size_params,
>> +				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> 	if (err)
>> 		return err;
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>> index ee5f12746371..a8bf052adf31 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>> @@ -292,68 +292,53 @@ static u64 mlxsw_sp1_kvdl_part_occ(struct mlxsw_sp1_kvdl_part *part)
>>
>> static u64 mlxsw_sp1_kvdl_occ_get(void *priv)
>> {
>> -	const struct mlxsw_sp1_kvdl *kvdl = priv;
>> +	struct mlxsw_sp1_kvdl_occ_ctx *ctx = priv;
>> +	bool cnt_all = ctx->count_all_parts;
>> +	int beg, end;
>> 	u64 occ = 0;
>> -	int i;
>>
>> -	for (i = 0; i < MLXSW_SP1_KVDL_PARTS_INFO_LEN; i++)
>> -		occ += mlxsw_sp1_kvdl_part_occ(kvdl->parts[i]);
>> +	beg = cnt_all ? 0 : ctx->first_part_id,
>> +	end = cnt_all ? MLXSW_SP1_KVDL_PARTS_INFO_LEN : beg + 1;
>> +	for (int i = beg; i < end; i++)
>> +		occ += mlxsw_sp1_kvdl_part_occ(ctx->kvdl->parts[i]);
>>
>> 	return occ;
> 
> I don't see the benefit, this just makes code harder to read.

You mean in general or this particular function?

Anyway, a part of motivation is to avoid dumb (even if easy to read in
isolation) getters and replace it with a one that exposes the logic.
(Now you have 2+ functions that reader needs to manually compare).

Re general oddities:
sizeof(void *) stuff just follows from the main idea, and is a temporary
solution (see this patch, it removes such).

> 
> 
>> }
>>
>> -static u64 mlxsw_sp1_kvdl_single_occ_get(void *priv)
>> -{
>> -	const struct mlxsw_sp1_kvdl *kvdl = priv;
>> -	struct mlxsw_sp1_kvdl_part *part;
>> -
>> -	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_SINGLE];
>> -	return mlxsw_sp1_kvdl_part_occ(part);
>> -}
>> -
>> -static u64 mlxsw_sp1_kvdl_chunks_occ_get(void *priv)
>> -{
>> -	const struct mlxsw_sp1_kvdl *kvdl = priv;
>> -	struct mlxsw_sp1_kvdl_part *part;
>> -
>> -	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_CHUNKS];
>> -	return mlxsw_sp1_kvdl_part_occ(part);
>> -}
>> -
>> -static u64 mlxsw_sp1_kvdl_large_chunks_occ_get(void *priv)
>> -{
>> -	const struct mlxsw_sp1_kvdl *kvdl = priv;
>> -	struct mlxsw_sp1_kvdl_part *part;
>> -
>> -	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_LARGE_CHUNKS];
>> -	return mlxsw_sp1_kvdl_part_occ(part);
>> -}
>> -
>> static int mlxsw_sp1_kvdl_init(struct mlxsw_sp *mlxsw_sp, void *priv)
>> {
>> 	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
>> -	struct mlxsw_sp1_kvdl *kvdl = priv;
>> +	struct mlxsw_sp1_kvdl_occ_ctx ctx = { priv };
>> 	int err;
>>
>> -	err = mlxsw_sp1_kvdl_parts_init(mlxsw_sp, kvdl);
>> +	err = mlxsw_sp1_kvdl_parts_init(mlxsw_sp, ctx.kvdl);
>> 	if (err)
>> 		return err;
>> -	devl_resource_occ_get_register(devlink,
>> -				       MLXSW_SP_RESOURCE_KVD_LINEAR,
>> -				       mlxsw_sp1_kvdl_occ_get,
>> -				       &kvdl, sizeof(kvdl));
>> +
>> +	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_SINGLE;
>> 	devl_resource_occ_get_register(devlink,
>> 				       MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
>> -				       mlxsw_sp1_kvdl_single_occ_get,
>> -				       &kvdl, sizeof(kvdl));
>> +				       mlxsw_sp1_kvdl_occ_get,
>> +				       &ctx, sizeof(ctx));
>> +
>> +	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_CHUNKS;
>> 	devl_resource_occ_get_register(devlink,
>> 				       MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
>> -				       mlxsw_sp1_kvdl_chunks_occ_get,
>> -				       &kvdl, sizeof(kvdl));
>> +				       mlxsw_sp1_kvdl_occ_get,
>> +				       &ctx, sizeof(ctx));
>> +
>> +	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_LARGE_CHUNKS;
>> 	devl_resource_occ_get_register(devlink,
>> 				       MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
>> -				       mlxsw_sp1_kvdl_large_chunks_occ_get,
>> -				       &kvdl, sizeof(kvdl));
>> +				       mlxsw_sp1_kvdl_occ_get,
>> +				       &ctx, sizeof(ctx));
>> +
>> +	ctx.count_all_parts = true;
>> +	devl_resource_occ_get_register(devlink,
>> +				       MLXSW_SP_RESOURCE_KVD_LINEAR,
>> +				       mlxsw_sp1_kvdl_occ_get,
>> +				       &ctx, sizeof(ctx));
>> +
>> 	return 0;
>> }
>>
>> @@ -400,7 +385,8 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
>> 				     MLXSW_SP1_KVDL_SINGLE_SIZE,
>> 				     MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
>> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>> -				     &size_params, sizeof(void *));
>> +				     &size_params,
>> +				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> 	if (err)
>> 		return err;
>>
>> @@ -411,7 +397,8 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
>> 				     MLXSW_SP1_KVDL_CHUNKS_SIZE,
>> 				     MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
>> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>> -				     &size_params, sizeof(void *));
>> +				     &size_params,
>> +				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> 	if (err)
>> 		return err;
>>
>> @@ -422,6 +409,7 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
>> 				     MLXSW_SP1_KVDL_LARGE_CHUNKS_SIZE,
>> 				     MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
>> 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>> -				     &size_params, sizeof(void *));
>> +				     &size_params,
>> +				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> 	return err;
>> }
>> -- 
>> 2.39.3
>>

Re: [PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource

[Przemek Kitszel]

On 8/9/24 13:02, Jiri Pirko wrote:
> Fri, Aug 09, 2024 at 04:41:50AM CEST, kuba@kernel.org wrote:
>> On Wed, 7 Aug 2024 08:49:57 +0200 Jiri Pirko wrote:
>>>> 	lockdep_assert_held(&devlink->lock);
>>>>
>>>> 	resource = devlink_resource_find(devlink, NULL, resource_id);
>>>> -	if (WARN_ON(!resource))
>>>> +	if (WARN_ON(!resource || occ_priv_size > resource->priv_size))
>>>
>>> Very odd. You allocate a mem in devl_resource_register() and here you
>>> copy data to it. Why the void pointer is not enough for you? You can
>>> easily alloc struct in the driver and pass a pointer to it.
>>>
>>> This is quite weird. Please don't.
>>
>> The patch is a bit of a half measure, true.

Another option to suit my wants would be to just pass resource_id to the
callbacks, would you accept that?

>>
>> Could you shed more light on the design choices for the resource API,
>> tho? Why the tying of objects by driver-defined IDs? It looks like
> 
> The ids are exposed all the way down to the user. They are the same
> across the reboots and allow user to use the same scripts. Similar to
> port index for example.
> 
> 
>> the callback for getting resources occupancy is "added" later once
>> the resource is registered? Is this some legacy of the old locking
>> scheme? It's quite unusual.

I did such review last month, many decisions really bother me :F, esp:
- whole thing is about limiting resources, driver asks HW for occupancy.

Some minor things:
- resizing request validation: parent asks children for permission;
- the function to commit the size after the reload is named
   devl_resource_size_get().

 From the user perspective, I'm going to add a setter, that will be
another mode of operation (if compared to the first thing on my complain
list):
+ there is a limit that is constant, and driver/user allocates resource
   from such pool.

> 
> It's been some while since I reviewed this, but afaik the reason is that
> the occupancy was not possible to obtain during reload, yet the resource
> itself stayed during reload. This is now not a problem, since
> devlink->lock protects it. I don't see why occupancy getter cannot be
> put during resource register, you are correct.
>
I could add that to my todo list

Re: [PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource

[Jiri Pirko]

Mon, Aug 12, 2024 at 01:50:06PM CEST, przemyslaw.kitszel@intel.com wrote:
>On 8/9/24 13:02, Jiri Pirko wrote:
>> Fri, Aug 09, 2024 at 04:41:50AM CEST, kuba@kernel.org wrote:
>> > On Wed, 7 Aug 2024 08:49:57 +0200 Jiri Pirko wrote:
>> > > > 	lockdep_assert_held(&devlink->lock);
>> > > > 
>> > > > 	resource = devlink_resource_find(devlink, NULL, resource_id);
>> > > > -	if (WARN_ON(!resource))
>> > > > +	if (WARN_ON(!resource || occ_priv_size > resource->priv_size))
>> > > 
>> > > Very odd. You allocate a mem in devl_resource_register() and here you
>> > > copy data to it. Why the void pointer is not enough for you? You can
>> > > easily alloc struct in the driver and pass a pointer to it.
>> > > 
>> > > This is quite weird. Please don't.
>> > 
>> > The patch is a bit of a half measure, true.
>
>Another option to suit my wants would be to just pass resource_id to the
>callbacks, would you accept that?

Why, the callback is registered for particular resource. Passing ID is
just redundant.


>
>> > 
>> > Could you shed more light on the design choices for the resource API,
>> > tho? Why the tying of objects by driver-defined IDs? It looks like
>> 
>> The ids are exposed all the way down to the user. They are the same
>> across the reboots and allow user to use the same scripts. Similar to
>> port index for example.
>> 
>> 
>> > the callback for getting resources occupancy is "added" later once
>> > the resource is registered? Is this some legacy of the old locking
>> > scheme? It's quite unusual.
>
>I did such review last month, many decisions really bother me :F, esp:
>- whole thing is about limiting resources, driver asks HW for occupancy.

Can you elaborate what's exactly wrong with that?


>
>Some minor things:
>- resizing request validation: parent asks children for permission;
>- the function to commit the size after the reload is named
>  devl_resource_size_get().
>
>From the user perspective, I'm going to add a setter, that will be
>another mode of operation (if compared to the first thing on my complain
>list):
>+ there is a limit that is constant, and driver/user allocates resource
>  from such pool.
>
>> 
>> It's been some while since I reviewed this, but afaik the reason is that
>> the occupancy was not possible to obtain during reload, yet the resource
>> itself stayed during reload. This is now not a problem, since
>> devlink->lock protects it. I don't see why occupancy getter cannot be
>> put during resource register, you are correct.
>> 
>I could add that to my todo list

Cool.

Re: [PATCH net-next 5/5] mlxsw: spectrum_kvdl: combine devlink resource occupation getters

[Jiri Pirko]

Mon, Aug 12, 2024 at 01:23:20PM CEST, przemyslaw.kitszel@intel.com wrote:
>On 8/7/24 08:47, Jiri Pirko wrote:
>> Tue, Aug 06, 2024 at 04:33:07PM CEST, przemyslaw.kitszel@intel.com wrote:
>> > Combine spectrum1 kvdl devlink resource occupation getters into one.
>> > 
>> > Thanks to previous commit of the series we could easily embed more than
>> > just a single pointer into devlink resource.
>> > 
>> > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
>> > Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> > ---
>> > .../net/ethernet/mellanox/mlxsw/spectrum.h    |  5 ++
>> > .../net/ethernet/mellanox/mlxsw/spectrum.c    |  3 +-
>> > .../ethernet/mellanox/mlxsw/spectrum1_kvdl.c  | 80 ++++++++-----------
>> > 3 files changed, 41 insertions(+), 47 deletions(-)
>> > 
>> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> > index 8d3c61287696..91fe5fffa675 100644
>> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
>> > @@ -836,6 +836,11 @@ int mlxsw_sp_kvdl_alloc_count_query(struct mlxsw_sp *mlxsw_sp,
>> > 				    unsigned int *p_alloc_count);
>> > 
>> > /* spectrum1_kvdl.c */
>> > +struct mlxsw_sp1_kvdl_occ_ctx {
>> > +	struct mlxsw_sp1_kvdl *kvdl;
>> > +	int first_part_id;
>> > +	bool count_all_parts;
>> > +};
>> > extern const struct mlxsw_sp_kvdl_ops mlxsw_sp1_kvdl_ops;
>> > int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core);
>> > 
>> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> > index 2730ae3d8fe6..3bda2b2d16f9 100644
>> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>> > @@ -3669,7 +3669,8 @@ static int mlxsw_sp1_resources_kvd_register(struct mlxsw_core *mlxsw_core)
>> > 				     linear_size,
>> > 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>> > 				     MLXSW_SP_RESOURCE_KVD,
>> > -				     &linear_size_params, sizeof(void *));
>> > +				     &linear_size_params,
>> > +				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> > 	if (err)
>> > 		return err;
>> > 
>> > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>> > index ee5f12746371..a8bf052adf31 100644
>> > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>> > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum1_kvdl.c
>> > @@ -292,68 +292,53 @@ static u64 mlxsw_sp1_kvdl_part_occ(struct mlxsw_sp1_kvdl_part *part)
>> > 
>> > static u64 mlxsw_sp1_kvdl_occ_get(void *priv)
>> > {
>> > -	const struct mlxsw_sp1_kvdl *kvdl = priv;
>> > +	struct mlxsw_sp1_kvdl_occ_ctx *ctx = priv;
>> > +	bool cnt_all = ctx->count_all_parts;
>> > +	int beg, end;
>> > 	u64 occ = 0;
>> > -	int i;
>> > 
>> > -	for (i = 0; i < MLXSW_SP1_KVDL_PARTS_INFO_LEN; i++)
>> > -		occ += mlxsw_sp1_kvdl_part_occ(kvdl->parts[i]);
>> > +	beg = cnt_all ? 0 : ctx->first_part_id,
>> > +	end = cnt_all ? MLXSW_SP1_KVDL_PARTS_INFO_LEN : beg + 1;
>> > +	for (int i = beg; i < end; i++)
>> > +		occ += mlxsw_sp1_kvdl_part_occ(ctx->kvdl->parts[i]);
>> > 
>> > 	return occ;
>> 
>> I don't see the benefit, this just makes code harder to read.
>
>You mean in general or this particular function?

In general.

>
>Anyway, a part of motivation is to avoid dumb (even if easy to read in
>isolation) getters and replace it with a one that exposes the logic.
>(Now you have 2+ functions that reader needs to manually compare).

I don't follow. Are you not able to implement what you need using the
existing api, or you just don't like it?


>
>Re general oddities:
>sizeof(void *) stuff just follows from the main idea, and is a temporary
>solution (see this patch, it removes such).
>
>> 
>> 
>> > }
>> > 
>> > -static u64 mlxsw_sp1_kvdl_single_occ_get(void *priv)
>> > -{
>> > -	const struct mlxsw_sp1_kvdl *kvdl = priv;
>> > -	struct mlxsw_sp1_kvdl_part *part;
>> > -
>> > -	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_SINGLE];
>> > -	return mlxsw_sp1_kvdl_part_occ(part);
>> > -}
>> > -
>> > -static u64 mlxsw_sp1_kvdl_chunks_occ_get(void *priv)
>> > -{
>> > -	const struct mlxsw_sp1_kvdl *kvdl = priv;
>> > -	struct mlxsw_sp1_kvdl_part *part;
>> > -
>> > -	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_CHUNKS];
>> > -	return mlxsw_sp1_kvdl_part_occ(part);
>> > -}
>> > -
>> > -static u64 mlxsw_sp1_kvdl_large_chunks_occ_get(void *priv)
>> > -{
>> > -	const struct mlxsw_sp1_kvdl *kvdl = priv;
>> > -	struct mlxsw_sp1_kvdl_part *part;
>> > -
>> > -	part = kvdl->parts[MLXSW_SP1_KVDL_PART_ID_LARGE_CHUNKS];
>> > -	return mlxsw_sp1_kvdl_part_occ(part);
>> > -}
>> > -
>> > static int mlxsw_sp1_kvdl_init(struct mlxsw_sp *mlxsw_sp, void *priv)
>> > {
>> > 	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
>> > -	struct mlxsw_sp1_kvdl *kvdl = priv;
>> > +	struct mlxsw_sp1_kvdl_occ_ctx ctx = { priv };
>> > 	int err;
>> > 
>> > -	err = mlxsw_sp1_kvdl_parts_init(mlxsw_sp, kvdl);
>> > +	err = mlxsw_sp1_kvdl_parts_init(mlxsw_sp, ctx.kvdl);
>> > 	if (err)
>> > 		return err;
>> > -	devl_resource_occ_get_register(devlink,
>> > -				       MLXSW_SP_RESOURCE_KVD_LINEAR,
>> > -				       mlxsw_sp1_kvdl_occ_get,
>> > -				       &kvdl, sizeof(kvdl));
>> > +
>> > +	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_SINGLE;
>> > 	devl_resource_occ_get_register(devlink,
>> > 				       MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
>> > -				       mlxsw_sp1_kvdl_single_occ_get,
>> > -				       &kvdl, sizeof(kvdl));
>> > +				       mlxsw_sp1_kvdl_occ_get,
>> > +				       &ctx, sizeof(ctx));
>> > +
>> > +	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_CHUNKS;
>> > 	devl_resource_occ_get_register(devlink,
>> > 				       MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
>> > -				       mlxsw_sp1_kvdl_chunks_occ_get,
>> > -				       &kvdl, sizeof(kvdl));
>> > +				       mlxsw_sp1_kvdl_occ_get,
>> > +				       &ctx, sizeof(ctx));
>> > +
>> > +	ctx.first_part_id = MLXSW_SP1_KVDL_PART_ID_LARGE_CHUNKS;
>> > 	devl_resource_occ_get_register(devlink,
>> > 				       MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
>> > -				       mlxsw_sp1_kvdl_large_chunks_occ_get,
>> > -				       &kvdl, sizeof(kvdl));
>> > +				       mlxsw_sp1_kvdl_occ_get,
>> > +				       &ctx, sizeof(ctx));
>> > +
>> > +	ctx.count_all_parts = true;
>> > +	devl_resource_occ_get_register(devlink,
>> > +				       MLXSW_SP_RESOURCE_KVD_LINEAR,
>> > +				       mlxsw_sp1_kvdl_occ_get,
>> > +				       &ctx, sizeof(ctx));
>> > +
>> > 	return 0;
>> > }
>> > 
>> > @@ -400,7 +385,8 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
>> > 				     MLXSW_SP1_KVDL_SINGLE_SIZE,
>> > 				     MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
>> > 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>> > -				     &size_params, sizeof(void *));
>> > +				     &size_params,
>> > +				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> > 	if (err)
>> > 		return err;
>> > 
>> > @@ -411,7 +397,8 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
>> > 				     MLXSW_SP1_KVDL_CHUNKS_SIZE,
>> > 				     MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
>> > 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>> > -				     &size_params, sizeof(void *));
>> > +				     &size_params,
>> > +				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> > 	if (err)
>> > 		return err;
>> > 
>> > @@ -422,6 +409,7 @@ int mlxsw_sp1_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
>> > 				     MLXSW_SP1_KVDL_LARGE_CHUNKS_SIZE,
>> > 				     MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
>> > 				     MLXSW_SP_RESOURCE_KVD_LINEAR,
>> > -				     &size_params, sizeof(void *));
>> > +				     &size_params,
>> > +				     sizeof(struct mlxsw_sp1_kvdl_occ_ctx));
>> > 	return err;
>> > }
>> > -- 
>> > 2.39.3
>> > 
>

Re: [PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource

[Przemek Kitszel]

On 8/12/24 17:00, Jiri Pirko wrote:
> Mon, Aug 12, 2024 at 01:50:06PM CEST, przemyslaw.kitszel@intel.com wrote:
>> On 8/9/24 13:02, Jiri Pirko wrote:
>>> Fri, Aug 09, 2024 at 04:41:50AM CEST, kuba@kernel.org wrote:
>>>> On Wed, 7 Aug 2024 08:49:57 +0200 Jiri Pirko wrote:
>>>>>> 	lockdep_assert_held(&devlink->lock);
>>>>>>
>>>>>> 	resource = devlink_resource_find(devlink, NULL, resource_id);
>>>>>> -	if (WARN_ON(!resource))
>>>>>> +	if (WARN_ON(!resource || occ_priv_size > resource->priv_size))
>>>>>
>>>>> Very odd. You allocate a mem in devl_resource_register() and here you
>>>>> copy data to it. Why the void pointer is not enough for you? You can
>>>>> easily alloc struct in the driver and pass a pointer to it.
>>>>>
>>>>> This is quite weird. Please don't.
>>>>
>>>> The patch is a bit of a half measure, true.
>>
>> Another option to suit my wants would be to just pass resource_id to the
>> callbacks, would you accept that?
> 
> Why, the callback is registered for particular resource. Passing ID is
> just redundant.

Yet enables one to nicely combine all occ getters/setters for given
resource group. It is also straightforward (compared to this series).
You are right it is not absolutely necessary, but does not hurt and
improves thing (this time I will don't update mlxsw just to have
consumer though, will just post later - as this is not so controversial,
I hope).

> 
> 
>>
>>>>
>>>> Could you shed more light on the design choices for the resource API,
>>>> tho? Why the tying of objects by driver-defined IDs? It looks like
>>>
>>> The ids are exposed all the way down to the user. They are the same
>>> across the reboots and allow user to use the same scripts. Similar to
>>> port index for example.
>>>
>>>
>>>> the callback for getting resources occupancy is "added" later once
>>>> the resource is registered? Is this some legacy of the old locking
>>>> scheme? It's quite unusual.
>>
>> I did such review last month, many decisions really bother me :F, esp:
>> - whole thing is about limiting resources, driver asks HW for occupancy.
> 
> Can you elaborate what's exactly wrong with that?

Typical way to think about resources is "there are X foos" (resource
register time), "give me one foo" (later, on user request). Users could
be heterogeneous, such as VFs and PFs, and resource pool shared over.
This is what I have for (different sizes of) RSS contexes.
(Limit is constant, need to "get*" resources by one at a time, so driver
knows occupancy and arbitrages usage requests).

"get*" == set usage to be increased by one

> 
> 
>>
>> Some minor things:
>> - resizing request validation: parent asks children for permission;
>> - the function to commit the size after the reload is named
>>   devl_resource_size_get().
>>
>>From the user perspective, I'm going to add a setter, that will be
>> another mode of operation (if compared to the first thing on my complain
>> list):
>> + there is a limit that is constant, and driver/user allocates resource
>>   from such pool.
>>
>>>
>>> It's been some while since I reviewed this, but afaik the reason is that
>>> the occupancy was not possible to obtain during reload, yet the resource
>>> itself stayed during reload. This is now not a problem, since
>>> devlink->lock protects it. I don't see why occupancy getter cannot be
>>> put during resource register, you are correct.
>>>
>> I could add that to my todo list
> 
> Cool.

I guess no one cared about it yet, as resource register and occ getter
register is much separated in code space (to the point of being in
different file).

Re: [PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource

[Jiri Pirko]

Tue, Aug 13, 2024 at 05:45:47AM CEST, przemyslaw.kitszel@intel.com wrote:
>On 8/12/24 17:00, Jiri Pirko wrote:
>> Mon, Aug 12, 2024 at 01:50:06PM CEST, przemyslaw.kitszel@intel.com wrote:
>> > On 8/9/24 13:02, Jiri Pirko wrote:
>> > > Fri, Aug 09, 2024 at 04:41:50AM CEST, kuba@kernel.org wrote:
>> > > > On Wed, 7 Aug 2024 08:49:57 +0200 Jiri Pirko wrote:
>> > > > > > 	lockdep_assert_held(&devlink->lock);
>> > > > > > 
>> > > > > > 	resource = devlink_resource_find(devlink, NULL, resource_id);
>> > > > > > -	if (WARN_ON(!resource))
>> > > > > > +	if (WARN_ON(!resource || occ_priv_size > resource->priv_size))
>> > > > > 
>> > > > > Very odd. You allocate a mem in devl_resource_register() and here you
>> > > > > copy data to it. Why the void pointer is not enough for you? You can
>> > > > > easily alloc struct in the driver and pass a pointer to it.
>> > > > > 
>> > > > > This is quite weird. Please don't.
>> > > > 
>> > > > The patch is a bit of a half measure, true.
>> > 
>> > Another option to suit my wants would be to just pass resource_id to the
>> > callbacks, would you accept that?
>> 
>> Why, the callback is registered for particular resource. Passing ID is
>> just redundant.
>
>Yet enables one to nicely combine all occ getters/setters for given

I don't see the benefit, sorry :/

>resource group. It is also straightforward (compared to this series).
>You are right it is not absolutely necessary, but does not hurt and
>improves thing (this time I will don't update mlxsw just to have
>consumer though, will just post later - as this is not so controversial,
>I hope).
>
>> 
>> 
>> > 
>> > > > 
>> > > > Could you shed more light on the design choices for the resource API,
>> > > > tho? Why the tying of objects by driver-defined IDs? It looks like
>> > > 
>> > > The ids are exposed all the way down to the user. They are the same
>> > > across the reboots and allow user to use the same scripts. Similar to
>> > > port index for example.
>> > > 
>> > > 
>> > > > the callback for getting resources occupancy is "added" later once
>> > > > the resource is registered? Is this some legacy of the old locking
>> > > > scheme? It's quite unusual.
>> > 
>> > I did such review last month, many decisions really bother me :F, esp:
>> > - whole thing is about limiting resources, driver asks HW for occupancy.
>> 
>> Can you elaborate what's exactly wrong with that?
>
>Typical way to think about resources is "there are X foos" (resource
>register time), "give me one foo" (later, on user request). Users could
>be heterogeneous, such as VFs and PFs, and resource pool shared over.
>This is what I have for (different sizes of) RSS contexes.
>(Limit is constant, need to "get*" resources by one at a time, so driver
>knows occupancy and arbitrages usage requests).
>
>"get*" == set usage to be increased by one
>
>> 
>> 
>> > 
>> > Some minor things:
>> > - resizing request validation: parent asks children for permission;
>> > - the function to commit the size after the reload is named
>> >   devl_resource_size_get().
>> > 
>> > From the user perspective, I'm going to add a setter, that will be
>> > another mode of operation (if compared to the first thing on my complain
>> > list):
>> > + there is a limit that is constant, and driver/user allocates resource
>> >   from such pool.
>> > 
>> > > 
>> > > It's been some while since I reviewed this, but afaik the reason is that
>> > > the occupancy was not possible to obtain during reload, yet the resource
>> > > itself stayed during reload. This is now not a problem, since
>> > > devlink->lock protects it. I don't see why occupancy getter cannot be
>> > > put during resource register, you are correct.
>> > > 
>> > I could add that to my todo list
>> 
>> Cool.
>
>I guess no one cared about it yet, as resource register and occ getter
>register is much separated in code space (to the point of being in
>different file).

Re: [PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource

[kernel test robot]

Hello,

kernel test robot noticed "WARNING:at_net/devlink/resource.c:#devl_resource_occ_get_register" on:

commit: ee141889828a13aa804849ebda6af39d6a8ebf87 ("[PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource")
url: https://github.com/intel-lab-lkp/linux/commits/Przemek-Kitszel/net-dsa-replace-devlink-resource-registration-calls-by-devl_-variants/20240806-224519
patch link: https://lore.kernel.org/all/20240806143307.14839-5-przemyslaw.kitszel@intel.com/
patch subject: [PATCH net-next 4/5] devlink: embed driver's priv data callback param into devlink_resource

in testcase: kernel-selftests-bpf
version: 
with following parameters:

	group: net



compiler: gcc-12
test machine: 36 threads 1 sockets Intel(R) Core(TM) i9-10980XE CPU @ 3.00GHz (Cascade Lake) with 32G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)



If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202408161558.b256545a-lkp@intel.com


kern  :warn  : [   81.007570] ------------[ cut here ]------------
kern :warn : [   81.008076] WARNING: CPU: 18 PID: 7588 at net/devlink/resource.c:484 devl_resource_occ_get_register (net/devlink/resource.c:484) 
kern  :warn  : [   81.009035] Modules linked in: netdevsim macsec vxlan 8021q garp mrp bridge stp llc ip6_gre ip_gre gre cls_u32 sch_htb macvtap macvlan tap dummy tun nf_conntrack_broadcast fou ip_tunnel ip6_udp_tunnel udp_tunnel rpcsec_gss_krb5 auth_rpcgss nfsv4 dns_resolver netconsole openvswitch nf_conncount nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 psample ipmi_devintf ipmi_msghandler binfmt_misc intel_rapl_msr intel_rapl_common btrfs skx_edac_common nfit libnvdimm x86_pkg_temp_thermal intel_powerclamp blake2b_generic coretemp xor zstd_compress kvm_intel kvm raid6_pq crct10dif_pclmul crc32_pclmul libcrc32c crc32c_intel ghash_clmulni_intel sha512_ssse3 rapl intel_cstate ahci libahci wmi_bmof intel_wmi_thunderbolt nvme mxm_wmi mei_me libata intel_uncore nvme_core mei ioatdma wdat_wdt dca wmi drm dm_mod fuse ip_tables x_tables sch_fq_codel [last unloaded: test_bpf]
kern  :warn  : [   81.015492] CPU: 18 UID: 0 PID: 7588 Comm: rtnetlink.sh Tainted: G S                 6.11.0-rc1-00254-gee141889828a #1
kern  :warn  : [   81.016497] Tainted: [S]=CPU_OUT_OF_SPEC
kern  :warn  : [   81.016936] Hardware name: Gigabyte Technology Co., Ltd. X299 UD4 Pro/X299 UD4 Pro-CF, BIOS F8a 04/27/2021
kern :warn : [   81.017853] RIP: 0010:devl_resource_occ_get_register (net/devlink/resource.c:484) 
kern :warn : [ 81.018451] Code: b0 4d 89 c5 49 3b 5e 08 74 35 48 89 da 4c 89 f6 48 89 ef e8 34 fa ff ff 48 85 c0 75 59 49 8b 46 50 4c 8d 70 b0 49 39 c4 75 da <0f> 0b 48 83 c4 08 5b 5d 41 5c 41 5d 41 5e 41 5f c3 cc cc cc cc 4d
All code
========
   0:	b0 4d                	mov    $0x4d,%al
   2:	89 c5                	mov    %eax,%ebp
   4:	49 3b 5e 08          	cmp    0x8(%r14),%rbx
   8:	74 35                	je     0x3f
   a:	48 89 da             	mov    %rbx,%rdx
   d:	4c 89 f6             	mov    %r14,%rsi
  10:	48 89 ef             	mov    %rbp,%rdi
  13:	e8 34 fa ff ff       	call   0xfffffffffffffa4c
  18:	48 85 c0             	test   %rax,%rax
  1b:	75 59                	jne    0x76
  1d:	49 8b 46 50          	mov    0x50(%r14),%rax
  21:	4c 8d 70 b0          	lea    -0x50(%rax),%r14
  25:	49 39 c4             	cmp    %rax,%r12
  28:	75 da                	jne    0x4
  2a:*	0f 0b                	ud2		<-- trapping instruction
  2c:	48 83 c4 08          	add    $0x8,%rsp
  30:	5b                   	pop    %rbx
  31:	5d                   	pop    %rbp
  32:	41 5c                	pop    %r12
  34:	41 5d                	pop    %r13
  36:	41 5e                	pop    %r14
  38:	41 5f                	pop    %r15
  3a:	c3                   	ret
  3b:	cc                   	int3
  3c:	cc                   	int3
  3d:	cc                   	int3
  3e:	cc                   	int3
  3f:	4d                   	rex.WRB

Code starting with the faulting instruction
===========================================
   0:	0f 0b                	ud2
   2:	48 83 c4 08          	add    $0x8,%rsp
   6:	5b                   	pop    %rbx
   7:	5d                   	pop    %rbp
   8:	41 5c                	pop    %r12
   a:	41 5d                	pop    %r13
   c:	41 5e                	pop    %r14
   e:	41 5f                	pop    %r15
  10:	c3                   	ret
  11:	cc                   	int3
  12:	cc                   	int3
  13:	cc                   	int3
  14:	cc                   	int3
  15:	4d                   	rex.WRB
kern  :warn  : [   81.020118] RSP: 0018:ffffc90001a53b70 EFLAGS: 00010293
kern  :warn  : [   81.020662] RAX: ffff8888753fd400 RBX: 0000000000000002 RCX: ffff888894939048
kern  :warn  : [   81.021374] RDX: ffff8888753fd060 RSI: ffff8888753fd000 RDI: ffff888894939048
kern  :warn  : [   81.022085] RBP: ffff888894939000 R08: 0000000000000008 R09: 0000000000000000
kern  :warn  : [   81.022795] R10: ffff8888753fd400 R11: 000000008b95c1e3 R12: ffff888894939048
kern  :warn  : [   81.023510] R13: 0000000000000008 R14: ffff8888753fd400 R15: ffffffffc0e37800
kern  :warn  : [   81.024222] FS:  00007f0d9d0e9c40(0000) GS:ffff88889f500000(0000) knlGS:0000000000000000
kern  :warn  : [   81.025012] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kern  :warn  : [   81.025599] CR2: 00005612292a7cd0 CR3: 0000000870ece002 CR4: 00000000003706f0
kern  :warn  : [   81.026311] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
kern  :warn  : [   81.027022] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
kern  :warn  : [   81.027731] Call Trace:
kern  :warn  : [   81.028052]  <TASK>
kern :warn : [   81.028344] ? __warn (kernel/panic.c:735) 
kern :warn : [   81.028728] ? devl_resource_occ_get_register (net/devlink/resource.c:484) 
kern :warn : [   81.029275] ? report_bug (lib/bug.c:180 lib/bug.c:219) 
kern :warn : [   81.029694] ? handle_bug (arch/x86/kernel/traps.c:239) 
kern :warn : [   81.030102] ? exc_invalid_op (arch/x86/kernel/traps.c:260 (discriminator 1)) 
kern :warn : [   81.030534] ? asm_exc_invalid_op (arch/x86/include/asm/idtentry.h:621) 
kern :warn : [   81.031000] ? __pfx_nsim_fib_ipv4_resource_occ_get (drivers/net/netdevsim/fib.c:1422) netdevsim
kern :warn : [   81.031689] ? devl_resource_occ_get_register (net/devlink/resource.c:484) 
kern :warn : [   81.032238] nsim_fib_create (drivers/net/netdevsim/fib.c:1606) netdevsim
kern :warn : [   81.032765] nsim_drv_probe (drivers/net/netdevsim/dev.c:1583) netdevsim
kern :warn : [   81.033287] really_probe (drivers/base/dd.c:578 drivers/base/dd.c:657) 
kern :warn : [   81.033698] ? __pfx___device_attach_driver (drivers/base/dd.c:921) 
kern :warn : [   81.034232] __driver_probe_device (drivers/base/dd.c:799) 
kern :warn : [   81.034707] driver_probe_device (drivers/base/dd.c:830) 
kern :warn : [   81.035164] __device_attach_driver (drivers/base/dd.c:958) 
kern :warn : [   81.035646] bus_for_each_drv (drivers/base/bus.c:457) 
kern :warn : [   81.036083] __device_attach (drivers/base/dd.c:1031) 
kern :warn : [   81.036514] bus_probe_device (drivers/base/bus.c:532) 
kern :warn : [   81.036946] device_add (drivers/base/core.c:3686) 
kern :warn : [   81.037353] new_device_store (drivers/net/netdevsim/bus.c:443 drivers/net/netdevsim/bus.c:173) netdevsim
kern :warn : [   81.037886] kernfs_fop_write_iter (fs/kernfs/file.c:334) 
kern :warn : [   81.038373] vfs_write (fs/read_write.c:497 fs/read_write.c:590) 
kern :warn : [   81.038772] ksys_write (fs/read_write.c:644) 
kern :warn : [   81.039164] do_syscall_64 (arch/x86/entry/common.c:52 arch/x86/entry/common.c:83) 
kern :warn : [   81.039582] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130) 
kern  :warn  : [   81.040116] RIP: 0033:0x7f0d9d1e3240
kern :warn : [ 81.040525] Code: 40 00 48 8b 15 c1 9b 0d 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 80 3d a1 23 0e 00 00 74 17 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 48 83 ec 28 48 89
All code
========
   0:	40 00 48 8b          	rex add %cl,-0x75(%rax)
   4:	15 c1 9b 0d 00       	adc    $0xd9bc1,%eax
   9:	f7 d8                	neg    %eax
   b:	64 89 02             	mov    %eax,%fs:(%rdx)
   e:	48 c7 c0 ff ff ff ff 	mov    $0xffffffffffffffff,%rax
  15:	eb b7                	jmp    0xffffffffffffffce
  17:	0f 1f 00             	nopl   (%rax)
  1a:	80 3d a1 23 0e 00 00 	cmpb   $0x0,0xe23a1(%rip)        # 0xe23c2
  21:	74 17                	je     0x3a
  23:	b8 01 00 00 00       	mov    $0x1,%eax
  28:	0f 05                	syscall
  2a:*	48 3d 00 f0 ff ff    	cmp    $0xfffffffffffff000,%rax		<-- trapping instruction
  30:	77 58                	ja     0x8a
  32:	c3                   	ret
  33:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
  3a:	48 83 ec 28          	sub    $0x28,%rsp
  3e:	48                   	rex.W
  3f:	89                   	.byte 0x89

Code starting with the faulting instruction
===========================================
   0:	48 3d 00 f0 ff ff    	cmp    $0xfffffffffffff000,%rax
   6:	77 58                	ja     0x60
   8:	c3                   	ret
   9:	0f 1f 80 00 00 00 00 	nopl   0x0(%rax)
  10:	48 83 ec 28          	sub    $0x28,%rsp
  14:	48                   	rex.W
  15:	89                   	.byte 0x89


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240816/202408161558.b256545a-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki