[PATCH] ASoC: dapm: Add locking to snd_soc_dapm_xxxx_pin functions

[PATCH] ASoC: dapm: Add locking to snd_soc_dapm_xxxx_pin functions

[Charles Keepax]

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;
 		}
 
@@ -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;
@@ -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;
 }
-- 
1.7.2.5

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

On Mon, Feb 10, 2014 at 11:05:36AM +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.

Unfortunately the fix needs to be in the callers to some extent - there
are situations where you want to do atomic updates of multiple pins so
that we don't end up bouncing the power up and down too much, we need
the unlocked version for things that care.  This means we need to at
least preserve an unlocked version and translate those callers that
might care over to it (not sure if any of them are in mainline).

It should also be safe to call the functions without explicit locking
during init since we won't run DAPM until we've finished init - it is
effectively locked even if we don't actually hold the mutex.  There
shouldn't be a race with marking the widget dirty since every widget
should start out dirty but I'd have to verify that one.  This will
account for a very large proportion of the callers so perhaps it's less
of an issue to add the locking to them than you had thought.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Takashi Iwai]

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

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

[Takashi Iwai]

[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

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

[Charles Keepax]

On Mon, Feb 10, 2014 at 12:08:03PM +0000, Mark Brown wrote:
> On Mon, Feb 10, 2014 at 11:05:36AM +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.
> 
> Unfortunately the fix needs to be in the callers to some extent - there
> are situations where you want to do atomic updates of multiple pins so
> that we don't end up bouncing the power up and down too much, we need
> the unlocked version for things that care.  This means we need to at
> least preserve an unlocked version and translate those callers that
> might care over to it (not sure if any of them are in mainline).

There are indeed none in mainline at the moment, or at least if
there are any they are not holding the lock at the moment so it
is not obviously they require atomic update.

> 
> It should also be safe to call the functions without explicit locking
> during init since we won't run DAPM until we've finished init - it is
> effectively locked even if we don't actually hold the mutex.  There
> shouldn't be a race with marking the widget dirty since every widget
> should start out dirty but I'd have to verify that one.  This will
> account for a very large proportion of the callers so perhaps it's less
> of an issue to add the locking to them than you had thought.

I will have a look I think this would cut the number down to low
10s of places that need fixed up.

How would you feel about adding an extra functions that allows the
caller to hold the lock and adding locking into the DAPM core for
the existing users? Feels like the intention would be more clear
and will allow out of mainline users requiring atomic updates to
still do that.

Thanks,
Charles

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

[Charles Keepax]

On Mon, Feb 10, 2014 at 02:05:26PM +0100, Takashi Iwai wrote:
> At Mon, 10 Feb 2014 11:05:36 +0000,
> Charles Keepax wrote:
> > 
<snip>
> > 
> 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().

Yeah spotted that as I was going through will do this in a
seperate patch if this one doesn't go in.

<snip>
> >  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.

Good spot thanks, I will update for this.

> 
> 
> > @@ -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.

Also a good point, thanks I will update.

> 
> 
> Takashi


Thanks,
Charles

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

[Takashi Iwai]

At Wed, 12 Feb 2014 11:26:40 +0000,
Charles Keepax wrote:
> 
> On Mon, Feb 10, 2014 at 02:05:26PM +0100, Takashi Iwai wrote:
> > At Mon, 10 Feb 2014 11:05:36 +0000,
> > Charles Keepax wrote:
> > > 
> <snip>
> > > 
> > 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().
> 
> Yeah spotted that as I was going through will do this in a
> seperate patch if this one doesn't go in.

As the deadlock seems present since long time ago, it'd be better to
have a fix patch (for stable kernels), then move to the new scheme, if
any.


Takashi

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]

On Wed, Feb 12, 2014 at 11:24:09AM +0000, Charles Keepax wrote:
> On Mon, Feb 10, 2014 at 12:08:03PM +0000, Mark Brown wrote:

> > Unfortunately the fix needs to be in the callers to some extent - there
> > are situations where you want to do atomic updates of multiple pins so
> > that we don't end up bouncing the power up and down too much, we need
> > the unlocked version for things that care.  This means we need to at
> > least preserve an unlocked version and translate those callers that
> > might care over to it (not sure if any of them are in mainline).

> There are indeed none in mainline at the moment, or at least if
> there are any they are not holding the lock at the moment so it
> is not obviously they require atomic update.

Anything doing this off jack detection probably falls into that category
so the core jack detection code ought to be doing a bulk update.  For
safety I'd say that anything that's doing multiple updates at once
should be assumed to expect them to go through together (that is the
most likely outcome after all and the whole update then sync idiom means
the code looks that way).

> How would you feel about adding an extra functions that allows the
> caller to hold the lock and adding locking into the DAPM core for
> the existing users? Feels like the intention would be more clear
> and will allow out of mainline users requiring atomic updates to
> still do that.

That was roughly what I was suggesting, yes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]