[PATCH] ALSA: ASoC: Fix memory leaks for the CS4270 driver

[PATCH] ALSA: ASoC: Fix memory leaks for the CS4270 driver

[Timur Tabi]

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 = {
-- 
1.5.5

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

[Takashi Iwai]

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
> 

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

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Takashi Iwai]

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

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

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Takashi Iwai]

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

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

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Takashi Iwai]

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

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

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Takashi Iwai]

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