Re: [alsa-devel] [PATCH] ASoC: dapm: Add locking to snd_soc_dapm_xxxx_pin functions

[Takashi Iwai <tiwai@suse.de>]

[it seems that my previous post didn't go out properly, so resending;
 please discard if you already received the same mail]

At Mon, 10 Feb 2014 11:05:36 +0000,
Charles Keepax wrote:
> 
> snd_soc_dapm_disable_pin, snd_soc_dapm_enable_pin and
> snd_soc_dapm_force_enable_pin all require the dapm_mutex to be held when
> they are called as they edit the dirty list. There are 385 usages of
> these functions and only 7 hold the lock whilst calling.
> 
> This patch moves the locking into snd_soc_dapm_set_pin and fixes up the
> places where the lock was held on the caller side. This saves on fixing
> up all the current users and also is much more consistant with the rest
> of the DAPM API which all handles the locking internally.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
> 
> Hi,
> 
> I have put the changes in a single patch to avoid bisect
> problems, but let me know if it would be better split into
> seperate patches.
> 
> Thanks,
> Charles
> 
>  drivers/extcon/extcon-arizona.c      |   11 -----------
>  drivers/input/misc/arizona-haptics.c |   19 -------------------
>  sound/soc/soc-dapm.c                 |    8 ++++----
>  3 files changed, 4 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index c20602f..84167f4 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -222,26 +222,19 @@ static void arizona_extcon_pulse_micbias(struct arizona_extcon_info *info)
>  	struct snd_soc_dapm_context *dapm = arizona->dapm;
>  	int ret;
>  
> -	mutex_lock(&dapm->card->dapm_mutex);
> -
>  	ret = snd_soc_dapm_force_enable_pin(dapm, widget);
>  	if (ret != 0)
>  		dev_warn(arizona->dev, "Failed to enable %s: %d\n",
>  			 widget, ret);
>  
> -	mutex_unlock(&dapm->card->dapm_mutex);
> -
>  	snd_soc_dapm_sync(dapm);
>  
>  	if (!arizona->pdata.micd_force_micbias) {
> -		mutex_lock(&dapm->card->dapm_mutex);
> -
>  		ret = snd_soc_dapm_disable_pin(arizona->dapm, widget);
>  		if (ret != 0)
>  			dev_warn(arizona->dev, "Failed to disable %s: %d\n",
>  				 widget, ret);
>  
> -		mutex_unlock(&dapm->card->dapm_mutex);
>  
>  		snd_soc_dapm_sync(dapm);
>  	}
> @@ -304,16 +297,12 @@ static void arizona_stop_mic(struct arizona_extcon_info *info)
>  				 ARIZONA_MICD_ENA, 0,
>  				 &change);
>  
> -	mutex_lock(&dapm->card->dapm_mutex);
> -
>  	ret = snd_soc_dapm_disable_pin(dapm, widget);
>  	if (ret != 0)
>  		dev_warn(arizona->dev,
>  			 "Failed to disable %s: %d\n",
>  			 widget, ret);
>  
> -	mutex_unlock(&dapm->card->dapm_mutex);
> -
>  	snd_soc_dapm_sync(dapm);
>  
>  	if (info->micd_reva) {
> diff --git a/drivers/input/misc/arizona-haptics.c b/drivers/input/misc/arizona-haptics.c
> index 7a04f54..ef2e281 100644
> --- a/drivers/input/misc/arizona-haptics.c
> +++ b/drivers/input/misc/arizona-haptics.c
> @@ -37,7 +37,6 @@ static void arizona_haptics_work(struct work_struct *work)
>  						       struct arizona_haptics,
>  						       work);
>  	struct arizona *arizona = haptics->arizona;
> -	struct mutex *dapm_mutex = &arizona->dapm->card->dapm_mutex;
>  	int ret;
>  
>  	if (!haptics->arizona->dapm) {
> @@ -67,13 +66,10 @@ static void arizona_haptics_work(struct work_struct *work)
>  			return;
>  		}
>  
> -		mutex_lock_nested(dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
> -
>  		ret = snd_soc_dapm_enable_pin(arizona->dapm, "HAPTICS");
>  		if (ret != 0) {
>  			dev_err(arizona->dev, "Failed to start HAPTICS: %d\n",
>  				ret);
> -			mutex_unlock(dapm_mutex);
>  			return;
>  		}
>  

Actually, this fixes also the double-lock in snd_soc_dapm_sync() call
in the line below the above.  snd_soc_dapm_sync() itself takes
mutex_lock_nested().


> @@ -81,21 +77,14 @@ static void arizona_haptics_work(struct work_struct *work)
>  		if (ret != 0) {
>  			dev_err(arizona->dev, "Failed to sync DAPM: %d\n",
>  				ret);
> -			mutex_unlock(dapm_mutex);
>  			return;
>  		}
> -
> -		mutex_unlock(dapm_mutex);
> -
>  	} else {
>  		/* This disable sequence will be a noop if already enabled */
> -		mutex_lock_nested(dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
> -
>  		ret = snd_soc_dapm_disable_pin(arizona->dapm, "HAPTICS");
>  		if (ret != 0) {
>  			dev_err(arizona->dev, "Failed to disable HAPTICS: %d\n",
>  				ret);
> -			mutex_unlock(dapm_mutex);
>  			return;
>  		}
>  
> @@ -103,12 +92,9 @@ static void arizona_haptics_work(struct work_struct *work)
>  		if (ret != 0) {
>  			dev_err(arizona->dev, "Failed to sync DAPM: %d\n",
>  				ret);
> -			mutex_unlock(dapm_mutex);
>  			return;
>  		}
>  
> -		mutex_unlock(dapm_mutex);
> -
>  		ret = regmap_update_bits(arizona->regmap,
>  					 ARIZONA_HAPTICS_CONTROL_1,
>  					 ARIZONA_HAP_CTRL_MASK,
> @@ -155,16 +141,11 @@ static int arizona_haptics_play(struct input_dev *input, void *data,
>  static void arizona_haptics_close(struct input_dev *input)
>  {
>  	struct arizona_haptics *haptics = input_get_drvdata(input);
> -	struct mutex *dapm_mutex = &haptics->arizona->dapm->card->dapm_mutex;
>  
>  	cancel_work_sync(&haptics->work);
>  
> -	mutex_lock_nested(dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
> -
>  	if (haptics->arizona->dapm)
>  		snd_soc_dapm_disable_pin(haptics->arizona->dapm, "HAPTICS");
> -
> -	mutex_unlock(dapm_mutex);
>  }
>  
>  static int arizona_haptics_probe(struct platform_device *pdev)
> diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
> index dc8ff13..a44b560 100644
> --- a/sound/soc/soc-dapm.c
> +++ b/sound/soc/soc-dapm.c
> @@ -2325,6 +2325,8 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
>  {
>  	struct snd_soc_dapm_widget *w = dapm_find_widget(dapm, pin, true);
>  
> +	mutex_lock_nested(&dapm->card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
> +
>  	if (!w) {
>  		dev_err(dapm->dev, "ASoC: DAPM unknown pin %s\n", pin);
>  		return -EINVAL;

Missing unlock here.


> @@ -2337,6 +2339,8 @@ static int snd_soc_dapm_set_pin(struct snd_soc_dapm_context *dapm,
>  	if (status == 0)
>  		w->force = 0;
>  
> +	mutex_unlock(&dapm->card->dapm_mutex);
> +
>  	return 0;
>  }
>  
> @@ -3210,15 +3214,11 @@ int snd_soc_dapm_put_pin_switch(struct snd_kcontrol *kcontrol,
>  	struct snd_soc_card *card = snd_kcontrol_chip(kcontrol);
>  	const char *pin = (const char *)kcontrol->private_value;
>  
> -	mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
> -
>  	if (ucontrol->value.integer.value[0])
>  		snd_soc_dapm_enable_pin(&card->dapm, pin);
>  	else
>  		snd_soc_dapm_disable_pin(&card->dapm, pin);
>  
> -	mutex_unlock(&card->dapm_mutex);
> -
>  	snd_soc_dapm_sync(&card->dapm);
>  	return 0;
>  }

I guess you forgot patching snd_soc_dapm_force_enable_pin()?
It's now left unprotected after this patch.


Takashi