diff mbox series

[2/2] ASoC: max98090: fix lockdep warning

Message ID 20200108115007.31095-2-m.szyprowski@samsung.com (mailing list archive)
State Accepted
Commit 2dc98af62c32ff6c8b9a32365346c5c407e291a8
Headers show
Series [1/2] ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double() | expand

Commit Message

Marek Szyprowski Jan. 8, 2020, 11:50 a.m. UTC
Commit 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when changing
sensitive registers") extended the code for handling many controls by
adding a custom put function to them. That new custom put function
properly handles relations between codec's hardware registers. However
they used card->dapm_mutex to properly serialize those operations. This
in turn triggers a lockdep warning about possible circular dependency.
Fix this by introducing a separate mutex only for serializing the SHDN
hardware register related operations.

This fixes the following lockdep warning observed on Exynos4412-based
Odroid U3 board:
======================================================
WARNING: possible circular locking dependency detected
5.5.0-rc5-next-20200107 #166 Not tainted
------------------------------------------------------
alsactl/1104 is trying to acquire lock:
ed0d50f4 (&card->dapm_mutex){+.+.}, at: max98090_shdn_save+0x1c/0x28

but task is already holding lock:
edb4b49c (&card->controls_rwsem){++++}, at: snd_ctl_ioctl+0xcc/0xbb8

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (&card->controls_rwsem){++++}:
       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
       odroid_audio_probe+0x288/0x34c
       platform_drv_probe+0x6c/0xa4
       really_probe+0x200/0x490
       driver_probe_device+0x78/0x1f8
       bus_for_each_drv+0x74/0xb8
       __device_attach+0xd4/0x16c
       bus_probe_device+0x88/0x90
       deferred_probe_work_func+0x3c/0xd0
       process_one_work+0x22c/0x7c4
       worker_thread+0x44/0x524
       kthread+0x130/0x164
       ret_from_fork+0x14/0x20
       0x0

-> #0 (&card->dapm_mutex){+.+.}:
       lock_acquire+0xe8/0x270
       __mutex_lock+0x9c/0xb18
       mutex_lock_nested+0x1c/0x24
       max98090_shdn_save+0x1c/0x28
       max98090_put_enum_double+0x20/0x40
       snd_ctl_ioctl+0x190/0xbb8
       ksys_ioctl+0x470/0xaf8
       ret_fast_syscall+0x0/0x28
       0xbefaa564

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&card->controls_rwsem);
                               lock(&card->dapm_mutex);
                               lock(&card->controls_rwsem);
  lock(&card->dapm_mutex);

 *** DEADLOCK ***

1 lock held by alsactl/1104:
 #0: edb4b49c (&card->controls_rwsem){++++}, at: snd_ctl_ioctl+0xcc/0xbb8

