[PATCH] ASoC: soc-pcm: Fix DPCM lockdep warning due to nested stream locks

[Takashi Iwai]

On Thu, 13 Jan 2022 15:59:31 +0100,
Marek Szyprowski wrote:
> 
> On 13.01.2022 15:18, Takashi Iwai wrote:
> > The recent change for DPCM locking caused spurious lockdep warnings.
> > Actually the warnings are false-positive, as those are triggered due
> > to the nested stream locks for FE and BE.  Since both locks belong to
> > the same lock class, lockdep sees it as if a deadlock.
> >
> > For fixing this, we need take PCM stream locks for BE with the nested
> > lock primitives.  Since currently snd_pcm_stream_lock*() helper
> > assumes only the top-level single locking, a new helper function
> > snd_pcm_stream_lock_irqsave_nested() is defined for a single-depth
> > nested lock, which is now used in the BE DAI trigger that is always
> > performed inside a FE stream lock.
> >
> > Fixes: b7898396f4bb ("ASoC: soc-pcm: Fix and cleanup DPCM locking")
> > Reported-and-tested-by: Hans de Goede <hdegoede at redhat.com>
> > Reported-by: Marek Szyprowski <m.szyprowski at samsung.com>
> > Link: https://lore.kernel.org/r/73018f3c-9769-72ea-0325-b3f8e2381e30@redhat.com
> > Link: https://lore.kernel.org/alsa-devel/9a0abddd-49e9-872d-2f00-a1697340f786@samsung.com
> > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> 
> Thanks for the fix! It helps a bit, but I still get a warning (a 
> different one now):

Thanks for the quick testing.  Actually we do have multiple issues.

> root at target:~# speaker-test -l1
> 
> speaker-test 1.1.8
> 
> Playback device is default
> Stream parameters are 48000Hz, S16_LE, 1 channels
> Using 16 octaves of pink noise
> Rate set to 48000Hz (requested 48000Hz)
> Buffer size range from 128 to 131072
> Period size range from 64 to 65536
> Using max buffer size 131072
> Periods = 4
> was set period_size = 32768
> was set buffer_size = 131072
>   0 - Front Left
> Time per period = 0.022199
> max98090 1-0010: PLL unlocked
> 
> =====================================================
> WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected
> 5.16.0-next-20220113-00001-g3967460dbcf4 #11212 Not tainted
> -----------------------------------------------------
> speaker-test/1319 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
> c1296410 (pin_fs_lock){+.+.}-{2:2}, at: simple_pin_fs+0x1c/0xac
> 
> and this task is already holding:
> c2fe6ea4 (&group->lock){..-.}-{2:2}, at: dpcm_be_disconnect+0x3c/0x348
> which would create a new lock dependency:
>   (&group->lock){..-.}-{2:2} -> (pin_fs_lock){+.+.}-{2:2}

So that's the problem: we call debugfs_remove_recursive() in the
spinlocked context.

Could you try the patch below?  It's a bit hackish, but let's check
whether that's the real cause or not.


Takashi

---
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1268,6 +1268,7 @@ static void dpcm_be_reparent(struct snd_soc_pcm_runtime *fe,
 void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
 {
 	struct snd_soc_dpcm *dpcm, *d;
+	LIST_HEAD(deleted_dpcms);
 
 	snd_soc_dpcm_mutex_assert_held(fe);
 
@@ -1287,13 +1288,18 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
 		/* BEs still alive need new FE */
 		dpcm_be_reparent(fe, dpcm->be, stream);
 
-		dpcm_remove_debugfs_state(dpcm);
-
 		list_del(&dpcm->list_be);
+		list_move(&dpcm->list_fe, &deleted_dpcms);
+	}
+	snd_soc_dpcm_stream_unlock_irq(fe, stream);
+
+	while (!list_empty(&deleted_dpcms)) {
+		dpcm = list_first_entry(&deleted_dpcms, struct snd_soc_dpcm,
+					list_fe);
 		list_del(&dpcm->list_fe);
+		dpcm_remove_debugfs_state(dpcm);
 		kfree(dpcm);
 	}
-	snd_soc_dpcm_stream_unlock_irq(fe, stream);
 }
 
 /* get BE for DAI widget and stream */