Messages in this thread | | | Subject | Re: [PATCH v1] clk: Convert managed get functions to devm_add_action API | From | Marc Gonzalez <> | Date | Thu, 12 Dec 2019 15:41:20 +0100 |
| |
On 12/12/2019 15:17, Russell King - ARM Linux admin wrote:
> On Thu, Dec 12, 2019 at 02:53:40PM +0100, Marc Gonzalez wrote: > >> 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); > > You leak clk here.
I don't think so ;-)
If devm_add() returns NULL, then we have called __clk_put(dev, &clk);
Regards.
| |