Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Thu, 23 Jan 2020 14:23:33 +0100 | Subject | Re: [RFC PATCH v2] clk: Use a new helper in managed functions |
| |
Hi Marc,
On Thu, Jan 23, 2020 at 1:18 PM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote: > On 23/01/2020 11:32, Geert Uytterhoeven wrote: > > On Thu, Jan 23, 2020 at 11:13 AM Marc Gonzalez wrote: > >> 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. > > I'm confused... > > void *devres_alloc(dr_release_t release, size_t size, gfp_t gfp); > int devm_add_action(struct device *dev, void (*action)(void *), void *data); > > devres_alloc() expects a dr_release_t argument; devm_add() is a thin wrapper > around devres_alloc(); ergo devm_add() expects that dr_release_t argument.
OK.
> devm_add_action() is a "heavier" wrapper around devres_alloc() which defines > a "private" release function which calls a user-defined "action". > (i.e. the extra level of indirection I mentioned above.) > > I don't understand the question about the advantage of dr_release_t.
OK. So devm_add_action() is the odd man out there.
> >>>> + 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. > > I think I see your point. You're saying it's not good to kick the can down > the road, because callers won't know what to do with the pointer.
Exactly.
> Actually, I'm in the same boat as these users. I looked at > devres_alloc -> devres_alloc_node -> alloc_dr -> kmalloc_node_track_caller -> __do_kmalloc > > Basically, the result is NULL when something went wrong, but the actual > error condition is not propagated. It could be: > 1) check_add_overflow() finds an overflow > 2) size > KMALLOC_MAX_CACHE_SIZE > 3) kmalloc_slab() or kasan_kmalloc() fail > 4) different errors on the CONFIG_NUMA path > > Basically, if lower-level functions don't propagate errors, it's not > easy for a wrapper to do something sensible... ENOMEM looks reasonable > for kmalloc-related failures.
Indeed. If devm_add() would return an error code, callers could just check for error, and propagate the error code, without a need for hardcoding -ENOMEM.
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
| |