Re: [PATCH v8 08/14] ALSA: pcm: stop paused streams before synchronizing teardown

[sashiko-bot@kernel.org]

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 <mynameisjune111@gmail.com>

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/20261029054354.2931-1-mynameisjune111@gmail.com?part=8