[alsa-devel] [PATCH] ALSA: ASoC: Fix memory leaks for the CS4270 driver
The CS4270 ASoC driver was not cleaning up resources completely during an I2C unbind. The same problem could occur if the bind failed.
Rearranged a couple functions to eliminate an unnecessary function declaration.
Signed-off-by: Timur Tabi timur@freescale.com ---
This patch is a fix for 2.6.27. It applies only to the ASoC V1 version of the driver. A similar fix for the V2 version will be posted later.
sound/soc/codecs/cs4270.c | 94 ++++++++++++++++++++++++++++++-------------- 1 files changed, 64 insertions(+), 30 deletions(-)
diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index 82d94f0..eb38e92 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -490,29 +490,6 @@ static int cs4270_mute(struct snd_soc_dai *dai, int mute)
#endif
-static int cs4270_i2c_probe(struct i2c_client *, const struct i2c_device_id *); - -/* A list of non-DAPM controls that the CS4270 supports */ -static const struct snd_kcontrol_new cs4270_snd_controls[] = { - SOC_DOUBLE_R("Master Playback Volume", - CS4270_VOLA, CS4270_VOLB, 0, 0xFF, 1) -}; - -static const struct i2c_device_id cs4270_id[] = { - {"cs4270", 0}, - {} -}; -MODULE_DEVICE_TABLE(i2c, cs4270_id); - -static struct i2c_driver cs4270_i2c_driver = { - .driver = { - .name = "CS4270 I2C", - .owner = THIS_MODULE, - }, - .id_table = cs4270_id, - .probe = cs4270_i2c_probe, -}; - /* * Global variable to store socdev for i2c probe function. * @@ -530,6 +507,12 @@ static struct i2c_driver cs4270_i2c_driver = { */ static struct snd_soc_device *cs4270_socdev;
+/* A list of non-DAPM controls that the CS4270 supports */ +static const struct snd_kcontrol_new cs4270_snd_controls[] = { + SOC_DOUBLE_R("Master Playback Volume", + CS4270_VOLA, CS4270_VOLB, 0, 0xFF, 1) +}; + /* * Initialize the I2C interface of the CS4270 * @@ -559,8 +542,7 @@ static int cs4270_i2c_probe(struct i2c_client *i2c_client, codec->reg_cache = kzalloc(CS4270_NUMREGS, GFP_KERNEL); if (!codec->reg_cache) { printk(KERN_ERR "cs4270: could not allocate register cache\n"); - ret = -ENOMEM; - goto error; + return -ENOMEM; }
/* Verify that we have a CS4270 */ @@ -610,20 +592,72 @@ static int cs4270_i2c_probe(struct i2c_client *i2c_client, return 0;
error: - if (codec->control_data) { - i2c_detach_client(i2c_client); - codec->control_data = NULL; + for (i = 1; i <= ARRAY_SIZE(cs4270_snd_controls); i++) { + struct snd_kcontrol *kctrl = + snd_ctl_find_numid(codec->card, i); + + if (kctrl) + snd_ctl_remove(codec->card, kctrl); }
+ codec->control_data = NULL; + kfree(codec->reg_cache); codec->reg_cache = NULL; codec->reg_cache_size = 0;
- kfree(i2c_client); - return ret; }
+/* + * Remove I2C interface of the CS4270 + * + * Since this driver cannot be compiled as a module, the only way this + * function can be called is if the I2C subsystem unbinds it. This can be + * triggered, for instance, by writing the device ID to this drivers + * 'unbind' file in /sys. + */ +static int cs4270_i2c_remove(struct i2c_client *i2c_client) +{ + struct snd_soc_codec *codec = i2c_get_clientdata(i2c_client); + + if (codec) { + unsigned int i; + + for (i = 1; i <= ARRAY_SIZE(cs4270_snd_controls); i++) { + struct snd_kcontrol *kctrl = + snd_ctl_find_numid(codec->card, i); + + if (kctrl) + snd_ctl_remove(codec->card, kctrl); + } + + codec->control_data = NULL; + + kfree(codec->reg_cache); + codec->reg_cache = NULL; + codec->reg_cache_size = 0; + } + + return 0; +} + +static const struct i2c_device_id cs4270_id[] = { + {"cs4270", 0}, + {} +}; +MODULE_DEVICE_TABLE(i2c, cs4270_id); + +static struct i2c_driver cs4270_i2c_driver = { + .driver = { + .name = "cs4270", + .owner = THIS_MODULE, + }, + .id_table = cs4270_id, + .probe = cs4270_i2c_probe, + .remove = cs4270_i2c_remove, +}; + #endif /* USE_I2C*/
struct snd_soc_dai cs4270_dai = {
At Wed, 3 Sep 2008 17:07:14 -0500, Timur Tabi wrote:
The CS4270 ASoC driver was not cleaning up resources completely during an I2C unbind. The same problem could occur if the bind failed.
Rearranged a couple functions to eliminate an unnecessary function declaration.
Signed-off-by: Timur Tabi timur@freescale.com
Thanks for the patch. But it can't be applied as is because of the following reasons:
- In cs4270_remove(), cs4270_i2c_detach() is called after snd_soc_free_pcms(). snd_soc_free_pcms() invokes snd_card_free() inside, and this releases the all resources already, including the control elements. That is, you free an already-freed item.
- The only case you'll need to care is the remaining controls at error since the driver still runs in the stand-alone mode as fallback. That is, the case snd_ctl_add() returns the error in cs4270_probe(). [BTW, this case is usually a fatal error and should be handled so in the caller side. But the error from attach_adapter callback is ignored in i2c core, so this still remains.] Well, looking the code again, you create only one element, and if this fails, there is no other remaining element. Thus, there is nothing to free there.
- The way you look for a kcontrol is wrong although it may work in practice. A usual way is to remember the kctl and use it for removal, or find the kctl via snd_ctl_find_id(), not _numid().
- Your patch merged in the ALSA tree already would conflict with this patch. This is a mess, results in the whole rebase of git tree.
So, I think the code in 2.6.27 tree can stay as is. There is no leak in the end. But, we can fix cs4270.c in the latest ALSA tree, e.g. add a missing remove callback to release codec->reg_cache.
thanks,
Takashi
This patch is a fix for 2.6.27. It applies only to the ASoC V1 version of the driver. A similar fix for the V2 version will be posted later.
sound/soc/codecs/cs4270.c | 94 ++++++++++++++++++++++++++++++-------------- 1 files changed, 64 insertions(+), 30 deletions(-)
diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index 82d94f0..eb38e92 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -490,29 +490,6 @@ static int cs4270_mute(struct snd_soc_dai *dai, int mute)
#endif
-static int cs4270_i2c_probe(struct i2c_client *, const struct i2c_device_id *);
-/* A list of non-DAPM controls that the CS4270 supports */ -static const struct snd_kcontrol_new cs4270_snd_controls[] = {
- SOC_DOUBLE_R("Master Playback Volume",
CS4270_VOLA, CS4270_VOLB, 0, 0xFF, 1)-};
-static const struct i2c_device_id cs4270_id[] = {
- {"cs4270", 0},
- {}
-}; -MODULE_DEVICE_TABLE(i2c, cs4270_id);
-static struct i2c_driver cs4270_i2c_driver = {
- .driver = {
.name = "CS4270 I2C",.owner = THIS_MODULE,- },
- .id_table = cs4270_id,
- .probe = cs4270_i2c_probe,
-};
/*
- Global variable to store socdev for i2c probe function.
@@ -530,6 +507,12 @@ static struct i2c_driver cs4270_i2c_driver = { */ static struct snd_soc_device *cs4270_socdev;
+/* A list of non-DAPM controls that the CS4270 supports */ +static const struct snd_kcontrol_new cs4270_snd_controls[] = {
- SOC_DOUBLE_R("Master Playback Volume",
CS4270_VOLA, CS4270_VOLB, 0, 0xFF, 1)+};
/*
- Initialize the I2C interface of the CS4270
@@ -559,8 +542,7 @@ static int cs4270_i2c_probe(struct i2c_client *i2c_client, codec->reg_cache = kzalloc(CS4270_NUMREGS, GFP_KERNEL); if (!codec->reg_cache) { printk(KERN_ERR "cs4270: could not allocate register cache\n");
ret = -ENOMEM;goto error;
return -ENOMEM;}
/* Verify that we have a CS4270 */
@@ -610,20 +592,72 @@ static int cs4270_i2c_probe(struct i2c_client *i2c_client, return 0;
error:
- if (codec->control_data) {
i2c_detach_client(i2c_client);codec->control_data = NULL;
for (i = 1; i <= ARRAY_SIZE(cs4270_snd_controls); i++) {
struct snd_kcontrol *kctrl =snd_ctl_find_numid(codec->card, i);if (kctrl)snd_ctl_remove(codec->card, kctrl);}
codec->control_data = NULL;
kfree(codec->reg_cache); codec->reg_cache = NULL; codec->reg_cache_size = 0;
- kfree(i2c_client);
- return ret;
}
+/*
- Remove I2C interface of the CS4270
- Since this driver cannot be compiled as a module, the only way this
- function can be called is if the I2C subsystem unbinds it. This can be
- triggered, for instance, by writing the device ID to this drivers
- 'unbind' file in /sys.
- */
+static int cs4270_i2c_remove(struct i2c_client *i2c_client) +{
- struct snd_soc_codec *codec = i2c_get_clientdata(i2c_client);
- if (codec) {
unsigned int i;for (i = 1; i <= ARRAY_SIZE(cs4270_snd_controls); i++) {struct snd_kcontrol *kctrl =snd_ctl_find_numid(codec->card, i);if (kctrl)snd_ctl_remove(codec->card, kctrl);}codec->control_data = NULL;kfree(codec->reg_cache);codec->reg_cache = NULL;codec->reg_cache_size = 0;- }
- return 0;
+}
+static const struct i2c_device_id cs4270_id[] = {
- {"cs4270", 0},
- {}
+}; +MODULE_DEVICE_TABLE(i2c, cs4270_id);
+static struct i2c_driver cs4270_i2c_driver = {
- .driver = {
.name = "cs4270",.owner = THIS_MODULE,- },
- .id_table = cs4270_id,
- .probe = cs4270_i2c_probe,
- .remove = cs4270_i2c_remove,
+};
#endif /* USE_I2C*/
struct snd_soc_dai cs4270_dai = {
1.5.5
Takashi Iwai wrote:
- In cs4270_remove(), cs4270_i2c_detach() is called after snd_soc_free_pcms(). snd_soc_free_pcms() invokes snd_card_free() inside, and this releases the all resources already, including the control elements. That is, you free an already-freed item.
I was wondering why so few drivers were calling snd_ctl_remove(). :-)
Well, looking the code again, you create only one element, and if this fails, there is no other remaining element. Thus, there is nothing to free there.
True, but I intend on adding more controls in the future, so I want to code to handle that automatically.
- The way you look for a kcontrol is wrong although it may work in practice. A usual way is to remember the kctl and use it for removal, or find the kctl via snd_ctl_find_id(), not _numid().
It would be nice if snd_kcontrol_new had a snd_kcontrol *. I could use it to store the pointer for later deallocation. Would you accept a patch to add that?
- Your patch merged in the ALSA tree already would conflict with this patch. This is a mess, results in the whole rebase of git tree.
What, wait other patch? I based this patch on git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git. It should apply cleanly on that repo.
So, I think the code in 2.6.27 tree can stay as is. There is no leak in the end. But, we can fix cs4270.c in the latest ALSA tree, e.g. add a missing remove callback to release codec->reg_cache.
I'm confused, because I thought I was fixing the latest ALSA tree.
At Thu, 04 Sep 2008 10:54:28 -0500, Timur Tabi wrote:
Takashi Iwai wrote:
- In cs4270_remove(), cs4270_i2c_detach() is called after snd_soc_free_pcms(). snd_soc_free_pcms() invokes snd_card_free() inside, and this releases the all resources already, including the control elements. That is, you free an already-freed item.
I was wondering why so few drivers were calling snd_ctl_remove(). :-)
Well, looking the code again, you create only one element, and if this fails, there is no other remaining element. Thus, there is nothing to free there.
True, but I intend on adding more controls in the future, so I want to code to handle that automatically.
- The way you look for a kcontrol is wrong although it may work in practice. A usual way is to remember the kctl and use it for removal, or find the kctl via snd_ctl_find_id(), not _numid().
It would be nice if snd_kcontrol_new had a snd_kcontrol *. I could use it to store the pointer for later deallocation. Would you accept a patch to add that?
Hmm.. Basically struct snd_kcontrol_new isn't for writing but only for reading. Maybe easier is to write a function to get snd_kcontrol pointer from the given snd_kcontrol_new.
- Your patch merged in the ALSA tree already would conflict with this patch. This is a mess, results in the whole rebase of git tree.
What, wait other patch? I based this patch on git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git. It should apply cleanly on that repo.
If the fix needs to go to 2.6.27, you need the patch against 2.6.27-rc5. And, this patch would conflict with the latest ALSA tree.
So, I think the code in 2.6.27 tree can stay as is. There is no leak in the end. But, we can fix cs4270.c in the latest ALSA tree, e.g. add a missing remove callback to release codec->reg_cache.
I'm confused, because I thought I was fixing the latest ALSA tree.
Well, the latest ALSA tree != 2.6.27. The question is whether we need a fix for 2.6.27 or not.
thanks,
Takashi
On Thu, Sep 4, 2008 at 11:32 AM, Takashi Iwai tiwai@suse.de wrote:
Well, the latest ALSA tree != 2.6.27. The question is whether we need a fix for 2.6.27 or not.
I guess not. Technically, there are bugs, but they'll never be seen. Kconfig doesn't allow the code to be compiled as modules anyway, and forcing an I2C unbind will break everything. Plus, none of these bugs exists in the ASoC V2 version of this driver, so any patch I submit will be superseded anyway.
Therefore, I official rescind this patch.
At Thu, 4 Sep 2008 13:20:33 -0500, Timur Tabi wrote:
On Thu, Sep 4, 2008 at 11:32 AM, Takashi Iwai tiwai@suse.de wrote:
Well, the latest ALSA tree != 2.6.27. The question is whether we need a fix for 2.6.27 or not.
I guess not. Technically, there are bugs, but they'll never be seen. Kconfig doesn't allow the code to be compiled as modules anyway, and forcing an I2C unbind will break everything. Plus, none of these bugs exists in the ASoC V2 version of this driver, so any patch I submit will be superseded anyway.
Therefore, I official rescind this patch.
OK, thanks. Let me know if you finish the revised patch.
Takashi
At Fri, 05 Sep 2008 08:45:40 -0500, Timur Tabi wrote:
Takashi Iwai wrote:
OK, thanks. Let me know if you finish the revised patch.
No, I'm just going to drop the whole thing. It's not worth fixing.
Hm, but I think the code in the current ALSA tree has leak at removal (i.e. missing remove callback), and ASoC v2 isn't for 2.6.28 yet. Not a big issue, though.
Takashi
Takashi Iwai wrote:
Hm, but I think the code in the current ALSA tree has leak at removal (i.e. missing remove callback), and ASoC v2 isn't for 2.6.28 yet. Not a big issue, though.
The drivers cannot be compiled as modules anyway, so they can never be removed. I only want to fix real bugs in the V1 drivers these days.
At Fri, 05 Sep 2008 09:11:57 -0500, Timur Tabi wrote:
Takashi Iwai wrote:
Hm, but I think the code in the current ALSA tree has leak at removal (i.e. missing remove callback), and ASoC v2 isn't for 2.6.28 yet. Not a big issue, though.
The drivers cannot be compiled as modules anyway, so they can never be removed.
OK, fair enough.
Takashi
participants (2)
-
Takashi Iwai -
Timur Tabi