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

[Cen Zhang]

Hi Takashi,

Takashi Iwai <tiwai@xxxxxxx> 于2026年10月14日周三 18:07写道：
>
> 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
>

This approach looks right and addresses this case. Thank you for your
patience and help.

>
> -- 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;

I'll rework the patch based on this approach.

Best regards,
Cen Zhang