[patch net] devlink: convert occ_get op to separate registration

[patch net] devlink: convert occ_get op to separate registration

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

This resolves race during initialization where the resources with
ops are registered before driver and the structures used by occ_get
op is initialized. So keep occ_get callbacks registered only when
all structs are initialized.

The example flows, as it is in mlxsw:
1) driver load/asic probe:
   mlxsw_core
      -> mlxsw_sp_resources_register
        -> mlxsw_sp_kvdl_resources_register
          -> devlink_resource_register IDX
   mlxsw_spectrum
      -> mlxsw_sp_kvdl_init
        -> mlxsw_sp_kvdl_parts_init
          -> mlxsw_sp_kvdl_part_init
            -> devlink_resource_size_get IDX (to get the current setup
                                              size from devlink)
        -> devlink_resource_occ_get_register IDX (register current
                                                  occupancy getter)
2) reload triggered by devlink command:
  -> mlxsw_devlink_core_bus_device_reload
    -> mlxsw_sp_fini
      -> mlxsw_sp_kvdl_fini
	-> devlink_resource_occ_get_unregister IDX
    (struct mlxsw_sp *mlxsw_sp is freed at this point, call to occ get
     which is using mlxsw_sp would cause use-after free)
    -> mlxsw_sp_init
      -> mlxsw_sp_kvdl_init
        -> mlxsw_sp_kvdl_parts_init
          -> mlxsw_sp_kvdl_part_init
            -> devlink_resource_size_get IDX (to get the current setup
                                              size from devlink)
        -> devlink_resource_occ_get_register IDX (register current
                                                  occupancy getter)

Fixes: d9f9b9a4d05f ("devlink: Add support for resource abstraction")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- rebased on top or netdevsim changes
- improved description
---
 drivers/net/ethernet/mellanox/mlxsw/spectrum.c     | 24 ++-----
 drivers/net/ethernet/mellanox/mlxsw/spectrum.h     |  1 -
 .../net/ethernet/mellanox/mlxsw/spectrum_kvdl.c    | 67 ++++++++++++--------
 drivers/net/netdevsim/devlink.c                    | 65 +++++++++----------
 include/net/devlink.h                              | 40 ++++++++----
 net/core/devlink.c                                 | 74 +++++++++++++++++++---
 6 files changed, 165 insertions(+), 106 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 53fffd09d133..ca38a30fbe91 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3805,18 +3805,6 @@ static const struct mlxsw_config_profile mlxsw_sp_config_profile = {
 	},
 };
 
-static u64 mlxsw_sp_resource_kvd_linear_occ_get(struct devlink *devlink)
-{
-	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
-
-	return mlxsw_sp_kvdl_occ_get(mlxsw_sp);
-}
-
-static const struct devlink_resource_ops mlxsw_sp_resource_kvd_linear_ops = {
-	.occ_get = mlxsw_sp_resource_kvd_linear_occ_get,
-};
-
 static void
 mlxsw_sp_resource_size_params_prepare(struct mlxsw_core *mlxsw_core,
 				      struct devlink_resource_size_params *kvd_size_params,
@@ -3877,8 +3865,7 @@ static int mlxsw_sp_resources_register(struct mlxsw_core *mlxsw_core)
 	err = devlink_resource_register(devlink, MLXSW_SP_RESOURCE_NAME_KVD,
 					kvd_size, MLXSW_SP_RESOURCE_KVD,
 					DEVLINK_RESOURCE_ID_PARENT_TOP,
-					&kvd_size_params,
-					NULL);
+					&kvd_size_params);
 	if (err)
 		return err;
 
@@ -3887,8 +3874,7 @@ static int mlxsw_sp_resources_register(struct mlxsw_core *mlxsw_core)
 					linear_size,
 					MLXSW_SP_RESOURCE_KVD_LINEAR,
 					MLXSW_SP_RESOURCE_KVD,
-					&linear_size_params,
-					&mlxsw_sp_resource_kvd_linear_ops);
+					&linear_size_params);
 	if (err)
 		return err;
 
@@ -3905,8 +3891,7 @@ static int mlxsw_sp_resources_register(struct mlxsw_core *mlxsw_core)
 					double_size,
 					MLXSW_SP_RESOURCE_KVD_HASH_DOUBLE,
 					MLXSW_SP_RESOURCE_KVD,
-					&hash_double_size_params,
-					NULL);
+					&hash_double_size_params);
 	if (err)
 		return err;
 
@@ -3915,8 +3900,7 @@ static int mlxsw_sp_resources_register(struct mlxsw_core *mlxsw_core)
 					single_size,
 					MLXSW_SP_RESOURCE_KVD_HASH_SINGLE,
 					MLXSW_SP_RESOURCE_KVD,
-					&hash_single_size_params,
-					NULL);
+					&hash_single_size_params);
 	if (err)
 		return err;
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
index 82820ba43728..804d4d2c8031 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.h
@@ -442,7 +442,6 @@ void mlxsw_sp_kvdl_free(struct mlxsw_sp *mlxsw_sp, int entry_index);
 int mlxsw_sp_kvdl_alloc_size_query(struct mlxsw_sp *mlxsw_sp,
 				   unsigned int entry_count,
 				   unsigned int *p_alloc_size);
-u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp);
 int mlxsw_sp_kvdl_resources_register(struct mlxsw_core *mlxsw_core);
 
 struct mlxsw_sp_acl_rule_info {
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
index 8796db44dcc3..fe4327f547d2 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
@@ -315,8 +315,9 @@ static u64 mlxsw_sp_kvdl_part_occ(struct mlxsw_sp_kvdl_part *part)
 	return occ;
 }
 
-u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp)
+static u64 mlxsw_sp_kvdl_occ_get(void *priv)
 {
+	const struct mlxsw_sp *mlxsw_sp = priv;
 	u64 occ = 0;
 	int i;
 
@@ -326,48 +327,33 @@ u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp)
 	return occ;
 }
 
-static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink)
+static u64 mlxsw_sp_kvdl_single_occ_get(void *priv)
 {
-	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
+	const struct mlxsw_sp *mlxsw_sp = priv;
 	struct mlxsw_sp_kvdl_part *part;
 
 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_SINGLE];
 	return mlxsw_sp_kvdl_part_occ(part);
 }
 
-static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink)
+static u64 mlxsw_sp_kvdl_chunks_occ_get(void *priv)
 {
-	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
+	const struct mlxsw_sp *mlxsw_sp = priv;
 	struct mlxsw_sp_kvdl_part *part;
 
 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_CHUNKS];
 	return mlxsw_sp_kvdl_part_occ(part);
 }
 
-static u64 mlxsw_sp_kvdl_large_chunks_occ_get(struct devlink *devlink)
+static u64 mlxsw_sp_kvdl_large_chunks_occ_get(void *priv)
 {
-	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
+	const struct mlxsw_sp *mlxsw_sp = priv;
 	struct mlxsw_sp_kvdl_part *part;
 
 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_LARGE_CHUNKS];
 	return mlxsw_sp_kvdl_part_occ(part);
 }
 
