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 <[email protected]>
---
drivers/clk/clk-devres.c | 101 ++++++++++++++-------------------------
1 file changed, 37 insertions(+), 64 deletions(-)
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index be160764911b..3f4b200b5328 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,25 +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);
- }
+
+ if (ret)
+ return ret;
+
+ ret = devm_vadd(dev, my_clk_bulk_put, clk_bulk_args, num_clks, clks);
return ret;
}
@@ -95,24 +83,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);
@@ -128,30 +109,22 @@ static int devm_clk_match(struct device *dev, void *res, void *data)
void devm_clk_put(struct device *dev, struct clk *clk)
{
- int ret;
-
- ret = devres_release(dev, devm_clk_release, devm_clk_match, clk);
-
- WARN_ON(ret);
+ WARN_ON(devres_release(dev, my_clk_put, devm_clk_match, clk));
}
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
Hi Marc,
Thanks for your patch!
On Wed, Feb 26, 2020 at 4:55 PM Marc Gonzalez <[email protected]> wrote:
> 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
And the size reduction could have been even more ;-)
> Signed-off-by: Marc Gonzalez <[email protected]>
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -55,25 +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);
> - }
> +
> + if (ret)
> + return ret;
> +
> + ret = devm_vadd(dev, my_clk_bulk_put, clk_bulk_args, num_clks, clks);
>
> return ret;
return devm_vadd(...);
> }
> @@ -128,30 +109,22 @@ static int devm_clk_match(struct device *dev, void *res, void *data)
>
> void devm_clk_put(struct device *dev, struct clk *clk)
> {
> - int ret;
> -
> - ret = devres_release(dev, devm_clk_release, devm_clk_match, clk);
> -
> - WARN_ON(ret);
> + WARN_ON(devres_release(dev, my_clk_put, devm_clk_match, clk));
Getting rid of "ret" is an unrelated change, which actually increases
kernel size, as the WARN_ON() parameter is stringified for the warning
message.
The rest looks good, so with the above fixed:
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 27/02/2020 14:36, Geert Uytterhoeven wrote:
> Hi Marc,
>
> Thanks for your patch!
>
> On Wed, Feb 26, 2020 at 4:55 PM Marc Gonzalez <[email protected]> wrote:
>> 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
>
> And the size reduction could have been even more ;-)
I'll see what I can do! ;-)
I have another patch with even smaller object code, but it requires
C11 to be well-defined (memcmp the whole struct, which requires zeros
in the padding holes).
>> --- a/drivers/clk/clk-devres.c
>> +++ b/drivers/clk/clk-devres.c
>
>> @@ -55,25 +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);
>> - }
>> +
>> + if (ret)
>> + return ret;
>> +
>> + ret = devm_vadd(dev, my_clk_bulk_put, clk_bulk_args, num_clks, clks);
>>
>> return ret;
>
> return devm_vadd(...);
If you think that makes it look better, I'll make the change!
>> @@ -128,30 +109,22 @@ static int devm_clk_match(struct device *dev, void *res, void *data)
>>
>> void devm_clk_put(struct device *dev, struct clk *clk)
>> {
>> - int ret;
>> -
>> - ret = devres_release(dev, devm_clk_release, devm_clk_match, clk);
>> -
>> - WARN_ON(ret);
>> + WARN_ON(devres_release(dev, my_clk_put, devm_clk_match, clk));
>
> Getting rid of "ret" is an unrelated change, which actually increases
> kernel size, as the WARN_ON() parameter is stringified for the warning
> message.
Weird... Are you sure about that? I built the preprocessed file,
and it didn't appear to be so.
#ifndef WARN_ON
#define WARN_ON(condition) ({ \
int __ret_warn_on = !!(condition); \
if (unlikely(__ret_warn_on)) \
__WARN(); \
unlikely(__ret_warn_on); \
})
#endif
Maybe you were thinking of i915's WARN_ON?
#define WARN_ON(x) WARN((x), "%s", "WARN_ON(" __stringify(x) ")")
Regards.
Hi Marc,
On Mon, Mar 2, 2020 at 11:01 AM Marc Gonzalez <[email protected]> wrote:
> On 27/02/2020 14:36, Geert Uytterhoeven wrote:
> > On Wed, Feb 26, 2020 at 4:55 PM Marc Gonzalez <[email protected]> wrote:
> >> Using the helper produces simpler code, and smaller object size.
> >> --- a/drivers/clk/clk-devres.c
> >> +++ b/drivers/clk/clk-devres.c
> >> @@ -128,30 +109,22 @@ static int devm_clk_match(struct device *dev, void *res, void *data)
> >>
> >> void devm_clk_put(struct device *dev, struct clk *clk)
> >> {
> >> - int ret;
> >> -
> >> - ret = devres_release(dev, devm_clk_release, devm_clk_match, clk);
> >> -
> >> - WARN_ON(ret);
> >> + WARN_ON(devres_release(dev, my_clk_put, devm_clk_match, clk));
> >
> > Getting rid of "ret" is an unrelated change, which actually increases
> > kernel size, as the WARN_ON() parameter is stringified for the warning
> > message.
>
> Weird... Are you sure about that? I built the preprocessed file,
> and it didn't appear to be so.
>
> #ifndef WARN_ON
> #define WARN_ON(condition) ({ \
> int __ret_warn_on = !!(condition); \
> if (unlikely(__ret_warn_on)) \
> __WARN(); \
> unlikely(__ret_warn_on); \
> })
> #endif
>
> Maybe you were thinking of i915's WARN_ON?
>
> #define WARN_ON(x) WARN((x), "%s", "WARN_ON(" __stringify(x) ")")
Oops, you're right. I got trapped again by an override of a standard macro
(IMHO this should be removed).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds