* [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-29 8:31 Yi Wang
2018-10-29 17:23 ` Stephen Boyd
0 siblings, 1 reply; 16+ messages in thread
From: Yi Wang @ 2018-10-29 8:31 UTC (permalink / raw)
To: paul.burton
Cc: mturquette, sboyd, linux-mips, linux-clk, linux-kernel,
zhong.weidong, Yi Wang
'onecell' is malloced in clk_boston_setup(), but is not freed
before leaving from the error handling cases.
Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
---
v2: fix syntax issue in comment, thanks to Sergei.
drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
index 15af423..f5d54a6 100644
--- a/drivers/clk/imgtec/clk-boston.c
+++ b/drivers/clk/imgtec/clk-boston.c
@@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
if (IS_ERR(hw)) {
pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
- return;
+ goto error;
}
onecell->hws[BOSTON_CLK_INPUT] = hw;
hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
if (IS_ERR(hw)) {
pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
- return;
+ goto error;
}
onecell->hws[BOSTON_CLK_SYS] = hw;
hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
if (IS_ERR(hw)) {
pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
- return;
+ goto error;
}
onecell->hws[BOSTON_CLK_CPU] = hw;
err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
if (err)
pr_err("failed to add DT provider: %d\n", err);
+
+ return;
+
+error:
+ kfree(onecell);
}
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-29 17:23 ` Stephen Boyd
0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2018-10-29 17:23 UTC (permalink / raw)
To: Yi Wang, paul.burton
Cc: mturquette, linux-mips, linux-clk, linux-kernel, zhong.weidong, Yi Wang
Quoting Yi Wang (2018-10-29 01:31:47)
> 'onecell' is malloced in clk_boston_setup(), but is not freed
> before leaving from the error handling cases.
How did you find this? Visual inspection? Some coccinelle script?
>
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> ---
> v2: fix syntax issue in comment, thanks to Sergei.
>
> drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> index 15af423..f5d54a6 100644
> --- a/drivers/clk/imgtec/clk-boston.c
> +++ b/drivers/clk/imgtec/clk-boston.c
> @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> if (IS_ERR(hw)) {
> pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> - return;
> + goto error;
> }
> onecell->hws[BOSTON_CLK_INPUT] = hw;
>
> hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> if (IS_ERR(hw)) {
> pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> - return;
> + goto error;
> }
> onecell->hws[BOSTON_CLK_SYS] = hw;
>
> hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> if (IS_ERR(hw)) {
> pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> - return;
> + goto error;
> }
> onecell->hws[BOSTON_CLK_CPU] = hw;
>
> err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> if (err)
> pr_err("failed to add DT provider: %d\n", err);
> +
> + return;
> +
> +error:
> + kfree(onecell);
Ok, sure. But then clks are still left registered on failure?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-29 17:23 ` Stephen Boyd
0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2018-10-29 17:23 UTC (permalink / raw)
To: Yi Wang, paul.burton
Cc: mturquette, linux-mips, linux-clk, linux-kernel, zhong.weidong
Quoting Yi Wang (2018-10-29 01:31:47)
> 'onecell' is malloced in clk_boston_setup(), but is not freed
> before leaving from the error handling cases.
How did you find this? Visual inspection? Some coccinelle script?
>
> Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> ---
> v2: fix syntax issue in comment, thanks to Sergei.
>
> drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> index 15af423..f5d54a6 100644
> --- a/drivers/clk/imgtec/clk-boston.c
> +++ b/drivers/clk/imgtec/clk-boston.c
> @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> if (IS_ERR(hw)) {
> pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> - return;
> + goto error;
> }
> onecell->hws[BOSTON_CLK_INPUT] = hw;
>
> hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> if (IS_ERR(hw)) {
> pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> - return;
> + goto error;
> }
> onecell->hws[BOSTON_CLK_SYS] = hw;
>
> hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> if (IS_ERR(hw)) {
> pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> - return;
> + goto error;
> }
> onecell->hws[BOSTON_CLK_CPU] = hw;
>
> err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> if (err)
> pr_err("failed to add DT provider: %d\n", err);
> +
> + return;
> +
> +error:
> + kfree(onecell);
Ok, sure. But then clks are still left registered on failure?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-30 6:13 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-30 6:13 UTC (permalink / raw)
To: sboyd
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel,
zhong.weidong, up2wing
[-- Attachment #1.1: Type: text/plain, Size: 2510 bytes --]
> Quoting Yi Wang (2018-10-29 01:31:47)
> > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > before leaving from the error handling cases.
>
> How did you find this? Visual inspection? Some coccinelle script?
Smatch report this:
drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> >
> > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > ---
> > v2: fix syntax issue in comment, thanks to Sergei.
> >
> > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > index 15af423..f5d54a6 100644
> > --- a/drivers/clk/imgtec/clk-boston.c
> > +++ b/drivers/clk/imgtec/clk-boston.c
> > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > if (IS_ERR(hw)) {
> > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > - return;
> > + goto error;
> > }
> > onecell->hws[BOSTON_CLK_INPUT] = hw;
> >
> > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > if (IS_ERR(hw)) {
> > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > - return;
> > + goto error;
> > }
> > onecell->hws[BOSTON_CLK_SYS] = hw;
> >
> > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > if (IS_ERR(hw)) {
> > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > - return;
> > + goto error;
> > }
> > onecell->hws[BOSTON_CLK_CPU] = hw;
> >
> > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > if (err)
> > pr_err("failed to add DT provider: %d\n", err);
> > +
> > + return;
> > +
> > +error:
> > + kfree(onecell);
>
> Ok, sure. But then clks are still left registered on failure?
Yeah, but this patch does not change the original flow of the function, so I suppose
if you deem this is not proper, it's better to improve that in another patch, what do
you think?
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-30 6:13 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-30 6:13 UTC (permalink / raw)
To: sboyd
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel,
zhong.weidong, up2wing
[-- Attachment #1.1: Type: text/plain, Size: 2510 bytes --]
> Quoting Yi Wang (2018-10-29 01:31:47)
> > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > before leaving from the error handling cases.
>
> How did you find this? Visual inspection? Some coccinelle script?
Smatch report this:
drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> >
> > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > ---
> > v2: fix syntax issue in comment, thanks to Sergei.
> >
> > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > index 15af423..f5d54a6 100644
> > --- a/drivers/clk/imgtec/clk-boston.c
> > +++ b/drivers/clk/imgtec/clk-boston.c
> > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > if (IS_ERR(hw)) {
> > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > - return;
> > + goto error;
> > }
> > onecell->hws[BOSTON_CLK_INPUT] = hw;
> >
> > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > if (IS_ERR(hw)) {
> > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > - return;
> > + goto error;
> > }
> > onecell->hws[BOSTON_CLK_SYS] = hw;
> >
> > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > if (IS_ERR(hw)) {
> > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > - return;
> > + goto error;
> > }
> > onecell->hws[BOSTON_CLK_CPU] = hw;
> >
> > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > if (err)
> > pr_err("failed to add DT provider: %d\n", err);
> > +
> > + return;
> > +
> > +error:
> > + kfree(onecell);
>
> Ok, sure. But then clks are still left registered on failure?
Yeah, but this patch does not change the original flow of the function, so I suppose
if you deem this is not proper, it's better to improve that in another patch, what do
you think?
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
2018-10-30 6:13 ` wang.yi59
(?)
@ 2018-10-30 16:25 ` Stephen Boyd
2018-10-31 2:42 ` wang.yi59
` (4 more replies)
-1 siblings, 5 replies; 16+ messages in thread
From: Stephen Boyd @ 2018-10-30 16:25 UTC (permalink / raw)
To: wang.yi59
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel,
zhong.weidong, up2wing
Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > Quoting Yi Wang (2018-10-29 01:31:47)
> > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > before leaving from the error handling cases.
> >
> > How did you find this? Visual inspection? Some coccinelle script?
>
> Smatch report this:
> drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
Ok. Please include those details in the commit text.
>
> >
> > >
> > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > ---
> > > v2: fix syntax issue in comment, thanks to Sergei.
> > >
> > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > index 15af423..f5d54a6 100644
> > > --- a/drivers/clk/imgtec/clk-boston.c
> > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > if (IS_ERR(hw)) {
> > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > - return;
> > > + goto error;
> > > }
> > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > >
> > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > if (IS_ERR(hw)) {
> > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > - return;
> > > + goto error;
> > > }
> > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > >
> > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > if (IS_ERR(hw)) {
> > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > - return;
> > > + goto error;
> > > }
> > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > >
> > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > if (err)
> > > pr_err("failed to add DT provider: %d\n", err);
> > > +
> > > + return;
> > > +
> > > +error:
> > > + kfree(onecell);
> >
> > Ok, sure. But then clks are still left registered on failure?
>
> Yeah, but this patch does not change the original flow of the function, so I suppose
> if you deem this is not proper, it's better to improve that in another patch, what do
> you think?
>
I think we should attempt to fix all the theoretical problems with the
driver instead of just fixing some things to make static checkers happy.
It looks like this driver was written with the assumption that if things
go bad we give up all hope. It would be better to clean everything up
properly if things go bad and have better code.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-31 2:42 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-31 2:42 UTC (permalink / raw)
To: sboyd, linux-kernel
Cc: paul.burton, mturquette, linux-mips, linux-clk, zhong.weidong, up2wing
[-- Attachment #1.1: Type: text/plain, Size: 3301 bytes --]
> Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > > Quoting Yi Wang (2018-10-29 01:31:47)
> > > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > > before leaving from the error handling cases.
> > >
> > > How did you find this? Visual inspection? Some coccinelle script?
> >
> > Smatch report this:
> > drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> Ok. Please include those details in the commit text.
Ok :)
>
> >
> > >
> > > >
> > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > > ---
> > > > v2: fix syntax issue in comment, thanks to Sergei.
> > > >
> > > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > > index 15af423..f5d54a6 100644
> > > > --- a/drivers/clk/imgtec/clk-boston.c
> > > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > > >
> > > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > > if (err)
> > > > pr_err("failed to add DT provider: %d\n", err);
> > > > +
> > > > + return;
> > > > +
> > > > +error:
> > > > + kfree(onecell);
> > >
> > > Ok, sure. But then clks are still left registered on failure?
> >
> > Yeah, but this patch does not change the original flow of the function, so I suppose
> > if you deem this is not proper, it's better to improve that in another patch, what do
> > you think?
> >
>
> I think we should attempt to fix all the theoretical problems with the
> driver instead of just fixing some things to make static checkers happy.
> It looks like this driver was written with the assumption that if things
> go bad we give up all hope. It would be better to clean everything up
> properly if things go bad and have better code.
Agreed. I will send another patch to fix this. Thanks for your advice.
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-31 2:42 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-31 2:42 UTC (permalink / raw)
To: sboyd, linux-kernel
Cc: paul.burton, mturquette, linux-mips, linux-clk, zhong.weidong, up2wing
[-- Attachment #1.1: Type: text/plain, Size: 3301 bytes --]
> Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > > Quoting Yi Wang (2018-10-29 01:31:47)
> > > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > > before leaving from the error handling cases.
> > >
> > > How did you find this? Visual inspection? Some coccinelle script?
> >
> > Smatch report this:
> > drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> Ok. Please include those details in the commit text.
Ok :)
>
> >
> > >
> > > >
> > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > > ---
> > > > v2: fix syntax issue in comment, thanks to Sergei.
> > > >
> > > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > > index 15af423..f5d54a6 100644
> > > > --- a/drivers/clk/imgtec/clk-boston.c
> > > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > > >
> > > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > > if (err)
> > > > pr_err("failed to add DT provider: %d\n", err);
> > > > +
> > > > + return;
> > > > +
> > > > +error:
> > > > + kfree(onecell);
> > >
> > > Ok, sure. But then clks are still left registered on failure?
> >
> > Yeah, but this patch does not change the original flow of the function, so I suppose
> > if you deem this is not proper, it's better to improve that in another patch, what do
> > you think?
> >
>
> I think we should attempt to fix all the theoretical problems with the
> driver instead of just fixing some things to make static checkers happy.
> It looks like this driver was written with the assumption that if things
> go bad we give up all hope. It would be better to clean everything up
> properly if things go bad and have better code.
Agreed. I will send another patch to fix this. Thanks for your advice.
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-31 3:05 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-31 3:05 UTC (permalink / raw)
To: sboyd
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel,
zhong.weidong, up2wing
[-- Attachment #1.1: Type: text/plain, Size: 3301 bytes --]
> Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > > Quoting Yi Wang (2018-10-29 01:31:47)
> > > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > > before leaving from the error handling cases.
> > >
> > > How did you find this? Visual inspection? Some coccinelle script?
> >
> > Smatch report this:
> > drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> Ok. Please include those details in the commit text.
Ok :)
>
> >
> > >
> > > >
> > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > > ---
> > > > v2: fix syntax issue in comment, thanks to Sergei.
> > > >
> > > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > > index 15af423..f5d54a6 100644
> > > > --- a/drivers/clk/imgtec/clk-boston.c
> > > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > > >
> > > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > > if (err)
> > > > pr_err("failed to add DT provider: %d\n", err);
> > > > +
> > > > + return;
> > > > +
> > > > +error:
> > > > + kfree(onecell);
> > >
> > > Ok, sure. But then clks are still left registered on failure?
> >
> > Yeah, but this patch does not change the original flow of the function, so I suppose
> > if you deem this is not proper, it's better to improve that in another patch, what do
> > you think?
> >
>
> I think we should attempt to fix all the theoretical problems with the
> driver instead of just fixing some things to make static checkers happy.
> It looks like this driver was written with the assumption that if things
> go bad we give up all hope. It would be better to clean everything up
> properly if things go bad and have better code.
Agreed. I will send another patch to fix this. Thanks for your advice.
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-31 3:05 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-31 3:05 UTC (permalink / raw)
To: sboyd
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel,
zhong.weidong, up2wing
[-- Attachment #1.1: Type: text/plain, Size: 3301 bytes --]
> Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > > Quoting Yi Wang (2018-10-29 01:31:47)
> > > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > > before leaving from the error handling cases.
> > >
> > > How did you find this? Visual inspection? Some coccinelle script?
> >
> > Smatch report this:
> > drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> Ok. Please include those details in the commit text.
Ok :)
>
> >
> > >
> > > >
> > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > > ---
> > > > v2: fix syntax issue in comment, thanks to Sergei.
> > > >
> > > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > > index 15af423..f5d54a6 100644
> > > > --- a/drivers/clk/imgtec/clk-boston.c
> > > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > > >
> > > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > > if (err)
> > > > pr_err("failed to add DT provider: %d\n", err);
> > > > +
> > > > + return;
> > > > +
> > > > +error:
> > > > + kfree(onecell);
> > >
> > > Ok, sure. But then clks are still left registered on failure?
> >
> > Yeah, but this patch does not change the original flow of the function, so I suppose
> > if you deem this is not proper, it's better to improve that in another patch, what do
> > you think?
> >
>
> I think we should attempt to fix all the theoretical problems with the
> driver instead of just fixing some things to make static checkers happy.
> It looks like this driver was written with the assumption that if things
> go bad we give up all hope. It would be better to clean everything up
> properly if things go bad and have better code.
Agreed. I will send another patch to fix this. Thanks for your advice.
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-31 3:30 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-31 3:30 UTC (permalink / raw)
To: sboyd
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel, up2wing
[-- Attachment #1.1: Type: text/plain, Size: 3302 bytes --]
> Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > > Quoting Yi Wang (2018-10-29 01:31:47)
> > > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > > before leaving from the error handling cases.
> > >
> > > How did you find this? Visual inspection? Some coccinelle script?
> >
> > Smatch report this:
> > drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> Ok. Please include those details in the commit text.
Ok :)
>
> >
> > >
> > > >
> > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > > ---
> > > > v2: fix syntax issue in comment, thanks to Sergei.
> > > >
> > > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > > index 15af423..f5d54a6 100644
> > > > --- a/drivers/clk/imgtec/clk-boston.c
> > > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > > >
> > > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > > if (err)
> > > > pr_err("failed to add DT provider: %d\n", err);
> > > > +
> > > > + return;
> > > > +
> > > > +error:
> > > > + kfree(onecell);
> > >
> > > Ok, sure. But then clks are still left registered on failure?
> >
> > Yeah, but this patch does not change the original flow of the function, so I suppose
> > if you deem this is not proper, it's better to improve that in another patch, what do
> > you think?
> >
>
> I think we should attempt to fix all the theoretical problems with the
> driver instead of just fixing some things to make static checkers happy.
> It looks like this driver was written with the assumption that if things
> go bad we give up all hope. It would be better to clean everything up
> properly if things go bad and have better code.
Agreed. I will send another patch to fix this. Thanks for your advice.
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-31 3:30 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-31 3:30 UTC (permalink / raw)
To: sboyd
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel, up2wing
[-- Attachment #1.1: Type: text/plain, Size: 3302 bytes --]
> Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > > Quoting Yi Wang (2018-10-29 01:31:47)
> > > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > > before leaving from the error handling cases.
> > >
> > > How did you find this? Visual inspection? Some coccinelle script?
> >
> > Smatch report this:
> > drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> Ok. Please include those details in the commit text.
Ok :)
>
> >
> > >
> > > >
> > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > > ---
> > > > v2: fix syntax issue in comment, thanks to Sergei.
> > > >
> > > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > > index 15af423..f5d54a6 100644
> > > > --- a/drivers/clk/imgtec/clk-boston.c
> > > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > > >
> > > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > > if (err)
> > > > pr_err("failed to add DT provider: %d\n", err);
> > > > +
> > > > + return;
> > > > +
> > > > +error:
> > > > + kfree(onecell);
> > >
> > > Ok, sure. But then clks are still left registered on failure?
> >
> > Yeah, but this patch does not change the original flow of the function, so I suppose
> > if you deem this is not proper, it's better to improve that in another patch, what do
> > you think?
> >
>
> I think we should attempt to fix all the theoretical problems with the
> driver instead of just fixing some things to make static checkers happy.
> It looks like this driver was written with the assumption that if things
> go bad we give up all hope. It would be better to clean everything up
> properly if things go bad and have better code.
Agreed. I will send another patch to fix this. Thanks for your advice.
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-31 3:44 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-31 3:44 UTC (permalink / raw)
To: sboyd
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel,
zhong.weidong, up2wing
[-- Attachment #1.1: Type: text/plain, Size: 3301 bytes --]
> Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > > Quoting Yi Wang (2018-10-29 01:31:47)
> > > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > > before leaving from the error handling cases.
> > >
> > > How did you find this? Visual inspection? Some coccinelle script?
> >
> > Smatch report this:
> > drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> Ok. Please include those details in the commit text.
Ok :)
>
> >
> > >
> > > >
> > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > > ---
> > > > v2: fix syntax issue in comment, thanks to Sergei.
> > > >
> > > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > > index 15af423..f5d54a6 100644
> > > > --- a/drivers/clk/imgtec/clk-boston.c
> > > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > > >
> > > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > > if (err)
> > > > pr_err("failed to add DT provider: %d\n", err);
> > > > +
> > > > + return;
> > > > +
> > > > +error:
> > > > + kfree(onecell);
> > >
> > > Ok, sure. But then clks are still left registered on failure?
> >
> > Yeah, but this patch does not change the original flow of the function, so I suppose
> > if you deem this is not proper, it's better to improve that in another patch, what do
> > you think?
> >
>
> I think we should attempt to fix all the theoretical problems with the
> driver instead of just fixing some things to make static checkers happy.
> It looks like this driver was written with the assumption that if things
> go bad we give up all hope. It would be better to clean everything up
> properly if things go bad and have better code.
Agreed. I will send another patch to fix this. Thanks for your advice.
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-31 3:44 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-31 3:44 UTC (permalink / raw)
To: sboyd
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel,
zhong.weidong, up2wing
[-- Attachment #1.1: Type: text/plain, Size: 3301 bytes --]
> Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > > Quoting Yi Wang (2018-10-29 01:31:47)
> > > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > > before leaving from the error handling cases.
> > >
> > > How did you find this? Visual inspection? Some coccinelle script?
> >
> > Smatch report this:
> > drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> Ok. Please include those details in the commit text.
Ok :)
>
> >
> > >
> > > >
> > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > > ---
> > > > v2: fix syntax issue in comment, thanks to Sergei.
> > > >
> > > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > > index 15af423..f5d54a6 100644
> > > > --- a/drivers/clk/imgtec/clk-boston.c
> > > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > > >
> > > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > > if (err)
> > > > pr_err("failed to add DT provider: %d\n", err);
> > > > +
> > > > + return;
> > > > +
> > > > +error:
> > > > + kfree(onecell);
> > >
> > > Ok, sure. But then clks are still left registered on failure?
> >
> > Yeah, but this patch does not change the original flow of the function, so I suppose
> > if you deem this is not proper, it's better to improve that in another patch, what do
> > you think?
> >
>
> I think we should attempt to fix all the theoretical problems with the
> driver instead of just fixing some things to make static checkers happy.
> It looks like this driver was written with the assumption that if things
> go bad we give up all hope. It would be better to clean everything up
> properly if things go bad and have better code.
Agreed. I will send another patch to fix this. Thanks for your advice.
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-31 6:59 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-31 6:59 UTC (permalink / raw)
To: sboyd
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel,
zhong.weidong, up2wing, wang.yi59
[-- Attachment #1.1: Type: text/plain, Size: 3301 bytes --]
> Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > > Quoting Yi Wang (2018-10-29 01:31:47)
> > > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > > before leaving from the error handling cases.
> > >
> > > How did you find this? Visual inspection? Some coccinelle script?
> >
> > Smatch report this:
> > drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> Ok. Please include those details in the commit text.
Ok :)
>
> >
> > >
> > > >
> > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > > ---
> > > > v2: fix syntax issue in comment, thanks to Sergei.
> > > >
> > > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > > index 15af423..f5d54a6 100644
> > > > --- a/drivers/clk/imgtec/clk-boston.c
> > > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > > >
> > > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > > if (err)
> > > > pr_err("failed to add DT provider: %d\n", err);
> > > > +
> > > > + return;
> > > > +
> > > > +error:
> > > > + kfree(onecell);
> > >
> > > Ok, sure. But then clks are still left registered on failure?
> >
> > Yeah, but this patch does not change the original flow of the function, so I suppose
> > if you deem this is not proper, it's better to improve that in another patch, what do
> > you think?
> >
>
> I think we should attempt to fix all the theoretical problems with the
> driver instead of just fixing some things to make static checkers happy.
> It looks like this driver was written with the assumption that if things
> go bad we give up all hope. It would be better to clean everything up
> properly if things go bad and have better code.
Agreed. I will send another patch to fix this. Thanks for your advice.
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: Re: [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup()
@ 2018-10-31 6:59 ` wang.yi59
0 siblings, 0 replies; 16+ messages in thread
From: wang.yi59 @ 2018-10-31 6:59 UTC (permalink / raw)
To: sboyd
Cc: paul.burton, mturquette, linux-mips, linux-clk, linux-kernel,
zhong.weidong, up2wing, wang.yi59
[-- Attachment #1.1: Type: text/plain, Size: 3301 bytes --]
> Quoting wang.yi59@zte.com.cn (2018-10-29 23:13:24)
> > > Quoting Yi Wang (2018-10-29 01:31:47)
> > > > 'onecell' is malloced in clk_boston_setup(), but is not freed
> > > > before leaving from the error handling cases.
> > >
> > > How did you find this? Visual inspection? Some coccinelle script?
> >
> > Smatch report this:
> > drivers/clk/imgtec/clk-boston.c:76 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:83 clk_boston_setup() warn: possible memory leak of 'onecell'
> > drivers/clk/imgtec/clk-boston.c:90 clk_boston_setup() warn: possible memory leak of 'onecell'
>
> Ok. Please include those details in the commit text.
Ok :)
>
> >
> > >
> > > >
> > > > Signed-off-by: Yi Wang <wang.yi59@zte.com.cn>
> > > > ---
> > > > v2: fix syntax issue in comment, thanks to Sergei.
> > > >
> > > > drivers/clk/imgtec/clk-boston.c | 11 ++++++++---
> > > > 1 file changed, 8 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
> > > > index 15af423..f5d54a6 100644
> > > > --- a/drivers/clk/imgtec/clk-boston.c
> > > > +++ b/drivers/clk/imgtec/clk-boston.c
> > > > @@ -73,27 +73,32 @@ static void __init clk_boston_setup(struct device_node *np)
> > > > hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_INPUT] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "sys", "input", 0, sys_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register sys clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_SYS] = hw;
> > > >
> > > > hw = clk_hw_register_fixed_rate(NULL, "cpu", "input", 0, cpu_freq);
> > > > if (IS_ERR(hw)) {
> > > > pr_err("failed to register cpu clock: %ld\n", PTR_ERR(hw));
> > > > - return;
> > > > + goto error;
> > > > }
> > > > onecell->hws[BOSTON_CLK_CPU] = hw;
> > > >
> > > > err = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, onecell);
> > > > if (err)
> > > > pr_err("failed to add DT provider: %d\n", err);
> > > > +
> > > > + return;
> > > > +
> > > > +error:
> > > > + kfree(onecell);
> > >
> > > Ok, sure. But then clks are still left registered on failure?
> >
> > Yeah, but this patch does not change the original flow of the function, so I suppose
> > if you deem this is not proper, it's better to improve that in another patch, what do
> > you think?
> >
>
> I think we should attempt to fix all the theoretical problems with the
> driver instead of just fixing some things to make static checkers happy.
> It looks like this driver was written with the assumption that if things
> go bad we give up all hope. It would be better to clean everything up
> properly if things go bad and have better code.
Agreed. I will send another patch to fix this. Thanks for your advice.
---
Best wishes
Yi Wang
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2018-10-31 7:00 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-29 8:31 [PATCH v2] clk: boston: fix possible memory leak in clk_boston_setup() Yi Wang
2018-10-29 17:23 ` Stephen Boyd
2018-10-29 17:23 ` Stephen Boyd
2018-10-30 6:13 ` wang.yi59
2018-10-30 6:13 ` wang.yi59
2018-10-30 16:25 ` Stephen Boyd
2018-10-31 2:42 ` wang.yi59
2018-10-31 2:42 ` wang.yi59
2018-10-31 3:05 ` wang.yi59
2018-10-31 3:05 ` wang.yi59
2018-10-31 3:30 ` wang.yi59
2018-10-31 3:30 ` wang.yi59
2018-10-31 3:44 ` wang.yi59
2018-10-31 3:44 ` wang.yi59
2018-10-31 6:59 ` wang.yi59
2018-10-31 6:59 ` wang.yi59
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.