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

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