* [PATCH v2 0/1] clkdev: prevent potential memory leak when used in modules @ 2015-02-23 11:49 Andy Shevchenko 2015-02-23 11:49 ` [PATCH v2 1/1] clkdev: change prototype of clk_register_clkdev() Andy Shevchenko 0 siblings, 1 reply; 8+ messages in thread From: Andy Shevchenko @ 2015-02-23 11:49 UTC (permalink / raw) To: linux-kernel, Stephen Boyd, Mike Turquette, Lee Jones, Bryan Huntsman, Lorenzo Pieralisi, Ralf Baechle, Sylwester Nawrocki, Russell King - ARM Linux Cc: 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 a 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 current change doesn't fix any present memory leak, thus developers and maintainers should take care of it if needed. The patch has been tested on x86 (*), thus requires to be tested on touched architectures. (*) On x86 there is no upstream users yet, though we have one pending. Changelog v2: - update inplace documentation to mention clkdev_drop() that should be used to free the resources - mention in cover letter that we have tested it on x86 - append Mika's Reviewed-by tag - add Russel to the Cc list - rebase on top of 4.0-rc1 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 | 15 ++++++++++----- 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, 75 insertions(+), 56 deletions(-) -- 2.1.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/1] clkdev: change prototype of clk_register_clkdev() 2015-02-23 11:49 [PATCH v2 0/1] clkdev: prevent potential memory leak when used in modules Andy Shevchenko @ 2015-02-23 11:49 ` Andy Shevchenko 2015-02-23 11:53 ` Lee Jones 0 siblings, 1 reply; 8+ messages in thread From: Andy Shevchenko @ 2015-02-23 11:49 UTC (permalink / raw) To: linux-kernel, Stephen Boyd, Mike Turquette, Lee Jones, Bryan Huntsman, Lorenzo Pieralisi, Ralf Baechle, Sylwester Nawrocki, Russell King - ARM Linux Cc: Andy Shevchenko, Tomeu Vizoso 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> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.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 | 15 ++++++++++----- 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, 75 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 043fd36..ebae180 100644 --- a/drivers/clk/clkdev.c +++ b/drivers/clk/clkdev.c @@ -348,29 +348,34 @@ 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: + * A pointer to the allocated struct clk_lookup is returned on success, or + * error pointer otherwise. The caller may use clkdev_drop() to free the + * allocated resources. */ -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 9e1f88c..eb52ab1 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] clkdev: change prototype of clk_register_clkdev() 2015-02-23 11:49 ` [PATCH v2 1/1] clkdev: change prototype of clk_register_clkdev() Andy Shevchenko @ 2015-02-23 11:53 ` Lee Jones 2015-02-23 11:57 ` Andy Shevchenko ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Lee Jones @ 2015-02-23 11:53 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-kernel, Stephen Boyd, Mike Turquette, Bryan Huntsman, Lorenzo Pieralisi, Ralf Baechle, Sylwester Nawrocki, Russell King - ARM Linux, Tomeu Vizoso On Mon, 23 Feb 2015, 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> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.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 | 15 ++++++++++----- > 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, 75 insertions(+), 56 deletions(-) What's tying all of these changes together? It would be better (simpler for you) if you split them up by subsystem and resubmitted. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] clkdev: change prototype of clk_register_clkdev() 2015-02-23 11:53 ` Lee Jones @ 2015-02-23 11:57 ` Andy Shevchenko 2015-02-23 12:02 ` Andy Shevchenko 2015-02-23 12:03 ` Russell King - ARM Linux 2 siblings, 0 replies; 8+ messages in thread From: Andy Shevchenko @ 2015-02-23 11:57 UTC (permalink / raw) To: Lee Jones Cc: linux-kernel, Stephen Boyd, Mike Turquette, Bryan Huntsman, Lorenzo Pieralisi, Ralf Baechle, Sylwester Nawrocki, Russell King - ARM Linux, Tomeu Vizoso On Mon, 2015-02-23 at 11:53 +0000, Lee Jones wrote: > On Mon, 23 Feb 2015, 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> > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.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 | 15 ++++++++++----- > > 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, 75 insertions(+), 56 deletions(-) > > What's tying all of these changes together? It would be better > (simpler for you) if you split them up by subsystem and resubmitted. In that case it will break bisectability. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] clkdev: change prototype of clk_register_clkdev() 2015-02-23 11:53 ` Lee Jones 2015-02-23 11:57 ` Andy Shevchenko @ 2015-02-23 12:02 ` Andy Shevchenko 2015-02-23 12:03 ` Russell King - ARM Linux 2 siblings, 0 replies; 8+ messages in thread From: Andy Shevchenko @ 2015-02-23 12:02 UTC (permalink / raw) To: Lee Jones Cc: linux-kernel, Stephen Boyd, Mike Turquette, Bryan Huntsman, Lorenzo Pieralisi, Ralf Baechle, Sylwester Nawrocki, Russell King - ARM Linux, Tomeu Vizoso On Mon, 2015-02-23 at 11:53 +0000, Lee Jones wrote: > On Mon, 23 Feb 2015, 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> > > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.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 | 15 ++++++++++----- > > 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, 75 insertions(+), 56 deletions(-) > > What's tying all of these changes together? It would be better > (simpler for you) if you split them up by subsystem and resubmitted. In that case it will break bisectability. -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] clkdev: change prototype of clk_register_clkdev() 2015-02-23 11:53 ` Lee Jones 2015-02-23 11:57 ` Andy Shevchenko 2015-02-23 12:02 ` Andy Shevchenko @ 2015-02-23 12:03 ` Russell King - ARM Linux 2015-02-23 12:23 ` Lee Jones 2 siblings, 1 reply; 8+ messages in thread From: Russell King - ARM Linux @ 2015-02-23 12:03 UTC (permalink / raw) To: Lee Jones Cc: Andy Shevchenko, linux-kernel, Stephen Boyd, Mike Turquette, Bryan Huntsman, Lorenzo Pieralisi, Ralf Baechle, Sylwester Nawrocki, Tomeu Vizoso On Mon, Feb 23, 2015 at 11:53:56AM +0000, Lee Jones wrote: > On Mon, 23 Feb 2015, Andy Shevchenko wrote: > > 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 | 15 ++++++++++----- > > 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, 75 insertions(+), 56 deletions(-) > > What's tying all of these changes together? It would be better > (simpler for you) if you split them up by subsystem and resubmitted. I think maybe, just maybe, you might get the answer to that if you read the patch. You can't change the function signature without creating lots of warnings and causing regressions. Yes, the function could be renamed whilst doing this change, but that's really quite sub-standard in an already busy namespace. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] clkdev: change prototype of clk_register_clkdev() 2015-02-23 12:03 ` Russell King - ARM Linux @ 2015-02-23 12:23 ` Lee Jones 2015-02-23 12:36 ` Andy Shevchenko 0 siblings, 1 reply; 8+ messages in thread From: Lee Jones @ 2015-02-23 12:23 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Andy Shevchenko, linux-kernel, Stephen Boyd, Mike Turquette, Bryan Huntsman, Lorenzo Pieralisi, Ralf Baechle, Sylwester Nawrocki, Tomeu Vizoso On Mon, 23 Feb 2015, Russell King - ARM Linux wrote: > On Mon, Feb 23, 2015 at 11:53:56AM +0000, Lee Jones wrote: > > On Mon, 23 Feb 2015, Andy Shevchenko wrote: > > > 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 | 15 ++++++++++----- > > > 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, 75 insertions(+), 56 deletions(-) > > > > What's tying all of these changes together? It would be better > > (simpler for you) if you split them up by subsystem and resubmitted. > > I think maybe, just maybe, you might get the answer to that if you read > the patch. > > You can't change the function signature without creating lots of warnings > and causing regressions. > > Yes, the function could be renamed whilst doing this change, but that's > really quite sub-standard in an already busy namespace. Okay, I can go with that. If I'm reading this patch correctly, it doesn't actually fix anything. What's the thinking behind stopping short? Why don't you go the extra inch and free the resources at the appropriate times? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/1] clkdev: change prototype of clk_register_clkdev() 2015-02-23 12:23 ` Lee Jones @ 2015-02-23 12:36 ` Andy Shevchenko 0 siblings, 0 replies; 8+ messages in thread From: Andy Shevchenko @ 2015-02-23 12:36 UTC (permalink / raw) To: Lee Jones Cc: Russell King - ARM Linux, linux-kernel, Stephen Boyd, Mike Turquette, Bryan Huntsman, Lorenzo Pieralisi, Ralf Baechle, Sylwester Nawrocki, Tomeu Vizoso On Mon, 2015-02-23 at 12:23 +0000, Lee Jones wrote: > On Mon, 23 Feb 2015, Russell King - ARM Linux wrote: > > > On Mon, Feb 23, 2015 at 11:53:56AM +0000, Lee Jones wrote: > > > On Mon, 23 Feb 2015, Andy Shevchenko wrote: > > > > 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 | 15 ++++++++++----- > > > > 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, 75 insertions(+), 56 deletions(-) > > > > > > What's tying all of these changes together? It would be better > > > (simpler for you) if you split them up by subsystem and resubmitted. > > > > I think maybe, just maybe, you might get the answer to that if you read > > the patch. > > > > You can't change the function signature without creating lots of warnings > > and causing regressions. > > > > Yes, the function could be renamed whilst doing this change, but that's > > really quite sub-standard in an already busy namespace. > > Okay, I can go with that. > > If I'm reading this patch correctly, it doesn't actually fix > anything. Right. Consider this as a stage 1 that allows you to make a proper fix on an actual use case. > What's the thinking behind stopping short? Why don't you go > the extra inch and free the resources at the appropriate times? As far as I can see most of the cases are for whole OS time to live, means no need to free resources. The really affected users should know by themselves and I'm not going to brainlessly fix every usage of the clkdev_register() (note most of them [> 90% use cases?] even didn't check the return code). -- Andy Shevchenko <andriy.shevchenko@intel.com> Intel Finland Oy ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-02-23 12:37 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-02-23 11:49 [PATCH v2 0/1] clkdev: prevent potential memory leak when used in modules Andy Shevchenko 2015-02-23 11:49 ` [PATCH v2 1/1] clkdev: change prototype of clk_register_clkdev() Andy Shevchenko 2015-02-23 11:53 ` Lee Jones 2015-02-23 11:57 ` Andy Shevchenko 2015-02-23 12:02 ` Andy Shevchenko 2015-02-23 12:03 ` Russell King - ARM Linux 2015-02-23 12:23 ` Lee Jones 2015-02-23 12:36 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox