Messages in this thread Patch in this message | | | Subject | Re: [PATCH v1] clk: Convert managed get functions to devm_add_action API | From | Marc Gonzalez <> | Date | Thu, 12 Dec 2019 14:53:40 +0100 |
| |
On 11/12/2019 23:28, Dmitry Torokhov wrote:
> On Wed, Dec 11, 2019 at 05:17:28PM +0100, Marc Gonzalez wrote: > >> What is the rationale for the devm_add_action API? > > For one-off and maybe complex unwind actions in drivers that wish to use > devm API (as mixing devm and manual release is verboten). Also is often > used when some core subsystem does not provide enough devm APIs.
Thanks for the insight, Dmitry. Thanks to Robin too.
This is what I understand so far:
devm_add_action() is nice because it hides/factorizes the complexity of the devres API, but it incurs a small storage overhead of one pointer per call, which makes it unfit for frequently used actions, such as clk_get.
Is that correct?
My question is: why not design the API without the small overhead?
Proof of concept below:
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..8db671823126 100644 --- a/drivers/clk/clk-devres.c +++ b/drivers/clk/clk-devres.c @@ -4,6 +4,11 @@ #include <linux/export.h> #include <linux/gfp.h> +static void __clk_put(struct device *dev, void *data) +{ + clk_put(*(struct clk **)data); +} + static void devm_clk_release(struct device *dev, void *res) { clk_put(*(struct clk **)res); @@ -11,19 +16,11 @@ static void devm_clk_release(struct device *dev, void *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); + struct clk *clk = clk_get(dev, id); - clk = clk_get(dev, id); - if (!IS_ERR(clk)) { - *ptr = clk; - devres_add(dev, ptr); - } else { - devres_free(ptr); - } + if (!IS_ERR(clk)) + if (!devm_add(dev, __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..5acb61ec39ab 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -970,6 +970,7 @@ void __iomem *devm_of_iomap(struct device *dev, resource_size_t *size); /* allows to add/remove a custom action to devres stack */ +void *devm_add(struct device *dev, dr_release_t func, void *arg, size_t size); int devm_add_action(struct device *dev, void (*action)(void *), void *data); void devm_remove_action(struct device *dev, void (*action)(void *), void *data); void devm_release_action(struct device *dev, void (*action)(void *), void *data);
| |