Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Thu, 23 Jan 2020 11:32:32 +0100 | Subject | 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 <marc.w.gonzalez@free.fr> wrote: > 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@free.fr>
> >> --- 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.
> >> +{ > >> + 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
| |