| Message ID | 20191023044737.2824-1-navid.emamdoost@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clocksource/drivers: Fix error handling in ttc_setup_clocksource | expand |
> 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. I got other wording preferences. Thus I imagine that such a change description can still be improved another bit. How do you think about to omit the word “should” for describing the previous software situation? > So, goto error handling to release the memory and return. 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? 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/2a6cdb63-397b-280a-7379-740e8f43ddf6@xilinx.com/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=3b7c59a1950c75f2c0152e5a9cd77675b09233d6#n584
Regards,
Markus
> Fixes: e932900a3279 ("arm: zynq: Use standard timer binding")
I find the commit 70504f311d4bd5b6a6d494f50c5ab0bd30fdf75c
("clocksource/drivers/cadence_ttc: Convert init function to return error" from 2016-06-28)
also interesting (besides the contribution from 2013-04-04) for your software update.
Regards,
Markus
Would you review this patch, please? On Tue, Oct 22, 2019 at 11:47 PM Navid Emamdoost <navid.emamdoost@gmail.com> 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 <navid.emamdoost@gmail.com> > --- > 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 >
Hi Navid, On 14/12/2019 23:54, Navid Emamdoost wrote: > Would you review this patch, please? please fix the version number, send without in-reply-to. Do the same for the other patch: clocksource/drivers: Fix memory leak in ttc_setup_clockevent It is a bit confuse what patch is ok or not. Thanks -- Daniel > On Tue, Oct 22, 2019 at 11:47 PM Navid Emamdoost > <navid.emamdoost@gmail.com> 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 <navid.emamdoost@gmail.com> >> --- >> 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 >> > >
On 16. 12. 19 14:41, Daniel Lezcano wrote: > > Hi Navid, > > On 14/12/2019 23:54, Navid Emamdoost wrote: >> Would you review this patch, please? > > please fix the version number, send without in-reply-to. > > Do the same for the other patch: > > clocksource/drivers: Fix memory leak in ttc_setup_clockevent > > It is a bit confuse what patch is ok or not. +1 on this. And patch looks good to me but as I said before the same changes should be done also in ttc_setup_clockevent. It means v2 with these two things together is the best way to go. Thanks, Michal
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,
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 <navid.emamdoost@gmail.com> --- drivers/clocksource/timer-cadence-ttc.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)