Re: Adding movable PCIe BARs support in snd_hda_intel

[Andrey Grodzovsky <andrey.grodzovsky@amd.com>]

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

Attached.

Andrey

On 2021-03-25 12:38 p.m., Takashi Iwai wrote:
> On Thu, 25 Mar 2021 17:13:41 +0100,
> Andrey Grodzovsky wrote:
>>
>>
>>
>> On 2021-03-25 2:58 a.m., Takashi Iwai wrote:
>>> On Wed, 24 Mar 2021 22:43:24 +0100,
>>> Andrey Grodzovsky wrote:
>>>>
>>>> Few comments -
>>>>
>>>> 1) Why we don't use snd_power_wait_and_ref in patch 3 in the common
>>>> handler ?
>>>> Don't we want the PCI rescan sequence to 'wait for' any in flight
>>>> taks that might be accessing registers and not only read/write/tlv
>>>> accesses ?
>>>
>>> Right, we don't need to block all ioctls, but only the ones that may
>>> access the hardware.  So basically the patches 3-5 can be dropped if
>>> we take the patch 6.  The current patch was written on top of the
>>> previous series, that's why it has both.
>>>
>>>> 2) Possible deadlock -
>>>> In azx_rescan_prepare - you put the card into SNDRV_CTL_POWER_D3hot
>>>> first and then 'wait for' all in flight tasks with the refcount.
>>>> The in flight tasks on the other hand, using snd_power_wait_and_ref,
>>>> may have already bumped up the refcount and now 'wait for' the card
>>>> to go into SNDRV_CTL_POWER_D0 which can't happen since PCI rescan
>>>> waits for the refocunt to drop to 0 before proceeding.
>>>>
>>>> Instead of snd_power_wait_and_ref can't we just call snd_power_ref
>>>> in common IOCTL before checking for power_state != SNDRV_CTL_POWER_D0 ?
>>>> Or is it because you don't want to fail IOCTLs ?
>>>
>>> No, this is the intended behavior and should work as-is because
>>> snd_power_wait_and_ref() drops the refcount in the loop before
>>> sleeping.  The inc before the state check is a must for covering the
>>> possible race, and ditto for changing the power_state to D3hot before
>>> syncing.
>>
>> Ohh, missed the refcount dec in the wait loop... My bad.
>>
>> On boot the latest patch-set was throwing refcount warnings since
>> u are not supposed to inc from zerro count and so I fixed
>> with the attached patch. That way seems to work fine.
> 
> Ah, I completely forgot about the oddness of refcount_t.
> Then I guess it's simpler to replace with atomic_*() instead.
> 
> 
> thanks,
> 
> Takashi
> 

