[alsa-devel] [PATCH 0/3] ASoC: max98090: fix lock issues

[alsa-devel] [PATCH 0/3] ASoC: max98090: fix lock issues

[Tzung-Bi Shih]

The 1st patch reverts an already merged patch which makes less sense.
(https://mailman.alsa-project.org/pipermail/alsa-devel/2020-January/161172.html)

The 3rd patch uses the function introduced from the 2nd patch to fix
a deadlock issue.  I wasn't aware of the deadlock issue because my
test environment doesn't change the MUX.  Could be manually tested by
something like: `amixer -c1 cset name='LTENL Mux' 0`.

Tzung-Bi Shih (3):
  ASoC: max98090: revert "ASoC: max98090: fix lockdep warning"
  ASoC: dapm: add snd_soc_dapm_put_enum_double_locked
  ASoC: max98090: fix deadlock in max98090_dapm_put_enum_double()

 include/sound/soc-dapm.h    |  2 ++
 sound/soc/codecs/max98090.c | 12 ++++-----
 sound/soc/codecs/max98090.h |  1 -
 sound/soc/soc-dapm.c        | 54 ++++++++++++++++++++++++++++---------
 4 files changed, 48 insertions(+), 21 deletions(-)

-- 
2.25.0.341.g760bfbb309-goog

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 1/3] ASoC: max98090: revert "ASoC: max98090: fix lockdep warning"

[Tzung-Bi Shih]

Commit 2dc98af62c32 ("ASoC: max98090: fix lockdep warning") introduced
a helpful-less small lock: shdn_lock.  Reverts the commit.

Reasons:

1. Lockdep should not be happy by either the original or current code.
From lockdep's point of view, there is a lock inversion anyway.

Let d = dapm_mutex, c = controls_rwsem, s = shdn_lock,

From the reported calling stack: lock acquisition order of
snd_soc_register_card() is: d -> c.
> snd_ctl_add_replace+0x3c/0x84
> dapm_create_or_share_kcontrol+0x24c/0x2e0
> snd_soc_dapm_new_widgets+0x308/0x594
> snd_soc_bind_card+0x80c/0xad4
> devm_snd_soc_register_card+0x34/0x6c

If calling snd_soc_dapm_put_enum_double() in kcontrol's put (e.g.
SOC_DAPM_ENUM_EXT), lock acquisition order is: c -> d.  Note that,
snd_soc_dapm_put_enum_double() acquires d.

The possible lock inversion is always there if registering sound card
and putting mixer control happen at the same time.  In fact, it never
happens because the control device don't show up to the userspace until
the sound card build success.

Commit 2dc98af62c32 ("ASoC: max98090: fix lockdep warning") changes the
order to: c -> s -> d.  The lock inversion is still there.

2. Commit 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when
changing sensitive registers SHDN bit") designed to use dapm_mutex to
protect SHDN bit.  Use a separate lock breaks the protection.

DAPM changes SHDN bit automatically when it finds the path.  Thus, any
code wants to change the SHDN bit, need to acquire the dapm_mutex first.

> SND_SOC_DAPM_SUPPLY("SHDN", M98090_REG_DEVICE_SHUTDOWN,
>        M98090_SHDNN_SHIFT, 0, NULL, 0),

