Re: [PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent
Thanks for the feedback, I updated this patch and sent v2.
Also, I submitted a patch to fix the error handling path in
ttc_setup_clocksource(). Here is the link to it:
https://lore.kernel.org/patchwork/patch/1143242/
On Tue, Oct 22, 2019 at 3:51 AM Michal Simek wrote:
>
> On 22. 10. 19 10:26, Markus Elfring wrote:
> >> In the impelementation of ttc_setup_clockevent() the allocated memory
> >> for ttcce should be released if clk_notifier_register() fails.
> >
> > * Please avoid the copying of typos from previous change descriptions.
> >
> > * Under which circumstances will an “imperative mood” matter for you here?
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n151
> >
> >
> >> +++ b/drivers/clocksource/timer-cadence-ttc.c
> >> @@ -424,6 +424,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
> >> >ttc.clk_rate_change_nb);
> >> if (err) {
> >> pr_warn("Unable to register clock notifier.\n");
> >> +kfree(ttcce);
> >> return err;
> >> }
> >
> > This addition looks correct.
> > But I would prefer to move such exception handling code to the end of
> > this function implementation so that duplicate source code will be reduced.
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n450
>
> Just a note. Maybe you should also consider to fix this error path in
> ttc_setup_clocksource() when notifier also can fail that there is no
> need to continue with code execution.
>
> Thanks,
> Michal
--
Navid.
Re: [PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent
On 22. 10. 19 10:26, Markus Elfring wrote:
>> In the impelementation of ttc_setup_clockevent() the allocated memory
>> for ttcce should be released if clk_notifier_register() fails.
>
> * Please avoid the copying of typos from previous change descriptions.
>
> * Under which circumstances will an “imperative mood” matter for you here?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n151
>
>
>> +++ b/drivers/clocksource/timer-cadence-ttc.c
>> @@ -424,6 +424,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
>> >ttc.clk_rate_change_nb);
>> if (err) {
>> pr_warn("Unable to register clock notifier.\n");
>> +kfree(ttcce);
>> return err;
>> }
>
> This addition looks correct.
> But I would prefer to move such exception handling code to the end of
> this function implementation so that duplicate source code will be reduced.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n450
Just a note. Maybe you should also consider to fix this error path in
ttc_setup_clocksource() when notifier also can fail that there is no
need to continue with code execution.
Thanks,
Michal
Re: [PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent
> In the impelementation of ttc_setup_clockevent() the allocated memory
> for ttcce should be released if clk_notifier_register() fails.
* Please avoid the copying of typos from previous change descriptions.
* Under which circumstances will an “imperative mood” matter for you here?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n151
> +++ b/drivers/clocksource/timer-cadence-ttc.c
> @@ -424,6 +424,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
> >ttc.clk_rate_change_nb);
> if (err) {
> pr_warn("Unable to register clock notifier.\n");
> + kfree(ttcce);
> return err;
> }
This addition looks correct.
But I would prefer to move such exception handling code to the end of
this function implementation so that duplicate source code will be reduced.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n450
Regards,
Markus
[PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent
In the impelementation of ttc_setup_clockevent() the allocated memory
for ttcce should be released if clk_notifier_register() fails.
Fixes: 70504f311d4b ("clocksource/drivers/cadence_ttc: Convert init function to
return error")
Signed-off-by: Navid Emamdoost
---
drivers/clocksource/timer-cadence-ttc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/clocksource/timer-cadence-ttc.c
b/drivers/clocksource/timer-cadence-ttc.c
index 88fe2e9ba9a3..b40fc6581389 100644
--- a/drivers/clocksource/timer-cadence-ttc.c
+++ b/drivers/clocksource/timer-cadence-ttc.c
@@ -424,6 +424,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
>ttc.clk_rate_change_nb);
if (err) {
pr_warn("Unable to register clock notifier.\n");
+ kfree(ttcce);
return err;
}
--
2.17.1
