[PATCH v1] clkdev: prevent potential memory leak when used in modules

[PATCH v1] clkdev: prevent potential memory leak when used in modules

[Andy Shevchenko]

Since clk_register_clkdev() is exported for modules the caller should get a
pointer to the allocated resources. Otherwise the memory leak is guaranteed on
the ->remove() stage.

The patch changes the prototype of the clk_register_clkdev() to return pointer
to the allocated resources. The caller should take care of the returned
variable and free allocated resources when needed. Together with the main
change the users are updated too.

The patch compile tested on x86, thus requires to be tested on touched
architectures.

Andy Shevchenko (1):
  clkdev: change prototype of clk_register_clkdev()

 arch/arm/mach-msm/clock-pcom.c         |  9 +++++----
 arch/arm/mach-vexpress/spc.c           |  5 ++++-
 arch/mips/ath79/clock.c                |  6 +++---
 drivers/clk/clk-bcm2835.c              | 12 +++++++-----
 drivers/clk/clk-max-gen.c              |  9 ++++-----
 drivers/clk/clk-xgene.c                |  6 +++---
 drivers/clk/clkdev.c                   | 14 +++++++++-----
 drivers/clk/samsung/clk-pll.c          | 13 ++++++++-----
 drivers/clk/samsung/clk-s3c2410-dclk.c | 19 +++++++++---------
 drivers/clk/samsung/clk.c              | 35 +++++++++++++++++++---------------
 include/linux/clkdev.h                 |  2 +-
 11 files changed, 74 insertions(+), 56 deletions(-)

-- 
2.1.4

[PATCH v1] clkdev: change prototype of clk_register_clkdev()

[Andy Shevchenko]

Since clk_register_clkdev() is exported for modules the caller should get a
pointer to the allocated resources. Otherwise the memory leak is guaranteed on
the ->remove() stage.

Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/arm/mach-msm/clock-pcom.c         |  9 +++++----
 arch/arm/mach-vexpress/spc.c           |  5 ++++-
 arch/mips/ath79/clock.c                |  6 +++---
 drivers/clk/clk-bcm2835.c              | 12 +++++++-----
 drivers/clk/clk-max-gen.c              |  9 ++++-----
 drivers/clk/clk-xgene.c                |  6 +++---
 drivers/clk/clkdev.c                   | 14 +++++++++-----
 drivers/clk/samsung/clk-pll.c          | 13 ++++++++-----
 drivers/clk/samsung/clk-s3c2410-dclk.c | 19 +++++++++---------
 drivers/clk/samsung/clk.c              | 35 +++++++++++++++++++---------------
 include/linux/clkdev.h                 |  2 +-
 11 files changed, 74 insertions(+), 56 deletions(-)

