Re: [PATCH v24 02/16] ALSA: pcm: report partial progress under the stream lock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] ALSA: pcm: premature drain done on capture in
  snd_pcm_update_state()
- [Medium] ALSA: pcm: stale avail in __snd_pcm_lib_xfer() drops capture
  drain tail
--

commit 729bfce97b1180fb1f0b36f0f5fb43c69406d7d3
Author: Joonho Lee <mynameisjune111@gmail.com>

ALSA: pcm: report partial progress under the stream lock

This commit introduces snd_pcm_progress_under_stream_lock() so drivers can
publish partial-period progress and final capture drain tails under the PCM
stream lock without ticking the PCM period timer.

> diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
> index bb16738bb5919..a5665bb2b0705 100644
> --- a/sound/core/pcm_lib.c
> +++ b/sound/core/pcm_lib.c
> @@ -1918,6 +1918,30 @@ void snd_pcm_period_elapsed_under_stream_lock(struct snd_pcm_substream *substrea
>  }
>  EXPORT_SYMBOL(snd_pcm_period_elapsed_under_stream_lock);
>  
> +/**
> + * snd_pcm_progress_under_stream_lock - Report progress without a period IRQ
> + * @substream: PCM substream
> + *
> + * Context: PCM stream lock held, with no driver pointer or trigger locks held.
> + * Updates the pointer and wakes readers for partial-period transfers. Capture
> + * streams may report their final tail while DRAINING. Unlike a period
> + * notification, this does not imply an elapsed period or tick the PCM timer.
> + * The same pointer, trigger and timestamp callbacks may be invoked.
> + */
> +void snd_pcm_progress_under_stream_lock(struct snd_pcm_substream *substream)
> +{
> +	if (PCM_RUNTIME_CHECK(substream))
> +		return;
> +	if (!snd_pcm_running(substream) &&
> +	    !(substream->stream == SNDRV_PCM_STREAM_CAPTURE &&
> +	      substream->runtime->state == SNDRV_PCM_STATE_DRAINING))
> +		return;
> +	snd_pcm_update_hw_ptr(substream);

[Severity: High]
Does calling snd_pcm_update_hw_ptr() on a capture substream in
SNDRV_PCM_STATE_DRAINING cause snd_pcm_update_state() to prematurely finish
draining when the capture buffer fills up?

Previously, snd_pcm_update_hw_ptr() was only called when snd_pcm_running()
was true, which includes SNDRV_PCM_STATE_DRAINING only for playback.  In
snd_pcm_update_state(), the draining check assumes playback semantics where
avail >= runtime->buffer_size means the playback buffer has emptied:

sound/core/pcm_lib.c:snd_pcm_update_state() {
	avail = snd_pcm_avail(substream);
	if (avail > runtime->avail_max)
		runtime->avail_max = avail;
	if (runtime->state == SNDRV_PCM_STATE_DRAINING) {
		if (avail >= runtime->buffer_size) {
			snd_pcm_drain_done(substream);
			return -EPIPE;
		}
	} else {
		if (avail >= runtime->stop_threshold) {
			__snd_pcm_xrun(substream);
			return -EPIPE;
		}
	}

For capture, avail is the number of unread frames in the buffer.  If a
capture stream in SNDRV_PCM_STATE_DRAINING reaches a full buffer (avail >=
runtime->buffer_size) when snd_pcm_progress_under_stream_lock() updates
hw_ptr, snd_pcm_update_state() calls snd_pcm_drain_done() and moves the
stream to SNDRV_PCM_STATE_SETUP, causing subsequent reads to fail with
-EBADFD and losing the buffered capture audio (as well as bypassing the
stop_threshold overrun check).

[Severity: Medium]
Can a capture tail published via snd_pcm_progress_under_stream_lock() be
discarded if it arrives while __snd_pcm_lib_xfer() is copying frames to
userspace?

CPU0                                    CPU1
__snd_pcm_lib_xfer()
  snd_pcm_stream_lock_irq()
  avail = snd_pcm_avail(substream)
  /* size > avail */
  frames = avail
  snd_pcm_stream_unlock_irq()
  writer(...)
                                        snd_pcm_stream_lock_irq()
                                        snd_pcm_progress_under_stream_lock()
                                          snd_pcm_update_hw_ptr()
                                        snd_pcm_stream_unlock_irq()
  snd_pcm_stream_lock_irq()
  ...
  avail -= frames; /* avail == 0 */
  /* next loop iteration */
  if (!avail)
    snd_pcm_drain_done(substream)  <-- drops newly published tail

Previously, hw_ptr did not advance once a capture stream entered
SNDRV_PCM_STATE_DRAINING, so tracking avail locally across the unlocked
writer() call via avail -= frames was exact.  When
snd_pcm_progress_under_stream_lock() advances hw_ptr during that window,
the cached avail reaches 0 on the next iteration and __snd_pcm_lib_xfer()
calls snd_pcm_drain_done() without re-checking snd_pcm_avail(substream),
transitioning the stream to SNDRV_PCM_STATE_SETUP and dropping the final
tail.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111000543.28550-1-mynameisjune111@gmail.com?part=2