Re: [PATCH] ALSA: dummy: Defer hrtimer data freeing until runtime detach

[Takashi Iwai <tiwai@suse.de>]

On Wed, 14 Oct 2026 11:33:54 +0200,
Cen Zhang wrote:
> 
> Hi Takashi,
> 
> >
> > That's not what I didn't understand: do both A and B use the same
> > device?  The release shouldn't happen until all detached, so the
> > prepare and the release can't happen concurrently for the same device.
> >
> > If A and B are for different devices, they don't interfere with each
> > other in the case of dummy driver.
> >
> 
> Yes, A and B use the same PCM device. The test opens
> /dev/snd/pcmC0D0c twice, and SNDRV_PCM_IOCTL_INFO reports:
> 
>   A: card=0, device=0, subdevice=0, capture
>   B: card=0, device=0, subdevice=1, capture
> 
> They are separate substreams with separate runtimes. With the hrtimer
> backend selected, dummy_hrtimer_create() allocates a separate dpcm for
> each runtime. The test configures both with HW_PARAMS and explicitly
> links them with SNDRV_PCM_IOCTL_LINK.
> 
> Sorry, I should have been clearer about what is being released. The
> PCM device remains alive throughout. The object being freed is A's
> runtime->private_data, from A's driver close callback.
> 
> snd_pcm_release_substream() checks A's own ref_count. When A's last
> file reference is closed, it calls A's driver close before unlinking
> A, even while B is still open:
> 
>   Final close of A                  PREPARE ioctl on B
>   ----------------                  ------------------
>   snd_pcm_drop(): group -> SETUP
>   do_hw_free(A)
>   dummy_pcm_close(A)
>     kfree(A's dpcm)
>                                     snd_pcm_action_nonatomic(B)
>                                       Take link read lock
>                                       Walk group: B and A
>                                       snd_pcm_do_prepare(A)
>                                         dummy_pcm_prepare(A)
>                                           Read freed timer_ops
>   pcm_release_private(A)
>     snd_pcm_unlink(A)
>   snd_pcm_detach_substream(A)
> 
> B reaches A through the PCM core's linked-group iteration:
> snd_pcm_action_group() invokes the prepare action for each member.
> A is still linked and in SETUP at that point, which the prepare state
> check accepts. The two dummy runtimes do not share their private data;
> the group action explicitly invokes A's callback with A's substream.
> 
> The final close holds pcm->open_mutex, but the PREPARE path does not
> take that mutex. PREPARE holds snd_pcm_link_rwsem for reading and the
> members' buffer mutexes; driver close does not hold those locks across
> the free. The link write lock is taken at the later unlink.
> 
> That is the overlap I meant: A's own backend data can be freed while
> A is still reachable by a PREPARE action issued through B.

OK, it's then about unlinking in addition to the sync-stop.

I believe that we should rearrange the call order of unlink; it should
be rather called at the very beginning and unconditionally.

So the revised patch is something like below.

(BTW, this will cover also a case where the link may remain when
linked to an OSS substream.)


Takashi

-- 8< --
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2792,19 +2792,17 @@ static int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream)
 	return 0;
 }
 
-static void pcm_release_private(struct snd_pcm_substream *substream)
-{
-	if (snd_pcm_stream_linked(substream))
-		snd_pcm_unlink(substream);
-}
-
 void snd_pcm_release_substream(struct snd_pcm_substream *substream)
 {
 	substream->ref_count--;
 	if (substream->ref_count > 0)
 		return;
 
+	snd_pcm_unlink(substream);
+
 	snd_pcm_drop(substream);
+	snd_pcm_sync_stop(substream, true);
+
 	if (substream->hw_opened) {
 		if (substream->runtime->state != SNDRV_PCM_STATE_OPEN)
 			do_hw_free(substream);
@@ -2887,8 +2885,6 @@ static int snd_pcm_open_file(struct file *file,
 		return -ENOMEM;
 	}
 	pcm_file->substream = substream;
-	if (substream->ref_count == 1)
-		substream->pcm_release = pcm_release_private;
 	file->private_data = pcm_file;
 
 	return 0;