The function imx8mm_clocks_probe() has two main issues:
- The of_iomap() function may cause a memory leak.
- Memory allocated for 'clk_hw_data' may not be freed properly
in some paths.
To fix these issues, this commit replaces the use of of_iomap()
with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
This ensures that all memory is properly managed and automatically
freed when the device is removed.
In addition, when devm_of_iomap() allocates memory with an error,
it will first jump to label "unregister_hws" and
then return PTR_ ERR(base).
Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
Signed-off-by: Zhou Shide <[email protected]>
---
The issue is discovered by static analysis, and the patch is not tested yet.
drivers/clk/imx/clk-imx8mm.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index b618892170f2..e6e0bb4805a6 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -303,7 +303,7 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
void __iomem *base;
int ret;
- clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
+ clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, hws,
IMX8MM_CLK_END), GFP_KERNEL);
if (WARN_ON(!clk_hw_data))
return -ENOMEM;
@@ -320,10 +320,12 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
hws[IMX8MM_CLK_EXT4] = imx_get_clk_hw_by_name(np, "clk_ext4");
np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-anatop");
- base = of_iomap(np, 0);
+ base = devm_of_iomap(dev, np, 0, NULL);
of_node_put(np);
- if (WARN_ON(!base))
- return -ENOMEM;
+ if (WARN_ON(IS_ERR(base))) {
+ ret = PTR_ERR(base);
+ goto unregister_hws;
+ }
hws[IMX8MM_AUDIO_PLL1_REF_SEL] = imx_clk_hw_mux("audio_pll1_ref_sel", base + 0x0, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
hws[IMX8MM_AUDIO_PLL2_REF_SEL] = imx_clk_hw_mux("audio_pll2_ref_sel", base + 0x14, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
@@ -399,8 +401,10 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
np = dev->of_node;
base = devm_platform_ioremap_resource(pdev, 0);
- if (WARN_ON(IS_ERR(base)))
- return PTR_ERR(base);
+ if (WARN_ON(IS_ERR(base))) {
+ ret = PTR_ERR(base);
+ goto unregister_hws;
+ }
/* Core Slice */
hws[IMX8MM_CLK_A53_DIV] = imx8m_clk_hw_composite_core("arm_a53_div", imx8mm_a53_sels, base + 0x8000);
--
2.34.1
Quoting Zhou Shide (2023-04-12 20:24:39)
> The function imx8mm_clocks_probe() has two main issues:
> - The of_iomap() function may cause a memory leak.
> - Memory allocated for 'clk_hw_data' may not be freed properly
> in some paths.
>
> To fix these issues, this commit replaces the use of of_iomap()
> with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> This ensures that all memory is properly managed and automatically
> freed when the device is removed.
>
> In addition, when devm_of_iomap() allocates memory with an error,
> it will first jump to label "unregister_hws" and
> then return PTR_ ERR(base).
>
> Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> Signed-off-by: Zhou Shide <[email protected]>
> ---
> The issue is discovered by static analysis, and the patch is not tested yet.
And you're not coordinating with each other?
> -----原始邮件-----
> 发件人: "Stephen Boyd" <[email protected]>
> 发送时间: 2023-04-14 03:06:59 (星期五)
> 收件人: "Abel Vesa" <[email protected]>, "Bai Ping" <[email protected]>, "Fabio Estevam" <[email protected]>, "Michael Turquette" <[email protected]>, "NXP Linux Team" <[email protected]>, "Peng Fan" <[email protected]>, "Pengutronix Kernel Team" <[email protected]>, "Sascha Hauer" <[email protected]>, "Shawn Guo" <[email protected]>, "Zhou Shide" <[email protected]>
> 抄送: [email protected], "Zhou Shide" <[email protected]>, [email protected], [email protected], [email protected], "Hao Luo" <[email protected]>
> 主题: Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'
>
> Quoting Zhou Shide (2023-04-12 20:24:39)
> > The function imx8mm_clocks_probe() has two main issues:
> > - The of_iomap() function may cause a memory leak.
> > - Memory allocated for 'clk_hw_data' may not be freed properly
> > in some paths.
> >
> > To fix these issues, this commit replaces the use of of_iomap()
> > with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> > This ensures that all memory is properly managed and automatically
> > freed when the device is removed.
> >
> > In addition, when devm_of_iomap() allocates memory with an error,
> > it will first jump to label "unregister_hws" and
> > then return PTR_ ERR(base).
> >
> > Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> > Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> > Signed-off-by: Zhou Shide <[email protected]>
> > ---
> > The issue is discovered by static analysis, and the patch is not tested yet.
>
> And you're not coordinating with each other?
What do you mean by "coordinating with each other"?
regards,
Zhou Shide
On Thu, Apr 13, 2023 at 12:06:59PM -0700, Stephen Boyd wrote:
> Quoting Zhou Shide (2023-04-12 20:24:39)
> > The function imx8mm_clocks_probe() has two main issues:
> > - The of_iomap() function may cause a memory leak.
> > - Memory allocated for 'clk_hw_data' may not be freed properly
> > in some paths.
> >
> > To fix these issues, this commit replaces the use of of_iomap()
> > with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> > This ensures that all memory is properly managed and automatically
> > freed when the device is removed.
> >
> > In addition, when devm_of_iomap() allocates memory with an error,
> > it will first jump to label "unregister_hws" and
> > then return PTR_ ERR(base).
> >
> > Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> > Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> > Signed-off-by: Zhou Shide <[email protected]>
> > ---
> > The issue is discovered by static analysis, and the patch is not tested yet.
>
> And you're not coordinating with each other?
>
This is a university program. The patches are reviewed by his professor
and teaching assistants etc. I've been reviewing some of these patches
as well because of they're using Smatch.
regards,
dan carpenter
On 2023/4/15 00:38, Dan Carpenter wrote:
> On Thu, Apr 13, 2023 at 12:06:59PM -0700, Stephen Boyd wrote:
>> Quoting Zhou Shide (2023-04-12 20:24:39)
>>> The function imx8mm_clocks_probe() has two main issues:
>>> - The of_iomap() function may cause a memory leak.
>>> - Memory allocated for 'clk_hw_data' may not be freed properly
>>> in some paths.
>>>
>>> To fix these issues, this commit replaces the use of of_iomap()
>>> with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
>>> This ensures that all memory is properly managed and automatically
>>> freed when the device is removed.
>>>
>>> In addition, when devm_of_iomap() allocates memory with an error,
>>> it will first jump to label "unregister_hws" and
>>> then return PTR_ ERR(base).
>>>
>>> Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
>>> Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
>>> Signed-off-by: Zhou Shide <[email protected]>
>>> ---
>>> The issue is discovered by static analysis, and the patch is not tested yet.
>> And you're not coordinating with each other?
>>
> This is a university program. The patches are reviewed by his professor
> and teaching assistants etc. I've been reviewing some of these patches
> as well because of they're using Smatch.
Thanks for your explanation, Dan. We are from Huazhong University of
Science and Technology.
Some undergraduate and graduatestudents who are interested in Linux
Kernel are guided by me [1] and Dan to contribute into our kernel community.
We found Smatch is really great in finding kernel issues and these
issues are suitable for undergraduate students. Therefore, I contacted
Dan to do a favor for patch interview. And our internal review are
publicly hosted in a google group [2].
Please let me know if you have any questions.
[1] https://mudongliang.github.io/about/
[2] https://groups.google.com/g/hust-os-kernel-patches
>
> regards,
> dan carpenter
>
Quoting 周师德 (2023-04-13 19:02:19)
>
>
>
> > -----原始邮件-----
> > 发件人: "Stephen Boyd" <[email protected]>
> > 发送时间: 2023-04-14 03:06:59 (星期五)
> > 收件人: "Abel Vesa" <[email protected]>, "Bai Ping" <[email protected]>, "Fabio Estevam" <[email protected]>, "Michael Turquette" <[email protected]>, "NXP Linux Team" <[email protected]>, "Peng Fan" <[email protected]>, "Pengutronix Kernel Team" <[email protected]>, "Sascha Hauer" <[email protected]>, "Shawn Guo" <[email protected]>, "Zhou Shide" <[email protected]>
> > 抄送: [email protected], "Zhou Shide" <[email protected]>, [email protected], [email protected], [email protected], "Hao Luo" <[email protected]>
> > 主题: Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'
> >
> > Quoting Zhou Shide (2023-04-12 20:24:39)
> > > The function imx8mm_clocks_probe() has two main issues:
> > > - The of_iomap() function may cause a memory leak.
> > > - Memory allocated for 'clk_hw_data' may not be freed properly
> > > in some paths.
> > >
> > > To fix these issues, this commit replaces the use of of_iomap()
> > > with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> > > This ensures that all memory is properly managed and automatically
> > > freed when the device is removed.
> > >
> > > In addition, when devm_of_iomap() allocates memory with an error,
> > > it will first jump to label "unregister_hws" and
> > > then return PTR_ ERR(base).
> > >
> > > Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> > > Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> > > Signed-off-by: Zhou Shide <[email protected]>
> > > ---
> > > The issue is discovered by static analysis, and the patch is not tested yet.
> >
> > And you're not coordinating with each other?
> What do you mean by "coordinating with each other"?
>
I see two patches to the same driver from the same university on the
list. Preferably you coordinate and decide who will fix what smatch
warnings.
On 2023/4/19 03:57, Stephen Boyd wrote:
> Quoting 周师德 (2023-04-13 19:02:19)
>>
>>
>>> -----原始邮件-----
>>> 发件人: "Stephen Boyd" <[email protected]>
>>> 发送时间: 2023-04-14 03:06:59 (星期五)
>>> 收件人: "Abel Vesa" <[email protected]>, "Bai Ping" <[email protected]>, "Fabio Estevam" <[email protected]>, "Michael Turquette" <[email protected]>, "NXP Linux Team" <[email protected]>, "Peng Fan" <[email protected]>, "Pengutronix Kernel Team" <[email protected]>, "Sascha Hauer" <[email protected]>, "Shawn Guo" <[email protected]>, "Zhou Shide" <[email protected]>
>>> 抄送: [email protected], "Zhou Shide" <[email protected]>, [email protected], [email protected], [email protected], "Hao Luo" <[email protected]>
>>> 主题: Re: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'
>>>
>>> Quoting Zhou Shide (2023-04-12 20:24:39)
>>>> The function imx8mm_clocks_probe() has two main issues:
>>>> - The of_iomap() function may cause a memory leak.
>>>> - Memory allocated for 'clk_hw_data' may not be freed properly
>>>> in some paths.
>>>>
>>>> To fix these issues, this commit replaces the use of of_iomap()
>>>> with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
>>>> This ensures that all memory is properly managed and automatically
>>>> freed when the device is removed.
>>>>
>>>> In addition, when devm_of_iomap() allocates memory with an error,
>>>> it will first jump to label "unregister_hws" and
>>>> then return PTR_ ERR(base).
>>>>
>>>> Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
>>>> Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
>>>> Signed-off-by: Zhou Shide <[email protected]>
>>>> ---
>>>> The issue is discovered by static analysis, and the patch is not tested yet.
>>> And you're not coordinating with each other?
>> What do you mean by "coordinating with each other"?
>>
> I see two patches to the same driver from the same university on the
> list. Preferably you coordinate and decide who will fix what smatch
> warnings.
Hi Stephen,
As their advisor, I coordinate and assign smatch warnings to each
student. I double check our assignment table:
drivers/clk/imx/clk-imx8mn.c:612 imx8mn_clocks_probe() warn: 'base' from
of_iomap() not released on lines: 612.
drivers/clk/imx/clk-imxrt1050.c:154 imxrt1050_clocks_probe() warn:
'pll_base' from of_iomap() not released on lines: 108,154.
drivers/clk/imx/clk-imx8mm.c:619 imx8mm_clocks_probe() warn: 'base' from
of_iomap() not released on lines: 403,619.
drivers/clk/imx/clk-imx8mq.c:611 imx8mq_clocks_probe() warn: 'base' from
of_iomap() not released on lines: 399,611.
There are four similar warnings from your subsystem. If I understand
correctly, two students are patching issues from different probe
functions in the different files since we assign all issues in one file
to one student. Maybe you mix clk-imx8mn (Hao Luo) and clk-imx8mm(Shide
Zhou). They only differ in one char.
If I miss anything, please let me know. Next time, I will ask one
student to fix the issues in one subsystem. This can simply the effort
spent by other student.
Dongliang Mu
>
> -----原始邮件-----
> 发件人: "Zhou Shide" <[email protected]>
> 发送时间: 2023-04-13 11:24:39 (星期四)
> 收件人: "Abel Vesa" <[email protected]>, "Peng Fan" <[email protected]>, "Michael Turquette" <[email protected]>, "Stephen Boyd" <[email protected]>, "Shawn Guo" <[email protected]>, "Sascha Hauer" <[email protected]>, "Pengutronix Kernel Team" <[email protected]>, "Fabio Estevam" <[email protected]>, "NXP Linux Team" <[email protected]>, "Bai Ping" <[email protected]>
> 抄送: [email protected], "Zhou Shide" <[email protected]>, [email protected], [email protected], [email protected]
> 主题: [PATCH] clk: imx: clk-imx8mm: fix memory leak issue in 'imx8mm_clocks_probe'
>
> The function imx8mm_clocks_probe() has two main issues:
> - The of_iomap() function may cause a memory leak.
> - Memory allocated for 'clk_hw_data' may not be freed properly
> in some paths.
>
> To fix these issues, this commit replaces the use of of_iomap()
> with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> This ensures that all memory is properly managed and automatically
> freed when the device is removed.
>
> In addition, when devm_of_iomap() allocates memory with an error,
> it will first jump to label "unregister_hws" and
> then return PTR_ ERR(base).
>
> Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> Signed-off-by: Zhou Shide <[email protected]>
> ---
> The issue is discovered by static analysis, and the patch is not tested yet.
>
> drivers/clk/imx/clk-imx8mm.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index b618892170f2..e6e0bb4805a6 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -303,7 +303,7 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
> void __iomem *base;
> int ret;
>
> - clk_hw_data = kzalloc(struct_size(clk_hw_data, hws,
> + clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, hws,
> IMX8MM_CLK_END), GFP_KERNEL);
> if (WARN_ON(!clk_hw_data))
> return -ENOMEM;
> @@ -320,10 +320,12 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
> hws[IMX8MM_CLK_EXT4] = imx_get_clk_hw_by_name(np, "clk_ext4");
>
> np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-anatop");
> - base = of_iomap(np, 0);
> + base = devm_of_iomap(dev, np, 0, NULL);
> of_node_put(np);
> - if (WARN_ON(!base))
> - return -ENOMEM;
> + if (WARN_ON(IS_ERR(base))) {
> + ret = PTR_ERR(base);
> + goto unregister_hws;
> + }
>
> hws[IMX8MM_AUDIO_PLL1_REF_SEL] = imx_clk_hw_mux("audio_pll1_ref_sel", base + 0x0, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
> hws[IMX8MM_AUDIO_PLL2_REF_SEL] = imx_clk_hw_mux("audio_pll2_ref_sel", base + 0x14, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
> @@ -399,8 +401,10 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
>
> np = dev->of_node;
> base = devm_platform_ioremap_resource(pdev, 0);
> - if (WARN_ON(IS_ERR(base)))
> - return PTR_ERR(base);
> + if (WARN_ON(IS_ERR(base))) {
> + ret = PTR_ERR(base);
> + goto unregister_hws;
> + }
>
> /* Core Slice */
> hws[IMX8MM_CLK_A53_DIV] = imx8m_clk_hw_composite_core("arm_a53_div", imx8mm_a53_sels, base + 0x8000);
> --
> 2.34.1
ping？
On 4/13/2023 11:24 AM, Zhou Shide wrote:
> The function imx8mm_clocks_probe() has two main issues:
> - The of_iomap() function may cause a memory leak.
> - Memory allocated for 'clk_hw_data' may not be freed properly
> in some paths.
>
> To fix these issues, this commit replaces the use of of_iomap()
> with devm_of_iomap() and replaces kzalloc() with devm_kzalloc().
> This ensures that all memory is properly managed and automatically
> freed when the device is removed.
>
> In addition, when devm_of_iomap() allocates memory with an error,
> it will first jump to label "unregister_hws" and
> then return PTR_ ERR(base).
>
> Fixes: 9c71f9ea35d7 ("clk: imx: imx8mm: Switch to clk_hw based API")
> Fixes: ba5625c3e272 ("clk: imx: Add clock driver support for imx8mm")
> Signed-off-by: Zhou Shide<[email protected]>
Reviewed-by: Peng Fan <[email protected]>