-static const struct devlink_resource_ops mlxsw_sp_kvdl_single_ops = {
-	.occ_get = mlxsw_sp_kvdl_single_occ_get,
-};
-
-static const struct devlink_resource_ops mlxsw_sp_kvdl_chunks_ops = {
-	.occ_get = mlxsw_sp_kvdl_chunks_occ_get,
-};
-
-static const struct devlink_resource_ops mlxsw_sp_kvdl_chunks_large_ops = {
-	.occ_get = mlxsw_sp_kvdl_large_chunks_occ_get,
-};
-
 int mlxsw_sp_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
 {
 	struct devlink *devlink = priv_to_devlink(mlxsw_core);
@@ -386,8 +372,7 @@ int mlxsw_sp_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
 					MLXSW_SP_KVDL_SINGLE_SIZE,
 					MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
 					MLXSW_SP_RESOURCE_KVD_LINEAR,
-					&size_params,
-					&mlxsw_sp_kvdl_single_ops);
+					&size_params);
 	if (err)
 		return err;
 
@@ -398,8 +383,7 @@ int mlxsw_sp_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
 					MLXSW_SP_KVDL_CHUNKS_SIZE,
 					MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
 					MLXSW_SP_RESOURCE_KVD_LINEAR,
-					&size_params,
-					&mlxsw_sp_kvdl_chunks_ops);
+					&size_params);
 	if (err)
 		return err;
 
@@ -410,13 +394,13 @@ int mlxsw_sp_kvdl_resources_register(struct mlxsw_core *mlxsw_core)
 					MLXSW_SP_KVDL_LARGE_CHUNKS_SIZE,
 					MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
 					MLXSW_SP_RESOURCE_KVD_LINEAR,
-					&size_params,
-					&mlxsw_sp_kvdl_chunks_large_ops);
+					&size_params);
 	return err;
 }
 
 int mlxsw_sp_kvdl_init(struct mlxsw_sp *mlxsw_sp)
 {
+	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
 	struct mlxsw_sp_kvdl *kvdl;
 	int err;
 
@@ -429,6 +413,23 @@ int mlxsw_sp_kvdl_init(struct mlxsw_sp *mlxsw_sp)
 	if (err)
 		goto err_kvdl_parts_init;
 
+	devlink_resource_occ_get_register(devlink,
+					  MLXSW_SP_RESOURCE_KVD_LINEAR,
+					  mlxsw_sp_kvdl_occ_get,
+					  mlxsw_sp);
+	devlink_resource_occ_get_register(devlink,
+					  MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE,
+					  mlxsw_sp_kvdl_single_occ_get,
+					  mlxsw_sp);
+	devlink_resource_occ_get_register(devlink,
+					  MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS,
+					  mlxsw_sp_kvdl_chunks_occ_get,
+					  mlxsw_sp);
+	devlink_resource_occ_get_register(devlink,
+					  MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS,
+					  mlxsw_sp_kvdl_large_chunks_occ_get,
+					  mlxsw_sp);
+
 	return 0;
 
 err_kvdl_parts_init:
@@ -438,6 +439,16 @@ int mlxsw_sp_kvdl_init(struct mlxsw_sp *mlxsw_sp)
 
 void mlxsw_sp_kvdl_fini(struct mlxsw_sp *mlxsw_sp)
 {
+	struct devlink *devlink = priv_to_devlink(mlxsw_sp->core);
+
+	devlink_resource_occ_get_unregister(devlink,
+					    MLXSW_SP_RESOURCE_KVD_LINEAR_LARGE_CHUNKS);
+	devlink_resource_occ_get_unregister(devlink,
+					    MLXSW_SP_RESOURCE_KVD_LINEAR_CHUNKS);
+	devlink_resource_occ_get_unregister(devlink,
+					    MLXSW_SP_RESOURCE_KVD_LINEAR_SINGLE);
+	devlink_resource_occ_get_unregister(devlink,
+					    MLXSW_SP_RESOURCE_KVD_LINEAR);
 	mlxsw_sp_kvdl_parts_fini(mlxsw_sp);
 	kfree(mlxsw_sp->kvdl);
 }
diff --git a/drivers/net/netdevsim/devlink.c b/drivers/net/netdevsim/devlink.c
index 1dba47936456..bef7db5d129a 100644
--- a/drivers/net/netdevsim/devlink.c
+++ b/drivers/net/netdevsim/devlink.c
@@ -30,52 +30,36 @@ static struct net *nsim_devlink_net(struct devlink *devlink)
 
 /* IPv4
  */
-static u64 nsim_ipv4_fib_resource_occ_get(struct devlink *devlink)
+static u64 nsim_ipv4_fib_resource_occ_get(void *priv)
 {
-	struct net *net = nsim_devlink_net(devlink);
+	struct net *net = priv;
 
 	return nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB, false);
 }
 
-static struct devlink_resource_ops nsim_ipv4_fib_res_ops = {
-	.occ_get = nsim_ipv4_fib_resource_occ_get,
-};
-
-static u64 nsim_ipv4_fib_rules_res_occ_get(struct devlink *devlink)
+static u64 nsim_ipv4_fib_rules_res_occ_get(void *priv)
 {
-	struct net *net = nsim_devlink_net(devlink);
+	struct net *net = priv;
 
 	return nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB_RULES, false);
 }
 
-static struct devlink_resource_ops nsim_ipv4_fib_rules_res_ops = {
-	.occ_get = nsim_ipv4_fib_rules_res_occ_get,
-};
-
 /* IPv6
  */
-static u64 nsim_ipv6_fib_resource_occ_get(struct devlink *devlink)
+static u64 nsim_ipv6_fib_resource_occ_get(void *priv)
 {
-	struct net *net = nsim_devlink_net(devlink);
+	struct net *net = priv;
 
 	return nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB, false);
 }
 
-static struct devlink_resource_ops nsim_ipv6_fib_res_ops = {
-	.occ_get = nsim_ipv6_fib_resource_occ_get,
-};
-
-static u64 nsim_ipv6_fib_rules_res_occ_get(struct devlink *devlink)
+static u64 nsim_ipv6_fib_rules_res_occ_get(void *priv)
 {
-	struct net *net = nsim_devlink_net(devlink);
+	struct net *net = priv;
 
 	return nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB_RULES, false);
 }
 
