[PATCH v5 0/2] Small devm helper for devm implementations

[PATCH v5 0/2] Small devm helper for devm implementations

[Marc Gonzalez]

Hello,

Differences from v4 to v5
x Fix the grammar in devm_add comments [Geert]
x Undo an unrelated change in devm_clk_put [Geert]

Differences from v3 to v4
x Add a bunch of kerneldoc above devm_add() [Greg KH]
x Split patch in two [Greg KH]

Differences from v2 to v3
x Make devm_add() return an error-code rather than the raw data pointer
  (in case devres_alloc ever returns an ERR_PTR) as suggested by Geert
x Provide a variadic version devm_vadd() to work with structs as suggested
  by Geert
x Don't use nested ifs in clk_devm* implementations (hopefully simpler
  code logic to follow) as suggested by Geert

Marc Gonzalez (2):
  devres: Provide new helper for devm functions
  clk: Use devm_add in managed functions

 drivers/base/devres.c    | 28 ++++++++++++
 drivers/clk/clk-devres.c | 97 +++++++++++++++-------------------------
 include/linux/device.h   |  3 ++
 3 files changed, 67 insertions(+), 61 deletions(-)

-- 
2.17.1

[PATCH v5 1/2] devres: Provide new helper for devm functions

[Marc Gonzalez]

Provide a simple wrapper for devres_alloc / devres_add.

Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/base/devres.c  | 28 ++++++++++++++++++++++++++++
 include/linux/device.h |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 0bbb328bd17f..b4c18c105f39 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -685,6 +685,34 @@ int devres_release_group(struct device *dev, void *id)
 }
 EXPORT_SYMBOL_GPL(devres_release_group);
 
+/**
+ * devm_add - allocate and register new device resource
+ * @dev: device to add resource to
+ * @func: resource release function
+ * @arg: resource data
+ * @size: resource data size
+ *
+ * Simple wrapper for devres_alloc / devres_add.
+ * Releases the resource if the allocation failed.
+ *
+ * RETURNS:
+ * 0 on success, -ENOMEM otherwise.
+ */
+int devm_add(struct device *dev, dr_release_t func, void *arg, size_t size)
+{
+	void *data = devres_alloc(func, size, GFP_KERNEL);
+
+	if (!data) {
+		func(dev, arg);
+		return -ENOMEM;
+	}
+
+	memcpy(data, arg, size);
+	devres_add(dev, data);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_add);
+
 /*
  * Custom devres actions allow inserting a simple function call
  * into the teadown sequence.
diff --git a/include/linux/device.h b/include/linux/device.h
index 0cd7c647c16c..55be3be9b276 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -247,6 +247,9 @@ void __iomem *devm_of_iomap(struct device *dev,
 			    struct device_node *node, int index,
 			    resource_size_t *size);
 
+int devm_add(struct device *dev, dr_release_t func, void *arg, size_t size);
+#define devm_vadd(dev, func, type, args...) \
+	devm_add(dev, func, &(struct type){args}, sizeof(struct type))
 /* allows to add/remove a custom action to devres stack */
 int devm_add_action(struct device *dev, void (*action)(void *), void *data);
 void devm_remove_action(struct device *dev, void (*action)(void *), void *data);
-- 
2.17.1

[PATCH v5 2/2] clk: Use devm_add in managed functions

[Marc Gonzalez]

Using the helper produces simpler code and smaller object size.
E.g. with gcc-arm-9.2-2019.12-x86_64-aarch64-none-linux-gnu:

    text           data     bss     dec     hex filename
-   1708             80       0    1788     6fc drivers/clk/clk-devres.o
+   1524             80       0    1604     644 drivers/clk/clk-devres.o

Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This patch needs testing on a platform with many clocks.
---
 drivers/clk/clk-devres.c | 97 +++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 61 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index be160764911b..537fabf3a2a4 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -4,26 +4,22 @@
 #include <linux/export.h>
 #include <linux/gfp.h>
 
