Commit 08df0d9a authored by Tzung-Bi Shih's avatar Tzung-Bi Shih Committed by Mark Brown

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

Commit 2dc98af6 ("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 2dc98af6 ("ASoC: max98090: fix lockdep warning") changes the
order to: c -> s -> d.  The lock inversion is still there.

2. Commit 62d5ae4c ("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: 2dc98af6 ("ASoC: max98090: fix lockdep warning")
Signed-off-by: default avatarTzung-Bi Shih <tzungbi@google.com>
Link: https://lore.kernel.org/r/20200117073814.82441-2-tzungbi@google.comSigned-off-by: default avatarMark Brown <broonie@kernel.org>
parent 250a15cf
...@@ -52,14 +52,14 @@ static void max98090_shdn_restore_locked(struct max98090_priv *max98090) ...@@ -52,14 +52,14 @@ static void max98090_shdn_restore_locked(struct max98090_priv *max98090)
static void max98090_shdn_save(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); max98090_shdn_save_locked(max98090);
} }
static void max98090_shdn_restore(struct max98090_priv *max98090) static void max98090_shdn_restore(struct max98090_priv *max98090)
{ {
max98090_shdn_restore_locked(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, static int max98090_put_volsw(struct snd_kcontrol *kcontrol,
...@@ -2313,12 +2313,12 @@ static void max98090_pll_work(struct max98090_priv *max98090) ...@@ -2313,12 +2313,12 @@ static void max98090_pll_work(struct max98090_priv *max98090)
*/ */
/* Toggle shutdown OFF then ON */ /* 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, snd_soc_component_update_bits(component, M98090_REG_DEVICE_SHUTDOWN,
M98090_SHDNN_MASK, 0); M98090_SHDNN_MASK, 0);
snd_soc_component_update_bits(component, M98090_REG_DEVICE_SHUTDOWN, snd_soc_component_update_bits(component, M98090_REG_DEVICE_SHUTDOWN,
M98090_SHDNN_MASK, M98090_SHDNN_MASK); M98090_SHDNN_MASK, M98090_SHDNN_MASK);
mutex_unlock(&max98090->shdn_lock); mutex_unlock(&component->card->dapm_mutex);
for (i = 0; i < 10; ++i) { for (i = 0; i < 10; ++i) {
/* Give PLL time to lock */ /* Give PLL time to lock */
...@@ -2731,8 +2731,6 @@ static int max98090_i2c_probe(struct i2c_client *i2c, ...@@ -2731,8 +2731,6 @@ static int max98090_i2c_probe(struct i2c_client *i2c,
if (max98090 == NULL) if (max98090 == NULL)
return -ENOMEM; return -ENOMEM;
mutex_init(&max98090->shdn_lock);
if (ACPI_HANDLE(&i2c->dev)) { if (ACPI_HANDLE(&i2c->dev)) {
acpi_id = acpi_match_device(i2c->dev.driver->acpi_match_table, acpi_id = acpi_match_device(i2c->dev.driver->acpi_match_table,
&i2c->dev); &i2c->dev);
......
...@@ -1539,7 +1539,6 @@ struct max98090_priv { ...@@ -1539,7 +1539,6 @@ struct max98090_priv {
unsigned int pa2en; unsigned int pa2en;
unsigned int sidetone; unsigned int sidetone;
bool master; bool master;
struct mutex shdn_lock;
int saved_count; int saved_count;
int saved_shdn; int saved_shdn;
}; };
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment