linux-kernel - Re: [RFC PATCH v2] clk: Use a new helper in managed functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 23 Jan 2020 11:13:33 +0100
From:   Marc Gonzalez <marc.w.gonzalez@...e.fr>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     linux-clk <linux-clk@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
        Russell King <linux@...linux.org.uk>,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Robin Murphy <robin.murphy@....com>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [RFC PATCH v2] clk: Use a new helper in managed functions

On 22/01/2020 14:33, Geert Uytterhoeven wrote:

> On Wed, Jan 22, 2020 at 2:02 PM Marc Gonzalez wrote:
>
>> Introduce devm_add() to factorize devres_alloc/devres_add calls.
>>
>> Using that helper produces simpler code and smaller object size:
>>
>> 1 file changed, 27 insertions(+), 66 deletions(-)
>>
>>     text           data     bss     dec     hex filename
>> -   1708             80       0    1788     6fc drivers/clk/clk-devres.o
>> +   1508             80       0    1588     634 drivers/clk/clk-devres.o
>>
>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@...e.fr>
> 
> Thanks for your patch!
> 
>> --- a/drivers/base/devres.c
>> +++ b/drivers/base/devres.c
>> @@ -685,6 +685,20 @@ int devres_release_group(struct device *dev, void *id)
>>  }
>>  EXPORT_SYMBOL_GPL(devres_release_group);
>>
>> +void *devm_add(struct device *dev, dr_release_t func, void *arg, size_t size)
> 
> Is there any advantage of using dr_release_t over "void (*action)(void *)",
> like devm_add_action() does?  The latter lacks the "device *" parameter.

(I did forget to mention that v1 used devm_add_action.)
https://patchwork.kernel.org/patch/11262685/

A limitation of devm_add_action is that it stores the void *data argument "as is".
Users cannot pass the address of a struct on the stack. devm_add() addresses that
specific use-case, while being a minimal wrapper around devres_alloc + devres_add.
(devm_add_action adds an extra level of indirection.)

>> +{
>> +       void *data = devres_alloc(func, size, GFP_KERNEL);
>> +
>> +       if (data) {
>> +               memcpy(data, arg, size);
>> +               devres_add(dev, data);
>> +       } else
>> +               func(dev, arg);
> 
> Both branchs should use { ...}

Ah yes, scripts/checkpatch.pl needs --strict to point this out.

>> +
>> +       return data;
> 
> Why return data or NULL, instead of 0 or -Efoo, like devm_add_action()?

My intent is to make devm_add a minimal wrapper (it even started out as
a macro). As such, I just transparently pass the result of devres_alloc.

Do you see an advantage in processing the result?

>> @@ -33,10 +25,7 @@ struct clk *devm_clk_get_optional(struct device *dev, const char *id)
>>  {
>>         struct clk *clk = devm_clk_get(dev, id);
>>
>> -       if (clk == ERR_PTR(-ENOENT))
>> -               return NULL;
>> -
>> -       return clk;
>> +       return clk == ERR_PTR(-ENOENT) ? NULL : clk;
> 
> Unrelated change (which is less readable than the original, IMHO).

I'd like to hear the maintainers' opinion. I defer to their preference.

>> +
>> +       if (!ret)
>> +               if (!devm_add(dev, wrap_clk_bulk_put, &arg, sizeof(arg)))
>> +                       ret = -ENOMEM;
> 
> Nested ifs are easier to read when the outer one uses curly braces:
> 
>         if (!ret) {
>                 if (!devm_add(dev, wrap_clk_bulk_put, &arg, sizeof(arg)))
>                         ret = -ENOMEM;
>         }
> 
> Or merge the condition with &&.
> 
>>
>>         return ret;
> 
> But in this case, I would write it as:
> 
>         if (ret)
>                 return ret;
> 
>         if (!devm_add(dev, wrap_clk_bulk_put, &arg, sizeof(arg)))
>                 return -ENOMEM;
> 
>         return 0;

I like the simplicity of this code.


> (+ consider devm_add() returning the error code instead, cfr. above).

Some functions return an int, some a pointer, some might store the
result through a pointer.


> BTW, I'm still wondering if the varargs macro discussed on #armlinux would
> help.  I.e.
> 
>     devm_add(dev, wrap_clk_bulk_put, struct clk_bulk_devres, clks, num_clks)
> 
> would create and populate the temporary arg variable.
> 
> That would require defining an argument struct for the use in devm_clk_get(),
> though.

There could be a helper for the "pass-a-struct" use-case, using a compound literal:

#define helper(dev, func, type, args...) devm_add(dev, func, &(type){args}, sizeof(type))

Regards.