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.
Andrey
Takashi
>From 96c05549bb01697b97204dfd51f8f917ab11a178 Mon Sep 17 00:00:00 2001
From: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
Date: Thu, 25 Mar 2021 12:07:38 -0400
Subject: 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.
[ 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@xxxxxxx>
---
include/sound/core.h | 4 ++--
sound/core/init.c | 3 ++-
sound/pci/hda/hda_intel.c | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/sound/core.h b/include/sound/core.h
index 0b220bfe001b..d5d5053e743e 100644
--- a/include/sound/core.h
+++ b/include/sound/core.h
@@ -157,8 +157,8 @@ static inline void snd_power_ref(struct snd_card *card)
static inline void snd_power_unref(struct snd_card *card)
{
- if (refcount_dec_and_test(&card->power_ref))
- wake_up(&card->power_ref_sleep);
+ refcount_dec(&card->power_ref);
+ wake_up(&card->power_ref_sleep);
}
/* init.c */
diff --git a/sound/core/init.c b/sound/core/init.c
index cb0ad0883329..31cd6830557d 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);
+ refcount_set(&card->power_ref, 1);
#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..10189d90416a 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));
+ refcount_read(&card->power_ref) == 1);
list_for_each_codec(codec, &chip->bus) {
pm_runtime_suspend(hda_codec_dev(codec));
pm_runtime_disable(hda_codec_dev(codec));
--
2.25.1
