[alsa-devel] [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 at 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
>