| Message ID | 1649837953-10984-1-git-send-email-baihaowen@meizu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [V2] clk: renesas: Fix memory leak of 'cpg' | expand |
Hi Haowen, On Wed, Apr 13, 2022 at 10:30 AM Haowen Bai <baihaowen@meizu.com> wrote: > Fix this issue by freeing the cpg when exiting the function in the > error/normal path. > > Signed-off-by: Haowen Bai <baihaowen@meizu.com> Thanks for your patch! > --- a/drivers/clk/renesas/clk-r8a73a4.c > +++ b/drivers/clk/renesas/clk-r8a73a4.c > @@ -215,7 +215,7 @@ static void __init r8a73a4_cpg_clocks_init(struct device_node *np) > > cpg->reg = of_iomap(np, 0); > if (WARN_ON(cpg->reg == NULL)) > - return; > + goto out_free_cpg; Note that this is a fatal error, i.e. no chance the system will survive this, so cleaning up is moot. > > for (i = 0; i < num_clks; ++i) { > const char *name; > @@ -233,6 +233,9 @@ static void __init r8a73a4_cpg_clocks_init(struct device_node *np) > } > > of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data); > +out_free_cpg: > + kfree(cpg); > + kfree(clks); Both cpg and clks are still used after returning from this function, through the registered clocks and clock provider. > } > CLK_OF_DECLARE(r8a73a4_cpg_clks, "renesas,r8a73a4-cpg-clocks", > r8a73a4_cpg_clocks_init); NAKed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
在 4/13/22 4:41 PM, Geert Uytterhoeven 写道: > Hi Haowen, > > On Wed, Apr 13, 2022 at 10:30 AM Haowen Bai <baihaowen@meizu.com> wrote: >> Fix this issue by freeing the cpg when exiting the function in the >> error/normal path. >> >> Signed-off-by: Haowen Bai <baihaowen@meizu.com> > Thanks for your patch! > >> --- a/drivers/clk/renesas/clk-r8a73a4.c >> +++ b/drivers/clk/renesas/clk-r8a73a4.c >> @@ -215,7 +215,7 @@ static void __init r8a73a4_cpg_clocks_init(struct device_node *np) >> >> cpg->reg = of_iomap(np, 0); >> if (WARN_ON(cpg->reg == NULL)) >> - return; >> + goto out_free_cpg; > Note that this is a fatal error, i.e. no chance the system will survive this, > so cleaning up is moot. > >> for (i = 0; i < num_clks; ++i) { >> const char *name; >> @@ -233,6 +233,9 @@ static void __init r8a73a4_cpg_clocks_init(struct device_node *np) >> } >> >> of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data); >> +out_free_cpg: >> + kfree(cpg); >> + kfree(clks); > Both cpg and clks are still used after returning from this function, > through the registered clocks and clock provider. > >> } >> CLK_OF_DECLARE(r8a73a4_cpg_clks, "renesas,r8a73a4-cpg-clocks", >> r8a73a4_cpg_clocks_init); > NAKed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds Dear Geert Uytterhoeven Could you show me how and when cpg & clks free ?
Hi Haowen, On Wed, Apr 13, 2022 at 11:24 AM baihaowen <baihaowen@meizu.com> wrote: > 在 4/13/22 4:41 PM, Geert Uytterhoeven 写道: > > On Wed, Apr 13, 2022 at 10:30 AM Haowen Bai <baihaowen@meizu.com> wrote: > >> Fix this issue by freeing the cpg when exiting the function in the > >> error/normal path. > >> > >> Signed-off-by: Haowen Bai <baihaowen@meizu.com> > > Thanks for your patch! > > > >> --- a/drivers/clk/renesas/clk-r8a73a4.c > >> +++ b/drivers/clk/renesas/clk-r8a73a4.c > >> @@ -215,7 +215,7 @@ static void __init r8a73a4_cpg_clocks_init(struct device_node *np) > >> > >> cpg->reg = of_iomap(np, 0); > >> if (WARN_ON(cpg->reg == NULL)) > >> - return; > >> + goto out_free_cpg; > > Note that this is a fatal error, i.e. no chance the system will survive this, > > so cleaning up is moot. > > > >> for (i = 0; i < num_clks; ++i) { > >> const char *name; > >> @@ -233,6 +233,9 @@ static void __init r8a73a4_cpg_clocks_init(struct device_node *np) > >> } > >> > >> of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data); > >> +out_free_cpg: > >> + kfree(cpg); > >> + kfree(clks); > > Both cpg and clks are still used after returning from this function, > > through the registered clocks and clock provider. > > > >> } > >> CLK_OF_DECLARE(r8a73a4_cpg_clks, "renesas,r8a73a4-cpg-clocks", > >> r8a73a4_cpg_clocks_init); > > NAKed-by: Geert Uytterhoeven <geert+renesas@glider.be> > Could you show me how and when cpg & clks free ? They are never freed, as they stay in-use for the lifetime of the system. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hello! On 4/13/22 11:19 AM, Haowen Bai wrote: > Fix this issue by freeing the cpg when exiting the function in the > error/normal path. > > Signed-off-by: Haowen Bai <baihaowen@meizu.com> > --- > V1->V2: free both cpg&clks. > > drivers/clk/renesas/clk-r8a73a4.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) 2 patches with the same name won't do -- you always need to include the chip name part of the file name in the subject (in this case r8a73a4). MBR, Sergey
在 4/14/22 5:29 PM, Sergey Shtylyov 写道: > Hello! > > On 4/13/22 11:19 AM, Haowen Bai wrote: > >> Fix this issue by freeing the cpg when exiting the function in the >> error/normal path. >> >> Signed-off-by: Haowen Bai <baihaowen@meizu.com> >> --- >> V1->V2: free both cpg&clks. >> >> drivers/clk/renesas/clk-r8a73a4.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) > 2 patches with the same name won't do -- you always need to include the chip name > part of the file name in the subject (in this case r8a73a4). > > MBR, Sergey Got it, thank you for your kindly reminder.
Hi Sergei, On Thu, Apr 14, 2022 at 11:29 AM Sergey Shtylyov <s.shtylyov@omp.ru> wrote: > On 4/13/22 11:19 AM, Haowen Bai wrote: > > Fix this issue by freeing the cpg when exiting the function in the > > error/normal path. > > > > Signed-off-by: Haowen Bai <baihaowen@meizu.com> > > --- > > V1->V2: free both cpg&clks. > > > > drivers/clk/renesas/clk-r8a73a4.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > 2 patches with the same name won't do -- you always need to include the chip name > part of the file name in the subject (in this case r8a73a4). Oh, they were for multiple drivers? I hadn't even noticed , as Gmail collapsed them all into the same thread... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 4/14/22 12:32 PM, Geert Uytterhoeven wrote: [...] >>> Fix this issue by freeing the cpg when exiting the function in the >>> error/normal path. >>> >>> Signed-off-by: Haowen Bai <baihaowen@meizu.com> >>> --- >>> V1->V2: free both cpg&clks. >>> >>> drivers/clk/renesas/clk-r8a73a4.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> 2 patches with the same name won't do -- you always need to include the chip name >> part of the file name in the subject (in this case r8a73a4). > > Oh, they were for multiple drivers? I counted 3! :-) > I hadn't even noticed , as Gmail collapsed them all into the same thread... Hm, they weren't actually posted in the same thread... > Gr{oetje,eeting}s, > > Geert [...] MBR, Sergey
diff --git a/drivers/clk/renesas/clk-r8a73a4.c b/drivers/clk/renesas/clk-r8a73a4.c index cfed11c659d9..5a8d976f49e0 100644 --- a/drivers/clk/renesas/clk-r8a73a4.c +++ b/drivers/clk/renesas/clk-r8a73a4.c @@ -215,7 +215,7 @@ static void __init r8a73a4_cpg_clocks_init(struct device_node *np) cpg->reg = of_iomap(np, 0); if (WARN_ON(cpg->reg == NULL)) - return; + goto out_free_cpg; for (i = 0; i < num_clks; ++i) { const char *name; @@ -233,6 +233,9 @@ static void __init r8a73a4_cpg_clocks_init(struct device_node *np) } of_clk_add_provider(np, of_clk_src_onecell_get, &cpg->data); +out_free_cpg: + kfree(cpg); + kfree(clks); } CLK_OF_DECLARE(r8a73a4_cpg_clks, "renesas,r8a73a4-cpg-clocks", r8a73a4_cpg_clocks_init);
Fix this issue by freeing the cpg when exiting the function in the error/normal path. Signed-off-by: Haowen Bai <baihaowen@meizu.com> --- V1->V2: free both cpg&clks. drivers/clk/renesas/clk-r8a73a4.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)