-static struct devlink_resource_ops nsim_ipv6_fib_rules_res_ops = {
-	.occ_get = nsim_ipv6_fib_rules_res_occ_get,
-};
-
 static int devlink_resources_register(struct devlink *devlink)
 {
 	struct devlink_resource_size_params params = {
@@ -91,7 +75,7 @@ static int devlink_resources_register(struct devlink *devlink)
 	err = devlink_resource_register(devlink, "IPv4", (u64)-1,
 					NSIM_RESOURCE_IPV4,
 					DEVLINK_RESOURCE_ID_PARENT_TOP,
-					&params, NULL);
+					&params);
 	if (err) {
 		pr_err("Failed to register IPv4 top resource\n");
 		goto out;
@@ -100,8 +84,7 @@ static int devlink_resources_register(struct devlink *devlink)
 	n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB, true);
 	err = devlink_resource_register(devlink, "fib", n,
 					NSIM_RESOURCE_IPV4_FIB,
-					NSIM_RESOURCE_IPV4,
-					&params, &nsim_ipv4_fib_res_ops);
+					NSIM_RESOURCE_IPV4, &params);
 	if (err) {
 		pr_err("Failed to register IPv4 FIB resource\n");
 		return err;
@@ -110,8 +93,7 @@ static int devlink_resources_register(struct devlink *devlink)
 	n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB_RULES, true);
 	err = devlink_resource_register(devlink, "fib-rules", n,
 					NSIM_RESOURCE_IPV4_FIB_RULES,
-					NSIM_RESOURCE_IPV4,
-					&params, &nsim_ipv4_fib_rules_res_ops);
+					NSIM_RESOURCE_IPV4, &params);
 	if (err) {
 		pr_err("Failed to register IPv4 FIB rules resource\n");
 		return err;
@@ -121,7 +103,7 @@ static int devlink_resources_register(struct devlink *devlink)
 	err = devlink_resource_register(devlink, "IPv6", (u64)-1,
 					NSIM_RESOURCE_IPV6,
 					DEVLINK_RESOURCE_ID_PARENT_TOP,
-					&params, NULL);
+					&params);
 	if (err) {
 		pr_err("Failed to register IPv6 top resource\n");
 		goto out;
@@ -130,8 +112,7 @@ static int devlink_resources_register(struct devlink *devlink)
 	n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB, true);
 	err = devlink_resource_register(devlink, "fib", n,
 					NSIM_RESOURCE_IPV6_FIB,
-					NSIM_RESOURCE_IPV6,
-					&params, &nsim_ipv6_fib_res_ops);
+					NSIM_RESOURCE_IPV6, &params);
 	if (err) {
 		pr_err("Failed to register IPv6 FIB resource\n");
 		return err;
@@ -140,12 +121,28 @@ static int devlink_resources_register(struct devlink *devlink)
 	n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB_RULES, true);
 	err = devlink_resource_register(devlink, "fib-rules", n,
 					NSIM_RESOURCE_IPV6_FIB_RULES,
-					NSIM_RESOURCE_IPV6,
-					&params, &nsim_ipv6_fib_rules_res_ops);
+					NSIM_RESOURCE_IPV6, &params);
 	if (err) {
 		pr_err("Failed to register IPv6 FIB rules resource\n");
 		return err;
 	}
+
+	devlink_resource_occ_get_register(devlink,
+					  NSIM_RESOURCE_IPV4_FIB,
+					  nsim_ipv4_fib_resource_occ_get,
+					  net);
+	devlink_resource_occ_get_register(devlink,
+					  NSIM_RESOURCE_IPV4_FIB_RULES,
+					  nsim_ipv4_fib_rules_res_occ_get,
+					  net);
+	devlink_resource_occ_get_register(devlink,
+					  NSIM_RESOURCE_IPV6_FIB,
+					  nsim_ipv6_fib_resource_occ_get,
+					  net);
+	devlink_resource_occ_get_register(devlink,
+					  NSIM_RESOURCE_IPV6_FIB_RULES,
+					  nsim_ipv6_fib_rules_res_occ_get,
+					  net);
 out:
 	return err;
 }
diff --git a/include/net/devlink.h b/include/net/devlink.h
index e21d8cadd480..2e4f71e16e95 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -231,14 +231,6 @@ struct devlink_dpipe_headers {
 	unsigned int headers_count;
 };
 
-/**
- * struct devlink_resource_ops - resource ops
- * @occ_get: get the occupied size
- */
-struct devlink_resource_ops {
-	u64 (*occ_get)(struct devlink *devlink);
-};
-
 /**
  * struct devlink_resource_size_params - resource's size parameters
  * @size_min: minimum size which can be set
@@ -265,6 +257,8 @@ devlink_resource_size_params_init(struct devlink_resource_size_params *size_para
 	size_params->unit = unit;
 }
 
+typedef u64 devlink_resource_occ_get_t(void *priv);
+
 /**
  * struct devlink_resource - devlink resource
  * @name: name of the resource
@@ -277,7 +271,6 @@ devlink_resource_size_params_init(struct devlink_resource_size_params *size_para
  * @size_params: size parameters
  * @list: parent list
  * @resource_list: list of child resources
- * @resource_ops: resource ops
  */
 struct devlink_resource {
 	const char *name;
@@ -289,7 +282,8 @@ struct devlink_resource {
 	struct devlink_resource_size_params size_params;
 	struct list_head list;
 	struct list_head resource_list;
-	const struct devlink_resource_ops *resource_ops;
+	devlink_resource_occ_get_t *occ_get;
+	void *occ_get_priv;
 };
 
 #define DEVLINK_RESOURCE_ID_PARENT_TOP 0
@@ -409,8 +403,7 @@ int devlink_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_ops *resource_ops);
+			      const struct devlink_resource_size_params *size_params);
 void devlink_resources_unregister(struct devlink *devlink,
 				  struct devlink_resource *resource);
 int devlink_resource_size_get(struct devlink *devlink,
@@ -419,6 +412,12 @@ int devlink_resource_size_get(struct devlink *devlink,
 int devlink_dpipe_table_resource_set(struct devlink *devlink,
 				     const char *table_name, u64 resource_id,
 				     u64 resource_units);
+void devlink_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_unregister(struct devlink *devlink,
+					 u64 resource_id);
 
 #else
 
@@ -562,8 +561,7 @@ devlink_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_ops *resource_ops)
+			  const struct devlink_resource_size_params *size_params)
 {
 	return 0;
 }
@@ -589,6 +587,20 @@ devlink_dpipe_table_resource_set(struct devlink *devlink,
 	return -EOPNOTSUPP;
 }
 
+static inline void
+devlink_resource_occ_get_register(struct devlink *devlink,
+				  u64 resource_id,
+				  devlink_resource_occ_get_t *occ_get,
+				  void *occ_get_priv)
+{
+}
+
+static inline void
+devlink_resource_occ_get_unregister(struct devlink *devlink,
+				    u64 resource_id)
+{
+}
+
 #endif
 
 #endif /* _NET_DEVLINK_H_ */
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 9236e421bd62..ad1317376798 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -2405,6 +2405,16 @@ devlink_resource_size_params_put(struct devlink_resource *resource,
 	return 0;
 }
 
+static int devlink_resource_occ_put(struct devlink_resource *resource,
+				    struct sk_buff *skb)
+{
+	if (!resource->occ_get)
+		return 0;
+	return nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_OCC,
+				 resource->occ_get(resource->occ_get_priv),
+				 DEVLINK_ATTR_PAD);
+}
+
 static int devlink_resource_put(struct devlink *devlink, struct sk_buff *skb,
 				struct devlink_resource *resource)
 {
@@ -2425,11 +2435,8 @@ static int devlink_resource_put(struct devlink *devlink, struct sk_buff *skb,
 	if (resource->size != resource->size_new)
 		nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_SIZE_NEW,
 				  resource->size_new, DEVLINK_ATTR_PAD);
-	if (resource->resource_ops && resource->resource_ops->occ_get)
-		if (nla_put_u64_64bit(skb, DEVLINK_ATTR_RESOURCE_OCC,
-				      resource->resource_ops->occ_get(devlink),
-				      DEVLINK_ATTR_PAD))
-			goto nla_put_failure;
+	if (devlink_resource_occ_put(resource, skb))
+		goto nla_put_failure;
 	if (devlink_resource_size_params_put(resource, skb))
 		goto nla_put_failure;
 	if (list_empty(&resource->resource_list))
@@ -3162,15 +3169,13 @@ EXPORT_SYMBOL_GPL(devlink_dpipe_table_unregister);
  *	@resource_id: resource's id
  *	@parent_reosurce_id: resource's parent id
  *	@size params: size parameters
- *	@resource_ops: resource ops
  */
 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,
-			      const struct devlink_resource_ops *resource_ops)
+			      const struct devlink_resource_size_params *size_params)
 {
 	struct devlink_resource *resource;
 	struct list_head *resource_list;
@@ -3213,7 +3218,6 @@ int devlink_resource_register(struct devlink *devlink,
 	resource->size = resource_size;
 	resource->size_new = resource_size;
 	resource->id = resource_id;
-	resource->resource_ops = resource_ops;
 	resource->size_valid = true;
 	memcpy(&resource->size_params, size_params,
 	       sizeof(resource->size_params));
@@ -3315,6 +3319,58 @@ int devlink_dpipe_table_resource_set(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devlink_dpipe_table_resource_set);
 
+/**
+ *	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
+ */
+void devlink_resource_occ_get_register(struct devlink *devlink,
+				       u64 resource_id,
+				       devlink_resource_occ_get_t *occ_get,
+				       void *occ_get_priv)
+{
+	struct devlink_resource *resource;
+
+	mutex_lock(&devlink->lock);
+	resource = devlink_resource_find(devlink, NULL, resource_id);
+	if (WARN_ON(!resource))
+		goto out;
+	WARN_ON(resource->occ_get);
+
+	resource->occ_get = occ_get;
+	resource->occ_get_priv = occ_get_priv;
+out:
+	mutex_unlock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_resource_occ_get_register);
+
+/**
+ *	devlink_resource_occ_get_unregister - unregister occupancy getter
+ *
+ *	@devlink: devlink
+ *	@resource_id: resource id
+ */
+void devlink_resource_occ_get_unregister(struct devlink *devlink,
+					 u64 resource_id)
+{
+	struct devlink_resource *resource;
+
+	mutex_lock(&devlink->lock);
+	resource = devlink_resource_find(devlink, NULL, resource_id);
+	if (WARN_ON(!resource))
+		goto out;
+	WARN_ON(!resource->occ_get);
+
+	resource->occ_get = NULL;
+	resource->occ_get_priv = NULL;
+out:
+	mutex_unlock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_resource_occ_get_unregister);
+
 static int __init devlink_module_init(void)
 {
 	return genl_register_family(&devlink_nl_family);
-- 
2.14.3

Re: [patch net] devlink: convert occ_get op to separate registration

[David Ahern]

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

On 4/5/18 2:13 PM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> This resolves race during initialization where the resources with
> ops are registered before driver and the structures used by occ_get
> op is initialized. So keep occ_get callbacks registered only when
> all structs are initialized.
> 
> The example flows, as it is in mlxsw:
> 1) driver load/asic probe:
>    mlxsw_core
>       -> mlxsw_sp_resources_register
>         -> mlxsw_sp_kvdl_resources_register
>           -> devlink_resource_register IDX
>    mlxsw_spectrum
>       -> mlxsw_sp_kvdl_init
>         -> mlxsw_sp_kvdl_parts_init
>           -> mlxsw_sp_kvdl_part_init
>             -> devlink_resource_size_get IDX (to get the current setup
>                                               size from devlink)
>         -> devlink_resource_occ_get_register IDX (register current
>                                                   occupancy getter)
> 2) reload triggered by devlink command:
>   -> mlxsw_devlink_core_bus_device_reload
>     -> mlxsw_sp_fini
>       -> mlxsw_sp_kvdl_fini
> 	-> devlink_resource_occ_get_unregister IDX
>     (struct mlxsw_sp *mlxsw_sp is freed at this point, call to occ get
>      which is using mlxsw_sp would cause use-after free)
>     -> mlxsw_sp_init
>       -> mlxsw_sp_kvdl_init
>         -> mlxsw_sp_kvdl_parts_init
>           -> mlxsw_sp_kvdl_part_init
>             -> devlink_resource_size_get IDX (to get the current setup
>                                               size from devlink)
>         -> devlink_resource_occ_get_register IDX (register current
>                                                   occupancy getter)
> 
> Fixes: d9f9b9a4d05f ("devlink: Add support for resource abstraction")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---


Why won't something like the attached work as opposed to playing
register / unregister games?

[-- Attachment #2: mlxsw-reload.patch --]
[-- Type: text/plain, Size: 4748 bytes --]

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 93ea56620a24..dcded613faa6 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -113,6 +113,7 @@ struct mlxsw_core {
 	struct mlxsw_thermal *thermal;
 	struct mlxsw_core_port *ports;
 	unsigned int max_ports;
+	bool reload_in_progress;
 	bool reload_fail;
 	unsigned long driver_priv[0];
 	/* driver_priv has to be always the last item */
@@ -154,6 +155,12 @@ void *mlxsw_core_driver_priv(struct mlxsw_core *mlxsw_core)
 }
 EXPORT_SYMBOL(mlxsw_core_driver_priv);
 
+bool mlxsw_core_reload_in_progress(struct mlxsw_core *mlxsw_core)
+{
+	return mlxsw_core->mlxsw_core_driver_priv;
+}
+EXPORT_SYMBOL(mlxsw_core_reload_in_progress);
+
 struct mlxsw_rx_listener_item {
 	struct list_head list;
 	struct mlxsw_rx_listener rxl;
@@ -972,12 +979,16 @@ static int mlxsw_devlink_core_bus_device_reload(struct devlink *devlink)
 	if (!mlxsw_bus->reset)
 		return -EOPNOTSUPP;
 
+	mlxsw_core->reload_in_progress = true;
+
 	mlxsw_core_bus_device_unregister(mlxsw_core, true);
 	mlxsw_bus->reset(mlxsw_core->bus_priv);
 	err = mlxsw_core_bus_device_register(mlxsw_core->bus_info,
 					     mlxsw_core->bus,
 					     mlxsw_core->bus_priv, true,
 					     devlink);
+	mlxsw_core->reload_in_progress = false;
+
 	if (err)
 		mlxsw_core->reload_fail = true;
 	return err;
diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
index 092d39399f3c..ffa1db154757 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
@@ -60,6 +60,7 @@ struct mlxsw_bus_info;
 unsigned int mlxsw_core_max_ports(const struct mlxsw_core *mlxsw_core);
 
 void *mlxsw_core_driver_priv(struct mlxsw_core *mlxsw_core);
+bool mlxsw_core_reload_in_progress(struct mlxsw_core *mlxsw_core);
 
 int mlxsw_core_driver_register(struct mlxsw_driver *mlxsw_driver);
 void mlxsw_core_driver_unregister(struct mlxsw_driver *mlxsw_driver);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 53fffd09d133..09b89af37d8a 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3808,8 +3808,12 @@ static const struct mlxsw_config_profile mlxsw_sp_config_profile = {
 static u64 mlxsw_sp_resource_kvd_linear_occ_get(struct devlink *devlink)
 {
 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
+	struct mlxsw_sp *mlxsw_sp;
+
+	if (mlxsw_core_reload_in_progress(mlxsw_core))
+		return 0;
 
+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	return mlxsw_sp_kvdl_occ_get(mlxsw_sp);
 }
 
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
index 8796db44dcc3..dd66285bafb5 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
@@ -329,9 +329,13 @@ u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp)
 static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink)
 {
 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	struct mlxsw_sp_kvdl_part *part;
+	struct mlxsw_sp *mlxsw_sp;
 
+	if (mlxsw_core_reload_in_progress(mlxsw_core))
+		return 0;
+
+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_SINGLE];
 	return mlxsw_sp_kvdl_part_occ(part);
 }
@@ -339,8 +343,13 @@ static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink)
 static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink)
 {
 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	struct mlxsw_sp_kvdl_part *part;
+	struct mlxsw_sp *mlxsw_sp;
+
+	if (mlxsw_core_reload_in_progress(mlxsw_core))
+		return 0;
+
+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 
 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_CHUNKS];
 	return mlxsw_sp_kvdl_part_occ(part);