stack backtrace:
CPU: 0 PID: 1104 Comm: alsactl Not tainted 5.5.0-rc5-next-20200107 #166
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
(unwind_backtrace) from [<c010e180>] (show_stack+0x10/0x14)
(show_stack) from [<c0b2a09c>] (dump_stack+0xb4/0xe0)
(dump_stack) from [<c018a1c0>] (check_noncircular+0x1ec/0x208)
(check_noncircular) from [<c018c5dc>] (__lock_acquire+0x1210/0x25ec)
(__lock_acquire) from [<c018e2d8>] (lock_acquire+0xe8/0x270)
(lock_acquire) from [<c0b49678>] (__mutex_lock+0x9c/0xb18)
(__mutex_lock) from [<c0b4a110>] (mutex_lock_nested+0x1c/0x24)
(mutex_lock_nested) from [<c0839b3c>] (max98090_shdn_save+0x1c/0x28)
(max98090_shdn_save) from [<c083a5b8>] (max98090_put_enum_double+0x20/0x40)
(max98090_put_enum_double) from [<c080d0e8>] (snd_ctl_ioctl+0x190/0xbb8)
(snd_ctl_ioctl) from [<c02cafec>] (ksys_ioctl+0x470/0xaf8)
(ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
...

Fixes: 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when changing sensitive registers")
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 sound/soc/codecs/max98090.c | 10 ++++++----
 sound/soc/codecs/max98090.h |  1 +
 2 files changed, 7 insertions(+), 4 deletions(-)

Comments

Tzung-Bi Shih Jan. 9, 2020, 5:36 a.m. UTC | #1
On Wed, Jan 8, 2020 at 7:50 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> Fix this by introducing a separate mutex only for serializing the SHDN
> hardware register related operations.

This fix makes less sense to me.  We used dapm_mutex intentionally
because: both DAPM and userspace mixer control would change SHDN bit
at the same time.

> This fixes the following lockdep warning observed on Exynos4412-based
> Odroid U3 board:
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.5.0-rc5-next-20200107 #166 Not tainted
> ------------------------------------------------------
> alsactl/1104 is trying to acquire lock:
> ed0d50f4 (&card->dapm_mutex){+.+.}, at: max98090_shdn_save+0x1c/0x28
>
> but task is already holding lock:
> edb4b49c (&card->controls_rwsem){++++}, at: snd_ctl_ioctl+0xcc/0xbb8
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&card->controls_rwsem){++++}:
>        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
>        odroid_audio_probe+0x288/0x34c
>        platform_drv_probe+0x6c/0xa4
>        really_probe+0x200/0x490
>        driver_probe_device+0x78/0x1f8
>        bus_for_each_drv+0x74/0xb8
>        __device_attach+0xd4/0x16c
>        bus_probe_device+0x88/0x90
>        deferred_probe_work_func+0x3c/0xd0
>        process_one_work+0x22c/0x7c4
>        worker_thread+0x44/0x524
>        kthread+0x130/0x164
>        ret_from_fork+0x14/0x20
>        0x0
>
> -> #0 (&card->dapm_mutex){+.+.}:
>        lock_acquire+0xe8/0x270
>        __mutex_lock+0x9c/0xb18
>        mutex_lock_nested+0x1c/0x24
>        max98090_shdn_save+0x1c/0x28
>        max98090_put_enum_double+0x20/0x40
>        snd_ctl_ioctl+0x190/0xbb8
>        ksys_ioctl+0x470/0xaf8
>        ret_fast_syscall+0x0/0x28
>        0xbefaa564

As the stack trace suggested: when the card was still registering,
alsactl can see the mixer control and try to change it.

