* [PATCH v2] clocksource/drivers: Fix memory leak in ttc_setup_clockevent
2019-10-22 8:51 ` Michal Simek
@ 2019-10-23 4:31 ` Navid Emamdoost
2019-10-23 7:32 ` Michal Simek
2019-10-23 8:00 ` Markus Elfring
2019-10-23 4:47 ` [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource Navid Emamdoost
2019-10-23 4:50 ` [PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent Navid Emamdoost
2 siblings, 2 replies; 14+ messages in thread
From: Navid Emamdoost @ 2019-10-23 4:31 UTC (permalink / raw)
To: michal.simek
Cc: emamd001, smccaman, kjlu, Markus.Elfring, Navid Emamdoost,
Daniel Lezcano, Thomas Gleixner, linux-arm-kernel, linux-kernel
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 <navid.emamdoost@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2] clocksource/drivers: Fix memory leak in ttc_setup_clockevent
2019-10-23 4:31 ` [PATCH v2] " Navid Emamdoost
@ 2019-10-23 7:32 ` Michal Simek
2019-10-23 8:00 ` Markus Elfring
1 sibling, 0 replies; 14+ messages in thread
From: Michal Simek @ 2019-10-23 7:32 UTC (permalink / raw)
To: Navid Emamdoost, michal.simek
Cc: emamd001, smccaman, kjlu, Markus.Elfring, Daniel Lezcano,
Thomas Gleixner, linux-arm-kernel, linux-kernel
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 <navid.emamdoost@gmail.com>
> ---
> 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 <michal.simek@xilinx.com>
Thanks,
Michal
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2] clocksource/drivers: Fix memory leak in ttc_setup_clockevent
2019-10-23 4:31 ` [PATCH v2] " Navid Emamdoost
2019-10-23 7:32 ` Michal Simek
@ 2019-10-23 8:00 ` Markus Elfring
1 sibling, 0 replies; 14+ messages in thread
From: Markus Elfring @ 2019-10-23 8:00 UTC (permalink / raw)
To: Navid Emamdoost, Michal Simek, linux-arm-kernel
Cc: linux-kernel, kernel-janitors, Navid Emamdoost, Kangjie Lu,
Stephen McCamant, Daniel Lezcano, Thomas Gleixner
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource
2019-10-22 8:51 ` Michal Simek
2019-10-23 4:31 ` [PATCH v2] " Navid Emamdoost
@ 2019-10-23 4:47 ` Navid Emamdoost
2019-10-23 7:24 ` Markus Elfring
` (3 more replies)
2019-10-23 4:50 ` [PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent Navid Emamdoost
2 siblings, 4 replies; 14+ messages in thread
From: Navid Emamdoost @ 2019-10-23 4:47 UTC (permalink / raw)
To: michal.simek
Cc: emamd001, smccaman, kjlu, Markus.Elfring, Navid Emamdoost,
Daniel Lezcano, Thomas Gleixner, linux-arm-kernel, linux-kernel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource
2019-10-23 4:47 ` [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource Navid Emamdoost
@ 2019-10-23 7:24 ` Markus Elfring
2019-10-23 8:20 ` Markus Elfring
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Markus Elfring @ 2019-10-23 7:24 UTC (permalink / raw)
To: Navid Emamdoost, linux-arm-kernel
Cc: kernel-janitors, linux-kernel, Navid Emamdoost, Kangjie Lu,
Stephen McCamant, Daniel Lezcano, Michal Simek, Thomas Gleixner
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource
2019-10-23 4:47 ` [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource Navid Emamdoost
2019-10-23 7:24 ` Markus Elfring
@ 2019-10-23 8:20 ` Markus Elfring
2019-10-23 10:31 ` Markus Elfring
2019-12-14 22:54 ` Navid Emamdoost
3 siblings, 0 replies; 14+ messages in thread
From: Markus Elfring @ 2019-10-23 8:20 UTC (permalink / raw)
To: Navid Emamdoost, Michal Simek, linux-arm-kernel
Cc: Navid Emamdoost, Stephen McCamant, Kangjie Lu, Daniel Lezcano,
Thomas Gleixner, linux-kernel, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource
2019-10-23 4:47 ` [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource Navid Emamdoost
2019-10-23 7:24 ` Markus Elfring
2019-10-23 8:20 ` Markus Elfring
@ 2019-10-23 10:31 ` Markus Elfring
2019-12-14 22:54 ` Navid Emamdoost
3 siblings, 0 replies; 14+ messages in thread
From: Markus Elfring @ 2019-10-23 10:31 UTC (permalink / raw)
To: Navid Emamdoost, Michal Simek, Daniel Lezcano, linux-arm-kernel
Cc: Navid Emamdoost, Stephen McCamant, Kangjie Lu, Thomas Gleixner,
linux-kernel, kernel-janitors
> 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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource
2019-10-23 4:47 ` [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource Navid Emamdoost
` (2 preceding siblings ...)
2019-10-23 10:31 ` Markus Elfring
@ 2019-12-14 22:54 ` Navid Emamdoost
2019-12-16 13:41 ` Daniel Lezcano
3 siblings, 1 reply; 14+ messages in thread
From: Navid Emamdoost @ 2019-12-14 22:54 UTC (permalink / raw)
To: Michal Simek
Cc: Navid Emamdoost, Daniel Lezcano, Thomas Gleixner, linux-arm-kernel, LKML
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
>
--
Navid.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource
2019-12-14 22:54 ` Navid Emamdoost
@ 2019-12-16 13:41 ` Daniel Lezcano
2019-12-17 15:09 ` Michal Simek
0 siblings, 1 reply; 14+ messages in thread
From: Daniel Lezcano @ 2019-12-16 13:41 UTC (permalink / raw)
To: Navid Emamdoost, Michal Simek
Cc: Navid Emamdoost, Thomas Gleixner, linux-arm-kernel, LKML
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
>>
>
>
--
<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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource
2019-12-16 13:41 ` Daniel Lezcano
@ 2019-12-17 15:09 ` Michal Simek
0 siblings, 0 replies; 14+ messages in thread
From: Michal Simek @ 2019-12-17 15:09 UTC (permalink / raw)
To: Daniel Lezcano, Navid Emamdoost, Michal Simek
Cc: Navid Emamdoost, Thomas Gleixner, linux-arm-kernel, LKML
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent
2019-10-22 8:51 ` Michal Simek
2019-10-23 4:31 ` [PATCH v2] " Navid Emamdoost
2019-10-23 4:47 ` [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource Navid Emamdoost
@ 2019-10-23 4:50 ` Navid Emamdoost
2 siblings, 0 replies; 14+ messages in thread
From: Navid Emamdoost @ 2019-10-23 4:50 UTC (permalink / raw)
To: Michal Simek
Cc: Markus Elfring, linux-arm-kernel, LKML, kernel-janitors,
Navid Emamdoost, Kangjie Lu, Stephen McCamant, Daniel Lezcano,
Thomas Gleixner
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 <michal.simek@xilinx.com> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread