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 <[email protected]>
---
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,
&ttcce->ttc.clk_rate_change_nb);
if (err) {
pr_warn("Unable to register clock notifier.\n");
+ kfree(ttcce);
return err;
}
--
2.17.1
> 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,
> &ttcce->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
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,
>> &ttcce->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
In the implementation of ttc_setup_clockevent() release the allocated
memory for ttcce if clk_notifier_register() fails.
Fixes: 70504f311d4b ("clocksource/drivers/cadence_ttc: Convert init function to return error")
Signed-off-by: Navid Emamdoost <[email protected]>
---
Changes in v2:
- Added goto label for error handling
- Update description and fix typo
drivers/clocksource/timer-cadence-ttc.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
index 88fe2e9ba9a3..0caacbc67102 100644
--- a/drivers/clocksource/timer-cadence-ttc.c
+++ b/drivers/clocksource/timer-cadence-ttc.c
@@ -411,10 +411,8 @@ static int __init ttc_setup_clockevent(struct clk *clk,
ttcce->ttc.clk = clk;
err = clk_prepare_enable(ttcce->ttc.clk);
- if (err) {
- kfree(ttcce);
- return err;
- }
+ if (err)
+ goto release_ttcce;
ttcce->ttc.clk_rate_change_nb.notifier_call =
ttc_rate_change_clockevent_cb;
@@ -424,7 +422,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
&ttcce->ttc.clk_rate_change_nb);
if (err) {
pr_warn("Unable to register clock notifier.\n");
- return err;
+ goto release_ttcce;
}
ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
@@ -453,15 +451,18 @@ static int __init ttc_setup_clockevent(struct clk *clk,
err = request_irq(irq, ttc_clock_event_interrupt,
IRQF_TIMER, ttcce->ce.name, ttcce);
- if (err) {
- kfree(ttcce);
- return err;
- }
+ if (err)
+ goto release_ttcce;
clockevents_config_and_register(&ttcce->ce,
ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
return 0;
+
+release_ttcce:
+
+ kfree(ttcce);
+ return err;
}
/**
--
2.17.1
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 <[email protected]> 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,
> >> &ttcce->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.
On 23. 10. 19 6:31, Navid Emamdoost wrote:
> In the implementation of ttc_setup_clockevent() release the allocated
> memory for ttcce if clk_notifier_register() fails.
>
> Fixes: 70504f311d4b ("clocksource/drivers/cadence_ttc: Convert init function to return error")
> Signed-off-by: Navid Emamdoost <[email protected]>
> ---
> Changes in v2:
> - Added goto label for error handling
> - Update description and fix typo
>
> drivers/clocksource/timer-cadence-ttc.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
> index 88fe2e9ba9a3..0caacbc67102 100644
> --- a/drivers/clocksource/timer-cadence-ttc.c
> +++ b/drivers/clocksource/timer-cadence-ttc.c
> @@ -411,10 +411,8 @@ static int __init ttc_setup_clockevent(struct clk *clk,
> ttcce->ttc.clk = clk;
>
> err = clk_prepare_enable(ttcce->ttc.clk);
> - if (err) {
> - kfree(ttcce);
> - return err;
> - }
> + if (err)
> + goto release_ttcce;
>
> ttcce->ttc.clk_rate_change_nb.notifier_call =
> ttc_rate_change_clockevent_cb;
> @@ -424,7 +422,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
> &ttcce->ttc.clk_rate_change_nb);
> if (err) {
> pr_warn("Unable to register clock notifier.\n");
> - return err;
> + goto release_ttcce;
> }
>
> ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
> @@ -453,15 +451,18 @@ static int __init ttc_setup_clockevent(struct clk *clk,
>
> err = request_irq(irq, ttc_clock_event_interrupt,
> IRQF_TIMER, ttcce->ce.name, ttcce);
> - if (err) {
> - kfree(ttcce);
> - return err;
> - }
> + if (err)
> + goto release_ttcce;
>
> clockevents_config_and_register(&ttcce->ce,
> ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
>
> return 0;
> +
> +release_ttcce:
> +
> + kfree(ttcce);
> + return err;
> }
>
> /**
>
Acked-by: Michal Simek <[email protected]>
Thanks,
Michal
In the implementation of ttc_setup_clocksource() when
clk_notifier_register() fails the execution should go to error handling.
Additionally, to avoid memory leak the allocated memory for ttccs should
be released, too. So, goto error handling to release the memory and
return.
Fixes: e932900a3279 ("arm: zynq: Use standard timer binding")
Signed-off-by: Navid Emamdoost <[email protected]>
---
drivers/clocksource/timer-cadence-ttc.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
index 88fe2e9ba9a3..035e16bc17d3 100644
--- a/drivers/clocksource/timer-cadence-ttc.c
+++ b/drivers/clocksource/timer-cadence-ttc.c
@@ -328,10 +328,8 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
ttccs->ttc.clk = clk;
err = clk_prepare_enable(ttccs->ttc.clk);
- if (err) {
- kfree(ttccs);
- return err;
- }
+ if (err)
+ goto release_ttccs;
ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk);
@@ -341,8 +339,10 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
err = clk_notifier_register(ttccs->ttc.clk,
&ttccs->ttc.clk_rate_change_nb);
- if (err)
+ if (err) {
pr_warn("Unable to register clock notifier.\n");
+ goto release_ttccs;
+ }
ttccs->ttc.base_addr = base;
ttccs->cs.name = "ttc_clocksource";
@@ -363,16 +363,18 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
- if (err) {
- kfree(ttccs);
- return err;
- }
+ if (err)
+ goto release_ttccs;
ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET;
sched_clock_register(ttc_sched_clock_read, timer_width,
ttccs->ttc.freq / PRESCALE);
return 0;
+
+release_ttccs:
+ kfree(ttccs);
+ return err;
}
static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
--
2.17.1
> In the implementation of ttc_setup_clockevent() release the allocated
> memory for ttcce if clk_notifier_register() fails.
I got other wording preferences. Thus I imagine that such a change
description can still be improved another bit.
Would you like to express the addition of a jump target (according to
the Linux coding style) for the completion of desired exception handling
in a different way?
…
> +++ b/drivers/clocksource/timer-cadence-ttc.c
…
> @@ -453,15 +451,18 @@ static int __init ttc_setup_clockevent(struct clk *clk,
…
> +release_ttcce:
> +
> + kfree(ttcce);
…
I would prefer that a blank line will not be added directly after such a label.
Regards,
Markus
> Fixes: e932900a3279 ("arm: zynq: Use standard timer binding")
How do you think about to add the tag “Reported-by” for Michal Simek?
https://lore.kernel.org/linux-arm-kernel/[email protected]/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=3b7c59a1950c75f2c0152e5a9cd77675b09233d6#n584
Regards,
Markus
Would you review this patch, please?
On Tue, Oct 22, 2019 at 11:47 PM Navid Emamdoost
<[email protected]> wrote:
>
> In the implementation of ttc_setup_clocksource() when
> clk_notifier_register() fails the execution should go to error handling.
> Additionally, to avoid memory leak the allocated memory for ttccs should
> be released, too. So, goto error handling to release the memory and
> return.
>
> Fixes: e932900a3279 ("arm: zynq: Use standard timer binding")
> Signed-off-by: Navid Emamdoost <[email protected]>
> ---
> drivers/clocksource/timer-cadence-ttc.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
> index 88fe2e9ba9a3..035e16bc17d3 100644
> --- a/drivers/clocksource/timer-cadence-ttc.c
> +++ b/drivers/clocksource/timer-cadence-ttc.c
> @@ -328,10 +328,8 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
> ttccs->ttc.clk = clk;
>
> err = clk_prepare_enable(ttccs->ttc.clk);
> - if (err) {
> - kfree(ttccs);
> - return err;
> - }
> + if (err)
> + goto release_ttccs;
>
> ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk);
>
> @@ -341,8 +339,10 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
>
> err = clk_notifier_register(ttccs->ttc.clk,
> &ttccs->ttc.clk_rate_change_nb);
> - if (err)
> + if (err) {
> pr_warn("Unable to register clock notifier.\n");
> + goto release_ttccs;
> + }
>
> ttccs->ttc.base_addr = base;
> ttccs->cs.name = "ttc_clocksource";
> @@ -363,16 +363,18 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
> ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
>
> err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
> - if (err) {
> - kfree(ttccs);
> - return err;
> - }
> + if (err)
> + goto release_ttccs;
>
> ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET;
> sched_clock_register(ttc_sched_clock_read, timer_width,
> ttccs->ttc.freq / PRESCALE);
>
> return 0;
> +
> +release_ttccs:
> + kfree(ttccs);
> + return err;
> }
>
> static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
> --
> 2.17.1
>
--
Navid.