* [PATCH] ALSA: pcm: update state after xrun @ 2026-10-27 13:12 Mikael Magnusson 2026-10-27 13:23 ` sashiko-bot 2026-10-27 14:50 ` Takashi Iwai 0 siblings, 2 replies; 4+ messages in thread From: Mikael Magnusson @ 2026-10-27 13:12 UTC (permalink / raw) To: linux-sound; +Cc: Takashi Iwai If the snd_pcm_update_hw_ptr call from snd_pcm_status is what triggers the xrun state (eg, as opposed to an IRQ), the returned status->state was stale and told userspace the device was still playing, so they could get confused about device state and hang indefinitely. Closes: https://bugzilla.kernel.org/show_bug.cgi?id=222201 Signed-off-by: Mikael Magnusson <mikachu@gmail.com> --- sound/core/pcm_native.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 35152fae74..62a2bea043 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1132,6 +1132,9 @@ int snd_pcm_status64(struct snd_pcm_substream *substream, status->trigger_tstamp_nsec = runtime->trigger_tstamp.tv_nsec; if (snd_pcm_running(substream)) { snd_pcm_update_hw_ptr(substream); + /* update_hw_ptr may have put us in xrun state, update state */ + status->state = runtime->state; + status->suspended_state = runtime->suspended_state; if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) { status->tstamp_sec = runtime->status->tstamp.tv_sec; status->tstamp_nsec = base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e -- 2.38.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: pcm: update state after xrun 2026-10-27 13:12 [PATCH] ALSA: pcm: update state after xrun Mikael Magnusson @ 2026-10-27 13:23 ` sashiko-bot 2026-10-27 14:50 ` Takashi Iwai 1 sibling, 0 replies; 4+ messages in thread From: sashiko-bot @ 2026-10-27 13:23 UTC (permalink / raw) To: Mikael Magnusson, linux-sound Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] ALSA: pcm: stale trigger_tstamp in snd_pcm_status64() after xrun -- commit 0ae9f8a317c199a098237f1b0f0b5ebbc84be3f4 Author: Mikael Magnusson <mikachu@gmail.com> ALSA: pcm: update state after xrun This commit updates status->state and status->suspended_state in snd_pcm_status64() after calling snd_pcm_update_hw_ptr(), so userspace receives the updated state if snd_pcm_update_hw_ptr() triggers an xrun. > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index 35152fae74ca2..62a2bea043e38 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -1132,6 +1132,9 @@ int snd_pcm_status64(struct snd_pcm_substream *substream, > status->trigger_tstamp_nsec = runtime->trigger_tstamp.tv_nsec; > if (snd_pcm_running(substream)) { > snd_pcm_update_hw_ptr(substream); > + /* update_hw_ptr may have put us in xrun state, update state */ > + status->state = runtime->state; > + status->suspended_state = runtime->suspended_state; [Severity: Medium] Should status->trigger_tstamp_sec and status->trigger_tstamp_nsec also be updated after snd_pcm_update_hw_ptr()? In sound/core/pcm_native.c, snd_pcm_status64() copies runtime->trigger_tstamp into status->trigger_tstamp_sec and status->trigger_tstamp_nsec before calling snd_pcm_update_hw_ptr(substream): sound/core/pcm_native.c:snd_pcm_status64() status->trigger_tstamp_sec = runtime->trigger_tstamp.tv_sec; status->trigger_tstamp_nsec = runtime->trigger_tstamp.tv_nsec; if (snd_pcm_running(substream)) { snd_pcm_update_hw_ptr(substream); When snd_pcm_update_hw_ptr() stops a running stream due to an xrun via __snd_pcm_xrun() -> snd_pcm_stop() or drain completion via snd_pcm_drain_done(), snd_pcm_post_stop() updates runtime->trigger_tstamp to the stop timestamp alongside runtime->state: sound/core/pcm_native.c:snd_pcm_post_stop() if (runtime->state != state) { snd_pcm_trigger_tstamp(substream); __snd_pcm_set_state(runtime, state); snd_pcm_timer_notify(substream, SNDRV_TIMER_EVENT_MSTOP); } Refreshing only status->state and status->suspended_state leaves status->trigger_tstamp_sec and status->trigger_tstamp_nsec holding the old start timestamp while reporting the new stopped or xrun state to userspace. -- Sashiko AI review · https://sashiko.dev/#/patchset/20261027131233.17692-1-mikachu@gmail.com?part=1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: pcm: update state after xrun 2026-10-27 13:12 [PATCH] ALSA: pcm: update state after xrun Mikael Magnusson 2026-10-27 13:23 ` sashiko-bot @ 2026-10-27 14:50 ` Takashi Iwai 2026-10-27 17:18 ` Mikael Magnusson 1 sibling, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2026-10-27 14:50 UTC (permalink / raw) To: Mikael Magnusson; +Cc: linux-sound, Takashi Iwai On Tue, 27 Oct 2026 14:12:33 +0100, Mikael Magnusson wrote: > > If the snd_pcm_update_hw_ptr call from snd_pcm_status is what triggers > the xrun state (eg, as opposed to an IRQ), the returned status->state > was stale and told userspace the device was still playing, so they could > get confused about device state and hang indefinitely. > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=222201 > Signed-off-by: Mikael Magnusson <mikachu@gmail.com> > --- > sound/core/pcm_native.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index 35152fae74..62a2bea043 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -1132,6 +1132,9 @@ int snd_pcm_status64(struct snd_pcm_substream *substream, > status->trigger_tstamp_nsec = runtime->trigger_tstamp.tv_nsec; > if (snd_pcm_running(substream)) { > snd_pcm_update_hw_ptr(substream); > + /* update_hw_ptr may have put us in xrun state, update state */ > + status->state = runtime->state; > + status->suspended_state = runtime->suspended_state; > if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) { > status->tstamp_sec = runtime->status->tstamp.tv_sec; > status->tstamp_nsec = > Thanks for the patch. I can take this as-is, but what Sashiko suggested in the review is also valid -- we may copy trigge_tstamp_*, too. Also, we don't have to always overwrite the values. As a further optimization, check the return value from snd_pcm_update_hw_ptr(), and update only if it returns a negative (-EPIPE). So, in the end, it'll be something like below. Could you check whether this works for you? thanks, Takashi -- 8< -- --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1131,7 +1131,13 @@ int snd_pcm_status64(struct snd_pcm_substream *substream, status->trigger_tstamp_sec = runtime->trigger_tstamp.tv_sec; status->trigger_tstamp_nsec = runtime->trigger_tstamp.tv_nsec; if (snd_pcm_running(substream)) { - snd_pcm_update_hw_ptr(substream); + if (snd_pcm_update_hw_ptr(substream) < 0) { + /* update_hw_ptr may have put us in xrun state, update state */ + status->state = runtime->state; + status->suspended_state = runtime->suspended_state; + status->trigger_tstamp_sec = runtime->trigger_tstamp.tv_sec; + status->trigger_tstamp_nsec = runtime->trigger_tstamp.tv_nsec; + } if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) { status->tstamp_sec = runtime->status->tstamp.tv_sec; status->tstamp_nsec = ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: pcm: update state after xrun 2026-10-27 14:50 ` Takashi Iwai @ 2026-10-27 17:18 ` Mikael Magnusson 0 siblings, 0 replies; 4+ messages in thread From: Mikael Magnusson @ 2026-10-27 17:18 UTC (permalink / raw) To: Takashi Iwai; +Cc: linux-sound On Tue, Oct 27, 2026 at 3:50 PM Takashi Iwai <tiwai@suse.de> wrote: > > On Tue, 27 Oct 2026 14:12:33 +0100, > Mikael Magnusson wrote: > > > > If the snd_pcm_update_hw_ptr call from snd_pcm_status is what triggers > > the xrun state (eg, as opposed to an IRQ), the returned status->state > > was stale and told userspace the device was still playing, so they could > > get confused about device state and hang indefinitely. > > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=222201 > > Signed-off-by: Mikael Magnusson <mikachu@gmail.com> > > --- > > sound/core/pcm_native.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > > index 35152fae74..62a2bea043 100644 > > --- a/sound/core/pcm_native.c > > +++ b/sound/core/pcm_native.c > > @@ -1132,6 +1132,9 @@ int snd_pcm_status64(struct snd_pcm_substream *substream, > > status->trigger_tstamp_nsec = runtime->trigger_tstamp.tv_nsec; > > if (snd_pcm_running(substream)) { > > snd_pcm_update_hw_ptr(substream); > > + /* update_hw_ptr may have put us in xrun state, update state */ > > + status->state = runtime->state; > > + status->suspended_state = runtime->suspended_state; > > if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) { > > status->tstamp_sec = runtime->status->tstamp.tv_sec; > > status->tstamp_nsec = > > > > Thanks for the patch. I can take this as-is, but what Sashiko > suggested in the review is also valid -- we may copy trigger_tstamp_*, > too. > > Also, we don't have to always overwrite the values. As a further > optimization, check the return value from snd_pcm_update_hw_ptr(), and > update only if it returns a negative (-EPIPE). > > So, in the end, it'll be something like below. > Could you check whether this works for you? Hi, this should be fine as well, from what I can see you are correct that it always returns -EPIPE when an xrun happens. I'm not sure if this actually ends up being an optimization (branch vs unconditional write), but it probably also doesn't really matter on this path. You can do whichever looks clearer, either is fine with me. Thanks! > -- 8< -- > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -1131,7 +1131,13 @@ int snd_pcm_status64(struct snd_pcm_substream *substream, > status->trigger_tstamp_sec = runtime->trigger_tstamp.tv_sec; > status->trigger_tstamp_nsec = runtime->trigger_tstamp.tv_nsec; > if (snd_pcm_running(substream)) { > - snd_pcm_update_hw_ptr(substream); > + if (snd_pcm_update_hw_ptr(substream) < 0) { > + /* update_hw_ptr may have put us in xrun state, update state */ > + status->state = runtime->state; > + status->suspended_state = runtime->suspended_state; > + status->trigger_tstamp_sec = runtime->trigger_tstamp.tv_sec; > + status->trigger_tstamp_nsec = runtime->trigger_tstamp.tv_nsec; > + } > if (runtime->tstamp_mode == SNDRV_PCM_TSTAMP_ENABLE) { > status->tstamp_sec = runtime->status->tstamp.tv_sec; > status->tstamp_nsec = > -- Mikael Magnusson ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-10-27 17:18 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-10-27 13:12 [PATCH] ALSA: pcm: update state after xrun Mikael Magnusson 2026-10-27 13:23 ` sashiko-bot 2026-10-27 14:50 ` Takashi Iwai 2026-10-27 17:18 ` Mikael Magnusson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.