-static void devm_clk_release(struct device *dev, void *res)
+static void my_clk_put(struct device *dev, void *res)
 {
 	clk_put(*(struct clk **)res);
 }
 
 struct clk *devm_clk_get(struct device *dev, const char *id)
 {
-	struct clk **ptr, *clk;
-
-	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
-
-	clk = clk_get(dev, id);
-	if (!IS_ERR(clk)) {
-		*ptr = clk;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	int ret;
+	struct clk *clk = clk_get(dev, id);
+
+	if (IS_ERR(clk))
+		return clk;
+
+	ret = devm_add(dev, my_clk_put, &clk, sizeof(clk));
+	if (ret)
+		return ERR_PTR(ret);
 
 	return clk;
 }
@@ -40,14 +36,14 @@ struct clk *devm_clk_get_optional(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL(devm_clk_get_optional);
 
-struct clk_bulk_devres {
-	struct clk_bulk_data *clks;
+struct clk_bulk_args {
 	int num_clks;
+	struct clk_bulk_data *clks;
 };
 
-static void devm_clk_bulk_release(struct device *dev, void *res)
+static void my_clk_bulk_put(struct device *dev, void *res)
 {
-	struct clk_bulk_devres *devres = res;
+	struct clk_bulk_args *devres = res;
 
 	clk_bulk_put(devres->num_clks, devres->clks);
 }
@@ -55,27 +51,17 @@ static void devm_clk_bulk_release(struct device *dev, void *res)
 static int __devm_clk_bulk_get(struct device *dev, int num_clks,
 			       struct clk_bulk_data *clks, bool optional)
 {
-	struct clk_bulk_devres *devres;
 	int ret;
 
-	devres = devres_alloc(devm_clk_bulk_release,
-			      sizeof(*devres), GFP_KERNEL);
-	if (!devres)
-		return -ENOMEM;
-
 	if (optional)
 		ret = clk_bulk_get_optional(dev, num_clks, clks);
 	else
 		ret = clk_bulk_get(dev, num_clks, clks);
-	if (!ret) {
-		devres->clks = clks;
-		devres->num_clks = num_clks;
-		devres_add(dev, devres);
-	} else {
-		devres_free(devres);
-	}
 
-	return ret;
+	if (ret)
+		return ret;
+
+	return devm_vadd(dev, my_clk_bulk_put, clk_bulk_args, num_clks, clks);
 }
 
 int __must_check devm_clk_bulk_get(struct device *dev, int num_clks,
@@ -95,24 +81,17 @@ EXPORT_SYMBOL_GPL(devm_clk_bulk_get_optional);
 int __must_check devm_clk_bulk_get_all(struct device *dev,
 				       struct clk_bulk_data **clks)
 {
-	struct clk_bulk_devres *devres;
 	int ret;
+	int num_clks = clk_bulk_get_all(dev, clks);
 
-	devres = devres_alloc(devm_clk_bulk_release,
-			      sizeof(*devres), GFP_KERNEL);
-	if (!devres)
-		return -ENOMEM;
-
-	ret = clk_bulk_get_all(dev, &devres->clks);
-	if (ret > 0) {
-		*clks = devres->clks;
-		devres->num_clks = ret;
-		devres_add(dev, devres);
-	} else {
-		devres_free(devres);
-	}
+	if (num_clks <= 0)
+		return num_clks;
 
-	return ret;
+	ret = devm_vadd(dev, my_clk_bulk_put, clk_bulk_args, num_clks, *clks);
+	if (ret)
+		return ret;
+
+	return num_clks;
 }
 EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all);
 
@@ -130,7 +109,7 @@ void devm_clk_put(struct device *dev, struct clk *clk)
 {
 	int ret;
 
-	ret = devres_release(dev, devm_clk_release, devm_clk_match, clk);
+	ret = devres_release(dev, my_clk_put, devm_clk_match, clk);
 
 	WARN_ON(ret);
 }
@@ -139,19 +118,15 @@ EXPORT_SYMBOL(devm_clk_put);
 struct clk *devm_get_clk_from_child(struct device *dev,
 				    struct device_node *np, const char *con_id)
 {
-	struct clk **ptr, *clk;
-
-	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
-
-	clk = of_clk_get_by_name(np, con_id);
-	if (!IS_ERR(clk)) {
-		*ptr = clk;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+	int ret;
+	struct clk *clk = of_clk_get_by_name(np, con_id);
+
+	if (IS_ERR(clk))
+		return clk;
+
+	ret = devm_add(dev, my_clk_put, &clk, sizeof(clk));
+	if (ret)
+		return ERR_PTR(ret);
 
 	return clk;
 }
-- 
2.17.1

Re: [PATCH v5 0/2] Small devm helper for devm implementations

[Marc Gonzalez]

Looks like this series has fallen through the cracks :(
Greg, you would be taking the drivers/base/devres.c changes?
As mentioned in patch 2, "This patch needs testing on a platform with many clocks."
(I've only tested using a trivial kernel module)

On 10/03/2020 11:11, Marc Gonzalez wrote:

> Differences from v4 to v5
> x Fix the grammar in devm_add comments [Geert]
> x Undo an unrelated change in devm_clk_put [Geert]
> 
> Differences from v3 to v4
> x Add a bunch of kerneldoc above devm_add() [Greg KH]
> x Split patch in two [Greg KH]
> 
> Differences from v2 to v3
> x Make devm_add() return an error-code rather than the raw data pointer
>   (in case devres_alloc ever returns an ERR_PTR) as suggested by Geert
> x Provide a variadic version devm_vadd() to work with structs as suggested
>   by Geert
> x Don't use nested ifs in clk_devm* implementations (hopefully simpler
>   code logic to follow) as suggested by Geert
> 
> Marc Gonzalez (2):
>   devres: Provide new helper for devm functions
>   clk: Use devm_add in managed functions
> 
>  drivers/base/devres.c    | 28 ++++++++++++
>  drivers/clk/clk-devres.c | 97 +++++++++++++++-------------------------
>  include/linux/device.h   |  3 ++
>  3 files changed, 67 insertions(+), 61 deletions(-)

Re: [PATCH v5 0/2] Small devm helper for devm implementations

[Greg Kroah-Hartman]

On Thu, Jun 04, 2020 at 06:13:21PM +0200, Marc Gonzalez wrote:
> Looks like this series has fallen through the cracks :(
> Greg, you would be taking the drivers/base/devres.c changes?
> As mentioned in patch 2, "This patch needs testing on a platform with many clocks."
> (I've only tested using a trivial kernel module)

I can't do anything during the merge window, sorry.  Please feel free to
resend it after 5.8-rc1 is out and I will be glad to review it then.

thanks,

greg k-h

Re: [PATCH v5 0/2] Small devm helper for devm implementations

[Marc Gonzalez]

Hello everyone,

In my opinion, the small and simple devm_add() helper
(and its cousin, devm_vadd) can help make devm code
slightly easier to write and maintain.

Would anyone care to agree or disagree? :-)

Regards.


On 10/03/2020 11:11, Marc Gonzalez wrote:

> Differences from v4 to v5
> x Fix the grammar in devm_add comments [Geert]
> x Undo an unrelated change in devm_clk_put [Geert]
> 
> Differences from v3 to v4
> x Add a bunch of kerneldoc above devm_add() [Greg KH]
> x Split patch in two [Greg KH]
> 
> Differences from v2 to v3
> x Make devm_add() return an error-code rather than the raw data pointer
>   (in case devres_alloc ever returns an ERR_PTR) as suggested by Geert
> x Provide a variadic version devm_vadd() to work with structs as suggested
>   by Geert
> x Don't use nested ifs in clk_devm* implementations (hopefully simpler
>   code logic to follow) as suggested by Geert
> 
> Marc Gonzalez (2):
>   devres: Provide new helper for devm functions
>   clk: Use devm_add in managed functions
> 
>  drivers/base/devres.c    | 28 ++++++++++++
>  drivers/clk/clk-devres.c | 97 +++++++++++++++-------------------------
>  include/linux/device.h   |  3 ++
>  3 files changed, 67 insertions(+), 61 deletions(-)

Re: [PATCH v5 0/2] Small devm helper for devm implementations

[Marc Gonzalez]

Hello Greg,

Would you agree to take this series?

Regards.

On 18/06/2020 13:38, Marc Gonzalez wrote:

> Hello everyone,
> 
> In my opinion, the small and simple devm_add() helper
> (and its cousin, devm_vadd) can help make devm code
> slightly easier to write and maintain.
> 
> Would anyone care to agree or disagree? :-)
> 
> Regards.
> 
> On 10/03/2020 11:11, Marc Gonzalez wrote:
> 
>> Differences from v4 to v5
>> x Fix the grammar in devm_add comments [Geert]
>> x Undo an unrelated change in devm_clk_put [Geert]
>>
>> Differences from v3 to v4
>> x Add a bunch of kerneldoc above devm_add() [Greg KH]
>> x Split patch in two [Greg KH]
>>
>> Differences from v2 to v3
>> x Make devm_add() return an error-code rather than the raw data pointer
>>   (in case devres_alloc ever returns an ERR_PTR) as suggested by Geert
>> x Provide a variadic version devm_vadd() to work with structs as suggested
>>   by Geert
>> x Don't use nested ifs in clk_devm* implementations (hopefully simpler
>>   code logic to follow) as suggested by Geert
>>
>> Marc Gonzalez (2):
>>   devres: Provide new helper for devm functions
>>   clk: Use devm_add in managed functions
>>
>>  drivers/base/devres.c    | 28 ++++++++++++
>>  drivers/clk/clk-devres.c | 97 +++++++++++++++-------------------------
>>  include/linux/device.h   |  3 ++
>>  3 files changed, 67 insertions(+), 61 deletions(-)

Re: [PATCH v5 0/2] Small devm helper for devm implementations

[Greg Kroah-Hartman]

On Mon, Jul 06, 2020 at 05:56:04PM +0200, Marc Gonzalez wrote:
> Hello Greg,
> 
> Would you agree to take this series?

Given the lack of testing of the patch, it doesn't seem wise to add
this, right?

Please get some testing, and some more users, and I'll be glad to
consider it.

thanks,

greg k-h

Re: [PATCH v5 0/2] Small devm helper for devm implementations

[Marc Gonzalez]

On 06/07/2020 21:57, Greg Kroah-Hartman wrote:

> Given the lack of testing of the patch, it doesn't seem wise to add
> this, right?

You're probably not wrong :)

> Please get some testing, and some more users, and I'll be glad to
> consider it.

"Users" == files modified to use the new helper?

How many files would you suggest? 3? 5? 10?

The idea being to have the helper gain some kind of "critical mass"?

Regards.

Re: [PATCH v5 0/2] Small devm helper for devm implementations

[Greg Kroah-Hartman]

On Tue, Jul 21, 2020 at 09:10:23AM +0200, Marc Gonzalez wrote:
> On 06/07/2020 21:57, Greg Kroah-Hartman wrote:
> 
> > Given the lack of testing of the patch, it doesn't seem wise to add
> > this, right?
> 
> You're probably not wrong :)
> 
> > Please get some testing, and some more users, and I'll be glad to
> > consider it.
> 
> "Users" == files modified to use the new helper?

Yes.

> How many files would you suggest? 3? 5? 10?

How many do you see that can use it?  I would suggest "all" :)

thanks,

greg k-h