[alsa-devel] [PATCH 1/2] ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double()
Commit 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when changing sensitive registers") extended the code for handling "LTENL Mux", "LTENR Mux", "LBENL Mux" and "LBENR Mux" controls by adding a custom max98090_dapm_put_enum_double() function to them. However that function used incorrect helper to get its component object. Fix this by using the proper snd_soc_dapm_* helper.
This fixes the following NULL pointer exception observed on Exynos4412-based Odroid U3 board: 8<--- cut here --- Unable to handle kernel NULL pointer dereference at virtual address 000000b0 pgd = (ptrval) [000000b0] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 1104 Comm: alsactl Not tainted 5.5.0-rc5-next-20200107 #166 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) PC is at __mutex_lock+0x54/0xb18 LR is at ___might_sleep+0x3c/0x2e0 ... Process alsactl (pid: 1104, stack limit = 0x(ptrval)) ... [<c0b49630>] (__mutex_lock) from [<c0b4a110>] (mutex_lock_nested+0x1c/0x24) [<c0b4a110>] (mutex_lock_nested) from [<c0839b3c>] (max98090_shdn_save+0x1c/0x28) [<c0839b3c>] (max98090_shdn_save) from [<c083a4f8>] (max98090_dapm_put_enum_double+0x20/0x40) [<c083a4f8>] (max98090_dapm_put_enum_double) from [<c080d0e8>] (snd_ctl_ioctl+0x190/0xbb8) [<c080d0e8>] (snd_ctl_ioctl) from [<c02cafec>] (ksys_ioctl+0x470/0xaf8) [<c02cafec>] (ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x28) ... ---[ end trace 0e93f0580f4b9241 ]---
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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c index 4c7b16d557e2..c01ce4a3f86d 100644 --- a/sound/soc/codecs/max98090.c +++ b/sound/soc/codecs/max98090.c @@ -82,7 +82,7 @@ static int max98090_dapm_put_enum_double(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_soc_component *component = - snd_soc_kcontrol_component(kcontrol); + snd_soc_dapm_kcontrol_component(kcontrol); struct max98090_priv *max98090 = snd_soc_component_get_drvdata(component); int ret;
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(-)
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; };
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.h...). 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.h...)
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.h...). 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#...) 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#...). 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)
- when userspace init scripts (alsactl) enumerates devices
(https://mailman.alsa-project.org/pipermail/alsa-devel/2019-December/160535.h...)
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.h...). 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?
The patch
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 2dc98af62c32ff6c8b9a32365346c5c407e291a8 Mon Sep 17 00:00:00 2001
From: Marek Szyprowski m.szyprowski@samsung.com Date: Wed, 8 Jan 2020 12:50:07 +0100 Subject: [PATCH] ASoC: max98090: fix lockdep warning
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 Link: https://lore.kernel.org/r/20200108115007.31095-2-m.szyprowski@samsung.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/codecs/max98090.c | 10 ++++++---- sound/soc/codecs/max98090.h | 1 + 2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c index ede03663cbed..ba0e3ba162f8 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; };
On Wed, Jan 8, 2020 at 7:50 PM Marek Szyprowski m.szyprowski@samsung.com wrote:
Fixes: 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when changing sensitive registers") Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com
Reviewed-by: Tzung-Bi Shih tzungbi@google.com
Thanks for finding and fixing the bug.
The fix also reminded me: there are two possible "context" to call max98090_dapm_put_enum_double( ): DAPM and userspace mixer control. - max98090_shdn_save( ) is designed for mixer control because it acquires dapm_mutex. - max98090_shdn_save_locked( ) is designed for DAPM without acquiring lock.
Current code:
+static int max98090_dapm_put_enum_double(struct snd_kcontrol *kcontrol,
[snip]
- max98090_shdn_save(max98090);
- ret = snd_soc_dapm_put_enum_double(kcontrol, ucontrol);
- max98090_shdn_restore(max98090);
Should it cause a deadlock if DAPM calls the max98090_dapm_put_enum_double( )? I didn't see a deadlock last time I tested the series. Will do further analysis on this.
The patch
ASoC: max98090: fix incorrect helper 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 4e93c1294f4b051d574d6bc59755d2863286990e Mon Sep 17 00:00:00 2001
From: Marek Szyprowski m.szyprowski@samsung.com Date: Wed, 8 Jan 2020 12:50:06 +0100 Subject: [PATCH] ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double()
Commit 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when changing sensitive registers") extended the code for handling "LTENL Mux", "LTENR Mux", "LBENL Mux" and "LBENR Mux" controls by adding a custom max98090_dapm_put_enum_double() function to them. However that function used incorrect helper to get its component object. Fix this by using the proper snd_soc_dapm_* helper.
This fixes the following NULL pointer exception observed on Exynos4412-based Odroid U3 board: 8<--- cut here --- Unable to handle kernel NULL pointer dereference at virtual address 000000b0 pgd = (ptrval) [000000b0] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 1104 Comm: alsactl Not tainted 5.5.0-rc5-next-20200107 #166 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) PC is at __mutex_lock+0x54/0xb18 LR is at ___might_sleep+0x3c/0x2e0 ... Process alsactl (pid: 1104, stack limit = 0x(ptrval)) ... [<c0b49630>] (__mutex_lock) from [<c0b4a110>] (mutex_lock_nested+0x1c/0x24) [<c0b4a110>] (mutex_lock_nested) from [<c0839b3c>] (max98090_shdn_save+0x1c/0x28) [<c0839b3c>] (max98090_shdn_save) from [<c083a4f8>] (max98090_dapm_put_enum_double+0x20/0x40) [<c083a4f8>] (max98090_dapm_put_enum_double) from [<c080d0e8>] (snd_ctl_ioctl+0x190/0xbb8) [<c080d0e8>] (snd_ctl_ioctl) from [<c02cafec>] (ksys_ioctl+0x470/0xaf8) [<c02cafec>] (ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x28) ... ---[ end trace 0e93f0580f4b9241 ]---
Fixes: 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when changing sensitive registers") Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com Reviewed-by: Tzung-Bi Shih tzungbi@google.com Link: https://lore.kernel.org/r/20200108115007.31095-1-m.szyprowski@samsung.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..ede03663cbed 100644 --- a/sound/soc/codecs/max98090.c +++ b/sound/soc/codecs/max98090.c @@ -98,7 +98,7 @@ static int max98090_put_enum_double(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_soc_component *component = - snd_soc_kcontrol_component(kcontrol); + snd_soc_dapm_kcontrol_component(kcontrol); struct max98090_priv *max98090 = snd_soc_component_get_drvdata(component); int ret;
Hi Mark
On 09.01.2020 22:29, Mark Brown wrote:
The patch
ASoC: max98090: fix incorrect helper 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.6All 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.
Something went wrong :/.
This patch has been applied twice. First time for the max98090_dapm_put_enum_double() function (as it has been designed), second time for max98090_put_enum_double(), what is completely wrong thing (it does exactly the opposite thing).
Mark, please revert/drop 4e93c1294f4b051d574d6bc59755d2863286990e commit.
Thanks, Mark
From 4e93c1294f4b051d574d6bc59755d2863286990e Mon Sep 17 00:00:00 2001
From: Marek Szyprowski m.szyprowski@samsung.com Date: Wed, 8 Jan 2020 12:50:06 +0100 Subject: [PATCH] ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double()
Commit 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when changing sensitive registers") extended the code for handling "LTENL Mux", "LTENR Mux", "LBENL Mux" and "LBENR Mux" controls by adding a custom max98090_dapm_put_enum_double() function to them. However that function used incorrect helper to get its component object. Fix this by using the proper snd_soc_dapm_* helper.
This fixes the following NULL pointer exception observed on Exynos4412-based Odroid U3 board: 8<--- cut here --- Unable to handle kernel NULL pointer dereference at virtual address 000000b0 pgd = (ptrval) [000000b0] *pgd=00000000 Internal error: Oops: 5 [#1] PREEMPT SMP ARM Modules linked in: CPU: 0 PID: 1104 Comm: alsactl Not tainted 5.5.0-rc5-next-20200107 #166 Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) PC is at __mutex_lock+0x54/0xb18 LR is at ___might_sleep+0x3c/0x2e0 ... Process alsactl (pid: 1104, stack limit = 0x(ptrval)) ... [<c0b49630>] (__mutex_lock) from [<c0b4a110>] (mutex_lock_nested+0x1c/0x24) [<c0b4a110>] (mutex_lock_nested) from [<c0839b3c>] (max98090_shdn_save+0x1c/0x28) [<c0839b3c>] (max98090_shdn_save) from [<c083a4f8>] (max98090_dapm_put_enum_double+0x20/0x40) [<c083a4f8>] (max98090_dapm_put_enum_double) from [<c080d0e8>] (snd_ctl_ioctl+0x190/0xbb8) [<c080d0e8>] (snd_ctl_ioctl) from [<c02cafec>] (ksys_ioctl+0x470/0xaf8) [<c02cafec>] (ksys_ioctl) from [<c0101000>] (ret_fast_syscall+0x0/0x28) ... ---[ end trace 0e93f0580f4b9241 ]---
Fixes: 62d5ae4cafb7 ("ASoC: max98090: save and restore SHDN when changing sensitive registers") Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com Reviewed-by: Tzung-Bi Shih tzungbi@google.com Link: https://lore.kernel.org/r/20200108115007.31095-1-m.szyprowski@samsung.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..ede03663cbed 100644 --- a/sound/soc/codecs/max98090.c +++ b/sound/soc/codecs/max98090.c @@ -98,7 +98,7 @@ static int max98090_put_enum_double(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol) { struct snd_soc_component *component =
snd_soc_kcontrol_component(kcontrol);
snd_soc_dapm_kcontrol_component(kcontrol);
Best regards
On Fri, Jan 10, 2020 at 10:02:05AM +0100, Marek Szyprowski wrote:
This patch has been applied twice. First time for the max98090_dapm_put_enum_double() function (as it has been designed), second time for max98090_put_enum_double(), what is completely wrong thing (it does exactly the opposite thing).
Mark, please revert/drop 4e93c1294f4b051d574d6bc59755d2863286990e commit.
OK. For things like this it's generally easiest to just send a revert that can be directly applied.
participants (3)
-
Marek Szyprowski -
Mark Brown -
Tzung-Bi Shih