Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] ALSA: pcm: unbalanced STOP trigger from PAUSED in snd_pcm_do_stop() --
commit 1209424a81e0273da5db98a8f56a0fd0d7015011 Author: Joonho Lee <[email protected]> ALSA: pcm: stop paused streams before synchronizing teardown This commit updates snd_pcm_do_stop() to issue the SNDRV_PCM_TRIGGER_STOP trigger and set stop_operating when a substream is in SNDRV_PCM_STATE_PAUSED, in addition to when it is running. > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index a2ea44c3c1d29..2b18c5f5801f9 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -1607,7 +1607,8 @@ static int snd_pcm_do_stop(struct snd_pcm_substream > *substream, > snd_pcm_state_t state) > { > if (substream->runtime->trigger_master == substream && > - snd_pcm_running(substream)) { > + (snd_pcm_running(substream) || > + substream->runtime->state == SNDRV_PCM_STATE_PAUSED)) { > substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP); > substream->runtime->stop_operating = true; > } [Severity: High] Can calling substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP) directly while in SNDRV_PCM_STATE_PAUSED cause regressions in drivers that expect start-class triggers (START, RESUME, PAUSE_RELEASE) and stop-class triggers (STOP, SUSPEND, PAUSE_PUSH) to be strictly paired? When a substream enters SNDRV_PCM_STATE_PAUSED via snd_pcm_do_pause(), the driver has already received SNDRV_PCM_TRIGGER_PAUSE_PUSH. Reaching snd_pcm_do_stop() while runtime->state is still SNDRV_PCM_STATE_PAUSED delivers SNDRV_PCM_TRIGGER_STOP without an intervening SNDRV_PCM_TRIGGER_PAUSE_RELEASE. This path can be reached from userspace when: 1. __snd_pcm_xrun() -> snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN) runs on a paused stream without calling snd_pcm_pause(substream, false), such as when ops->ack() returns -EPIPE in pcm_lib_apply_appl_ptr() during write, read, rewind, forward, or SNDRV_PCM_IOCTL_SYNC_PTR, when avail reaches runtime->stop_threshold in snd_pcm_update_state(), or via a direct driver snd_pcm_stop() call. 2. snd_pcm_pause(substream, false) in snd_pcm_drop() or snd_pcm_prepare() fails (its return value is ignored), for example if the driver's SNDRV_PCM_TRIGGER_PAUSE_RELEASE trigger returns an error or if a linked substream is not in SNDRV_PCM_STATE_PAUSED so snd_pcm_pre_pause() returns -EBADFD. 3. snd_pcm_drop(), snd_pcm_prepare(), or snd_pcm_stop() is called on a non-paused substream that is linked to a paused substream. Several in-tree drivers appear to rely on never receiving SNDRV_PCM_TRIGGER_STOP after SNDRV_PCM_TRIGGER_PAUSE_PUSH without an intervening release: 1. Clock enable_count underflow in sound/soc/qcom/lpass-cpu.c (lpass_cpu_daiops_trigger()) and sound/soc/spear/spdif_in.c (spdif_in_trigger()): sound/soc/qcom/lpass-cpu.c:lpass_cpu_daiops_trigger() { ... case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: ... clk_disable(drvdata->mi2s_bit_clk[dai->driver->id]); break; } Receiving STOP while PAUSED calls clk_disable() a second time for a single clk_enable(). 2. Runtime PM usage count underflow in sound/soc/samsung/i2s.c (i2s_trigger()): sound/soc/samsung/i2s.c:i2s_trigger() { ... case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: ... pm_runtime_put(dai->dev); break; } Receiving STOP after PAUSE_PUSH calls pm_runtime_put(dai->dev) twice. 3. Reference and active counter underflows or premature shared hardware shutdown in sound/soc/stm/stm32_i2s.c (stm32_i2s_trigger() decrementing i2s->refcount), sound/soc/rockchip/rockchip_i2s_tdm.c (rockchip_snd_txrxctrl() decrementing i2s_tdm->refcount), sound/soc/atmel/atmel-i2s.c (atmel_i2s_trigger() decrementing dev->clk_use_no), sound/soc/dwc/dwc-i2s.c (dw_i2s_trigger() decrementing dev->active), sound/soc/ti/omap-mcbsp.c (omap_mcbsp_dai_trigger() decrementing mcbsp->active), sound/soc/pxa/mmp-sspa.c (mmp_sspa_trigger()), sound/soc/bcm/cygnus-ssp.c (cygnus_ssp_trigger()), and sound/soc/fsl/imx-pcm-fiq.c (snd_imx_pcm_trigger() calling disable_fiq(imx_pcm_fiq) twice): sound/soc/stm/stm32_i2s.c:stm32_i2s_trigger() { ... case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: ... scoped_guard(spinlock, &i2s->lock_fd) { i2s->refcount--; } 4. SOF IPC4 pipeline state leak in sound/soc/sof/ipc4-pcm.c: sound/soc/sof/ipc4-pcm.c:sof_ipc4_add_pipeline_to_trigger_list() { ... case SOF_IPC4_PIPE_RESET: /* RESET if the pipeline is neither running nor paused */ if (!spipe->started_count && !spipe->paused_count) sof_ipc4_add_pipeline_by_priority(trigger_list, pipe_widget, pipe_priority, true); break; case SOF_IPC4_PIPE_PAUSED: case SOF_IPC4_PIPE_EOS: if (spipe->paused_count == (spipe->started_count - 1)) sof_ipc4_add_pipeline_by_priority(trigger_list, pipe_widget, pipe_priority, true); break; } When START is followed by PAUSE_PUSH, spipe->started_count is 1 and spipe->paused_count is 1. If SNDRV_PCM_TRIGGER_STOP is then issued while PAUSED, sof_ipc4_pcm_trigger() requests SOF_IPC4_PIPE_PAUSED, the check spipe->paused_count == (spipe->started_count - 1) evaluates to false, and neither started_count nor paused_count is decremented (and sof_ipc4_update_pipeline_state() only decrements paused_count on SNDRV_PCM_TRIGGER_PAUSE_RELEASE). Subsequent sof_ipc4_pcm_hw_free() then skips SOF_IPC4_PIPE_RESET because started_count and paused_count remain non-zero. 5. Stuck hardware pause bits in sound/pci/cmipci.c (snd_cmipci_pcm_trigger()), sound/pci/oxygen/oxygen_pcm.c (oxygen_trigger()), sound/pci/als300.c (snd_als300_trigger()), sound/pci/ens1370.c (snd_ensoniq_trigger()), and sound/pci/ice1712/ice1712.c (snd_ice1712_pcm_trigger()): sound/pci/cmipci.c:snd_cmipci_pcm_trigger() { ... case SNDRV_PCM_TRIGGER_PAUSE_PUSH: case SNDRV_PCM_TRIGGER_SUSPEND: cm->ctrl |= pause; snd_cmipci_write(cm, CM_REG_FUNCTRL0, cm->ctrl); break; case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: case SNDRV_PCM_TRIGGER_RESUME: cm->ctrl &= ~pause; snd_cmipci_write(cm, CM_REG_FUNCTRL0, cm->ctrl); break; } In these drivers, the pause bit is only cleared on PAUSE_RELEASE or RESUME, not by SNDRV_PCM_TRIGGER_STOP or prepare(). 6. Duplicate VIRTIO_SND_R_PCM_STOP in sound/virtio/virtio_pcm_ops.c (virtsnd_pcm_trigger()): sound/virtio/virtio_pcm_ops.c:virtsnd_pcm_trigger() { ... case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: scoped_guard(spinlock_irqsave, &vss->lock) { if (command != SNDRV_PCM_TRIGGER_PAUSE_PUSH) vss->stopped = true; vss->xfer_enabled = false; } msg = virtsnd_pcm_ctl_msg_alloc(vss, VIRTIO_SND_R_PCM_STOP, GFP_KERNEL); if (!msg) return -ENOMEM; return virtsnd_ctl_msg_send_sync(snd, msg); } Because SNDRV_PCM_TRIGGER_PAUSE_PUSH already sends VIRTIO_SND_R_PCM_STOP to the device, triggering SNDRV_PCM_TRIGGER_STOP from SNDRV_PCM_STATE_PAUSED sends a second VIRTIO_SND_R_PCM_STOP while the device stream is already stopped. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
