[PATCH] clk: hisilicon: Fix the memory leak issues

[PATCH] clk: hisilicon: Fix the memory leak issues

[Dongjiu Geng]

When return errors, the clock driver does not unmap
the mapped memory, so fix this issue.

Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
 drivers/clk/hisilicon/clk-hi3620.c | 8 ++++++--
 drivers/clk/hisilicon/clk.c        | 5 ++++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/hisilicon/clk-hi3620.c b/drivers/clk/hisilicon/clk-hi3620.c
index a3d04c7c3da8..d5f1a8df4b1c 100644
--- a/drivers/clk/hisilicon/clk-hi3620.c
+++ b/drivers/clk/hisilicon/clk-hi3620.c
@@ -463,12 +463,16 @@ static void __init hi3620_mmc_clk_init(struct device_node *node)
 	}
 
 	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
-	if (WARN_ON(!clk_data))
+	if (WARN_ON(!clk_data)) {
+		iounmap(base);
 		return;
+	}
 
 	clk_data->clks = kcalloc(num, sizeof(*clk_data->clks), GFP_KERNEL);
-	if (!clk_data->clks)
+	if (!clk_data->clks) {
+		iounmap(base);
 		return;
+	}
 
 	for (i = 0; i < num; i++) {
 		struct hisi_mmc_clock *mmc_clk = &hi3620_mmc_clks[i];
diff --git a/drivers/clk/hisilicon/clk.c b/drivers/clk/hisilicon/clk.c
index 54d9fdc93599..53acaff32934 100644
--- a/drivers/clk/hisilicon/clk.c
+++ b/drivers/clk/hisilicon/clk.c
@@ -69,8 +69,10 @@ struct hisi_clock_data *hisi_clk_init(struct device_node *np,
 	}
 
 	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
-	if (!clk_data)
+	if (!clk_data) {
+		iounmap(base);
 		goto err;
+	}
 
 	clk_data->base = base;
 	clk_table = kcalloc(nr_clks, sizeof(*clk_table), GFP_KERNEL);
@@ -82,6 +84,7 @@ struct hisi_clock_data *hisi_clk_init(struct device_node *np,
 	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data->clk_data);
 	return clk_data;
 err_data:
+	iounmap(base);
 	kfree(clk_data);
 err:
 	return NULL;
-- 
2.17.1

Re: [PATCH] clk: hisilicon: Fix the memory leak issues

[Dongjiu Geng]

On 2020/11/8 21:55, Markus Elfring wrote:
>> When return errors, …
> 
> I would find an other wording more appropriate for this change description.
> 
> 
>> …, so fix this issue.
> 
> I suggest to replace this information by an other imperative wording
> and the tag “Fixes”.

OK, done， I have submitted the version 2 patch

> 
> 
> …
>> +++ b/drivers/clk/hisilicon/clk-hi3620.c
>> @@ -463,12 +463,16 @@ static void __init hi3620_mmc_clk_init(struct device_node *node)
>>  	}
>>
>>  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>> -	if (WARN_ON(!clk_data))
>> +	if (WARN_ON(!clk_data)) {
>> +		iounmap(base);
> 
> Can a statement like “goto unmap_io;” make sense here?
OK， I have changed it.

> 
> 
>>  		return;
>> +	}
>>
>>  	clk_data->clks = kcalloc(num, sizeof(*clk_data->clks), GFP_KERNEL);
>> -	if (!clk_data->clks)
>> +	if (!clk_data->clks) {
> 
> How do you think about to add the function call “kfree(clk_data)” in this if branch?
OK, I have changed it.

> 
> 
> …
>> +++ b/drivers/clk/hisilicon/clk.c
> …
>  	if (!base) {
>  		pr_err("%s: failed to map clock registers\n", __func__);
> -		goto err;
> +		return NULL;
> 
> 
>> @@ -69,8 +69,10 @@ struct hisi_clock_data *hisi_clk_init(struct device_node *np,
>>  	}
>>
>>  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>> -	if (!clk_data)
>> +	if (!clk_data) {
>> +		iounmap(base);
>>  		goto err;
> 
> Please use another jump target.
OK, thanks, I have changed it.

> 
> 
>> @@ -82,6 +84,7 @@ struct hisi_clock_data *hisi_clk_init(struct device_node *np,
>>  	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data->clk_data);
>>  	return clk_data;
>>  err_data:
>> +	iounmap(base);
>>  	kfree(clk_data);
>>  err:
>>  	return NULL;
> 
> I propose to apply the following code variant.
OK, have modified.

> 
> 	return clk_data;
> 
> free_clk_data:
> 	kfree(clk_data);
> unmap_io:
> 	iounmap(base);
> 	return NULL;
> 
> 
> Regards,
> Markus
> .
>