Fixes: 2dc98af62c32 ("ASoC: max98090: fix lockdep warning")
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
 sound/soc/codecs/max98090.c | 10 ++++------
 sound/soc/codecs/max98090.h |  1 -
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 454cb8e5b0a1..c01ce4a3f86d 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -52,14 +52,14 @@ static void max98090_shdn_restore_locked(struct max98090_priv *max98090)
 
 static void max98090_shdn_save(struct max98090_priv *max98090)
 {
-	mutex_lock(&max98090->shdn_lock);
+	mutex_lock(&max98090->component->card->dapm_mutex);
 	max98090_shdn_save_locked(max98090);
 }
 
 static void max98090_shdn_restore(struct max98090_priv *max98090)
 {
 	max98090_shdn_restore_locked(max98090);
-	mutex_unlock(&max98090->shdn_lock);
+	mutex_unlock(&max98090->component->card->dapm_mutex);
 }
 
 static int max98090_put_volsw(struct snd_kcontrol *kcontrol,
@@ -2313,12 +2313,12 @@ static void max98090_pll_work(struct max98090_priv *max98090)
 	 */
 
 	/* Toggle shutdown OFF then ON */
-	mutex_lock(&max98090->shdn_lock);
+	mutex_lock(&component->card->dapm_mutex);
 	snd_soc_component_update_bits(component, M98090_REG_DEVICE_SHUTDOWN,
 			    M98090_SHDNN_MASK, 0);
 	snd_soc_component_update_bits(component, M98090_REG_DEVICE_SHUTDOWN,
 			    M98090_SHDNN_MASK, M98090_SHDNN_MASK);
-	mutex_unlock(&max98090->shdn_lock);
+	mutex_unlock(&component->card->dapm_mutex);
 
 	for (i = 0; i < 10; ++i) {
 		/* Give PLL time to lock */
@@ -2731,8 +2731,6 @@ static int max98090_i2c_probe(struct i2c_client *i2c,
 	if (max98090 == NULL)
 		return -ENOMEM;
 
-	mutex_init(&max98090->shdn_lock);
-
 	if (ACPI_HANDLE(&i2c->dev)) {
 		acpi_id = acpi_match_device(i2c->dev.driver->acpi_match_table,
 					    &i2c->dev);
diff --git a/sound/soc/codecs/max98090.h b/sound/soc/codecs/max98090.h
index dabd8be34a01..0a31708b7df7 100644
--- a/sound/soc/codecs/max98090.h
+++ b/sound/soc/codecs/max98090.h
@@ -1539,7 +1539,6 @@ struct max98090_priv {
 	unsigned int pa2en;
 	unsigned int sidetone;
 	bool master;
-	struct mutex shdn_lock;
 	int saved_count;
 	int saved_shdn;
 };
-- 
2.25.0.341.g760bfbb309-goog

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 2/3] ASoC: dapm: add snd_soc_dapm_put_enum_double_locked

[Tzung-Bi Shih]

Adds snd_soc_dapm_put_enum_double_locked() for those use cases if
dapm_mutex has already locked.

Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
 include/sound/soc-dapm.h |  2 ++
 sound/soc/soc-dapm.c     | 54 ++++++++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 1b6afbc1a4ed..2a306c6f3fbc 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -392,6 +392,8 @@ int snd_soc_dapm_get_enum_double(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_dapm_put_enum_double_locked(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_dapm_info_pin_switch(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_info *uinfo);
 int snd_soc_dapm_get_pin_switch(struct snd_kcontrol *kcontrol,
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 442846f12cd4..bc20ad9abf8b 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3441,17 +3441,8 @@ int snd_soc_dapm_get_enum_double(struct snd_kcontrol *kcontrol,
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_get_enum_double);
 
-/**
- * snd_soc_dapm_put_enum_double - dapm enumerated double mixer set callback
- * @kcontrol: mixer control
- * @ucontrol: control element information
- *
- * Callback to set the value of a dapm enumerated double mixer control.
- *
- * Returns 0 for success.
- */
-int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+static int __snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol, int locked)
 {
 	struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kcontrol);
 	struct snd_soc_card *card = dapm->card;
@@ -3474,7 +3465,9 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 		mask |= e->mask << e->shift_r;
 	}
 
-	mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
+	if (!locked)
+		mutex_lock_nested(&card->dapm_mutex,
+				  SND_SOC_DAPM_CLASS_RUNTIME);
 
 	change = dapm_kcontrol_set_value(kcontrol, val);
 
@@ -3496,15 +3489,50 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 		card->update = NULL;
 	}
 
-	mutex_unlock(&card->dapm_mutex);
+	if (!locked)
+		mutex_unlock(&card->dapm_mutex);
 
 	if (ret > 0)
 		soc_dpcm_runtime_update(card);
 
 	return change;
 }
+
+/**
+ * snd_soc_dapm_put_enum_double - dapm enumerated double mixer set callback
+ * @kcontrol: mixer control
+ * @ucontrol: control element information
+ *
+ * Callback to set the value of a dapm enumerated double mixer control.
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	return __snd_soc_dapm_put_enum_double(kcontrol, ucontrol, 0);
+}
 EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_double);
 
+/**
+ * snd_soc_dapm_put_enum_double_locked - dapm enumerated double mixer set
+ * callback
+ * @kcontrol: mixer control
+ * @ucontrol: control element information
+ *
+ * Callback to set the value of a dapm enumerated double mixer control.
+ * Must acquire dapm_mutex before calling the function.
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_dapm_put_enum_double_locked(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	dapm_assert_locked(snd_soc_dapm_kcontrol_dapm(kcontrol));
+	return __snd_soc_dapm_put_enum_double(kcontrol, ucontrol, 1);
+}
+EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_double_locked);
+
 /**
  * snd_soc_dapm_info_pin_switch - Info for a pin switch
  *
-- 
2.25.0.341.g760bfbb309-goog

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] [PATCH 3/3] ASoC: max98090: fix deadlock in max98090_dapm_put_enum_double()

[Tzung-Bi Shih]

Commit 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when
changing sensitive registers SHDN bit") uses dapm_mutex to protect SHDN
bit.  However, snd_soc_dapm_put_enum_double() in
max98090_dapm_put_enum_double() acquires the dapm_mutex again which
cause a deadlock.

Use snd_soc_dapm_put_enum_double_locked() instead to fix the deadlock.

Fixes: 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when changing sensitive registers SHDN bit")
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
---
 sound/soc/codecs/max98090.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index c01ce4a3f86d..0313e1183167 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -88,7 +88,7 @@ static int max98090_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 	int ret;
 
 	max98090_shdn_save(max98090);
-	ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol);
+	ret = snd_soc_dapm_put_enum_double_locked(kcontrol, ucontrol);
 	max98090_shdn_restore(max98090);
 
 	return ret;
-- 
2.25.0.341.g760bfbb309-goog

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: max98090: fix deadlock in max98090_dapm_put_enum_double()" to the asoc tree

[Mark Brown]

The patch

   ASoC: max98090: fix deadlock in max98090_dapm_put_enum_double()

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 294b7380ffe88d4a0626af1fee5843a3db0c913c Mon Sep 17 00:00:00 2001
From: Tzung-Bi Shih <tzungbi@google.com>
Date: Fri, 17 Jan 2020 15:38:14 +0800
Subject: [PATCH] ASoC: max98090: fix deadlock in
 max98090_dapm_put_enum_double()

Commit 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when
changing sensitive registers SHDN bit") uses dapm_mutex to protect SHDN
bit.  However, snd_soc_dapm_put_enum_double() in
max98090_dapm_put_enum_double() acquires the dapm_mutex again which
cause a deadlock.

Use snd_soc_dapm_put_enum_double_locked() instead to fix the deadlock.

Fixes: 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when changing sensitive registers SHDN bit")
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
Link: https://lore.kernel.org/r/20200117073814.82441-4-tzungbi@google.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/max98090.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index c01ce4a3f86d..0313e1183167 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -88,7 +88,7 @@ static int max98090_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 	int ret;
 
 	max98090_shdn_save(max98090);
-	ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol);
+	ret = snd_soc_dapm_put_enum_double_locked(kcontrol, ucontrol);
 	max98090_shdn_restore(max98090);
 
 	return ret;
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: dapm: add snd_soc_dapm_put_enum_double_locked" to the asoc tree

[Mark Brown]

The patch

   ASoC: dapm: add snd_soc_dapm_put_enum_double_locked

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 839284e794822ebc8b66f32006a5d4cad863e1fb Mon Sep 17 00:00:00 2001
From: Tzung-Bi Shih <tzungbi@google.com>
Date: Fri, 17 Jan 2020 15:38:13 +0800
Subject: [PATCH] ASoC: dapm: add snd_soc_dapm_put_enum_double_locked

Adds snd_soc_dapm_put_enum_double_locked() for those use cases if
dapm_mutex has already locked.

Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
Link: https://lore.kernel.org/r/20200117073814.82441-3-tzungbi@google.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/sound/soc-dapm.h |  2 ++
 sound/soc/soc-dapm.c     | 54 ++++++++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 1b6afbc1a4ed..2a306c6f3fbc 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -392,6 +392,8 @@ int snd_soc_dapm_get_enum_double(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_value *ucontrol);
+int snd_soc_dapm_put_enum_double_locked(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol);
 int snd_soc_dapm_info_pin_switch(struct snd_kcontrol *kcontrol,
 	struct snd_ctl_elem_info *uinfo);
 int snd_soc_dapm_get_pin_switch(struct snd_kcontrol *kcontrol,
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index 442846f12cd4..bc20ad9abf8b 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3441,17 +3441,8 @@ int snd_soc_dapm_get_enum_double(struct snd_kcontrol *kcontrol,
 }
 EXPORT_SYMBOL_GPL(snd_soc_dapm_get_enum_double);
 
-/**
- * snd_soc_dapm_put_enum_double - dapm enumerated double mixer set callback
- * @kcontrol: mixer control
- * @ucontrol: control element information
- *
- * Callback to set the value of a dapm enumerated double mixer control.
- *
- * Returns 0 for success.
- */
-int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
-	struct snd_ctl_elem_value *ucontrol)
+static int __snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol, int locked)
 {
 	struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kcontrol);
 	struct snd_soc_card *card = dapm->card;