@@ -349,9 +358,13 @@ static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink)
 static u64 mlxsw_sp_kvdl_large_chunks_occ_get(struct devlink *devlink)
 {
 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	struct mlxsw_sp_kvdl_part *part;
+	struct mlxsw_sp *mlxsw_sp;
+
+	if (mlxsw_core_reload_in_progress(mlxsw_core))
+		return 0;
 
+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_LARGE_CHUNKS];
 	return mlxsw_sp_kvdl_part_occ(part);
 }

Re: [patch net] devlink: convert occ_get op to separate registration

[David Ahern]

On 4/5/18 2:55 PM, David Ahern wrote:
> @@ -154,6 +155,12 @@ void *mlxsw_core_driver_priv(struct mlxsw_core *mlxsw_core)
>  }
>  EXPORT_SYMBOL(mlxsw_core_driver_priv);
>  
> +bool mlxsw_core_reload_in_progress(struct mlxsw_core *mlxsw_core)
> +{
> +	return mlxsw_core->mlxsw_core_driver_priv;

Oops, that is supposed to be:
	return mlxsw_core->reload_in_progress;

but I think you get the point.

> +}
> +EXPORT_SYMBOL(mlxsw_core_reload_in_progress);
> +
>  struct mlxsw_rx_listener_item {
>  	struct list_head list;
>  	struct mlxsw_rx_listener rxl;

Re: [patch net] devlink: convert occ_get op to separate registration

[Jiri Pirko]

Thu, Apr 05, 2018 at 10:55:58PM CEST, dsahern@gmail.com wrote:
>On 4/5/18 2:13 PM, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> This resolves race during initialization where the resources with
>> ops are registered before driver and the structures used by occ_get
>> op is initialized. So keep occ_get callbacks registered only when
>> all structs are initialized.
>> 
>> The example flows, as it is in mlxsw:
>> 1) driver load/asic probe:
>>    mlxsw_core
>>       -> mlxsw_sp_resources_register
>>         -> mlxsw_sp_kvdl_resources_register
>>           -> devlink_resource_register IDX
>>    mlxsw_spectrum
>>       -> mlxsw_sp_kvdl_init
>>         -> mlxsw_sp_kvdl_parts_init
>>           -> mlxsw_sp_kvdl_part_init
>>             -> devlink_resource_size_get IDX (to get the current setup
>>                                               size from devlink)
>>         -> devlink_resource_occ_get_register IDX (register current
>>                                                   occupancy getter)
>> 2) reload triggered by devlink command:
>>   -> mlxsw_devlink_core_bus_device_reload
>>     -> mlxsw_sp_fini
>>       -> mlxsw_sp_kvdl_fini
>> 	-> devlink_resource_occ_get_unregister IDX
>>     (struct mlxsw_sp *mlxsw_sp is freed at this point, call to occ get
>>      which is using mlxsw_sp would cause use-after free)
>>     -> mlxsw_sp_init
>>       -> mlxsw_sp_kvdl_init
>>         -> mlxsw_sp_kvdl_parts_init
>>           -> mlxsw_sp_kvdl_part_init
>>             -> devlink_resource_size_get IDX (to get the current setup
>>                                               size from devlink)
>>         -> devlink_resource_occ_get_register IDX (register current
>>                                                   occupancy getter)
>> 
>> Fixes: d9f9b9a4d05f ("devlink: Add support for resource abstraction")
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>
>
>Why won't something like the attached work as opposed to playing
>register / unregister games?

Because it is quite driver-specific. For example in netdevsim, with
current implementation you don't have to unreg as the priv is there the
whole time.

Also, I think it is more correct to register getter with the priv
directly. Not to depend on getting with via devlink struct somehow.

I'm not saying that my solution is super nice. But what you suggest
seems much more uglier to me.



>diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
>index 93ea56620a24..dcded613faa6 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
>@@ -113,6 +113,7 @@ struct mlxsw_core {
> 	struct mlxsw_thermal *thermal;
> 	struct mlxsw_core_port *ports;
> 	unsigned int max_ports;
>+	bool reload_in_progress;
> 	bool reload_fail;
> 	unsigned long driver_priv[0];
> 	/* driver_priv has to be always the last item */
>@@ -154,6 +155,12 @@ void *mlxsw_core_driver_priv(struct mlxsw_core *mlxsw_core)
> }
> EXPORT_SYMBOL(mlxsw_core_driver_priv);
> 
>+bool mlxsw_core_reload_in_progress(struct mlxsw_core *mlxsw_core)
>+{
>+	return mlxsw_core->mlxsw_core_driver_priv;
>+}
>+EXPORT_SYMBOL(mlxsw_core_reload_in_progress);
>+
> struct mlxsw_rx_listener_item {
> 	struct list_head list;
> 	struct mlxsw_rx_listener rxl;
>@@ -972,12 +979,16 @@ static int mlxsw_devlink_core_bus_device_reload(struct devlink *devlink)
> 	if (!mlxsw_bus->reset)
> 		return -EOPNOTSUPP;
> 
>+	mlxsw_core->reload_in_progress = true;
>+
> 	mlxsw_core_bus_device_unregister(mlxsw_core, true);
> 	mlxsw_bus->reset(mlxsw_core->bus_priv);
> 	err = mlxsw_core_bus_device_register(mlxsw_core->bus_info,
> 					     mlxsw_core->bus,
> 					     mlxsw_core->bus_priv, true,
> 					     devlink);
>+	mlxsw_core->reload_in_progress = false;
>+
> 	if (err)
> 		mlxsw_core->reload_fail = true;
> 	return err;
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.h b/drivers/net/ethernet/mellanox/mlxsw/core.h
>index 092d39399f3c..ffa1db154757 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/core.h
>+++ b/drivers/net/ethernet/mellanox/mlxsw/core.h
>@@ -60,6 +60,7 @@ struct mlxsw_bus_info;
> unsigned int mlxsw_core_max_ports(const struct mlxsw_core *mlxsw_core);
> 
> void *mlxsw_core_driver_priv(struct mlxsw_core *mlxsw_core);
>+bool mlxsw_core_reload_in_progress(struct mlxsw_core *mlxsw_core);
> 
> int mlxsw_core_driver_register(struct mlxsw_driver *mlxsw_driver);
> void mlxsw_core_driver_unregister(struct mlxsw_driver *mlxsw_driver);
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>index 53fffd09d133..09b89af37d8a 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
>@@ -3808,8 +3808,12 @@ static const struct mlxsw_config_profile mlxsw_sp_config_profile = {
> static u64 mlxsw_sp_resource_kvd_linear_occ_get(struct devlink *devlink)
> {
> 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
>+	struct mlxsw_sp *mlxsw_sp;
>+
>+	if (mlxsw_core_reload_in_progress(mlxsw_core))
>+		return 0;
> 
>+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> 	return mlxsw_sp_kvdl_occ_get(mlxsw_sp);
> }
> 
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
>index 8796db44dcc3..dd66285bafb5 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_kvdl.c
>@@ -329,9 +329,13 @@ u64 mlxsw_sp_kvdl_occ_get(const struct mlxsw_sp *mlxsw_sp)
> static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink)
> {
> 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> 	struct mlxsw_sp_kvdl_part *part;
>+	struct mlxsw_sp *mlxsw_sp;
> 
>+	if (mlxsw_core_reload_in_progress(mlxsw_core))
>+		return 0;
>+
>+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_SINGLE];
> 	return mlxsw_sp_kvdl_part_occ(part);
> }
>@@ -339,8 +343,13 @@ static u64 mlxsw_sp_kvdl_single_occ_get(struct devlink *devlink)
> static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink)
> {
> 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> 	struct mlxsw_sp_kvdl_part *part;
>+	struct mlxsw_sp *mlxsw_sp;
>+
>+	if (mlxsw_core_reload_in_progress(mlxsw_core))
>+		return 0;
>+
>+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> 
> 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_CHUNKS];
> 	return mlxsw_sp_kvdl_part_occ(part);
>@@ -349,9 +358,13 @@ static u64 mlxsw_sp_kvdl_chunks_occ_get(struct devlink *devlink)
> static u64 mlxsw_sp_kvdl_large_chunks_occ_get(struct devlink *devlink)
> {
> 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>-	struct mlxsw_sp *mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> 	struct mlxsw_sp_kvdl_part *part;
>+	struct mlxsw_sp *mlxsw_sp;
>+
>+	if (mlxsw_core_reload_in_progress(mlxsw_core))
>+		return 0;
> 
>+	mlxsw_sp = mlxsw_core_driver_priv(mlxsw_core);
> 	part = mlxsw_sp->kvdl->parts[MLXSW_SP_KVDL_PART_ID_LARGE_CHUNKS];
> 	return mlxsw_sp_kvdl_part_occ(part);
> }

Re: [patch net] devlink: convert occ_get op to separate registration

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu,  5 Apr 2018 22:13:21 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> This resolves race during initialization where the resources with
> ops are registered before driver and the structures used by occ_get
> op is initialized. So keep occ_get callbacks registered only when
> all structs are initialized.
 ...
> Fixes: d9f9b9a4d05f ("devlink: Add support for resource abstraction")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Applied and queued up for -stable, thanks.

Re: [patch net] devlink: convert occ_get op to separate registration

[Sasha Levin]

Hi,

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag,
fixing commit: d9f9b9a4d05f devlink: Add support for resource abstraction.

The bot has tested the following trees: v4.16.1.

v4.16.1: Failed to apply! Possible dependencies:
    37923ed6b8ce ("netdevsim: Add simple FIB resource controller via devlink")
    3ed898e8cd05 ("mlxsw: spectrum_kvdl: Make some functions static")
    4f4bbf7c4e3d ("devlink: Perform cleanup of resource_set cb")
    51d3c08e3371 ("mlxsw: spectrum_kvdl: Add support for linear division resources")
    7f47b19bd744 ("mlxsw: spectrum_kvdl: Add support for per part occupancy")
    88d2fbcda145 ("mlxsw: spectrum: Pass mlxsw_core as arg of mlxsw_sp_kvdl_resources_register()")
    c8276dd250e9 ("mlxsw: spectrum_kvdl: Fix handling of resource_size_param")
    f9b9120119ca ("mlxsw: Constify devlink_resource_ops")

Re: [patch net] devlink: convert occ_get op to separate registration

[Jiri Pirko]

Tue, Apr 10, 2018 at 03:49:40PM CEST, Alexander.Levin@microsoft.com wrote:
>Hi,
>
>[This is an automated email]
>
>This commit has been processed because it contains a "Fixes:" tag,
>fixing commit: d9f9b9a4d05f devlink: Add support for resource abstraction.

I don't think we need this fix in stable. Thanks.


>
>The bot has tested the following trees: v4.16.1.
>
>v4.16.1: Failed to apply! Possible dependencies:
>    37923ed6b8ce ("netdevsim: Add simple FIB resource controller via devlink")
>    3ed898e8cd05 ("mlxsw: spectrum_kvdl: Make some functions static")
>    4f4bbf7c4e3d ("devlink: Perform cleanup of resource_set cb")
>    51d3c08e3371 ("mlxsw: spectrum_kvdl: Add support for linear division resources")
>    7f47b19bd744 ("mlxsw: spectrum_kvdl: Add support for per part occupancy")
>    88d2fbcda145 ("mlxsw: spectrum: Pass mlxsw_core as arg of mlxsw_sp_kvdl_resources_register()")
>    c8276dd250e9 ("mlxsw: spectrum_kvdl: Fix handling of resource_size_param")
>    f9b9120119ca ("mlxsw: Constify devlink_resource_ops")
>
>
>--
>Thanks,
>Sasha

Re: [patch net] devlink: convert occ_get op to separate registration

