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:

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>

Thanks for your patch!

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

quoted

+{
+       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.

quoted

+
+       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?

quoted

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

quoted

+
+       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 &&.

quoted

        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.

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