| Message ID | 56c7b6d5-1248-15bd-8441-5d80557455b3@free.fr (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [RFC,v2] clk: Use a new helper in managed functions | expand |
Hi Marc, On Wed, Jan 22, 2020 at 2:02 PM Marc Gonzalez <marc.w.gonzalez@free.fr> 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> 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) I there any advantage of using dr_release_t over "void (*action)(void *)", like devm_add_action() does? The latter lacks the "device *" parameter. > +{ > + 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 { ...} > + > + return data; Why return data or NULL, instead of 0 or -Efoo, like devm_add_action()? > +} > +EXPORT_SYMBOL_GPL(devm_add); > + > /* > * Custom devres actions allow inserting a simple function call > * into the teadown sequence. > diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c > index be160764911b..582fda9ad6a6 100644 > --- a/drivers/clk/clk-devres.c > +++ b/drivers/clk/clk-devres.c > @@ -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). > } > EXPORT_SYMBOL(devm_clk_get_optional); > > @@ -45,7 +34,7 @@ struct clk_bulk_devres { > int num_clks; > }; > > -static void devm_clk_bulk_release(struct device *dev, void *res) > +static void wrap_clk_bulk_put(struct device *dev, void *res) > { > struct clk_bulk_devres *devres = res; > > @@ -55,25 +44,17 @@ static void devm_clk_bulk_release(struct device *dev, void *res) > static int __devm_clk_bulk_get(struct device *dev, int num_clks, > struct clk_bulk_data *clks, bool optional) > { > - struct clk_bulk_devres *devres; > int ret; > - > - devres = devres_alloc(devm_clk_bulk_release, > - sizeof(*devres), GFP_KERNEL); > - if (!devres) > - return -ENOMEM; > + struct clk_bulk_devres arg = { clks, num_clks }; > > if (optional) > ret = clk_bulk_get_optional(dev, num_clks, clks); > else > ret = clk_bulk_get(dev, num_clks, clks); > - if (!ret) { > - devres->clks = clks; > - devres->num_clks = num_clks; > - devres_add(dev, devres); > - } else { > - devres_free(devres); > - } > + > + 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; (+ consider devm_add() returning the error code instead, cfr. above). 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. Gr{oetje,eeting}s, Geert
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> > > 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.
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
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. 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. >>>> + 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. 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. Regards.
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
diff --git a/drivers/base/devres.c b/drivers/base/devres.c index 0bbb328bd17f..76392dd6273b 100644 --- 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) +{ + void *data = devres_alloc(func, size, GFP_KERNEL); + + if (data) { + memcpy(data, arg, size); + devres_add(dev, data); + } else + func(dev, arg); + + return data; +} +EXPORT_SYMBOL_GPL(devm_add); + /* * Custom devres actions allow inserting a simple function call * into the teadown sequence. diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c index be160764911b..582fda9ad6a6 100644 --- a/drivers/clk/clk-devres.c +++ b/drivers/clk/clk-devres.c @@ -4,26 +4,18 @@ #include <linux/export.h> #include <linux/gfp.h> -static void devm_clk_release(struct device *dev, void *res) +static void wrap_clk_put(struct device *dev, void *res) { clk_put(*(struct clk **)res); } struct clk *devm_clk_get(struct device *dev, const char *id) { - struct clk **ptr, *clk; - - ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL); - if (!ptr) - return ERR_PTR(-ENOMEM); - - clk = clk_get(dev, id); - if (!IS_ERR(clk)) { - *ptr = clk; - devres_add(dev, ptr); - } else { - devres_free(ptr); - } + struct clk *clk = clk_get(dev, id); + + if (!IS_ERR(clk)) + if (!devm_add(dev, wrap_clk_put, &clk, sizeof(clk))) + clk = ERR_PTR(-ENOMEM); return clk; } @@ -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; } EXPORT_SYMBOL(devm_clk_get_optional); @@ -45,7 +34,7 @@ struct clk_bulk_devres { int num_clks; }; -static void devm_clk_bulk_release(struct device *dev, void *res) +static void wrap_clk_bulk_put(struct device *dev, void *res) { struct clk_bulk_devres *devres = res; @@ -55,25 +44,17 @@ static void devm_clk_bulk_release(struct device *dev, void *res) static int __devm_clk_bulk_get(struct device *dev, int num_clks, struct clk_bulk_data *clks, bool optional) { - struct clk_bulk_devres *devres; int ret; - - devres = devres_alloc(devm_clk_bulk_release, - sizeof(*devres), GFP_KERNEL); - if (!devres) - return -ENOMEM; + struct clk_bulk_devres arg = { clks, num_clks }; if (optional) ret = clk_bulk_get_optional(dev, num_clks, clks); else ret = clk_bulk_get(dev, num_clks, clks); - if (!ret) { - devres->clks = clks; - devres->num_clks = num_clks; - devres_add(dev, devres); - } else { - devres_free(devres); - } + + if (!ret) + if (!devm_add(dev, wrap_clk_bulk_put, &arg, sizeof(arg))) + ret = -ENOMEM; return ret; } @@ -95,24 +76,16 @@ EXPORT_SYMBOL_GPL(devm_clk_bulk_get_optional); int __must_check devm_clk_bulk_get_all(struct device *dev, struct clk_bulk_data **clks) { - struct clk_bulk_devres *devres; - int ret; + struct clk_bulk_devres arg; - devres = devres_alloc(devm_clk_bulk_release, - sizeof(*devres), GFP_KERNEL); - if (!devres) - return -ENOMEM; - - ret = clk_bulk_get_all(dev, &devres->clks); - if (ret > 0) { - *clks = devres->clks; - devres->num_clks = ret; - devres_add(dev, devres); - } else { - devres_free(devres); - } + arg.num_clks = clk_bulk_get_all(dev, clks); + arg.clks = *clks; - return ret; + if (arg.num_clks > 0) + if (!devm_add(dev, wrap_clk_bulk_put, &arg, sizeof(arg))) + arg.num_clks = -ENOMEM; + + return arg.num_clks; } EXPORT_SYMBOL_GPL(devm_clk_bulk_get_all); @@ -128,30 +101,18 @@ static int devm_clk_match(struct device *dev, void *res, void *data) void devm_clk_put(struct device *dev, struct clk *clk) { - int ret; - - ret = devres_release(dev, devm_clk_release, devm_clk_match, clk); - - WARN_ON(ret); + WARN_ON(devres_release(dev, wrap_clk_put, devm_clk_match, clk)); } EXPORT_SYMBOL(devm_clk_put); struct clk *devm_get_clk_from_child(struct device *dev, struct device_node *np, const char *con_id) { - struct clk **ptr, *clk; - - ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL); - if (!ptr) - return ERR_PTR(-ENOMEM); - - clk = of_clk_get_by_name(np, con_id); - if (!IS_ERR(clk)) { - *ptr = clk; - devres_add(dev, ptr); - } else { - devres_free(ptr); - } + struct clk *clk = of_clk_get_by_name(np, con_id); + + if (!IS_ERR(clk)) + if (!devm_add(dev, wrap_clk_put, &clk, sizeof(clk))) + clk = ERR_PTR(-ENOMEM); return clk; } diff --git a/include/linux/device.h b/include/linux/device.h index e226030c1df3..d47872c5ab72 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -969,6 +969,7 @@ void __iomem *devm_of_iomap(struct device *dev, struct device_node *node, int index, resource_size_t *size); +void *devm_add(struct device *dev, dr_release_t func, void *arg, size_t size); /* allows to add/remove a custom action to devres stack */ int devm_add_action(struct device *dev, void (*action)(void *), void *data); void devm_remove_action(struct device *dev, void (*action)(void *), void *data);
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> --- drivers/base/devres.c | 14 ++++++ drivers/clk/clk-devres.c | 93 ++++++++++++---------------------------- include/linux/device.h | 1 + 3 files changed, 42 insertions(+), 66 deletions(-)