[PATCH net-next v1] devlink: Simplify devlink resources unregister call

[PATCH net-next v1] devlink: Simplify devlink resources unregister call

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

The devlink_resources_unregister() used second parameter as an
entry point for the recursive removal of devlink resources. None
of external to devlink users needed to use this field, so lat's
remove it.

Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
v1:
 * Sent as standalone change
 * Moved struct recource from .h to .c file
 * Added missing descriptions fields in struct resource
v0: https://lore.kernel.org/all/cover.1637173517.git.leonro@nvidia.com
---
 drivers/net/ethernet/mellanox/mlxsw/core.c    |  8 +--
 .../net/ethernet/mellanox/mlxsw/spectrum.c    |  4 +-
 drivers/net/netdevsim/dev.c                   |  4 +-
 include/net/devlink.h                         | 30 +--------
 net/core/devlink.c                            | 63 ++++++++++++++-----
 net/dsa/dsa.c                                 |  2 +-
 6 files changed, 58 insertions(+), 53 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 3fd3812b8f31..0d1f08bbf631 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -160,7 +160,7 @@ static void mlxsw_ports_fini(struct mlxsw_core *mlxsw_core, bool reload)
 
 	devlink_resource_occ_get_unregister(devlink, MLXSW_CORE_RESOURCE_PORTS);
 	if (!reload)
-		devlink_resources_unregister(priv_to_devlink(mlxsw_core), NULL);
+		devlink_resources_unregister(priv_to_devlink(mlxsw_core));
 
 	kfree(mlxsw_core->ports);
 }
@@ -2033,7 +2033,7 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 	mlxsw_ports_fini(mlxsw_core, reload);
 err_ports_init:
 	if (!reload)
-		devlink_resources_unregister(devlink, NULL);
+		devlink_resources_unregister(devlink);
 err_register_resources:
 	mlxsw_bus->fini(bus_priv);
 err_bus_init:
@@ -2099,7 +2099,7 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
 	kfree(mlxsw_core->lag.mapping);
 	mlxsw_ports_fini(mlxsw_core, reload);
 	if (!reload)
-		devlink_resources_unregister(devlink, NULL);
+		devlink_resources_unregister(devlink);
 	mlxsw_core->bus->fini(mlxsw_core->bus_priv);
 	if (!reload)
 		devlink_free(devlink);
@@ -2108,7 +2108,7 @@ void mlxsw_core_bus_device_unregister(struct mlxsw_core *mlxsw_core,
 
 reload_fail_deinit:
 	mlxsw_core_params_unregister(mlxsw_core);
-	devlink_resources_unregister(devlink, NULL);
+	devlink_resources_unregister(devlink);
 	devlink_free(devlink);
 }
 EXPORT_SYMBOL(mlxsw_core_bus_device_unregister);
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
index 24157bb59881..f1ed7660b0b5 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum.c
@@ -3336,7 +3336,7 @@ static int mlxsw_sp1_resources_register(struct mlxsw_core *mlxsw_core)
 err_policer_resources_register:
 err_resources_counter_register:
 err_resources_span_register:
-	devlink_resources_unregister(priv_to_devlink(mlxsw_core), NULL);
+	devlink_resources_unregister(priv_to_devlink(mlxsw_core));
 	return err;
 }
 
@@ -3370,7 +3370,7 @@ static int mlxsw_sp2_resources_register(struct mlxsw_core *mlxsw_core)
 err_policer_resources_register:
 err_resources_counter_register:
 err_resources_span_register:
-	devlink_resources_unregister(priv_to_devlink(mlxsw_core), NULL);
+	devlink_resources_unregister(priv_to_devlink(mlxsw_core));
 	return err;
 }
 
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 54345c096a16..08d7b465a0de 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -1622,7 +1622,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	devlink_params_unregister(devlink, nsim_devlink_params,
 				  ARRAY_SIZE(nsim_devlink_params));
 err_dl_unregister:
-	devlink_resources_unregister(devlink, NULL);
+	devlink_resources_unregister(devlink);
 err_vfc_free:
 	kfree(nsim_dev->vfconfigs);
 err_devlink_free:
@@ -1668,7 +1668,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
 	nsim_dev_debugfs_exit(nsim_dev);
 	devlink_params_unregister(devlink, nsim_devlink_params,
 				  ARRAY_SIZE(nsim_devlink_params));
