[PATCH 1/2] ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double()

[PATCH 1/2] ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double()

[Marek Szyprowski]

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;
-- 
2.17.1

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

[Marek Szyprowski]

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;
 };
-- 
2.17.1

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

[Tzung-Bi Shih]

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...).

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

[Tzung-Bi Shih]

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)

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

[Marek Szyprowski]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Tzung-Bi Shih]

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?

Applied "ASoC: max98090: fix lockdep warning" to the asoc tree

[Mark Brown]

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;
 };
-- 
2.20.1

Re: [PATCH 1/2] ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double()

[Tzung-Bi Shih]

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.

Applied "ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double()" to the asoc tree

[Mark Brown]

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;
-- 
2.20.1

Applied "ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double()" to the asoc tree

[Mark Brown]

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 

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 1d7b051891722a36ee0b228bc940dd245f161ab1 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 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;
-- 
2.20.1

Re: Applied "ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double()" to the asoc tree

[Marek Szyprowski]

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.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.

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);
>   	struct max98090_priv *max98090 =
>   		snd_soc_component_get_drvdata(component);
>   	int ret;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: Applied "ASoC: max98090: fix incorrect helper in max98090_dapm_put_enum_double()" to the asoc tree

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]