Re: [alsa-devel] [PATCH] ALSA: ASoC: Fix memory leaks for the CS4270 driver

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