diff --git a/arch/arm/mach-msm/clock-pcom.c b/arch/arm/mach-msm/clock-pcom.c
index f5b69d7..d1ebbfe 100644
--- a/arch/arm/mach-msm/clock-pcom.c
+++ b/arch/arm/mach-msm/clock-pcom.c
@@ -128,7 +128,7 @@ static struct clk_ops clk_ops_pcom = {
 static int msm_clock_pcom_probe(struct platform_device *pdev)
 {
 	const struct pcom_clk_pdata *pdata = pdev->dev.platform_data;
-	int i, ret;
+	int i;
 
 	for (i = 0; i < pdata->num_lookups; i++) {
 		const struct clk_pcom_desc *desc = &pdata->lookup[i];
@@ -136,6 +136,7 @@ static int msm_clock_pcom_probe(struct platform_device *pdev)
 		struct clk_pcom *p;
 		struct clk_hw *hw;
 		struct clk_init_data init;
+		struct clk_lookup *cl;
 
 		p = devm_kzalloc(&pdev->dev, sizeof(*p), GFP_KERNEL);
 		if (!p)
@@ -157,9 +158,9 @@ static int msm_clock_pcom_probe(struct platform_device *pdev)
 			init.flags |= CLK_IGNORE_UNUSED;
 
 		c = devm_clk_register(&pdev->dev, hw);
-		ret = clk_register_clkdev(c, desc->con, desc->dev);
-		if (ret)
-			return ret;
+		cl = clk_register_clkdev(c, desc->con, desc->dev);
+		if (IS_ERR(cl))
+			return PTR_ERR(cl);
 	}
 
 	return 0;
diff --git a/arch/arm/mach-vexpress/spc.c b/arch/arm/mach-vexpress/spc.c
index f61158c..0477007 100644
--- a/arch/arm/mach-vexpress/spc.c
+++ b/arch/arm/mach-vexpress/spc.c
@@ -568,6 +568,8 @@ static int __init ve_spc_clk_init(void)
 
 	for_each_possible_cpu(cpu) {
 		struct device *cpu_dev = get_cpu_device(cpu);
+		struct clk_lookup *cl;
+
 		if (!cpu_dev) {
 			pr_warn("failed to get cpu%d device\n", cpu);
 			continue;
@@ -577,7 +579,8 @@ static int __init ve_spc_clk_init(void)
 			pr_warn("failed to register cpu%d clock\n", cpu);
 			continue;
 		}
-		if (clk_register_clkdev(clk, NULL, dev_name(cpu_dev))) {
+		cl = clk_register_clkdev(clk, NULL, dev_name(cpu_dev));
+		if (IS_ERR(cl)) {
 			pr_warn("failed to register cpu%d clock lookup\n", cpu);
 			continue;
 		}
diff --git a/arch/mips/ath79/clock.c b/arch/mips/ath79/clock.c
index 26479f4..db5ed95 100644
--- a/arch/mips/ath79/clock.c
+++ b/arch/mips/ath79/clock.c
@@ -35,7 +35,7 @@ struct clk {
 static void __init ath79_add_sys_clkdev(const char *id, unsigned long rate)
 {
 	struct clk *clk;
-	int err;
+	struct clk_lookup *cl;
 
 	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
 	if (!clk)
@@ -43,8 +43,8 @@ static void __init ath79_add_sys_clkdev(const char *id, unsigned long rate)
 
 	clk->rate = rate;
 
-	err = clk_register_clkdev(clk, id, NULL);
-	if (err)
+	cl = clk_register_clkdev(clk, id, NULL);
+	if (IS_ERR(cl))
 		panic("unable to register %s clock device", id);
 }
 
diff --git a/drivers/clk/clk-bcm2835.c b/drivers/clk/clk-bcm2835.c
index 6b950ca..6243527 100644
--- a/drivers/clk/clk-bcm2835.c
+++ b/drivers/clk/clk-bcm2835.c
@@ -30,7 +30,7 @@
 void __init bcm2835_init_clocks(void)
 {
 	struct clk *clk;
-	int ret;
+	struct clk_lookup *cl;
 
 	clk = clk_register_fixed_rate(NULL, "sys_pclk", NULL, CLK_IS_ROOT,
 					250000000);
@@ -46,15 +46,17 @@ void __init bcm2835_init_clocks(void)
 					3000000);
 	if (IS_ERR(clk))
 		pr_err("uart0_pclk not registered\n");
-	ret = clk_register_clkdev(clk, NULL, "20201000.uart");
-	if (ret)
+
+	cl = clk_register_clkdev(clk, NULL, "20201000.uart");
+	if (IS_ERR(cl))
 		pr_err("uart0_pclk alias not registered\n");
 
 	clk = clk_register_fixed_rate(NULL, "uart1_pclk", NULL, CLK_IS_ROOT,
 					125000000);
 	if (IS_ERR(clk))
 		pr_err("uart1_pclk not registered\n");
-	ret = clk_register_clkdev(clk, NULL, "20215000.uart");
-	if (ret)
+
+	cl = clk_register_clkdev(clk, NULL, "20215000.uart");
+	if (IS_ERR(cl))
 		pr_err("uart1_pclk alias not registered\n");
 }
diff --git a/drivers/clk/clk-max-gen.c b/drivers/clk/clk-max-gen.c
index 6505049..cd19136 100644
--- a/drivers/clk/clk-max-gen.c
+++ b/drivers/clk/clk-max-gen.c
@@ -92,16 +92,15 @@ static struct clk *max_gen_clk_register(struct device *dev,
 {
 	struct clk *clk;
 	struct clk_hw *hw = &max_gen->hw;
-	int ret;
+	struct clk_lookup *cl;
 
 	clk = devm_clk_register(dev, hw);
 	if (IS_ERR(clk))
 		return clk;
 
-	ret = clk_register_clkdev(clk, hw->init->name, NULL);
-
-	if (ret)
-		return ERR_PTR(ret);
+	cl = clk_register_clkdev(clk, hw->init->name, NULL);
+	if (IS_ERR(cl))
+		return ERR_CAST(cl);
 
 	return clk;
 }
diff --git a/drivers/clk/clk-xgene.c b/drivers/clk/clk-xgene.c
index dd8a62d..ce5bf4d 100644
--- a/drivers/clk/clk-xgene.c
+++ b/drivers/clk/clk-xgene.c
@@ -401,8 +401,8 @@ static struct clk *xgene_register_clk(struct device *dev,
 {
 	struct xgene_clk *apmclk;
 	struct clk *clk;
+	struct clk_lookup *cl;
 	struct clk_init_data init;
-	int rc;
 
 	/* allocate the APM clock structure */
 	apmclk = kzalloc(sizeof(*apmclk), GFP_KERNEL);
@@ -431,8 +431,8 @@ static struct clk *xgene_register_clk(struct device *dev,
 	}
 
 	/* Register the clock for lookup */
-	rc = clk_register_clkdev(clk, name, NULL);
-	if (rc != 0) {
+	cl = clk_register_clkdev(clk, name, NULL);
+	if (IS_ERR(cl)) {
 		pr_err("%s: could not register lookup clk %s\n",
 			__func__, name);
 	}
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 6e5c504..e07b1e2 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -307,29 +307,33 @@ EXPORT_SYMBOL(clkdev_drop);
  * clkdev.
  *
  * To make things easier for mass registration, we detect error clks
- * from a previous clk_register() call, and return the error code for
+ * from a previous clk_register() call, and return the error pointer for
  * those.  This is to permit this function to be called immediately
  * after clk_register().
+ *
+ * Return:
+ * pointer to the allocated struct clk_lookup on success, or error pointer
+ * otherwise.
  */
-int clk_register_clkdev(struct clk *clk, const char *con_id,
+struct clk_lookup *clk_register_clkdev(struct clk *clk, const char *con_id,
 	const char *dev_fmt, ...)
 {
 	struct clk_lookup *cl;
 	va_list ap;
 
 	if (IS_ERR(clk))
-		return PTR_ERR(clk);
+		return ERR_CAST(clk);
 
 	va_start(ap, dev_fmt);
 	cl = vclkdev_alloc(clk, con_id, dev_fmt, ap);
 	va_end(ap);
 
 	if (!cl)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	clkdev_add(cl);
 
-	return 0;
+	return cl;
 }
 EXPORT_SYMBOL(clk_register_clkdev);
 
diff --git a/drivers/clk/samsung/clk-pll.c b/drivers/clk/samsung/clk-pll.c
index 9d70e5c..f952e36 100644
--- a/drivers/clk/samsung/clk-pll.c
+++ b/drivers/clk/samsung/clk-pll.c
@@ -927,6 +927,7 @@ struct clk * __init samsung_clk_register_pll2550x(const char *name,
 {
 	struct samsung_clk_pll2550x *pll;
 	struct clk *clk;
+	struct clk_lookup *cl;
 	struct clk_init_data init;
 
 	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
@@ -952,7 +953,8 @@ struct clk * __init samsung_clk_register_pll2550x(const char *name,
 		kfree(pll);
 	}
 
-	if (clk_register_clkdev(clk, name, NULL))
+	cl = clk_register_clkdev(clk, name, NULL);
+	if (IS_ERR(cl))
 		pr_err("%s: failed to register lookup for %s", __func__, name);
 
 	return clk;
@@ -1161,8 +1163,9 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
 {
 	struct samsung_clk_pll *pll;
 	struct clk *clk;
+	struct clk_lookup *cl;
 	struct clk_init_data init;
-	int ret, len;
+	int len;
 
 	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
 	if (!pll) {
@@ -1296,10 +1299,10 @@ static void __init _samsung_clk_register_pll(struct samsung_clk_provider *ctx,
 	if (!pll_clk->alias)
 		return;
 
-	ret = clk_register_clkdev(clk, pll_clk->alias, pll_clk->dev_name);
-	if (ret)
+	cl = clk_register_clkdev(clk, pll_clk->alias, pll_clk->dev_name);
+	if (IS_ERR(cl))
 		pr_err("%s: failed to register lookup for %s : %d",
-			__func__, pll_clk->name, ret);
+			__func__, pll_clk->name, PTR_ERR(cl));
 }
 
 void __init samsung_clk_register_pll(struct samsung_clk_provider *ctx,
diff --git a/drivers/clk/samsung/clk-s3c2410-dclk.c b/drivers/clk/samsung/clk-s3c2410-dclk.c
index f4f29ed6..1f9dd36 100644
--- a/drivers/clk/samsung/clk-s3c2410-dclk.c
+++ b/drivers/clk/samsung/clk-s3c2410-dclk.c
@@ -238,6 +238,7 @@ static int s3c24xx_dclk_probe(struct platform_device *pdev)
 	struct s3c24xx_dclk *s3c24xx_dclk;
 	struct resource *mem;
 	struct clk **clk_table;
+	struct clk_lookup *cl;
 	struct s3c24xx_dclk_drv_data *dclk_variant;
 	int ret, i;
 
@@ -309,17 +310,17 @@ static int s3c24xx_dclk_probe(struct platform_device *pdev)
 			goto err_clk_register;
 		}
 
-	ret = clk_register_clkdev(clk_table[MUX_DCLK0], "dclk0", NULL);
-	if (!ret)
-		ret = clk_register_clkdev(clk_table[MUX_DCLK1], "dclk1", NULL);
-	if (!ret)
-		ret = clk_register_clkdev(clk_table[MUX_CLKOUT0],
+	cl = clk_register_clkdev(clk_table[MUX_DCLK0], "dclk0", NULL);
+	if (!IS_ERR(cl))
+		cl = clk_register_clkdev(clk_table[MUX_DCLK1], "dclk1", NULL);
+	if (!IS_ERR(cl))
+		cl = clk_register_clkdev(clk_table[MUX_CLKOUT0],
 					  "clkout0", NULL);
-	if (!ret)
-		ret = clk_register_clkdev(clk_table[MUX_CLKOUT1],
+	if (!IS_ERR(cl))
+		cl = clk_register_clkdev(clk_table[MUX_CLKOUT1],
 					  "clkout1", NULL);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register aliases, %d\n", ret);
+	if (IS_ERR(cl)) {
+		dev_err(&pdev->dev, "failed to register aliases, %d\n", PTR_ERR(cl));
 		goto err_clk_register;
 	}
 
diff --git a/drivers/clk/samsung/clk.c b/drivers/clk/samsung/clk.c
index 4bda540..a08e6f0 100644
--- a/drivers/clk/samsung/clk.c
+++ b/drivers/clk/samsung/clk.c
@@ -102,7 +102,8 @@ void __init samsung_clk_register_alias(struct samsung_clk_provider *ctx,
 				unsigned int nr_clk)
 {
 	struct clk *clk;
-	unsigned int idx, ret;
+	struct clk_lookup *cl;
+	unsigned int idx;
 
 	if (!ctx->clk_data.clks) {
 		pr_err("%s: clock table missing\n", __func__);
@@ -123,8 +124,8 @@ void __init samsung_clk_register_alias(struct samsung_clk_provider *ctx,
 			continue;
 		}
 
-		ret = clk_register_clkdev(clk, list->alias, list->dev_name);
-		if (ret)
+		cl = clk_register_clkdev(clk, list->alias, list->dev_name);
+		if (IS_ERR(cl))
 			pr_err("%s: failed to register lookup %s\n",
 					__func__, list->alias);
 	}
@@ -135,7 +136,8 @@ void __init samsung_clk_register_fixed_rate(struct samsung_clk_provider *ctx,
 		struct samsung_fixed_rate_clock *list, unsigned int nr_clk)
 {
 	struct clk *clk;
-	unsigned int idx, ret;
+	struct clk_lookup *cl;
+	unsigned int idx;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		clk = clk_register_fixed_rate(NULL, list->name,
@@ -152,8 +154,8 @@ void __init samsung_clk_register_fixed_rate(struct samsung_clk_provider *ctx,
 		 * Unconditionally add a clock lookup for the fixed rate clocks.
 		 * There are not many of these on any of Samsung platforms.
 		 */
-		ret = clk_register_clkdev(clk, list->name, NULL);
-		if (ret)
+		cl = clk_register_clkdev(clk, list->name, NULL);
+		if (IS_ERR(cl))
 			pr_err("%s: failed to register clock lookup for %s",
 				__func__, list->name);
 	}
@@ -185,7 +187,8 @@ void __init samsung_clk_register_mux(struct samsung_clk_provider *ctx,
 				unsigned int nr_clk)
 {
 	struct clk *clk;
-	unsigned int idx, ret;
+	struct clk_lookup *cl;
+	unsigned int idx;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		clk = clk_register_mux(NULL, list->name, list->parent_names,
@@ -202,9 +205,9 @@ void __init samsung_clk_register_mux(struct samsung_clk_provider *ctx,
 
 		/* register a clock lookup only if a clock alias is specified */
 		if (list->alias) {
-			ret = clk_register_clkdev(clk, list->alias,
+			cl = clk_register_clkdev(clk, list->alias,
 						list->dev_name);
-			if (ret)
+			if (IS_ERR(cl))
 				pr_err("%s: failed to register lookup %s\n",
 						__func__, list->alias);
 		}
@@ -217,7 +220,8 @@ void __init samsung_clk_register_div(struct samsung_clk_provider *ctx,
 				unsigned int nr_clk)
 {
 	struct clk *clk;
-	unsigned int idx, ret;
+	struct clk_lookup *cl;
+	unsigned int idx;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		if (list->table)
@@ -241,9 +245,9 @@ void __init samsung_clk_register_div(struct samsung_clk_provider *ctx,
 
 		/* register a clock lookup only if a clock alias is specified */
 		if (list->alias) {
-			ret = clk_register_clkdev(clk, list->alias,
+			cl = clk_register_clkdev(clk, list->alias,
 						list->dev_name);
-			if (ret)
+			if (IS_ERR(cl))
 				pr_err("%s: failed to register lookup %s\n",
 						__func__, list->alias);
 		}
@@ -256,7 +260,8 @@ void __init samsung_clk_register_gate(struct samsung_clk_provider *ctx,
 				unsigned int nr_clk)
 {
 	struct clk *clk;
-	unsigned int idx, ret;
+	struct clk_lookup *cl;
+	unsigned int idx;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		clk = clk_register_gate(NULL, list->name, list->parent_name,
@@ -270,9 +275,9 @@ void __init samsung_clk_register_gate(struct samsung_clk_provider *ctx,
 
 		/* register a clock lookup only if a clock alias is specified */
 		if (list->alias) {
-			ret = clk_register_clkdev(clk, list->alias,
+			cl = clk_register_clkdev(clk, list->alias,
 							list->dev_name);
-			if (ret)
+			if (IS_ERR(cl))
 				pr_err("%s: failed to register lookup %s\n",
 					__func__, list->alias);
 		}
diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
index 94bad77..87ace2c 100644
--- a/include/linux/clkdev.h
+++ b/include/linux/clkdev.h
@@ -40,7 +40,7 @@ void clkdev_drop(struct clk_lookup *cl);
 void clkdev_add_table(struct clk_lookup *, size_t);
 int clk_add_alias(const char *, const char *, char *, struct device *);
 
-int clk_register_clkdev(struct clk *, const char *, const char *, ...);
+struct clk_lookup *clk_register_clkdev(struct clk *, const char *, const char *, ...);
 int clk_register_clkdevs(struct clk *, struct clk_lookup *, size_t);
 
 #ifdef CONFIG_COMMON_CLK
-- 
2.1.4

Re: [PATCH v1] clkdev: change prototype of clk_register_clkdev()

[Mika Westerberg]

On Tue, Feb 03, 2015 at 08:18:54PM +0200, Andy Shevchenko wrote:
> Since clk_register_clkdev() is exported for modules the caller should get a
> pointer to the allocated resources. Otherwise the memory leak is guaranteed on
> the ->remove() stage.
> 
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

One comment, see below. Nothing major so feel free to add,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

> ---
>  arch/arm/mach-msm/clock-pcom.c         |  9 +++++----
>  arch/arm/mach-vexpress/spc.c           |  5 ++++-
>  arch/mips/ath79/clock.c                |  6 +++---
>  drivers/clk/clk-bcm2835.c              | 12 +++++++-----
>  drivers/clk/clk-max-gen.c              |  9 ++++-----
>  drivers/clk/clk-xgene.c                |  6 +++---
>  drivers/clk/clkdev.c                   | 14 +++++++++-----
>  drivers/clk/samsung/clk-pll.c          | 13 ++++++++-----
>  drivers/clk/samsung/clk-s3c2410-dclk.c | 19 +++++++++---------
>  drivers/clk/samsung/clk.c              | 35 +++++++++++++++++++---------------
>  include/linux/clkdev.h                 |  2 +-
>  11 files changed, 74 insertions(+), 56 deletions(-)

...

> index 6e5c504..e07b1e2 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -307,29 +307,33 @@ EXPORT_SYMBOL(clkdev_drop);
>   * clkdev.
>   *
>   * To make things easier for mass registration, we detect error clks
> - * from a previous clk_register() call, and return the error code for
> + * from a previous clk_register() call, and return the error pointer for
>   * those.  This is to permit this function to be called immediately
>   * after clk_register().
> + *
> + * Return:
> + * pointer to the allocated struct clk_lookup on success, or error pointer
> + * otherwise.

This should probably say how these resources are supposed to be
released.

>   */
> -int clk_register_clkdev(struct clk *clk, const char *con_id,
> +struct clk_lookup *clk_register_clkdev(struct clk *clk, const char *con_id,
>  	const char *dev_fmt, ...)
>  {
>  	struct clk_lookup *cl;
>  	va_list ap;
>  
>  	if (IS_ERR(clk))
> -		return PTR_ERR(clk);
> +		return ERR_CAST(clk);
>  
>  	va_start(ap, dev_fmt);
>  	cl = vclkdev_alloc(clk, con_id, dev_fmt, ap);
>  	va_end(ap);
>  
>  	if (!cl)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	clkdev_add(cl);
>  
> -	return 0;
> +	return cl;
>  }
>  EXPORT_SYMBOL(clk_register_clkdev);

Re: [PATCH v1] clkdev: change prototype of clk_register_clkdev()

[Andy Shevchenko]

On Wed, 2015-02-04 at 17:50 +0200, Mika Westerberg wrote:
> On Tue, Feb 03, 2015 at 08:18:54PM +0200, Andy Shevchenko wrote:
> > Since clk_register_clkdev() is exported for modules the caller should get a
> > pointer to the allocated resources. Otherwise the memory leak is guaranteed on
> > the ->remove() stage.
> > 
> > Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> One comment, see below. Nothing major so feel free to add,
> 
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks for review!

Though still wait for others to comment and test if possible.

By the way, I have to add that we have one unpublished (yet) user of
this. So, we are testing this internally on x86. Just to note that the
user is a module which might be unloaded. That's why important to
release the acquired resources.

> 
> > ---
> >  arch/arm/mach-msm/clock-pcom.c         |  9 +++++----
> >  arch/arm/mach-vexpress/spc.c           |  5 ++++-
> >  arch/mips/ath79/clock.c                |  6 +++---
> >  drivers/clk/clk-bcm2835.c              | 12 +++++++-----
> >  drivers/clk/clk-max-gen.c              |  9 ++++-----
> >  drivers/clk/clk-xgene.c                |  6 +++---
> >  drivers/clk/clkdev.c                   | 14 +++++++++-----
> >  drivers/clk/samsung/clk-pll.c          | 13 ++++++++-----
> >  drivers/clk/samsung/clk-s3c2410-dclk.c | 19 +++++++++---------
> >  drivers/clk/samsung/clk.c              | 35 +++++++++++++++++++---------------
> >  include/linux/clkdev.h                 |  2 +-
> >  11 files changed, 74 insertions(+), 56 deletions(-)
> 
> ...
> 
> > index 6e5c504..e07b1e2 100644
> > --- a/drivers/clk/clkdev.c
> > +++ b/drivers/clk/clkdev.c
> > @@ -307,29 +307,33 @@ EXPORT_SYMBOL(clkdev_drop);
> >   * clkdev.
> >   *
> >   * To make things easier for mass registration, we detect error clks
> > - * from a previous clk_register() call, and return the error code for
> > + * from a previous clk_register() call, and return the error pointer for
> >   * those.  This is to permit this function to be called immediately
> >   * after clk_register().
> > + *
> > + * Return:
> > + * pointer to the allocated struct clk_lookup on success, or error pointer
> > + * otherwise.
> 
> This should probably say how these resources are supposed to be
> released.

Agree. I would add this in v2.

-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCH v1] clkdev: change prototype of clk_register_clkdev()

[Stephen Boyd]

On 02/03/15 10:18, Andy Shevchenko wrote:
> Since clk_register_clkdev() is exported for modules the caller should get a
> pointer to the allocated resources. Otherwise the memory leak is guaranteed on
> the ->remove() stage.
>
> Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

The idea seems ok to me. Please Cc Russell on the next version. It would
also be good to make a note that this patch doesn't attempt to fix any
memory leaks that may exist by adding calls to clkdev_drop().

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project