We have a discussion on an older thread
(https://mailman.alsa-project.org/pipermail/alsa-devel/2019-December/160454.html).
It is weird: userspace should not see things (e.g. no controlC0) until
snd_card_register( ).

I would like to spend some time to find the root cause.  It would be a
little challenging though (I have no real runtime to test...).
Tzung-Bi Shih Jan. 9, 2020, 10:59 a.m. UTC | #2
On Thu, Jan 9, 2020 at 1:36 PM Tzung-Bi Shih <tzungbi@google.com> wrote:
> On Wed, Jan 8, 2020 at 7:50 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> > This fixes the following lockdep warning observed on Exynos4412-based
> > Odroid U3 board:
> > ======================================================
> > -> #1 (&card->controls_rwsem){++++}:
> >        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
> >        odroid_audio_probe+0x288/0x34c
> >        platform_drv_probe+0x6c/0xa4

I noticed the stack is a little different than the last time
(odroid_audio_probe vs. asoc_simple_probe).  Did you use the same
machine to test?
>        asoc_simple_probe+0x244/0x4a0
>        platform_drv_probe+0x6c/0xa4
(https://mailman.alsa-project.org/pipermail/alsa-devel/2019-December/160142.html)

> I would like to spend some time to find the root cause.  It would be a
> little challenging though (I have no real runtime to test...).

After a few hours of study, I failed to find the reason to cause the
possible circular locking.  And would need more of your input.

Followed the information provided in the message
(https://mailman.alsa-project.org/pipermail/alsa-devel/2019-December/160535.html).
As the message said "snd_soc_of_get_dai_link_codecs() return
-EPROBE_DEFER".  The snd_soc_of_get_dai_link_codecs( ) is before
devm_snd_soc_register_card( ), and I didn't find any side effects in
odroid_audio_probe( ).

Only a very minor issue: snd_soc_of_put_dai_link_codecs(codec_link)
will be called twice.  One in snd_soc_of_get_dai_link_codecs( ) when
return -EPROBE_DEFER; another one is under the label
"err_put_cpu_dai".
(https://elixir.bootlin.com/linux/v5.5-rc5/source/sound/soc/samsung/odroid.c#L328)
 The code doesn't generate any side effects because of
snd_soc_of_put_dai_link_codecs( )'s robustness.

Another minor thing: odroid_card_dais is not immutable but
odroid_audio_probe( ) would try to modify it
(https://elixir.bootlin.com/linux/v5.5-rc5/source/sound/soc/samsung/odroid.c#L295).
Again, I don't think it would produce any troubles.  I guess no
machine would have multiple sound cards, share the same machine
driver, and unbind/bind in runtime.

> It is weird: userspace should not see things (e.g. no controlC0) until
> snd_card_register( ).

(based on today's broonie/sound.git/for-next) I would like to provide
you more information about this statement to help you find further
information.
When userspace can see the control device?  Ideally,
snd_soc_bind_card( ) -> snd_card_register( ) ->
snd_device_register_all( ) -> __snd_device_register( ) ->
snd_ctl_dev_register( ) -> snd_register_device( ).
If you look at the calling stack of possible circular locking,
snd_soc_dapm_new_widgets( ) is before snd_card_register( ).  That's
why we think userspace should not see control devices (i.e. controlC0,
controlC1, ...) and should not be able to set mixer control via ioctl(
).


As this may not directly be related to the issue, could you share the
init script of alsactl in your system?  Does it follow the convention?
 (i.e. sound card is ready when receives controlC* changed event in
udev rule 78-sound-card.rules)
> 6. when userspace init scripts (alsactl) enumerates devices
(https://mailman.alsa-project.org/pipermail/alsa-devel/2019-December/160535.html)
Marek Szyprowski Jan. 9, 2020, 11:09 a.m. UTC | #3
Hi

On 09.01.2020 06:36, Tzung-Bi Shih wrote:
> On Wed, Jan 8, 2020 at 7:50 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> Fix this by introducing a separate mutex only for serializing the SHDN
>> hardware register related operations.
> This fix makes less sense to me.  We used dapm_mutex intentionally
> because: both DAPM and userspace mixer control would change SHDN bit
> at the same time.
>
>> This fixes the following lockdep warning observed on Exynos4412-based
>> Odroid U3 board:
>> ======================================================
>> WARNING: possible circular locking dependency detected
>> 5.5.0-rc5-next-20200107 #166 Not tainted
>> ------------------------------------------------------
>> alsactl/1104 is trying to acquire lock:
>> ed0d50f4 (&card->dapm_mutex){+.+.}, at: max98090_shdn_save+0x1c/0x28
>>
>> but task is already holding lock:
>> edb4b49c (&card->controls_rwsem){++++}, at: snd_ctl_ioctl+0xcc/0xbb8
>>
>> which lock already depends on the new lock.
>>
>> the existing dependency chain (in reverse order) is:
>>
>> -> #1 (&card->controls_rwsem){++++}:
>>         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
>>         odroid_audio_probe+0x288/0x34c
>>         platform_drv_probe+0x6c/0xa4
>>         really_probe+0x200/0x490
>>         driver_probe_device+0x78/0x1f8
>>         bus_for_each_drv+0x74/0xb8
>>         __device_attach+0xd4/0x16c
>>         bus_probe_device+0x88/0x90
>>         deferred_probe_work_func+0x3c/0xd0
>>         process_one_work+0x22c/0x7c4
>>         worker_thread+0x44/0x524
>>         kthread+0x130/0x164
>>         ret_from_fork+0x14/0x20
>>         0x0
>>
>> -> #0 (&card->dapm_mutex){+.+.}:
>>         lock_acquire+0xe8/0x270
>>         __mutex_lock+0x9c/0xb18
>>         mutex_lock_nested+0x1c/0x24
>>         max98090_shdn_save+0x1c/0x28
>>         max98090_put_enum_double+0x20/0x40
>>         snd_ctl_ioctl+0x190/0xbb8
>>         ksys_ioctl+0x470/0xaf8
>>         ret_fast_syscall+0x0/0x28
>>         0xbefaa564
> As the stack trace suggested: when the card was still registering,
> alsactl can see the mixer control and try to change it.

Nope. This is just a lockdep warning about possible hypothetical 
situation on the test system during the normal boot. It doesn't mean 
that the circular dependency actually happens (if so, it would end in 
deadlock). It also doesn't mean that such circular dependency can be 
really triggered, because some other dependencies that not known to 
lockdep engine might protect against it. However the easiest way to fix 
it is to use fine-grained locking instead of reusing some framework 
locks for other purposes. Such approach is also usually a good practice.

> We have a discussion on an older thread
> (https://mailman.alsa-project.org/pipermail/alsa-devel/2019-December/160454.html).
> It is weird: userspace should not see things (e.g. no controlC0) until
> snd_card_register( ).
>
> I would like to spend some time to find the root cause.  It would be a
> little challenging though (I have no real runtime to test...).


Best regards
Tzung-Bi Shih Jan. 10, 2020, 1:05 a.m. UTC | #4
On Thu, Jan 9, 2020 at 7:09 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
> On 09.01.2020 06:36, Tzung-Bi Shih wrote:
> > On Wed, Jan 8, 2020 at 7:50 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> Fix this by introducing a separate mutex only for serializing the SHDN
> >> hardware register related operations.
> > This fix makes less sense to me.  We used dapm_mutex intentionally
> > because: both DAPM and userspace mixer control would change SHDN bit
> > at the same time.

We should not use a separate lock.  Either mixer control or DAPM would
change the SHDN bit.  The patch overlooks the calling path from DAPM.
As a result, DAPM can change the bit in the middle of mixer control.

> Nope. This is just a lockdep warning about possible hypothetical
> situation on the test system during the normal boot. It doesn't mean
> that the circular dependency actually happens (if so, it would end in
> deadlock). It also doesn't mean that such circular dependency can be
> really triggered, because some other dependencies that not known to
> lockdep engine might protect against it. However the easiest way to fix
> it is to use fine-grained locking instead of reusing some framework
> locks for other purposes. Such approach is also usually a good practice.

If the possible circular locking is a hypothetical situation, shall we
ignore it since we are very sure userspace cannot see the control
devices when building the sound card?
diff mbox series

Patch

diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index c01ce4a3f86d..454cb8e5b0a1 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->component->card->dapm_mutex);
+	mutex_lock(&max98090->shdn_lock);
 	max98090_shdn_save_locked(max98090);
 }
 
 static void max98090_shdn_restore(struct max98090_priv *max98090)
 {
 	max98090_shdn_restore_locked(max98090);
-	mutex_unlock(&max98090->component->card->dapm_mutex);
+	mutex_unlock(&max98090->shdn_lock);
 }
 
 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(&component->card->dapm_mutex);
+	mutex_lock(&max98090->shdn_lock);
 	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(&component->card->dapm_mutex);
+	mutex_unlock(&max98090->shdn_lock);
 
 	for (i = 0; i < 10; ++i) {
 		/* Give PLL time to lock */
@@ -2731,6 +2731,8 @@  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 0a31708b7df7..dabd8be34a01 100644
--- a/sound/soc/codecs/max98090.h
+++ b/sound/soc/codecs/max98090.h
@@ -1539,6 +1539,7 @@  struct max98090_priv {
 	unsigned int pa2en;
 	unsigned int sidetone;
 	bool master;
+	struct mutex shdn_lock;
 	int saved_count;
 	int saved_shdn;
 };