@@ -3474,7 +3465,9 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 		mask |= e->mask << e->shift_r;
 	}
 
-	mutex_lock_nested(&card->dapm_mutex, SND_SOC_DAPM_CLASS_RUNTIME);
+	if (!locked)
+		mutex_lock_nested(&card->dapm_mutex,
+				  SND_SOC_DAPM_CLASS_RUNTIME);
 
 	change = dapm_kcontrol_set_value(kcontrol, val);
 
@@ -3496,15 +3489,50 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
 		card->update = NULL;
 	}
 
-	mutex_unlock(&card->dapm_mutex);
+	if (!locked)
+		mutex_unlock(&card->dapm_mutex);
 
 	if (ret > 0)
 		soc_dpcm_runtime_update(card);
 
 	return change;
 }
+
+/**
+ * snd_soc_dapm_put_enum_double - dapm enumerated double mixer set callback
+ * @kcontrol: mixer control
+ * @ucontrol: control element information
+ *
+ * Callback to set the value of a dapm enumerated double mixer control.
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	return __snd_soc_dapm_put_enum_double(kcontrol, ucontrol, 0);
+}
 EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_double);
 
+/**
+ * snd_soc_dapm_put_enum_double_locked - dapm enumerated double mixer set
+ * callback
+ * @kcontrol: mixer control
+ * @ucontrol: control element information
+ *
+ * Callback to set the value of a dapm enumerated double mixer control.
+ * Must acquire dapm_mutex before calling the function.
+ *
+ * Returns 0 for success.
+ */
+int snd_soc_dapm_put_enum_double_locked(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	dapm_assert_locked(snd_soc_dapm_kcontrol_dapm(kcontrol));
+	return __snd_soc_dapm_put_enum_double(kcontrol, ucontrol, 1);
+}
+EXPORT_SYMBOL_GPL(snd_soc_dapm_put_enum_double_locked);
+
 /**
  * snd_soc_dapm_info_pin_switch - Info for a pin switch
  *
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[alsa-devel] Applied "ASoC: max98090: revert "ASoC: max98090: fix lockdep warning"" to the asoc tree

[Mark Brown]

The patch

   ASoC: max98090: revert "ASoC: max98090: fix lockdep warning"

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 08df0d9a00f7ca07511614dd3b137fadd2de56fc Mon Sep 17 00:00:00 2001
From: Tzung-Bi Shih <tzungbi@google.com>
Date: Fri, 17 Jan 2020 15:38:12 +0800
Subject: [PATCH] ASoC: max98090: revert "ASoC: max98090: fix lockdep warning"

Commit 2dc98af62c32 ("ASoC: max98090: fix lockdep warning") introduced
a helpful-less small lock: shdn_lock.  Reverts the commit.

Reasons:

1. Lockdep should not be happy by either the original or current code.
From lockdep's point of view, there is a lock inversion anyway.

Let d = dapm_mutex, c = controls_rwsem, s = shdn_lock,

From the reported calling stack: lock acquisition order of
snd_soc_register_card() is: d -> c.
> snd_ctl_add_replace+0x3c/0x84
> dapm_create_or_share_kcontrol+0x24c/0x2e0
> snd_soc_dapm_new_widgets+0x308/0x594
> snd_soc_bind_card+0x80c/0xad4
> devm_snd_soc_register_card+0x34/0x6c

If calling snd_soc_dapm_put_enum_double() in kcontrol's put (e.g.
SOC_DAPM_ENUM_EXT), lock acquisition order is: c -> d.  Note that,
snd_soc_dapm_put_enum_double() acquires d.

The possible lock inversion is always there if registering sound card
and putting mixer control happen at the same time.  In fact, it never
happens because the control device don't show up to the userspace until
the sound card build success.

Commit 2dc98af62c32 ("ASoC: max98090: fix lockdep warning") changes the
order to: c -> s -> d.  The lock inversion is still there.

2. Commit 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when
changing sensitive registers SHDN bit") designed to use dapm_mutex to
protect SHDN bit.  Use a separate lock breaks the protection.

DAPM changes SHDN bit automatically when it finds the path.  Thus, any
code wants to change the SHDN bit, need to acquire the dapm_mutex first.

> SND_SOC_DAPM_SUPPLY("SHDN", M98090_REG_DEVICE_SHUTDOWN,
>        M98090_SHDNN_SHIFT, 0, NULL, 0),

Fixes: 2dc98af62c32 ("ASoC: max98090: fix lockdep warning")
Signed-off-by: Tzung-Bi Shih <tzungbi@google.com>
Link: https://lore.kernel.org/r/20200117073814.82441-2-tzungbi@google.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/max98090.c | 10 ++++------
 sound/soc/codecs/max98090.h |  1 -
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index 454cb8e5b0a1..c01ce4a3f86d 100644
--- a/sound/soc/codecs/max98090.c
+++ b/sound/soc/codecs/max98090.c
@@ -52,14 +52,14 @@ static void max98090_shdn_restore_locked(struct max98090_priv *max98090)
 
 static void max98090_shdn_save(struct max98090_priv *max98090)
 {
-	mutex_lock(&max98090->shdn_lock);
+	mutex_lock(&max98090->component->card->dapm_mutex);
 	max98090_shdn_save_locked(max98090);
 }
 
 static void max98090_shdn_restore(struct max98090_priv *max98090)
 {
 	max98090_shdn_restore_locked(max98090);
-	mutex_unlock(&max98090->shdn_lock);
+	mutex_unlock(&max98090->component->card->dapm_mutex);
 }
 
 static int max98090_put_volsw(struct snd_kcontrol *kcontrol,
@@ -2313,12 +2313,12 @@ static void max98090_pll_work(struct max98090_priv *max98090)
 	 */
 
 	/* Toggle shutdown OFF then ON */
