| 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 |
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...).
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)
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
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 --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; };
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(-)