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

[Takashi Iwai <tiwai@suse.de>]

On Wed, 14 Oct 2026 11:54:37 +0200,
Takashi Iwai wrote:
> 
> 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.

... and after re-reading the code, I noticed that the
snd_pcm_sync_stop() call is redundant.  After snd_pcm_drop(), the code
continues as:

	if (substream->hw_opened) {
		if (substream->runtime->state != SNDRV_PCM_STATE_OPEN)
			do_hw_free(substream);
		substream->ops->close(substream);
		substream->hw_opened = 0;
	}

and do_hw_free() does call snd_pcm_sync_stop()

static int do_hw_free(struct snd_pcm_substream *substream)
{
	int result = 0;

	snd_pcm_sync_stop(substream, true);
	if (substream->ops->hw_free)
	.....

That said, the only missing piece is the rearrangement of unlinking
call order.  The hrtimer was a red herring, after all, and rather the
access to a PCM substream that is being released via the PCM link was
the real problem.

Please verify whether the fix below works for your case.


thanks,

Takashi


-- 8< --
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index a845f242a163..4fea3f6df682 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2792,18 +2792,14 @@ 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);
 	if (substream->hw_opened) {
 		if (substream->runtime->state != SNDRV_PCM_STATE_OPEN)
@@ -2887,8 +2883,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;