[-- Attachment #2: 0001-ALSA-Fix-refcount-splat.patch --]
[-- Type: text/x-patch, Size: 6429 bytes --]

From 0347ad666d2f0a20d1074a64dc5a1f6d9cec44b2 Mon Sep 17 00:00:00 2001
From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Date: Thu, 25 Mar 2021 12:07:38 -0400
Subject: ALSA: Fix refcount splat

Righ on boot bellow WARN observed

Fix by setting refcount to 1 and also
drop the optimization in snd_power_unref
since I can't rely on atomic refcount_sub_and_test
anymore.

v2: Switch to atomic type instead of refcount

[   41.710577 <    0.000078>] ------------[ cut here ]------------
[   41.710580 <    0.000003>] refcount_t: addition on 0; use-after-free.
[   41.710603 <    0.000023>] WARNING: CPU: 10 PID: 2781 at lib/refcount.c:25 refcount_warn_saturate+0x86/0x110
[   41.710614 <    0.000011>] Modules linked in: snd_hda_codec_hdmi(OE) snd_hda_intel(OE) snd_intel_dspcfg(OE) snd_hda_codec(OE) snd_hda_core(OE) snd_pcm(OE) snd_seq_midi(OE) snd_seq_midi_event(OE) snd_rawmidi(OE) snd_seq(OE) snd_seq_device(OE) snd_timer(OE) snd(OE) input_leds led_class joydev kvm k10temp drm(OE) drm_panel_orientation_quirks(OE) igb i2c_algo_bit pinctrl_amd
[   41.710679 <    0.000065>] CPU: 10 PID: 2781 Comm: alsactl Tainted: G           OE     5.11.0-rc3-pci-bars+ #7
[   41.710685 <    0.000006>] Hardware name: System manufacturer System Product Name/PRIME X470-PRO, BIOS 4406 02/28/2019
[   41.710689 <    0.000004>] RIP: 0010:refcount_warn_saturate+0x86/0x110
[   41.710696 <    0.000007>] Code: dc dd 27 02 01 e8 22 92 9f 00 0f 0b eb d9 80 3d cb dd 27 02 00 75 d0 48 c7 c7 40 f0 d9 83 c6 05 bb dd 27 02 01 e8 02 92 9f 00 <0f> 0b eb b9 80 3d ad dd 27 02 00 75 b0 48 c7 c7 a0 ef d9 83 c6 05
[   41.710701 <    0.000005>] RSP: 0018:ffff888259bcfb98 EFLAGS: 00010286
[   41.710707 <    0.000006>] RAX: 0000000000000000 RBX: 0000000000000002 RCX: 0000000000000000
[   41.710711 <    0.000004>] RDX: 0000000000000027 RSI: 0000000000000004 RDI: ffffed104b379f65
[   41.710715 <    0.000004>] RBP: ffff88816a60ef40 R08: ffffffff822b270e R09: ffffed10df33e014
[   41.710719 <    0.000004>] R10: ffff8886f99f009b R11: ffffed10df33e013 R12: 0000000000000001
[   41.710723 <    0.000004>] R13: 1ffff1104b379f79 R14: ffff88816a60ef40 R15: 0000000000000000
[   41.710728 <    0.000005>] FS:  00007f0e1817e440(0000) GS:ffff8886f9800000(0000) knlGS:0000000000000000
[   41.710732 <    0.000004>] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   41.710736 <    0.000004>] CR2: 00007f0e185f7000 CR3: 00000001532fa000 CR4: 00000000003506e0
[   41.710740 <    0.000004>] Call Trace:
[   41.710744 <    0.000004>]  snd_power_wait_and_ref+0x315/0x320 [snd]
[   41.710768 <    0.000024>]  ? snd_card_new+0x5e0/0x5e0 [snd]
[   41.710790 <    0.000022>]  ? lock_acquire+0xee/0x570
[   41.710797 <    0.000007>]  ? lock_acquire+0xee/0x570
[   41.710804 <    0.000007>]  ? lock_acquired+0xb4/0x5a0
[   41.710810 <    0.000006>]  ? snd_ctl_ioctl+0x240/0xb60 [snd]
[   41.710832 <    0.000022>]  ? snd_ctl_find_id+0x161/0x180 [snd]
[   41.710855 <    0.000023>]  snd_ctl_elem_write+0x1c4/0x350 [snd]
[   41.710878 <    0.000023>]  ? copy_ctl_value_from_user+0x2f0/0x2f0 [snd]
[   41.710901 <    0.000023>]  ? down_write+0x16c/0x2e0
[   41.710907 <    0.000006>]  ? snd_ctl_ioctl+0x240/0xb60 [snd]
[   41.710929 <    0.000022>]  ? rwsem_down_read_slowpath+0x5f0/0x5f0
[   41.710935 <    0.000006>]  ? _copy_from_user+0xa0/0xf0
[   41.710942 <    0.000007>]  snd_ctl_ioctl+0x24e/0xb60 [snd]
[   41.710965 <    0.000023>]  ? snd_ctl_elem_add_user+0x150/0x150 [snd]
[   41.710988 <    0.000023>]  ? selinux_capable+0x20/0x20
[   41.710995 <    0.000007>]  ? kmem_cache_free+0x145/0x390
[   41.711001 <    0.000006>]  ? lockdep_hardirqs_on_prepare+0xe/0x210
[   41.711007 <    0.000006>]  ? blkcg_maybe_throttle_current+0x7f/0x7f0
[   41.711014 <    0.000007>]  ? call_rcu+0x20b/0x3a0
[   41.711024 <    0.000010>]  __x64_sys_ioctl+0xe0/0x120
[   41.711029 <    0.000005>]  ? syscall_trace_enter.isra.0+0xc9/0x270
[   41.711036 <    0.000007>]  do_syscall_64+0x33/0x80
[   41.711042 <    0.000006>]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   41.711048 <    0.000006>] RIP: 0033:0x7f0e183eb50b

Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 include/sound/core.h      | 6 +++---
 sound/core/init.c         | 3 ++-
 sound/pci/hda/hda_intel.c | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/sound/core.h b/include/sound/core.h
index 0b220bfe001b..8ac6647bc39a 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -126,7 +126,7 @@ struct snd_card {
 #ifdef CONFIG_PM
 	unsigned int power_state;	/* power state */
 	wait_queue_head_t power_sleep;
-	refcount_t power_ref;
+	atomic_t power_ref;
 	wait_queue_head_t power_ref_sleep;
 #endif
 
@@ -152,12 +152,12 @@ static inline void snd_power_change_state(struct snd_card *card, unsigned int st
 
 static inline void snd_power_ref(struct snd_card *card)
 {
-	refcount_inc(&card->power_ref);
+	atomic_inc(&card->power_ref);
 }
 
 static inline void snd_power_unref(struct snd_card *card)
 {
-	if (refcount_dec_and_test(&card->power_ref))
+	if (atomic_dec_and_test(&card->power_ref))
 		wake_up(&card->power_ref_sleep);
 }
 
diff --git a/sound/core/init.c b/sound/core/init.c
index cb0ad0883329..d26f904e9d45 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -215,8 +215,8 @@ int snd_card_new(struct device *parent, int idx, const char *xid,
 	mutex_init(&card->memory_mutex);
 #ifdef CONFIG_PM
 	init_waitqueue_head(&card->power_sleep);
-	refcount_set(&card->power_ref, 0);
 	init_waitqueue_head(&card->power_ref_sleep);
+	atomic_set(&card->power_ref, 0);
 #endif
 	init_waitqueue_head(&card->remove_sleep);
 	card->sync_irq = -1;
@@ -1007,6 +1007,7 @@ int snd_power_wait_and_ref(struct snd_card *card, bool ref)
 	/* fastpath */
 	if (ref)
 		snd_power_ref(card);
+
 	if (snd_power_get_state(card) == SNDRV_CTL_POWER_D0)
 		return 0;
 	init_waitqueue_entry(&wait, current);
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index b6d118c2b75b..11138b47fc73 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2457,7 +2457,7 @@ static void azx_rescan_prepare(struct pci_dev *pdev)
 		azx_prepare(&pdev->dev);
 		azx_suspend_streams(chip);
 		wait_event(card->power_ref_sleep,
-			   !refcount_read(&card->power_ref));
+			   !atomic_read(&card->power_ref));
 		list_for_each_codec(codec, &chip->bus) {
 			pm_runtime_suspend(hda_codec_dev(codec));
 			pm_runtime_disable(hda_codec_dev(codec));
-- 
2.25.1