Messages in this thread | | | Subject | Re: [PATCH] clocksource/drivers/integrator-ap: add missing of_node_put() | From | Daniel Lezcano <> | Date | Sun, 25 Nov 2018 10:41:44 +0100 |
| |
On 25/11/2018 05:25, Frank Lee wrote: > On Sun, Nov 25, 2018 at 3:49 AM Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: >> >> On 24/11/2018 15:58, Frank Lee wrote: >>> On Thu, Nov 22, 2018 at 11:23 PM Yangtao Li <tiny.windzz@gmail.com> wrote: >>>> >>>> of_find_node_by_path() acquires a reference to the node >>>> returned by it and that reference needs to be dropped by its caller. >>>> integrator_ap_timer_init_of() doesn't do that, so fix it. >>>> >>>> Signed-off-by: Yangtao Li <tiny.windzz@gmail.com> >>>> --- >>>> drivers/clocksource/timer-integrator-ap.c | 20 ++++++++++++++------ >>>> 1 file changed, 14 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/clocksource/timer-integrator-ap.c b/drivers/clocksource/timer-integrator-ap.c >>>> index 76e526f58620..1f04069470bb 100644 >>>> --- a/drivers/clocksource/timer-integrator-ap.c >>>> +++ b/drivers/clocksource/timer-integrator-ap.c >>>> @@ -177,7 +177,7 @@ static int __init integrator_ap_timer_init_of(struct device_node *node) >>>> { >>>> const char *path; >>>> void __iomem *base; >>>> - int err; >>>> + int err,rc = 0; >>>> int irq; >>>> struct clk *clk; >>>> unsigned long rate; >>>> @@ -210,26 +210,34 @@ static int __init integrator_ap_timer_init_of(struct device_node *node) >>>> "arm,timer-secondary", &path); >>>> if (err) { >>>> pr_warn("Failed to read property\n"); >>>> - return err; >>>> + rc = err; >>>> + goto out_put_pri_node; >>>> } >>>> >>>> >>>> sec_node = of_find_node_by_path(path); >>>> >>>> - if (node == pri_node) >>>> + if (node == pri_node){ >>>> /* The primary timer lacks IRQ, use as clocksource */ >>>> - return integrator_clocksource_init(rate, base); >>>> + rc = integrator_clocksource_init(rate, base); >>>> + goto out; >>>> + } >>>> >>>> if (node == sec_node) { >>>> /* The secondary timer will drive the clock event */ >>>> irq = irq_of_parse_and_map(node, 0); >>>> - return integrator_clockevent_init(rate, base, irq); >>>> + rc = integrator_clockevent_init(rate, base, irq); >>>> + goto out; >>>> } >>>> >>>> pr_info("Timer @%p unused\n", base); >>>> clk_disable_unprepare(clk); >>>> +out: >>>> + of_node_put(sec_node); >>>> +out_put_pri_node: >>>> + of_node_put(pri_node); >>>> >>>> - return 0; >>>> + return rc; >>>> } >>>> >>>> TIMER_OF_DECLARE(integrator_ap_timer, "arm,integrator-timer", >>>> -- >>>> 2.17.0 >>>> >>> Hi Daniel: >>> >>> How about this? >> >> Hi, >> >> I think there is a simpler fix. The pri_node and the sec_node are used >> as an identifier to compare against the current node, we can directly >> drop the refcount after getting the node from path as it is not used as >> pointer. In addition, a single variable is needed, so we end up with a >> fix like that. >> >> alias_node = of_find_node_by_path(path); >> of_node_put(alias_node); >> if (node == alias_node) >> return integrator_clocksource_init(rate, base); > > Daniel, > > OK,I will simplify it like you said.Although I think of_node_put > should be called > after we don't use node.
Except I'm missing something, we don't use the alias_node at any time. The node itself has already a refcount which gives us the guarantee it is valid.
I agree it can appear a bit strange to put the node right after getting it but in our case the alias_node is used as an ID, not a pointer, so it is fine.
-- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
| |