-	devlink_resources_unregister(devlink, NULL);
+	devlink_resources_unregister(devlink);
 	kfree(nsim_dev->vfconfigs);
 	devlink_free(devlink);
 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
diff --git a/include/net/devlink.h b/include/net/devlink.h
index e3c88fabd700..043fcec8b0aa 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -361,33 +361,6 @@ devlink_resource_size_params_init(struct devlink_resource_size_params *size_para
 
 typedef u64 devlink_resource_occ_get_t(void *priv);
 
-/**
- * struct devlink_resource - devlink resource
- * @name: name of the resource
- * @id: id, per devlink instance
- * @size: size of the resource
- * @size_new: updated size of the resource, reload is needed
- * @size_valid: valid in case the total size of the resource is valid
- *              including its children
- * @parent: parent resource
- * @size_params: size parameters
- * @list: parent list
- * @resource_list: list of child resources
- */
-struct devlink_resource {
-	const char *name;
-	u64 id;
-	u64 size;
-	u64 size_new;
-	bool size_valid;
-	struct devlink_resource *parent;
-	struct devlink_resource_size_params size_params;
-	struct list_head list;
-	struct list_head resource_list;
-	devlink_resource_occ_get_t *occ_get;
-	void *occ_get_priv;
-};
-
 #define DEVLINK_RESOURCE_ID_PARENT_TOP 0
 
 #define DEVLINK_RESOURCE_GENERIC_NAME_PORTS "physical_ports"
@@ -1571,8 +1544,7 @@ int devlink_resource_register(struct devlink *devlink,
 			      u64 resource_id,
 			      u64 parent_resource_id,
 			      const struct devlink_resource_size_params *size_params);
-void devlink_resources_unregister(struct devlink *devlink,
-				  struct devlink_resource *resource);
+void devlink_resources_unregister(struct devlink *devlink);
 int devlink_resource_size_get(struct devlink *devlink,
 			      u64 resource_id,
 			      u64 *p_resource_size);
diff --git a/net/core/devlink.c b/net/core/devlink.c
index fd21022145a3..db3b52110cf2 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -69,6 +69,35 @@ struct devlink {
 	char priv[] __aligned(NETDEV_ALIGN);
 };
 
+/**
+ * struct devlink_resource - devlink resource
+ * @name: name of the resource
+ * @id: id, per devlink instance
+ * @size: size of the resource
+ * @size_new: updated size of the resource, reload is needed
+ * @size_valid: valid in case the total size of the resource is valid
+ *              including its children
+ * @parent: parent resource
+ * @size_params: size parameters
+ * @list: parent list
+ * @resource_list: list of child resources
+ * @occ_get: occupancy getter callback
+ * @occ_get_priv: occupancy getter callback priv
+ */
+struct devlink_resource {
+	const char *name;
+	u64 id;
+	u64 size;
+	u64 size_new;
+	bool size_valid;
+	struct devlink_resource *parent;
+	struct devlink_resource_size_params size_params;
+	struct list_head list;
+	struct list_head resource_list;
+	devlink_resource_occ_get_t *occ_get;
+	void *occ_get_priv;
+};
+
 void *devlink_priv(struct devlink *devlink)
 {
 	return &devlink->priv;
@@ -9908,34 +9937,38 @@ int devlink_resource_register(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devlink_resource_register);
 
+static void devlink_resource_unregister(struct devlink *devlink,
+					struct devlink_resource *resource)
+{
+	struct devlink_resource *tmp, *child_resource;
+
+	list_for_each_entry_safe(child_resource, tmp, &resource->resource_list,
+				 list) {
+		devlink_resource_unregister(devlink, child_resource);
+		list_del(&child_resource->list);
+		kfree(child_resource);
+	}
+}
+
 /**
  *	devlink_resources_unregister - free all resources
  *
  *	@devlink: devlink
- *	@resource: resource
  */
-void devlink_resources_unregister(struct devlink *devlink,
-				  struct devlink_resource *resource)
+void devlink_resources_unregister(struct devlink *devlink)
 {
 	struct devlink_resource *tmp, *child_resource;
-	struct list_head *resource_list;
-
-	if (resource)
-		resource_list = &resource->resource_list;
-	else
-		resource_list = &devlink->resource_list;
 
-	if (!resource)
-		mutex_lock(&devlink->lock);
+	mutex_lock(&devlink->lock);
 
-	list_for_each_entry_safe(child_resource, tmp, resource_list, list) {
-		devlink_resources_unregister(devlink, child_resource);
+	list_for_each_entry_safe(child_resource, tmp, &devlink->resource_list,
+				 list) {
+		devlink_resource_unregister(devlink, child_resource);
 		list_del(&child_resource->list);
 		kfree(child_resource);
 	}
 
-	if (!resource)
-		mutex_unlock(&devlink->lock);
+	mutex_unlock(&devlink->lock);
 }
 EXPORT_SYMBOL_GPL(devlink_resources_unregister);
 
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index ea5169e671ae..d9d0d227092c 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -406,7 +406,7 @@ EXPORT_SYMBOL_GPL(dsa_devlink_resource_register);
 
 void dsa_devlink_resources_unregister(struct dsa_switch *ds)
 {
-	devlink_resources_unregister(ds->devlink, NULL);
+	devlink_resources_unregister(ds->devlink);
 }
 EXPORT_SYMBOL_GPL(dsa_devlink_resources_unregister);
 
-- 
2.33.1

Re: [PATCH net-next v1] devlink: Simplify devlink resources unregister call

[Jakub Kicinski]

On Sun, 28 Nov 2021 14:42:44 +0200 Leon Romanovsky wrote:
> The devlink_resources_unregister() used second parameter as an
> entry point for the recursive removal of devlink resources. None
> of external to devlink users needed to use this field, so lat's

None of the callers outside of devlink core...
s/lat/let/

> remove it.

> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index e3c88fabd700..043fcec8b0aa 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -361,33 +361,6 @@ devlink_resource_size_params_init(struct devlink_resource_size_params *size_para
>  
>  typedef u64 devlink_resource_occ_get_t(void *priv);
>  
> -/**
> - * struct devlink_resource - devlink resource
> - * @name: name of the resource
> - * @id: id, per devlink instance
> - * @size: size of the resource
> - * @size_new: updated size of the resource, reload is needed
> - * @size_valid: valid in case the total size of the resource is valid
> - *              including its children
> - * @parent: parent resource
> - * @size_params: size parameters
> - * @list: parent list
> - * @resource_list: list of child resources
> - */
> -struct devlink_resource {
> -	const char *name;
> -	u64 id;
> -	u64 size;
> -	u64 size_new;
> -	bool size_valid;
> -	struct devlink_resource *parent;
> -	struct devlink_resource_size_params size_params;
> -	struct list_head list;
> -	struct list_head resource_list;
> -	devlink_resource_occ_get_t *occ_get;
> -	void *occ_get_priv;
> -};

> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index fd21022145a3..db3b52110cf2 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -69,6 +69,35 @@ struct devlink {
>  	char priv[] __aligned(NETDEV_ALIGN);
>  };
>  
> +/**
> + * struct devlink_resource - devlink resource
> + * @name: name of the resource
> + * @id: id, per devlink instance
> + * @size: size of the resource
> + * @size_new: updated size of the resource, reload is needed
> + * @size_valid: valid in case the total size of the resource is valid
> + *              including its children
> + * @parent: parent resource
> + * @size_params: size parameters
> + * @list: parent list
> + * @resource_list: list of child resources
> + * @occ_get: occupancy getter callback
> + * @occ_get_priv: occupancy getter callback priv
> + */
> +struct devlink_resource {
> +	const char *name;
> +	u64 id;
> +	u64 size;
> +	u64 size_new;
> +	bool size_valid;
> +	struct devlink_resource *parent;
> +	struct devlink_resource_size_params size_params;
> +	struct list_head list;
> +	struct list_head resource_list;
> +	devlink_resource_occ_get_t *occ_get;
> +	void *occ_get_priv;
> +};

Hiding struct devlink_resource is not mentioned in the commit message
and entirely unrelated to removal of the unused argument.

Re: [PATCH net-next v1] devlink: Simplify devlink resources unregister call

[Leon Romanovsky]

On Mon, Nov 29, 2021 at 08:14:00PM -0800, Jakub Kicinski wrote:
> On Sun, 28 Nov 2021 14:42:44 +0200 Leon Romanovsky wrote:
> > The devlink_resources_unregister() used second parameter as an
> > entry point for the recursive removal of devlink resources. None
> > of external to devlink users needed to use this field, so lat's
> 
> None of the callers outside of devlink core...
> s/lat/let/
> 
> > remove it.
> 
> > diff --git a/include/net/devlink.h b/include/net/devlink.h
> > index e3c88fabd700..043fcec8b0aa 100644
> > --- a/include/net/devlink.h
> > +++ b/include/net/devlink.h
> > @@ -361,33 +361,6 @@ devlink_resource_size_params_init(struct devlink_resource_size_params *size_para
> >  
> >  typedef u64 devlink_resource_occ_get_t(void *priv);
> >  
> > -/**
> > - * struct devlink_resource - devlink resource
> > - * @name: name of the resource
> > - * @id: id, per devlink instance
> > - * @size: size of the resource
> > - * @size_new: updated size of the resource, reload is needed
> > - * @size_valid: valid in case the total size of the resource is valid
> > - *              including its children
> > - * @parent: parent resource
> > - * @size_params: size parameters
> > - * @list: parent list
> > - * @resource_list: list of child resources
> > - */
> > -struct devlink_resource {
> > -	const char *name;
> > -	u64 id;
> > -	u64 size;
> > -	u64 size_new;
> > -	bool size_valid;
> > -	struct devlink_resource *parent;
> > -	struct devlink_resource_size_params size_params;
> > -	struct list_head list;
> > -	struct list_head resource_list;
> > -	devlink_resource_occ_get_t *occ_get;
> > -	void *occ_get_priv;
> > -};
> 
> > diff --git a/net/core/devlink.c b/net/core/devlink.c
> > index fd21022145a3..db3b52110cf2 100644
> > --- a/net/core/devlink.c
> > +++ b/net/core/devlink.c
> > @@ -69,6 +69,35 @@ struct devlink {
> >  	char priv[] __aligned(NETDEV_ALIGN);
> >  };
> >  
> > +/**
> > + * struct devlink_resource - devlink resource
> > + * @name: name of the resource
> > + * @id: id, per devlink instance
> > + * @size: size of the resource
> > + * @size_new: updated size of the resource, reload is needed
> > + * @size_valid: valid in case the total size of the resource is valid
> > + *              including its children
> > + * @parent: parent resource
> > + * @size_params: size parameters
> > + * @list: parent list
> > + * @resource_list: list of child resources
> > + * @occ_get: occupancy getter callback
> > + * @occ_get_priv: occupancy getter callback priv
> > + */
> > +struct devlink_resource {
> > +	const char *name;
> > +	u64 id;
> > +	u64 size;
> > +	u64 size_new;
> > +	bool size_valid;
> > +	struct devlink_resource *parent;
> > +	struct devlink_resource_size_params size_params;
> > +	struct list_head list;
> > +	struct list_head resource_list;
> > +	devlink_resource_occ_get_t *occ_get;
> > +	void *occ_get_priv;
> > +};
> 
> Hiding struct devlink_resource is not mentioned in the commit message
> and entirely unrelated to removal of the unused argument.

devlink_resources_unregister() was the API function that is declared in
the devlink.h that used "struct devlink_resource". Once we removed extra
parameter from that function, the "struct devlink_resource" left as not
used at all. So this "hiding" is related and part of this simplification
patch.

I will add it to the commit message.

Thanks

Re: [PATCH net-next v1] devlink: Simplify devlink resources unregister call

[Jakub Kicinski]

On Tue, 30 Nov 2021 10:00:28 +0200 Leon Romanovsky wrote:
> > Hiding struct devlink_resource is not mentioned in the commit message
> > and entirely unrelated to removal of the unused argument.  
> 
> devlink_resources_unregister() was the API function that is declared in
> the devlink.h that used "struct devlink_resource". Once we removed extra
> parameter from that function, the "struct devlink_resource" left as not
> used at all. So this "hiding" is related and part of this simplification
> patch.
> 
> I will add it to the commit message.

Forward declarations exist. The author did not have to disclose the
definition of the structure. Removing this definition is not related,
you just like doing it.