From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752151AbaBJNr1 (ORCPT ); Mon, 10 Feb 2014 08:47:27 -0500 Received: from cantor2.suse.de ([195.135.220.15]:48934 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751449AbaBJNr0 (ORCPT ); Mon, 10 Feb 2014 08:47:26 -0500 Date: Mon, 10 Feb 2014 14:47:24 +0100 Message-ID: From: Takashi Iwai To: Charles Keepax Cc: broonie@kernel.org, alsa-devel@alsa-project.org, patches@opensource.wolfsonmicro.com, dmitry.torokhov@gmail.com, lgirdwood@gmail.com, linux-kernel@vger.kernel.org, cw00.choi@samsung.com, myungjoo.ham@samsung.com Subject: Re: [alsa-devel] [PATCH] ASoC: dapm: Add locking to snd_soc_dapm_xxxx_pin functions In-Reply-To: <20140210110536.GB6856@opensource.wolfsonmicro.com> References: <20140210110536.GB6856@opensource.wolfsonmicro.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [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 > --- > > 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