[David Miller]

From: Sasha Levin <Alexander.Levin@microsoft.com>
Date: Tue, 10 Apr 2018 13:49:40 +0000

> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: d9f9b9a4d05f devlink: Add support for resource abstraction.
> 
> The bot has tested the following trees: v4.16.1.

This is nice, but it's becomming noise.

I pick and choose specific networking changes to queue up for -stable
and whether it has a Fixes tag or applies cleanly are not the primary
considerations I take into account when deciding what goes to stable
or not.

What matters primarily are two attributes:

1) What is the impact of the bug?

2) How risky is the change?

3) Did someone explicitly ask for the backport?  Why?

These automated emails tell me nothing about either attribute.

I figure out what Fixes: tags are involved and whether the patch
applies cleanly when I process my stable queue of networking patches.

For various reasons I have to do this all manually anyways.  I always
do a full analysis of where the bug came from, and also do manual
code inspection to find the cases where a Fixes: tag indicates a commit
that was also put into stable branches and what impact that has on
the backport of the stable fix for a particular stable tree.

So I would like to kindly ask that you stop sending these automated
emails, they really are not helping my networking stable backport
process at all, and instead are just making more emails I have to
delete from my inbox every day.

Thank you very much for your kind consideration in this matter.

Re: [patch net] devlink: convert occ_get op to separate registration

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 10 Apr 2018 16:13:09 +0200

> Tue, Apr 10, 2018 at 03:49:40PM CEST, Alexander.Levin@microsoft.com wrote:
>>Hi,
>>
>>[This is an automated email]
>>
>>This commit has been processed because it contains a "Fixes:" tag,
>>fixing commit: d9f9b9a4d05f devlink: Add support for resource abstraction.
> 
> I don't think we need this fix in stable. Thanks.

And as I stated in another email, I don't think we need these automated
things either.

Re: [patch net] devlink: convert occ_get op to separate registration

[Sasha Levin]

On Tue, Apr 10, 2018 at 10:35:07AM -0400, David Miller wrote:
>From: Sasha Levin <Alexander.Levin@microsoft.com>
>Date: Tue, 10 Apr 2018 13:49:40 +0000
>
>> This commit has been processed because it contains a "Fixes:" tag,
>> fixing commit: d9f9b9a4d05f devlink: Add support for resource abstraction.
>>
>> The bot has tested the following trees: v4.16.1.
>
>This is nice, but it's becomming noise.

Thank you, and sorry. I've blacklisted net/ and drivers/net/.

>I pick and choose specific networking changes to queue up for -stable
>and whether it has a Fixes tag or applies cleanly are not the primary
>considerations I take into account when deciding what goes to stable
>or not.
>
>What matters primarily are two attributes:
>
>1) What is the impact of the bug?
>
>2) How risky is the change?
>
>3) Did someone explicitly ask for the backport?  Why?

When I started the autoselection project, I found it interesting that
the networking subsystem has a unique workflow for stable commits where
a single person would review every single patch that gets merged, and
would select patches purely based on whether the patch follows the
stable rules or not, without relying on stable tags.

For the first few iterations of the neural network, I thought that the
networking commit set is valuable since if I tried to learn on commits
that were tagged for stable, the neural network would develop a bias
towards the tag which will prevent it from detecting bug fixing patches
that do not contain such tags.

However, during validation, I found that the quality of the selection on
a random set of commits wasn't what I expected it to be. I reviewed a
random sample of networking commits and found that a high percentage of
commits that are clearly "bug fixing", were not included in any stable
tree.

This changed my view on how the stable process works for different
subsystems; I realized that there are 3 main things we look for in
stable trees:

 1. They have all bug fixing commits that are relevant to that tree.
 2. There are none non-bug-fixing commits in the tree.
 3. The commits are backported correctly to that tree.

And since subsystems have different workflows for stable, I could see
how different approaches balanced these goals. For networking, it was
easy to see that a high quality of review by the maintainer prevented
and non-bug-fixing commits from creeping in, and the error rates for
backports were much smaller than any other subsystem I looked at.

What was missing was the first goal. My hypothesis was that after all,
we are only humans; with the high rate of commits that flow into a
subsystem, and the opt-in nature of the process, we are bound to
incorrectly miss commits that would otherwise be in a stable tree.

I could back up my theory with experiences other folks had [1][2], 
so one of my biggest hopes was to see how the autoselection would could
improve the quality of selection and build on the biggest advantage the
networking subsystem over other subsystems: a dedicated maintainer who
actually gives a crap and reviews these patches himself.

When the work on the neural network itself quieted down, and the first
results started trickling in, I was delighted that we can now help
select commits that were missed by manual inspection, but are clearly
bug fixing. For example, [3][4][5].

>These automated emails tell me nothing about either attribute.

The attributes you've listed are subjective, and are the final step of
selecting patches for stable trees. Consider the following additional
attributes you undoubtfully look at when going through commits:

 1. Is it fixing a bug?
 2. Is it relevant to any of the stable trees?

Only after deciding on these attributes, it makes sense to look at ones
that require actual thinking (such as the ones you've listed).

>I figure out what Fixes: tags are involved and whether the patch
>applies cleanly when I process my stable queue of networking patches.

This is another thing that doesn't always end up right. When done
manually patches tend to get lost, forgotten, or end up with a partial
backport.

Doing this is a combination of "dumb" manual labor where for each stable
tree we try to find whether the commit we fix exists in it, and which
other commits we may depend on, and "smart" labor where we try to figure
out whether the commit should actually be in that tree.

The bot tries to take the "dumb" part out of your way, by letting
you know from the start which trees this applied/built on and what
dependencies it might have. It comes for free, why not use it?

As an example, a fix for CVE-2017-16939 [6] was backported to v4.9
but not to v4.4 or v3.18 because of missing dependencies.

>For various reasons I have to do this all manually anyways.  I always
>do a full analysis of where the bug came from, and also do manual
>code inspection to find the cases where a Fixes: tag indicates a commit
>that was also put into stable branches and what impact that has on
>the backport of the stable fix for a particular stable tree.

Would you be interested in going over these reasons, and whether the bot
can automate any of these? Things like determining if a Fixes: commit is
already in stable are already automated (excluding the impact analysis,
ofcourse).

>So I would like to kindly ask that you stop sending these automated
>emails, they really are not helping my networking stable backport
>process at all, and instead are just making more emails I have to
>delete from my inbox every day.
>
>Thank you very much for your kind consideration in this matter.

What would be the appropriate way to submit these to the networking
stable branch?

Let's look at a concrete example: I've sent a mail for a dccp fix[7] a
few days ago. Could we discuss why it's not in any stable tree?



[1] https://www.spinics.net/lists/stable/msg173995.html
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c58d6c93680f28ac58984af61d0a7ebf4319c241
[3] https://patchwork.kernel.org/patch/10188235/
[4] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=linux-4.4.y&id=e29066778bc28eff5f63616800c6b60f12c87267
[5] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/?h=linux-4.4.y&id=7c5deeccc664a79d5586df86b4221493f28e9ef9
[6] https://www.spinics.net/lists/netdev/msg469458.html
[7] https://lkml.org/lkml/2018/4/8/762

Re: [patch net] devlink: convert occ_get op to separate registration

[David Miller]

From: Sasha Levin <Alexander.Levin@microsoft.com>
Date: Tue, 10 Apr 2018 19:08:20 +0000

> The bot tries to take the "dumb" part out of your way, by letting
> you know from the start which trees this applied/built on and what
> dependencies it might have. It comes for free, why not use it?

I do this already while I'm processing the -stable queue in patchwork
and it automatically falls right out of the process I use to extract
patches out of Linus's GIT tree.

I manually pull the commit out of Linus's tree, verify that it is
actually the commit I'm interested in, then I do two things:

1) I look at the exact tag that the commit landed in using
   "git describe --contains SHA1_ID"

