[alsa-devel] [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path
This serial of patches fixes memory leak and resource relaim in error path
[PATCH 1/12] ad1836: fix a memory leak if another ad1836 is registered [PATCH 2/12] ak4642: fix a memory leak if failed to initialise AK4642 [PATCH 3/12] da7210: fix a memory leak if failed to initialise da7210 audio codec [PATCH 4/12] wm8523: fix resource reclaim in wm8523_register error path [PATCH 5/12] wm8711: fix a memory leak if another WM8711 is registered [PATCH 6/12] wm8904: fix resource reclaim in wm8904_register error path [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register error path [PATCH 8/12] wm8955: fix resource reclaim in wm8955_register error path [PATCH 9/12] wm8961: fix resource reclaim in wm8961_register error path [PATCH 10/12] wm8974: fix a memory leak if another WM8974 is registered [PATCH 11/12] wm8978: fix a memory leak if another WM8978 is registered [PATCH 12/12] wm9081: fix resource reclaim in wm9081_register error path
Regards, Axel
ad1836 is allocated in ad1836_spi_probe() but is not freed if ad1836_register() return -EINVAL (if another ad1836 is registered).
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/ad1836.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/ad1836.c b/sound/soc/codecs/ad1836.c index 2175384..a01006c 100644 --- a/sound/soc/codecs/ad1836.c +++ b/sound/soc/codecs/ad1836.c @@ -272,6 +272,7 @@ static int ad1836_register(struct ad1836_priv *ad1836)
if (ad1836_codec) { dev_err(codec->dev, "Another ad1836 is registered\n"); + kfree(ad1836); return -EINVAL; }
On Thu, Jul 15, 2010 at 10:50 AM, Axel Lin axel.lin@gmail.com wrote:
ad1836 is allocated in ad1836_spi_probe() but is not freed if ad1836_register() return -EINVAL (if another ad1836 is registered).
Signed-off-by: Axel Lin axel.lin@gmail.com
Acked-by: Barry Song 21cnbao@gmail.com
sound/soc/codecs/ad1836.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sound/soc/codecs/ad1836.c b/sound/soc/codecs/ad1836.c index 2175384..a01006c 100644 --- a/sound/soc/codecs/ad1836.c +++ b/sound/soc/codecs/ad1836.c @@ -272,6 +272,7 @@ static int ad1836_register(struct ad1836_priv *ad1836)
if (ad1836_codec) { dev_err(codec->dev, "Another ad1836 is registered\n");
- kfree(ad1836);
return -EINVAL; }
-- 1.5.4.3
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
ak4642 should be kfreed if ak4642_init() return error.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/ak4642.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/sound/soc/codecs/ak4642.c b/sound/soc/codecs/ak4642.c index 7528a54..4feefa8 100644 --- a/sound/soc/codecs/ak4642.c +++ b/sound/soc/codecs/ak4642.c @@ -491,8 +491,10 @@ static int ak4642_i2c_probe(struct i2c_client *i2c, codec->control_data = i2c;
ret = ak4642_init(ak4642); - if (ret < 0) + if (ret < 0) { printk(KERN_ERR "failed to initialise AK4642\n"); + kfree(ak4642); + }
return ret; }
Dear Axel
Thanks about this patch
--- a/sound/soc/codecs/ak4642.c +++ b/sound/soc/codecs/ak4642.c @@ -491,8 +491,10 @@ static int ak4642_i2c_probe(struct i2c_client *i2c, codec->control_data = i2c;
ret = ak4642_init(ak4642);
- if (ret < 0)
if (ret < 0) { printk(KERN_ERR "failed to initialise AK4642\n");
kfree(ak4642);}
return ret;
}
Indeed. this operation is needed when error case.
I think i2c_set_clientdata(i2c, NULL); is needed here too. (da7210 also)
And why ak4642's patch doesn't have snd_soc_unregister_codec fix like da7210 ? I think ak4642 have same issue
Best regards -- Kuninori Morimoto
Hi Kuninori,
2010/7/15 Kuninori Morimoto kuninori.morimoto.gx@renesas.com:
Dear Axel
Thanks about this patch
--- a/sound/soc/codecs/ak4642.c +++ b/sound/soc/codecs/ak4642.c @@ -491,8 +491,10 @@ static int ak4642_i2c_probe(struct i2c_client *i2c, codec->control_data = i2c;
ret = ak4642_init(ak4642);
- if (ret < 0)
- if (ret < 0) {
printk(KERN_ERR "failed to initialise AK4642\n");
- kfree(ak4642);
- }
return ret; }
Indeed. this operation is needed when error case.
I think i2c_set_clientdata(i2c, NULL); is needed here too. (da7210 also)
I think the i2c core will take care of it now. see below commit log: commit e4a7b9b04de15f6b63da5ccdd373ffa3057a3681 Author: Wolfram Sang w.sang@pengutronix.de Date: Tue May 4 11:09:27 2010 +0200
i2c-core: Erase pointer to clientdata on removal
After discovering that a lot of i2c-drivers leave the pointer to their clientdata dangling, it was decided to let the core handle this issue. It is assumed that the core may access the private data after remove() as there are no guarantees for the lifetime of such pointers anyhow (see thread starting at http://lkml.org/lkml/2010/3/21/68)
Signed-off-by: Wolfram Sang w.sang@pengutronix.de Signed-off-by: Jean Delvare khali@linux-fr.org
And why ak4642's patch doesn't have snd_soc_unregister_codec fix like da7210 ? I think ak4642 have same issue
In ak4642_init, current implementation will call snd_soc_unregister_codec(codec); before goto reg_cache_err; Therefore, it should be no problem.
Or do you mean just using goto style cleanup instead of mixed in-line and goto style cleanup? I can fix it if you like this change.
Regards, Axel
Best regards
Kuninori Morimoto
Dear Axel
I think the i2c core will take care of it now. see below commit log: commit e4a7b9b04de15f6b63da5ccdd373ffa3057a3681
Ohh !! I didn't know that.
In ak4642_init, current implementation will call snd_soc_unregister_codec(codec); before goto reg_cache_err; Therefore, it should be no problem.
Indeed =)
So now, your patches are very good !!
Best regards -- Kuninori Morimoto
da7210 should be kfreed if da7210_init() return error. This patch also fixes the error handing in the case of snd_soc_register_dai() fail by adding snd_soc_unregister_codec() in error path.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/da7210.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/da7210.c b/sound/soc/codecs/da7210.c index 75af2d6..3b9a6cc 100644 --- a/sound/soc/codecs/da7210.c +++ b/sound/soc/codecs/da7210.c @@ -488,7 +488,7 @@ static int da7210_init(struct da7210_priv *da7210) ret = snd_soc_register_dai(&da7210_dai); if (ret) { dev_err(codec->dev, "Failed to register DAI: %d\n", ret); - goto init_err; + goto codec_err; }
/* FIXME @@ -574,6 +574,8 @@ static int da7210_init(struct da7210_priv *da7210)
return ret;
+codec_err: + snd_soc_unregister_codec(codec); init_err: kfree(codec->reg_cache); codec->reg_cache = NULL; @@ -601,8 +603,10 @@ static int __devinit da7210_i2c_probe(struct i2c_client *i2c, codec->control_data = i2c;
ret = da7210_init(da7210); - if (ret < 0) + if (ret < 0) { pr_err("Failed to initialise da7210 audio codec\n"); + kfree(da7210); + }
return ret; }
This patch includes below fixes: 1. If another WM8523 is registered, need to kfree wm8523 before return -EINVAL. 2. If snd_soc_register_codec failed, goto error path to properly free resources. 3. Instead of using mixed in-line and goto style cleanup, use goto style error handling if snd_soc_register_dai failed.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm8523.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/wm8523.c b/sound/soc/codecs/wm8523.c index 37242a7..0ad039b 100644 --- a/sound/soc/codecs/wm8523.c +++ b/sound/soc/codecs/wm8523.c @@ -482,7 +482,8 @@ static int wm8523_register(struct wm8523_priv *wm8523,
if (wm8523_codec) { dev_err(codec->dev, "Another WM8523 is registered\n"); - return -EINVAL; + ret = -EINVAL; + goto err; }
mutex_init(&codec->mutex); @@ -570,18 +571,19 @@ static int wm8523_register(struct wm8523_priv *wm8523, ret = snd_soc_register_codec(codec); if (ret != 0) { dev_err(codec->dev, "Failed to register codec: %d\n", ret); - return ret; + goto err_enable; }
ret = snd_soc_register_dai(&wm8523_dai); if (ret != 0) { dev_err(codec->dev, "Failed to register DAI: %d\n", ret); - snd_soc_unregister_codec(codec); - return ret; + goto err_codec; }
return 0;
+err_codec: + snd_soc_unregister_codec(codec); err_enable: regulator_bulk_disable(ARRAY_SIZE(wm8523->supplies), wm8523->supplies); err_get:
wm8711 is allocated in either wm8711_spi_probe() or wm8711_i2c_probe() but is not freed if wm8711_register() return -EINVAL(if another ad1836 is registered).
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm8711.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/sound/soc/codecs/wm8711.c b/sound/soc/codecs/wm8711.c index effb14e..e2dba07 100644 --- a/sound/soc/codecs/wm8711.c +++ b/sound/soc/codecs/wm8711.c @@ -439,7 +439,8 @@ static int wm8711_register(struct wm8711_priv *wm8711,
if (wm8711_codec) { dev_err(codec->dev, "Another WM8711 is registered\n"); - return -EINVAL; + ret = -EINVAL; + goto err; }
mutex_init(&codec->mutex);
This patch includes below fixes: 1. wm8904 need to be kfreed in wm8904_register() error path before return. 2. fix the error path for snd_soc_register_codec() fail and snd_soc_register_dai() fail to properly free resources.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm8904.c | 13 ++++++++----- 1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/wm8904.c b/sound/soc/codecs/wm8904.c index 87f14f8..f7dcabf 100644 --- a/sound/soc/codecs/wm8904.c +++ b/sound/soc/codecs/wm8904.c @@ -2433,7 +2433,8 @@ static int wm8904_register(struct wm8904_priv *wm8904,
if (wm8904_codec) { dev_err(codec->dev, "Another WM8904 is registered\n"); - return -EINVAL; + ret = -EINVAL; + goto err; }
mutex_init(&codec->mutex); @@ -2462,7 +2463,8 @@ static int wm8904_register(struct wm8904_priv *wm8904, default: dev_err(codec->dev, "Unknown device type %d\n", wm8904->devtype); - return -EINVAL; + ret = -EINVAL; + goto err; }
memcpy(codec->reg_cache, wm8904_reg, sizeof(wm8904_reg)); @@ -2566,18 +2568,19 @@ static int wm8904_register(struct wm8904_priv *wm8904, ret = snd_soc_register_codec(codec); if (ret != 0) { dev_err(codec->dev, "Failed to register codec: %d\n", ret); - return ret; + goto err_enable; }
ret = snd_soc_register_dai(&wm8904_dai); if (ret != 0) { dev_err(codec->dev, "Failed to register DAI: %d\n", ret); - snd_soc_unregister_codec(codec); - return ret; + goto err_codec; }
return 0;
+err_codec: + snd_soc_unregister_codec(codec); err_enable: regulator_bulk_disable(ARRAY_SIZE(wm8904->supplies), wm8904->supplies); err_get:
From 44c93254e5158e3086c4707148d24746067f6b14 Mon Sep 17 00:00:00 2001
From: Axel Lin axel.lin@gmail.com Date: Thu, 15 Jul 2010 10:13:34 +0800 Subject: [PATCH] wm8940: fix resource reclaim in wm8940_register error path
This patch fixes the error path in wm8940_register to properly free resources.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm8940.c | 22 ++++++++++++++-------- 1 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c index e3c4bbf..9673e6f 100644 --- a/sound/soc/codecs/wm8940.c +++ b/sound/soc/codecs/wm8940.c @@ -766,7 +766,8 @@ static int wm8940_register(struct wm8940_priv *wm8940, u16 reg; if (wm8940_codec) { dev_err(codec->dev, "Another WM8940 is registered\n"); - return -EINVAL; + ret = -EINVAL; + goto err; }
INIT_LIST_HEAD(&codec->dapm_widgets); @@ -785,7 +786,7 @@ static int wm8940_register(struct wm8940_priv *wm8940, ret = snd_soc_codec_set_cache_io(codec, 8, 16, control); if (ret < 0) { dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret); - return ret; + goto err; }
memcpy(codec->reg_cache, wm8940_reg_defaults, @@ -794,7 +795,7 @@ static int wm8940_register(struct wm8940_priv *wm8940, ret = wm8940_reset(codec); if (ret < 0) { dev_err(codec->dev, "Failed to issue reset\n"); - return ret; + goto err; }
wm8940_dai.dev = codec->dev; @@ -803,7 +804,7 @@ static int wm8940_register(struct wm8940_priv *wm8940,
ret = snd_soc_write(codec, WM8940_POWER1, 0x180); if (ret < 0) - return ret; + goto err;
if (!pdata) dev_warn(codec->dev, "No platform data supplied\n"); @@ -811,7 +812,7 @@ static int wm8940_register(struct wm8940_priv *wm8940, reg = snd_soc_read(codec, WM8940_OUTPUTCTL); ret = snd_soc_write(codec, WM8940_OUTPUTCTL, reg | pdata->vroi); if (ret < 0) - return ret; + goto err; }
@@ -820,17 +821,22 @@ static int wm8940_register(struct wm8940_priv *wm8940, ret = snd_soc_register_codec(codec); if (ret) { dev_err(codec->dev, "Failed to register codec: %d\n", ret); - return ret; + goto err; }
ret = snd_soc_register_dai(&wm8940_dai); if (ret) { dev_err(codec->dev, "Failed to register DAI: %d\n", ret); - snd_soc_unregister_codec(codec); - return ret; + goto err_codec; }
return 0; + +err_codec: + snd_soc_unregister_codec(codec); +err: + kfree(wm8940); + return ret; }
static void wm8940_unregister(struct wm8940_priv *wm8940)
Hi Axel
On Thu, 15 Jul 2010, Axel Lin wrote:
From 44c93254e5158e3086c4707148d24746067f6b14 Mon Sep 17 00:00:00 2001
From: Axel Lin axel.lin@gmail.com Date: Thu, 15 Jul 2010 10:13:34 +0800 Subject: [PATCH] wm8940: fix resource reclaim in wm8940_register error path
This patch fixes the error path in wm8940_register to properly free resources.
Signed-off-by: Axel Lin axel.lin@gmail.com
Ok, these bugs are real, but let's fix them properly, i.e., release resources at the same level, where they have been acquired:
static int register_codec(struct drv_priv *data) { int ret = snd_soc_register_codec(); if (ret < 0) return ret;
ret = try(); if (failed) goto err;
...
err: snd_soc_unregister_codec(); }
static int probe() { int ret; struct drv_priv *data = kzalloc(sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM;
ret = register_codec(data); if (ret < 0) goto err;
...
err: kfree(data); return ret; }
That is, kfree() belongs to probe() and not to register_codec. This, of course, concerns all aour patches, at least those of them, that I have seen.
Thanks Guennadi
sound/soc/codecs/wm8940.c | 22 ++++++++++++++-------- 1 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c index e3c4bbf..9673e6f 100644 --- a/sound/soc/codecs/wm8940.c +++ b/sound/soc/codecs/wm8940.c @@ -766,7 +766,8 @@ static int wm8940_register(struct wm8940_priv *wm8940, u16 reg; if (wm8940_codec) { dev_err(codec->dev, "Another WM8940 is registered\n");
return -EINVAL;
ret = -EINVAL;goto err;}
INIT_LIST_HEAD(&codec->dapm_widgets);
@@ -785,7 +786,7 @@ static int wm8940_register(struct wm8940_priv *wm8940, ret = snd_soc_codec_set_cache_io(codec, 8, 16, control); if (ret < 0) { dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
return ret;
goto err;}
memcpy(codec->reg_cache, wm8940_reg_defaults,
@@ -794,7 +795,7 @@ static int wm8940_register(struct wm8940_priv *wm8940, ret = wm8940_reset(codec); if (ret < 0) { dev_err(codec->dev, "Failed to issue reset\n");
return ret;
goto err;}
wm8940_dai.dev = codec->dev;
@@ -803,7 +804,7 @@ static int wm8940_register(struct wm8940_priv *wm8940,
ret = snd_soc_write(codec, WM8940_POWER1, 0x180); if (ret < 0)
return ret;
goto err;if (!pdata) dev_warn(codec->dev, "No platform data supplied\n");
@@ -811,7 +812,7 @@ static int wm8940_register(struct wm8940_priv *wm8940, reg = snd_soc_read(codec, WM8940_OUTPUTCTL); ret = snd_soc_write(codec, WM8940_OUTPUTCTL, reg | pdata->vroi); if (ret < 0)
return ret;
goto err;@@ -820,17 +821,22 @@ static int wm8940_register(struct wm8940_priv *wm8940, ret = snd_soc_register_codec(codec); if (ret) { dev_err(codec->dev, "Failed to register codec: %d\n", ret);
return ret;
goto err;}
ret = snd_soc_register_dai(&wm8940_dai); if (ret) { dev_err(codec->dev, "Failed to register DAI: %d\n", ret);
snd_soc_unregister_codec(codec);return ret;
goto err_codec;}
return 0;
+err_codec:
- snd_soc_unregister_codec(codec);
+err:
- kfree(wm8940);
- return ret;
}
static void wm8940_unregister(struct wm8940_priv *wm8940)
1.5.4.3
--- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
Hi Guennadi,
2010/7/15 Guennadi Liakhovetski g.liakhovetski@gmx.de:
Hi Axel
On Thu, 15 Jul 2010, Axel Lin wrote:
From 44c93254e5158e3086c4707148d24746067f6b14 Mon Sep 17 00:00:00 2001
From: Axel Lin axel.lin@gmail.com Date: Thu, 15 Jul 2010 10:13:34 +0800 Subject: [PATCH] wm8940: fix resource reclaim in wm8940_register error path
This patch fixes the error path in wm8940_register to properly free resources.
Signed-off-by: Axel Lin axel.lin@gmail.com
Ok, these bugs are real, but let's fix them properly, i.e., release resources at the same level, where they have been acquired:
OK. I'll send a revised patch now. I change the patch titile to: [PATCH v2 7/12] wm8940: fix a memory leak if wm8940_register return error
static int register_codec(struct drv_priv *data) { int ret = snd_soc_register_codec(); if (ret < 0) return ret;
ret = try(); if (failed) goto err;
...
err: snd_soc_unregister_codec(); }
static int probe() { int ret; struct drv_priv *data = kzalloc(sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM;
ret = register_codec(data); if (ret < 0) goto err;
...
err: kfree(data); return ret; }
That is, kfree() belongs to probe() and not to register_codec. This, of course, concerns all aour patches, at least those of them, that I have seen.
I agree that it is good to release resources at the same level, where they have been acquired. But I prefer to follow the maintainer/author's coding style.
For the changes in other drivers, I'd like to see Liam and Mark's comments. Or if the driver maintainers request it, I can fix it.
Regards, Axel
Thanks Guennadi
sound/soc/codecs/wm8940.c | 22 ++++++++++++++-------- 1 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c index e3c4bbf..9673e6f 100644 --- a/sound/soc/codecs/wm8940.c +++ b/sound/soc/codecs/wm8940.c @@ -766,7 +766,8 @@ static int wm8940_register(struct wm8940_priv *wm8940, u16 reg; if (wm8940_codec) { dev_err(codec->dev, "Another WM8940 is registered\n");
- return -EINVAL;
- ret = -EINVAL;
- goto err;
}
INIT_LIST_HEAD(&codec->dapm_widgets); @@ -785,7 +786,7 @@ static int wm8940_register(struct wm8940_priv *wm8940, ret = snd_soc_codec_set_cache_io(codec, 8, 16, control); if (ret < 0) { dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
- return ret;
- goto err;
}
memcpy(codec->reg_cache, wm8940_reg_defaults, @@ -794,7 +795,7 @@ static int wm8940_register(struct wm8940_priv *wm8940, ret = wm8940_reset(codec); if (ret < 0) { dev_err(codec->dev, "Failed to issue reset\n");
- return ret;
- goto err;
}
wm8940_dai.dev = codec->dev; @@ -803,7 +804,7 @@ static int wm8940_register(struct wm8940_priv *wm8940,
ret = snd_soc_write(codec, WM8940_POWER1, 0x180); if (ret < 0)
- return ret;
- goto err;
if (!pdata) dev_warn(codec->dev, "No platform data supplied\n"); @@ -811,7 +812,7 @@ static int wm8940_register(struct wm8940_priv *wm8940, reg = snd_soc_read(codec, WM8940_OUTPUTCTL); ret = snd_soc_write(codec, WM8940_OUTPUTCTL, reg | pdata->vroi); if (ret < 0)
- return ret;
- goto err;
}
@@ -820,17 +821,22 @@ static int wm8940_register(struct wm8940_priv *wm8940, ret = snd_soc_register_codec(codec); if (ret) { dev_err(codec->dev, "Failed to register codec: %d\n", ret);
- return ret;
- goto err;
}
ret = snd_soc_register_dai(&wm8940_dai); if (ret) { dev_err(codec->dev, "Failed to register DAI: %d\n", ret);
- snd_soc_unregister_codec(codec);
- return ret;
- goto err_codec;
}
return 0;
+err_codec:
- snd_soc_unregister_codec(codec);
+err:
- kfree(wm8940);
- return ret;
}
static void wm8940_unregister(struct wm8940_priv *wm8940)
1.5.4.3
Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
On Thu, 15 Jul 2010, Axel Lin wrote:
I agree that it is good to release resources at the same level, where they have been acquired. But I prefer to follow the maintainer/author's coding style.
Yeah, unless that "style" is a bug. IMHO this design is just buggy, unmaintainable, error-prone, and unjustified.
For the changes in other drivers, I'd like to see Liam and Mark's comments. Or if the driver maintainers request it, I can fix it.
Well, as the author of sound/soc/codecs/wm8978.c, I'd be happy if you change it in this way;) And also move the
kfree(wm8978);
line from wm8978_unregister() to wm8978_i2c_remove().
Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
On Thu, 2010-07-15 at 15:42 +0800, Axel Lin wrote:
I agree that it is good to release resources at the same level, where they have been acquired. But I prefer to follow the maintainer/author's coding style.
For the changes in other drivers, I'd like to see Liam and Mark's comments. Or if the driver maintainers request it, I can fix it.
Fwiw, the soon to be merged ASoC multi-component branch simplifies the codec probe() and remove() as follows (e.g. from I2C and SPI WM8750 codec) :-
static int wm8750_probe(struct snd_soc_codec *codec) { struct wm8750_priv *wm8750 = snd_soc_codec_get_drvdata(codec); int reg, ret;
codec->control_data = wm8750->control_data; ret = snd_soc_codec_set_cache_io(codec, 7, 9, wm8750->control_type); if (ret < 0) { printk(KERN_ERR "wm8750: failed to set cache I/O: %d\n", ret); return ret; }
ret = wm8750_reset(codec); if (ret < 0) { printk(KERN_ERR "wm8750: failed to reset: %d\n", ret); return ret; }
/* charge output caps */ wm8750_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
snd_soc_add_controls(codec, wm8750_snd_controls, ARRAY_SIZE(wm8750_snd_controls)); wm8750_add_widgets(codec); return ret; }
static int wm8750_remove(struct snd_soc_codec *codec) { wm8750_set_bias_level(codec, SND_SOC_BIAS_OFF); return 0; }
#if defined(CONFIG_SPI_MASTER) static int __devinit wm8750_spi_probe(struct spi_device *spi) { struct wm8750_priv *wm8750; int ret;
wm8750 = kzalloc(sizeof(struct wm8750_priv), GFP_KERNEL); if (wm8750 == NULL) return -ENOMEM;
wm8750->control_data = spi; wm8750->control_type = SND_SOC_SPI; spi_set_drvdata(spi, wm8750);
ret = snd_soc_register_codec(&spi->dev, spi->chip_select, &soc_codec_dev_wm8750, &wm8750_dai, 1); if (ret < 0) kfree(wm8750); return ret; }
static int __devexit wm8750_spi_remove(struct spi_device *spi) { snd_soc_unregister_codec(&spi->dev, spi->chip_select); kfree(spi_get_drvdata(spi)); return 0; }
static struct spi_driver wm8750_spi_driver = { .driver = { .name = "wm8750 SPI Codec", .bus = &spi_bus_type, .owner = THIS_MODULE, }, .probe = wm8750_spi_probe, .remove = __devexit_p(wm8750_spi_remove), }; #endif /* CONFIG_SPI_MASTER */
#if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) static __devinit int wm8750_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { struct wm8750_priv *wm8750; int ret;
wm8750 = kzalloc(sizeof(struct wm8750_priv), GFP_KERNEL); if (wm8750 == NULL) return -ENOMEM;
i2c_set_clientdata(i2c, wm8750); wm8750->control_data = i2c; wm8750->control_type = SND_SOC_I2C;
ret = snd_soc_register_codec(&i2c->dev, i2c->addr, &soc_codec_dev_wm8750, &wm8750_dai, 1); if (ret < 0) kfree(wm8750); return ret; }
static __devexit int wm8750_i2c_remove(struct i2c_client *client) { snd_soc_unregister_codec(&client->dev, client->addr); kfree(i2c_get_clientdata(client)); return 0; }
static const struct i2c_device_id wm8750_i2c_id[] = { { "wm8750", 0 }, { "wm8987", 0 }, { } }; MODULE_DEVICE_TABLE(i2c, wm8750_i2c_id);
static struct i2c_driver wm8750_i2c_driver = { .driver = { .name = "wm8750 I2C Codec", .owner = THIS_MODULE, }, .probe = wm8750_i2c_probe, .remove = __devexit_p(wm8750_i2c_remove), .id_table = wm8750_i2c_id, }; #endif
static int __init wm8750_modinit(void) { int ret = 0; #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) ret = i2c_add_driver(&wm8750_i2c_driver); if (ret != 0) { printk(KERN_ERR "Failed to register wm8750 I2C driver: %d\n", ret); } #endif #if defined(CONFIG_SPI_MASTER) ret = spi_register_driver(&wm8750_spi_driver); if (ret != 0) { printk(KERN_ERR "Failed to register wm8750 SPI driver: %d\n", ret); } #endif return ret; } module_init(wm8750_modinit);
static void __exit wm8750_exit(void) { #if defined(CONFIG_I2C) || defined(CONFIG_I2C_MODULE) i2c_del_driver(&wm8750_i2c_driver); #endif #if defined(CONFIG_SPI_MASTER) spi_unregister_driver(&wm8750_spi_driver); #endif } module_exit(wm8750_exit);
So while this patch series is useful (from a minor bug fix pov), it will be overwritten shortly in the ASoC multi-component merge. Probably delaying the multi-component merge too (since the changes are in the same place).
Can we hold off this series for a week or two. I have one more update for multi-component and if we don't have multi-component upstreamed in that time frame we can apply this series.
Thanks
Liam
This patch adds checking for wm8940_register return value, and does kfree(wm8940) if wm8940_register() fail.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm8940.c | 7 ++++++- 1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/sound/soc/codecs/wm8940.c b/sound/soc/codecs/wm8940.c index e3c4bbf..f0c1113 100644 --- a/sound/soc/codecs/wm8940.c +++ b/sound/soc/codecs/wm8940.c @@ -845,6 +845,7 @@ static void wm8940_unregister(struct wm8940_priv *wm8940) static int wm8940_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { + int ret; struct wm8940_priv *wm8940; struct snd_soc_codec *codec;
@@ -858,7 +859,11 @@ static int wm8940_i2c_probe(struct i2c_client *i2c, codec->control_data = i2c; codec->dev = &i2c->dev;
- return wm8940_register(wm8940, SND_SOC_I2C); + ret = wm8940_register(wm8940, SND_SOC_I2C); + if (ret < 0) + kfree(wm8940); + + return ret; }
static int __devexit wm8940_i2c_remove(struct i2c_client *client)
This patch fixes the error path in wm8955_register to properly free resources.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm8955.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/wm8955.c b/sound/soc/codecs/wm8955.c index fedb764..5f02559 100644 --- a/sound/soc/codecs/wm8955.c +++ b/sound/soc/codecs/wm8955.c @@ -964,7 +964,8 @@ static int wm8955_register(struct wm8955_priv *wm8955,
if (wm8955_codec) { dev_err(codec->dev, "Another WM8955 is registered\n"); - return -EINVAL; + ret = -EINVAL; + goto err; }
mutex_init(&codec->mutex); @@ -1047,18 +1048,19 @@ static int wm8955_register(struct wm8955_priv *wm8955, ret = snd_soc_register_codec(codec); if (ret != 0) { dev_err(codec->dev, "Failed to register codec: %d\n", ret); - return ret; + goto err_enable; }
ret = snd_soc_register_dai(&wm8955_dai); if (ret != 0) { dev_err(codec->dev, "Failed to register DAI: %d\n", ret); - snd_soc_unregister_codec(codec); - return ret; + goto err_codec; }
return 0;
+err_codec: + snd_soc_unregister_codec(codec); err_enable: regulator_bulk_disable(ARRAY_SIZE(wm8955->supplies), wm8955->supplies); err_get:
This patch fixes the error path in wm8961_register to properly free resources.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm8961.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/wm8961.c b/sound/soc/codecs/wm8961.c index 5b9a756..2549d3a 100644 --- a/sound/soc/codecs/wm8961.c +++ b/sound/soc/codecs/wm8961.c @@ -1102,7 +1102,7 @@ static int wm8961_register(struct wm8961_priv *wm8961) ret = wm8961_reset(codec); if (ret < 0) { dev_err(codec->dev, "Failed to issue reset\n"); - return ret; + goto err; }
/* Enable class W */ @@ -1147,18 +1147,19 @@ static int wm8961_register(struct wm8961_priv *wm8961) ret = snd_soc_register_codec(codec); if (ret != 0) { dev_err(codec->dev, "Failed to register codec: %d\n", ret); - return ret; + goto err; }
ret = snd_soc_register_dai(&wm8961_dai); if (ret != 0) { dev_err(codec->dev, "Failed to register DAI: %d\n", ret); - snd_soc_unregister_codec(codec); - return ret; + goto err_codec; }
return 0;
+err_codec: + snd_soc_unregister_codec(codec); err: kfree(wm8961); return ret;
wm8974 is allocated in wm8974_i2c_probe() but is not freed if wm8974_register() return -EINVAL (if another WM8974 is registered).
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm8974.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/sound/soc/codecs/wm8974.c b/sound/soc/codecs/wm8974.c index a2c4b2f..1468fe1 100644 --- a/sound/soc/codecs/wm8974.c +++ b/sound/soc/codecs/wm8974.c @@ -670,7 +670,8 @@ static __devinit int wm8974_register(struct wm8974_priv *wm8974)
if (wm8974_codec) { dev_err(codec->dev, "Another WM8974 is registered\n"); - return -EINVAL; + ret = -EINVAL; + goto err; }
mutex_init(&codec->mutex);
wm8978 is allocated in wm8978_i2c_probe() but is not freed if wm8978_register() return -EINVAL (if another WM8978 is registered).
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm8978.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/sound/soc/codecs/wm8978.c b/sound/soc/codecs/wm8978.c index 51d5f43..4e80323 100644 --- a/sound/soc/codecs/wm8978.c +++ b/sound/soc/codecs/wm8978.c @@ -1007,7 +1007,8 @@ static __devinit int wm8978_register(struct wm8978_priv *wm8978)
if (wm8978_codec) { dev_err(codec->dev, "Another WM8978 is registered\n"); - return -EINVAL; + ret = -EINVAL; + goto err; }
/*
From e53ff15ee6843d6591509f9aeeb2fa1a0c1eccee Mon Sep 17 00:00:00 2001
From: Axel Lin axel.lin@gmail.com Date: Thu, 15 Jul 2010 16:33:20 +0800 Subject: [PATCH] wm8978: fix a memory leak if a wm8978_register fail
There is a memory leak found if wm8978_register() fail. This patch moves the buffer allocate and release at the same level to prevent the memory leak.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm8978.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/wm8978.c b/sound/soc/codecs/wm8978.c index 51d5f43..8a1ad77 100644 --- a/sound/soc/codecs/wm8978.c +++ b/sound/soc/codecs/wm8978.c @@ -1076,7 +1076,6 @@ static __devinit int wm8978_register(struct wm8978_priv *wm8978) err_codec: snd_soc_unregister_codec(codec); err: - kfree(wm8978); return ret; }
@@ -1085,13 +1084,13 @@ static __devexit void wm8978_unregister(struct wm8978_priv *wm8978) wm8978_set_bias_level(&wm8978->codec, SND_SOC_BIAS_OFF); snd_soc_unregister_dai(&wm8978_dai); snd_soc_unregister_codec(&wm8978->codec); - kfree(wm8978); wm8978_codec = NULL; }
static __devinit int wm8978_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { + int ret; struct wm8978_priv *wm8978; struct snd_soc_codec *codec;
@@ -1107,13 +1106,18 @@ static __devinit int wm8978_i2c_probe(struct i2c_client *i2c,
codec->dev = &i2c->dev;
- return wm8978_register(wm8978); + ret = wm8978_register(wm8978); + if (ret < 0) + kfree(wm8978); + + return ret; }
static __devexit int wm8978_i2c_remove(struct i2c_client *client) { struct wm8978_priv *wm8978 = i2c_get_clientdata(client); wm8978_unregister(wm8978); + kfree(wm8978); return 0; }
On Thu, 15 Jul 2010, Axel Lin wrote:
From e53ff15ee6843d6591509f9aeeb2fa1a0c1eccee Mon Sep 17 00:00:00 2001
From: Axel Lin axel.lin@gmail.com Date: Thu, 15 Jul 2010 16:33:20 +0800 Subject: [PATCH] wm8978: fix a memory leak if a wm8978_register fail
There is a memory leak found if wm8978_register() fail. This patch moves the buffer allocate and release at the same level to prevent the memory leak.
Signed-off-by: Axel Lin axel.lin@gmail.com
Thanks for the patch, Axel! Only compile-tested, so, here's
Reviewed-by: Guennadi Liakhovetski g.liakhovetski@gmx.de
Guennadi
sound/soc/codecs/wm8978.c | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/wm8978.c b/sound/soc/codecs/wm8978.c index 51d5f43..8a1ad77 100644 --- a/sound/soc/codecs/wm8978.c +++ b/sound/soc/codecs/wm8978.c @@ -1076,7 +1076,6 @@ static __devinit int wm8978_register(struct wm8978_priv *wm8978) err_codec: snd_soc_unregister_codec(codec); err:
- kfree(wm8978); return ret;
}
@@ -1085,13 +1084,13 @@ static __devexit void wm8978_unregister(struct wm8978_priv *wm8978) wm8978_set_bias_level(&wm8978->codec, SND_SOC_BIAS_OFF); snd_soc_unregister_dai(&wm8978_dai); snd_soc_unregister_codec(&wm8978->codec);
- kfree(wm8978); wm8978_codec = NULL;
}
static __devinit int wm8978_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) {
- int ret; struct wm8978_priv *wm8978; struct snd_soc_codec *codec;
@@ -1107,13 +1106,18 @@ static __devinit int wm8978_i2c_probe(struct i2c_client *i2c,
codec->dev = &i2c->dev;
- return wm8978_register(wm8978);
- ret = wm8978_register(wm8978);
- if (ret < 0)
kfree(wm8978);- return ret;
}
static __devexit int wm8978_i2c_remove(struct i2c_client *client) { struct wm8978_priv *wm8978 = i2c_get_clientdata(client); wm8978_unregister(wm8978);
- kfree(wm8978); return 0;
}
-- 1.5.4.3
--- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
This patch fixes the error path in wm9081_register to properly free resources.
Signed-off-by: Axel Lin axel.lin@gmail.com --- sound/soc/codecs/wm9081.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/wm9081.c b/sound/soc/codecs/wm9081.c index 13186fb..76b37ff 100644 --- a/sound/soc/codecs/wm9081.c +++ b/sound/soc/codecs/wm9081.c @@ -1356,7 +1356,7 @@ static int wm9081_register(struct wm9081_priv *wm9081, ret = snd_soc_codec_set_cache_io(codec, 8, 16, control); if (ret != 0) { dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret); - return ret; + goto err; }
reg = snd_soc_read(codec, WM9081_SOFTWARE_RESET); @@ -1369,7 +1369,7 @@ static int wm9081_register(struct wm9081_priv *wm9081, ret = wm9081_reset(codec); if (ret < 0) { dev_err(codec->dev, "Failed to issue reset\n"); - return ret; + goto err; }
wm9081_set_bias_level(codec, SND_SOC_BIAS_STANDBY); @@ -1388,18 +1388,19 @@ static int wm9081_register(struct wm9081_priv *wm9081, ret = snd_soc_register_codec(codec); if (ret != 0) { dev_err(codec->dev, "Failed to register codec: %d\n", ret); - return ret; + goto err; }
ret = snd_soc_register_dai(&wm9081_dai); if (ret != 0) { dev_err(codec->dev, "Failed to register DAI: %d\n", ret); - snd_soc_unregister_codec(codec); - return ret; + goto err_codec; }
return 0;
+err_codec: + snd_soc_unregister_codec(codec); err: kfree(wm9081); return ret;
On Thu, Jul 15, 2010 at 10:49:07AM +0800, Axel Lin wrote:
[PATCH 1/12] ad1836: fix a memory leak if another ad1836 is registered [PATCH 2/12] ak4642: fix a memory leak if failed to initialise AK4642 [PATCH 3/12] da7210: fix a memory leak if failed to initialise da7210 audio codec [PATCH 4/12] wm8523: fix resource reclaim in wm8523_register error path [PATCH 5/12] wm8711: fix a memory leak if another WM8711 is registered [PATCH 6/12] wm8904: fix resource reclaim in wm8904_register error path [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register error path [PATCH 8/12] wm8955: fix resource reclaim in wm8955_register error path [PATCH 9/12] wm8961: fix resource reclaim in wm8961_register error path [PATCH 10/12] wm8974: fix a memory leak if another WM8974 is registered [PATCH 11/12] wm8978: fix a memory leak if another WM8978 is registered [PATCH 12/12] wm9081: fix resource reclaim in wm9081_register error path
If you're going to be submiting large batches of small patches like this it would be really helpful if you could make sure you format the subject lines for your patches so that they match the standards for the area of the kernel you're updating. This makes them much easier to apply.
hi Mark,
2010/7/15 Mark Brown broonie@opensource.wolfsonmicro.com:
On Thu, Jul 15, 2010 at 10:49:07AM +0800, Axel Lin wrote:
[PATCH 1/12] ad1836: fix a memory leak if another ad1836 is registered [PATCH 2/12] ak4642: fix a memory leak if failed to initialise AK4642 [PATCH 3/12] da7210: fix a memory leak if failed to initialise da7210 audio codec [PATCH 4/12] wm8523: fix resource reclaim in wm8523_register error path [PATCH 5/12] wm8711: fix a memory leak if another WM8711 is registered [PATCH 6/12] wm8904: fix resource reclaim in wm8904_register error path [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register error path [PATCH 8/12] wm8955: fix resource reclaim in wm8955_register error path [PATCH 9/12] wm8961: fix resource reclaim in wm8961_register error path [PATCH 10/12] wm8974: fix a memory leak if another WM8974 is registered [PATCH 11/12] wm8978: fix a memory leak if another WM8978 is registered [PATCH 12/12] wm9081: fix resource reclaim in wm9081_register error path
If you're going to be submiting large batches of small patches like this it would be really helpful if you could make sure you format the subject lines for your patches so that they match the standards for the area of the kernel you're updating. This makes them much easier to apply.
After checking Liam's multi-component branch, I think this serial of fix is not necessary. If you think it still has value to merge this serial of fix for current implementation, just let me know.
Regards, Axel
participants (6)
-
Axel Lin -
Barry Song -
Guennadi Liakhovetski -
Kuninori Morimoto -
Liam Girdwood -
Mark Brown