Re: [RFC PATCH 2/2] ALSA: pcm: refresh capture pointers before drain

[Takashi Iwai]

On Wed, 14 Oct 2026 17:20:55 +0200,
Joonho Lee wrote:
> 
> Hi Takashi,
> 
> Thanks for the suggestion and thoughts. I tried the smaller change and hit a
> recursive locking issue with linked capture streams.
> 
> I used a test driver with two linked capture substreams and made the
> later member's pointer callback return SNDRV_PCM_POS_XRUN during
> drain. This led to the function calls as follows! :
> 
>   snd_pcm_do_drain_init()
>     snd_pcm_update_hw_ptr()
>       snd_pcm_update_hw_ptr0()
>         __snd_pcm_xrun()
>           snd_pcm_stop()
>             snd_pcm_action()
>               snd_pcm_stream_group_ref()
> 
> The outer drain action already holds the group locks here. Lockdep
> reported the recursive acquisition of group->lock for atomic streams and
> group->mutex for nonatomic streams. Both tests remained stuck until
> the 45-second timeout. Removing the link let both tests complete.

OK, so my previous assumption was wrong, obviously...

> I went back to the notify=false approach from your first sketch, which is keeping the pointer update inside snd_pcm_do_drain_init(). I also
> added the guard to the early SNDRV_PCM_POS_XRUN branch, since that
> path still called __snd_pcm_xrun(). Drain init then handles the stop,
> while ordinary pointer updates keep their existing behavior.
> 
> With this change, all 24 linked test cases passed across both locking
> modes, including pointer errors, tail reads and reopen/recovery.
> KASAN and lockdep stayed quiet. This was tested with a synthetic
> driver under ARM64 QEMU; physical-device testing is still pending.
> 
> One thing I wanted to check before sending a revision is, how should
> drain report a pointer error?
> 
> The current change puts the failing member in SETUP and allows a
> healthy linked member to keep its tail in DRAINING. A blocking
> capture-only drain returns 0 if the rest of the action succeeds,
> and reading the failed member afterwards returns -EBADFD.
> 
> Would you prefer that behavior, or should we remember the error,
> finish stopping all members, and return -EPIPE from drain? My v2 proposal
> returned -EPIPE and moved the linked group to XRUN, so this changes
> what userspace sees.

Honestly speaking, I'm no longer sure whether my suggestion is better
for now.  It's certainly clearer if we can call the update_hwptr stuff
inside drain_init call, but this looks like a hard way, and the
conditional call at each PCM operation sounds fragile, too.

Maybe we can go back to your way as v2, but slightly differently.
Namely, factor out the snd_pcm_link_rwsem and update hwptr loop into a
helper function, and just call it in snd_pcm_drain().  The already
existing calls of snd_pcm_pause() and snd_pcm_action() with
snd_pcm_action_drain_init can be kept as-is; they need no protection
of snd_pcm_link_rwsem.

Also, I think it's fine to call snd_pcm_update_hw_ptr() no matter
whether it's playback or capture stream.


thanks,

Takashi