2) I look at the exact tag that the commit the Fixes: tag
   points at landed using "git describe --contains SHA1_ID"

I double check #2 to see for cases whether the Fixes: tag
itself was backported to stable trees.

I use these two tag values to organize the -stable queue into
subdirectories.  This guides my patch applying in order to minimize
useless work.

So I have to do all of this work anyways.

Even if the bot provided these values, I would still double
check them, every single one of them.

Therefore, the net result from my perspective is that for most
patches fixing bugs on this list, instead of N list postings,
there will now be at least N * 2.

Bots are starting to overwhelm actual content from human beings
on this list, and I want to put my foot on the brake right now
before it gets even more out of control.

Thank you.

Re: [patch net] devlink: convert occ_get op to separate registration

[Sasha Levin]

On Tue, Apr 10, 2018 at 03:22:57PM -0400, David Miller wrote:
>From: Sasha Levin <Alexander.Levin@microsoft.com>
>Date: Tue, 10 Apr 2018 19:08:20 +0000
>
>> The bot tries to take the "dumb" part out of your way, by letting
>> you know from the start which trees this applied/built on and what
>> dependencies it might have. It comes for free, why not use it?
>
>I do this already while I'm processing the -stable queue in patchwork
>and it automatically falls right out of the process I use to extract
>patches out of Linus's GIT tree.
>
>I manually pull the commit out of Linus's tree, verify that it is
>actually the commit I'm interested in, then I do two things:
>
>1) I look at the exact tag that the commit landed in using
>   "git describe --contains SHA1_ID"
>
>2) I look at the exact tag that the commit the Fixes: tag
>   points at landed using "git describe --contains SHA1_ID"
>
>I double check #2 to see for cases whether the Fixes: tag
>itself was backported to stable trees.
>
>I use these two tag values to organize the -stable queue into
>subdirectories.  This guides my patch applying in order to minimize
>useless work.
>
>So I have to do all of this work anyways.
>
>Even if the bot provided these values, I would still double
>check them, every single one of them.
>
>Therefore, the net result from my perspective is that for most
>patches fixing bugs on this list, instead of N list postings,
>there will now be at least N * 2.

Gotcha. I can keep it out from the regular patch flow. How could we
address commits that might have been missed? Let's say I look at commits
that are older than ~1 month, how can I get those looked at again?

>Bots are starting to overwhelm actual content from human beings
>on this list, and I want to put my foot on the brake right now
>before it gets even more out of control.

I think we're just hitting the limitations of using a mailing list. Bots
aren't around to spam everyone for fun, right?

0day bot is spammy because patches fail to compile, syzbot is spammy
because we have tons of bugs users can hit and the stable bot is
spammy because we miss lots of commits that should be in stable.

As the kernel grows, not only the codebase is expanding but also the
tooling around it. While spammy, bots provide valuable input that in the
past would take blood, sweat and tears to acquire. We just need a better
way to consume it rather than blocking off these inputs.

Thanks!

Re: [patch net] devlink: convert occ_get op to separate registration

[gregkh]

On Tue, Apr 10, 2018 at 08:43:31PM +0000, Sasha Levin wrote:
> >Bots are starting to overwhelm actual content from human beings
> >on this list, and I want to put my foot on the brake right now
> >before it gets even more out of control.
> 
> I think we're just hitting the limitations of using a mailing list. Bots
> aren't around to spam everyone for fun, right?
> 
> 0day bot is spammy because patches fail to compile, syzbot is spammy
> because we have tons of bugs users can hit and the stable bot is
> spammy because we miss lots of commits that should be in stable.
> 
> As the kernel grows, not only the codebase is expanding but also the
> tooling around it. While spammy, bots provide valuable input that in the
> past would take blood, sweat and tears to acquire. We just need a better
> way to consume it rather than blocking off these inputs.

As one of the primary abusers of the "I send too much email" flood of
mess, I'm going to start cutting down on what type of message I respond
to a public vger list from now on.  I'll write more on the stable@ list
about this, but for your individual patches like this, how about just
responding to the developers themselves and not a public list?  I do
that when I commit a patch to my tree, stripping out lists, as it's not
a useful message for anyone other than the person who wrote the patch
and the reviewers.

thanks,

greg k-h

Re: [patch net] devlink: convert occ_get op to separate registration

[Sasha Levin]

On Wed, Apr 11, 2018 at 10:46:00AM +0200, gregkh@linuxfoundation.org wrote:
>On Tue, Apr 10, 2018 at 08:43:31PM +0000, Sasha Levin wrote:
>> >Bots are starting to overwhelm actual content from human beings
>> >on this list, and I want to put my foot on the brake right now
>> >before it gets even more out of control.
>>
>> I think we're just hitting the limitations of using a mailing list. Bots
>> aren't around to spam everyone for fun, right?
>>
>> 0day bot is spammy because patches fail to compile, syzbot is spammy
>> because we have tons of bugs users can hit and the stable bot is
>> spammy because we miss lots of commits that should be in stable.
>>
>> As the kernel grows, not only the codebase is expanding but also the
>> tooling around it. While spammy, bots provide valuable input that in the
>> past would take blood, sweat and tears to acquire. We just need a better
>> way to consume it rather than blocking off these inputs.
>
>As one of the primary abusers of the "I send too much email" flood of
>mess, I'm going to start cutting down on what type of message I respond
>to a public vger list from now on.  I'll write more on the stable@ list
>about this, but for your individual patches like this, how about just
>responding to the developers themselves and not a public list?  I do
>that when I commit a patch to my tree, stripping out lists, as it's not
>a useful message for anyone other than the person who wrote the patch
>and the reviewers.

Sure.

I think we should have a discussion as to the best way to "spam" people.
There are a few alternatives I can think of (a "digest" mail once a
day/week? Web UI? different mailing list?) but we keep using the one
size fits all approach for all our bots.

I don't see a reason why we can't tailor bot output based on
maintainer/author preference? Maybe David would be less upset if he'd
see just one mail per week with commits we ask to review?

We're stuck with mailing lists for kernel development, might as well
make the best out of it.