-	mutex_lock(&max98090->shdn_lock);
+	mutex_lock(&component->card->dapm_mutex);
 	snd_soc_component_update_bits(component, M98090_REG_DEVICE_SHUTDOWN,
 			    M98090_SHDNN_MASK, 0);
 	snd_soc_component_update_bits(component, M98090_REG_DEVICE_SHUTDOWN,
 			    M98090_SHDNN_MASK, M98090_SHDNN_MASK);
-	mutex_unlock(&max98090->shdn_lock);
+	mutex_unlock(&component->card->dapm_mutex);
 
 	for (i = 0; i < 10; ++i) {
 		/* Give PLL time to lock */
@@ -2731,8 +2731,6 @@ static int max98090_i2c_probe(struct i2c_client *i2c,
 	if (max98090 == NULL)
 		return -ENOMEM;
 
-	mutex_init(&max98090->shdn_lock);
-
 	if (ACPI_HANDLE(&i2c->dev)) {
 		acpi_id = acpi_match_device(i2c->dev.driver->acpi_match_table,
 					    &i2c->dev);
diff --git a/sound/soc/codecs/max98090.h b/sound/soc/codecs/max98090.h
index dabd8be34a01..0a31708b7df7 100644
--- a/sound/soc/codecs/max98090.h
+++ b/sound/soc/codecs/max98090.h
@@ -1539,7 +1539,6 @@ struct max98090_priv {
 	unsigned int pa2en;
 	unsigned int sidetone;
 	bool master;
-	struct mutex shdn_lock;
 	int saved_count;
 	int saved_shdn;
 };
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel