* [PATCH 1/12] ad1836: fix a memory leak if another ad1836 is registered
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
@ 2010-07-15 2:50 ` Axel Lin
2010-07-20 10:31 ` Barry Song
2010-07-15 2:52 ` [PATCH 2/12] ak4642: fix a memory leak if failed to initialise AK4642 Axel Lin
` (11 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Axel Lin @ 2010-07-15 2:50 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Barry Song, Tejun Heo, alsa-devel
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;
}
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 1/12] ad1836: fix a memory leak if another ad1836 is registered
2010-07-15 2:50 ` [PATCH 1/12] ad1836: fix a memory leak if another ad1836 is registered Axel Lin
@ 2010-07-20 10:31 ` Barry Song
0 siblings, 0 replies; 26+ messages in thread
From: Barry Song @ 2010-07-20 10:31 UTC (permalink / raw)
To: Axel Lin
Cc: alsa-devel, Takashi Iwai, Mark Brown, linux-kernel, Tejun Heo,
Barry Song, Liam Girdwood
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/
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/12] ak4642: fix a memory leak if failed to initialise AK4642
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
2010-07-15 2:50 ` [PATCH 1/12] ad1836: fix a memory leak if another ad1836 is registered Axel Lin
@ 2010-07-15 2:52 ` Axel Lin
2010-07-15 5:53 ` Kuninori Morimoto
2010-07-15 2:53 ` [PATCH 3/12] da7210: fix a memory leak if failed to initialise da7210 audio codec Axel Lin
` (10 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Axel Lin @ 2010-07-15 2:52 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Kuninori Morimoto, Tejun Heo, alsa-devel
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;
}
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 2/12] ak4642: fix a memory leak if failed to initialise AK4642
2010-07-15 2:52 ` [PATCH 2/12] ak4642: fix a memory leak if failed to initialise AK4642 Axel Lin
@ 2010-07-15 5:53 ` Kuninori Morimoto
2010-07-15 6:19 ` Axel Lin
0 siblings, 1 reply; 26+ messages in thread
From: Kuninori Morimoto @ 2010-07-15 5:53 UTC (permalink / raw)
To: Axel Lin
Cc: alsa-devel, Takashi Iwai, Mark Brown, linux-kernel, Tejun Heo,
Liam Girdwood
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/12] ak4642: fix a memory leak if failed to initialise AK4642
2010-07-15 5:53 ` Kuninori Morimoto
@ 2010-07-15 6:19 ` Axel Lin
2010-07-15 6:29 ` Kuninori Morimoto
0 siblings, 1 reply; 26+ messages in thread
From: Axel Lin @ 2010-07-15 6:19 UTC (permalink / raw)
To: Kuninori Morimoto
Cc: linux-kernel, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Tejun Heo, alsa-devel
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
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 2/12] ak4642: fix a memory leak if failed to initialise AK4642
2010-07-15 6:19 ` Axel Lin
@ 2010-07-15 6:29 ` Kuninori Morimoto
0 siblings, 0 replies; 26+ messages in thread
From: Kuninori Morimoto @ 2010-07-15 6:29 UTC (permalink / raw)
To: axel.lin
Cc: alsa-devel, Takashi Iwai, Mark Brown, linux-kernel, Tejun Heo,
Liam Girdwood
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 3/12] da7210: fix a memory leak if failed to initialise da7210 audio codec
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
2010-07-15 2:50 ` [PATCH 1/12] ad1836: fix a memory leak if another ad1836 is registered Axel Lin
2010-07-15 2:52 ` [PATCH 2/12] ak4642: fix a memory leak if failed to initialise AK4642 Axel Lin
@ 2010-07-15 2:53 ` Axel Lin
2010-07-15 2:56 ` [PATCH 4/12] wm8523: fix resource reclaim in wm8523_register error path Axel Lin
` (9 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 2:53 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Kuninori Morimoto, Phil Edworthy, Tejun Heo, alsa-devel
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;
}
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 4/12] wm8523: fix resource reclaim in wm8523_register error path
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
` (2 preceding siblings ...)
2010-07-15 2:53 ` [PATCH 3/12] da7210: fix a memory leak if failed to initialise da7210 audio codec Axel Lin
@ 2010-07-15 2:56 ` Axel Lin
2010-07-15 2:57 ` [PATCH 5/12] wm8711: fix a memory leak if another WM8711 is registered Axel Lin
` (8 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 2:56 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Tejun Heo, alsa-devel
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:
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 5/12] wm8711: fix a memory leak if another WM8711 is registered
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
` (3 preceding siblings ...)
2010-07-15 2:56 ` [PATCH 4/12] wm8523: fix resource reclaim in wm8523_register error path Axel Lin
@ 2010-07-15 2:57 ` Axel Lin
2010-07-15 2:59 ` [PATCH 6/12] wm8904: fix resource reclaim in wm8904_register error path Axel Lin
` (7 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 2:57 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Tejun Heo, alsa-devel
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);
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 6/12] wm8904: fix resource reclaim in wm8904_register error path
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
` (4 preceding siblings ...)
2010-07-15 2:57 ` [PATCH 5/12] wm8711: fix a memory leak if another WM8711 is registered Axel Lin
@ 2010-07-15 2:59 ` Axel Lin
2010-07-15 3:01 ` [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register " Axel Lin
` (6 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 2:59 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Tejun Heo, alsa-devel
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:
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register error path
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
` (5 preceding siblings ...)
2010-07-15 2:59 ` [PATCH 6/12] wm8904: fix resource reclaim in wm8904_register error path Axel Lin
@ 2010-07-15 3:01 ` Axel Lin
2010-07-15 7:04 ` Guennadi Liakhovetski
2010-07-15 7:48 ` [PATCH v2 7/12] wm8940: fix a memory leak if wm8940_register return error Axel Lin
2010-07-15 3:03 ` [PATCH 8/12] wm8955: fix resource reclaim in wm8955_register error path Axel Lin
` (5 subsequent siblings)
12 siblings, 2 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 3:01 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Tejun Heo, Guennadi Liakhovetski, alsa-devel
>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)
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register error path
2010-07-15 3:01 ` [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register " Axel Lin
@ 2010-07-15 7:04 ` Guennadi Liakhovetski
2010-07-15 7:42 ` Axel Lin
2010-07-15 7:48 ` [PATCH v2 7/12] wm8940: fix a memory leak if wm8940_register return error Axel Lin
1 sibling, 1 reply; 26+ messages in thread
From: Guennadi Liakhovetski @ 2010-07-15 7:04 UTC (permalink / raw)
To: Axel Lin
Cc: alsa-devel, Takashi Iwai, Mark Brown, linux-kernel, Tejun Heo,
Liam Girdwood
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/
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register error path
2010-07-15 7:04 ` Guennadi Liakhovetski
@ 2010-07-15 7:42 ` Axel Lin
2010-07-15 7:59 ` Guennadi Liakhovetski
2010-07-15 8:54 ` Liam Girdwood
0 siblings, 2 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 7:42 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: linux-kernel, Liam Girdwood, Mark Brown, Jaroslav Kysela,
Takashi Iwai, Tejun Heo, alsa-devel
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/
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register error path
2010-07-15 7:42 ` Axel Lin
@ 2010-07-15 7:59 ` Guennadi Liakhovetski
2010-07-15 8:54 ` Liam Girdwood
1 sibling, 0 replies; 26+ messages in thread
From: Guennadi Liakhovetski @ 2010-07-15 7:59 UTC (permalink / raw)
To: Axel Lin
Cc: alsa-devel, Takashi Iwai, Mark Brown, linux-kernel, Tejun Heo,
Liam Girdwood
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/
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register error path
2010-07-15 7:42 ` Axel Lin
2010-07-15 7:59 ` Guennadi Liakhovetski
@ 2010-07-15 8:54 ` Liam Girdwood
1 sibling, 0 replies; 26+ messages in thread
From: Liam Girdwood @ 2010-07-15 8:54 UTC (permalink / raw)
To: axel.lin
Cc: alsa-devel, Takashi Iwai, Mark Brown, linux-kernel, Tejun Heo,
Guennadi Liakhovetski
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
--
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 7/12] wm8940: fix a memory leak if wm8940_register return error
2010-07-15 3:01 ` [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register " Axel Lin
2010-07-15 7:04 ` Guennadi Liakhovetski
@ 2010-07-15 7:48 ` Axel Lin
1 sibling, 0 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 7:48 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Tejun Heo, Guennadi Liakhovetski, alsa-devel
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)
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 8/12] wm8955: fix resource reclaim in wm8955_register error path
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
` (6 preceding siblings ...)
2010-07-15 3:01 ` [PATCH 7/12] wm8940: fix resource reclaim in wm8940_register " Axel Lin
@ 2010-07-15 3:03 ` Axel Lin
2010-07-15 3:06 ` [PATCH 9/12] wm8961: fix resource reclaim in wm8961_register " Axel Lin
` (4 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 3:03 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Tejun Heo, alsa-devel
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:
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 9/12] wm8961: fix resource reclaim in wm8961_register error path
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
` (7 preceding siblings ...)
2010-07-15 3:03 ` [PATCH 8/12] wm8955: fix resource reclaim in wm8955_register error path Axel Lin
@ 2010-07-15 3:06 ` Axel Lin
2010-07-15 3:07 ` [PATCH 10/12] wm8974: fix a memory leak if another WM8974 is registered Axel Lin
` (3 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 3:06 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Tejun Heo, alsa-devel
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;
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 10/12] wm8974: fix a memory leak if another WM8974 is registered
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
` (8 preceding siblings ...)
2010-07-15 3:06 ` [PATCH 9/12] wm8961: fix resource reclaim in wm8961_register " Axel Lin
@ 2010-07-15 3:07 ` Axel Lin
2010-07-15 3:08 ` [PATCH 11/12] wm8978: fix a memory leak if another WM8978 " Axel Lin
` (2 subsequent siblings)
12 siblings, 0 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 3:07 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Guennadi Liakhovetski, Javier Martin, Tejun Heo, alsa-devel
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);
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 11/12] wm8978: fix a memory leak if another WM8978 is registered
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
` (9 preceding siblings ...)
2010-07-15 3:07 ` [PATCH 10/12] wm8974: fix a memory leak if another WM8974 is registered Axel Lin
@ 2010-07-15 3:08 ` Axel Lin
2010-07-15 8:37 ` [PATCH v2 " Axel Lin
2010-07-15 3:11 ` [PATCH 12/12] wm9081: fix resource reclaim in wm9081_register error path Axel Lin
2010-07-15 8:59 ` [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in " Mark Brown
12 siblings, 1 reply; 26+ messages in thread
From: Axel Lin @ 2010-07-15 3:08 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Guennadi Liakhovetski, Tejun Heo, alsa-devel
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;
}
/*
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 11/12] wm8978: fix a memory leak if another WM8978 is registered
2010-07-15 3:08 ` [PATCH 11/12] wm8978: fix a memory leak if another WM8978 " Axel Lin
@ 2010-07-15 8:37 ` Axel Lin
2010-07-15 8:53 ` Guennadi Liakhovetski
0 siblings, 1 reply; 26+ messages in thread
From: Axel Lin @ 2010-07-15 8:37 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Guennadi Liakhovetski, Tejun Heo, alsa-devel
>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;
}
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v2 11/12] wm8978: fix a memory leak if another WM8978 is registered
2010-07-15 8:37 ` [PATCH v2 " Axel Lin
@ 2010-07-15 8:53 ` Guennadi Liakhovetski
0 siblings, 0 replies; 26+ messages in thread
From: Guennadi Liakhovetski @ 2010-07-15 8:53 UTC (permalink / raw)
To: Axel Lin
Cc: alsa-devel, Takashi Iwai, Mark Brown, linux-kernel, Tejun Heo,
Liam Girdwood
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/
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 12/12] wm9081: fix resource reclaim in wm9081_register error path
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
` (10 preceding siblings ...)
2010-07-15 3:08 ` [PATCH 11/12] wm8978: fix a memory leak if another WM8978 " Axel Lin
@ 2010-07-15 3:11 ` Axel Lin
2010-07-15 8:59 ` [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in " Mark Brown
12 siblings, 0 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-15 3:11 UTC (permalink / raw)
To: linux-kernel
Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai,
Daniel Ribeiro, Tejun Heo, Jiri Kosina, alsa-devel
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;
--
1.5.4.3
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path
2010-07-15 2:49 [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path Axel Lin
` (11 preceding siblings ...)
2010-07-15 3:11 ` [PATCH 12/12] wm9081: fix resource reclaim in wm9081_register error path Axel Lin
@ 2010-07-15 8:59 ` Mark Brown
2010-07-19 1:41 ` Axel Lin
12 siblings, 1 reply; 26+ messages in thread
From: Mark Brown @ 2010-07-15 8:59 UTC (permalink / raw)
To: Axel Lin; +Cc: alsa-devel, Takashi Iwai, linux-kernel, Tejun Heo, Liam Girdwood
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.
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in error path
2010-07-15 8:59 ` [PATCH 0/12] sound/alsa/soc/codec: fix memory leak and resource relaim in " Mark Brown
@ 2010-07-19 1:41 ` Axel Lin
0 siblings, 0 replies; 26+ messages in thread
From: Axel Lin @ 2010-07-19 1:41 UTC (permalink / raw)
To: Mark Brown
Cc: linux-kernel, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
Tejun Heo, alsa-devel
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
^ permalink raw reply [flat|nested] 26+ messages in thread