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

Hi Marc,

On Thu, Jan 23, 2020 at 11:13 AM Marc Gonzalez [off-list ref] wrote:

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

quoted

On Wed, Jan 22, 2020 at 2:02 PM Marc Gonzalez wrote:

quoted

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 <redacted>

quoted

quoted

--- 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.)

I didn't mean the advantage of devm_add() over devm_add_action(),
but the advantage of dr_release_t, which has a device pointer.

quoted

quoted

+{
+       void *data = devres_alloc(func, size, GFP_KERNEL);
+
+       if (data) {
+               memcpy(data, arg, size);
+               devres_add(dev, data);
+       } else
+               func(dev, arg);
+
+       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?

There are actually two questions to consider here:
  1. Is there a use case for returning the data pointer?
     I.e. will the caller ever use it?
  2. Can there be another failure mode than out-of-memory?
     Changing from NULL to ERR_PTR() later means that all callers
     need to be updated.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel