Re: [PATCH] ASoC: imx-pcm-rpmsg: Flush pending work in hw_free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: imx-pcm-rpmsg: unstopped timer in imx_rpmsg_pcm_hw_free() races with hw_free and close
- [Medium] ASoC: imx-pcm-rpmsg: missing .sync_stop races TERMINATE work with hw_params
--

commit 9ebe6dce779f3a2f344611c3f847f58528684b1b
Author: Chancel Liu <chancel.liu@nxp.com>

ASoC: imx-pcm-rpmsg: Flush pending work in hw_free

This commit moves the flush_workqueue() call from imx_rpmsg_pcm_close() into
a new imx_rpmsg_pcm_hw_free() component callback so that queued TERMINATE
work completes before the DAI hw_free() gates the shared audio clock.

> diff --git a/sound/soc/fsl/imx-pcm-rpmsg.c b/sound/soc/fsl/imx-pcm-rpmsg.c
> index 85b7c929a28a4..6a0174d6c78df 100644
> --- a/sound/soc/fsl/imx-pcm-rpmsg.c
> +++ b/sound/soc/fsl/imx-pcm-rpmsg.c
> @@ -178,6 +178,16 @@ static int imx_rpmsg_pcm_hw_params(struct snd_soc_component *component,
>  	return 0;
>  }
>  
> +static int imx_rpmsg_pcm_hw_free(struct snd_soc_component *component,
> +				 struct snd_pcm_substream *substream)
> +{
> +	struct rpmsg_info *info = dev_get_drvdata(component->dev);
> +
> +	flush_workqueue(info->rpmsg_wq);
> +
> +	return 0;
> +}
> +
>  static snd_pcm_uframes_t imx_rpmsg_pcm_pointer(struct snd_soc_component *component,
>  					       struct snd_pcm_substream *substream)
>  {
> @@ -285,9 +295,6 @@ static int imx_rpmsg_pcm_close(struct snd_soc_component *component,
>  	struct rpmsg_info *info = dev_get_drvdata(component->dev);
>  	struct rpmsg_msg *msg;
>  
> -	/* Flush work in workqueue to make TX_CLOSE is the last message */
> -	flush_workqueue(info->rpmsg_wq);
> -
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>  		msg = &info->msg[TX_CLOSE];
>  		msg->s_msg.header.cmd = TX_CLOSE;

[Severity: High]
Can info->stream_timer[substream->stream].timer still fire and queue work to
info->rpmsg_wq after imx_rpmsg_pcm_hw_free() flushes the workqueue?

On an LPA-enabled stream where rpmsg->force_lpa is set in
imx_rpmsg_pcm_prepare(), advancing appl_ptr while in SNDRV_PCM_STATE_PREPARED
(for example via write before start_threshold is reached, snd_pcm_forward(),
or snd_pcm_sync_ptr()) invokes pcm_lib_apply_appl_ptr() -> imx_rpmsg_pcm_ack(),
which arms the stream timer:

sound/soc/fsl/imx-pcm-rpmsg.c:imx_rpmsg_pcm_ack() {
	...
		if ((avail - written_num * period_size) <= period_size) {
			imx_rpmsg_insert_workqueue(substream, msg, info);
		} else if (rpmsg->force_lpa && !timer_pending(timer)) {
			int time_msec;

			time_msec = (int)(runtime->period_size * 1000 / runtime->rate);
			mod_timer(timer, jiffies + msecs_to_jiffies(time_msec));
		}
	...
}

If the substream is then freed via SNDRV_PCM_IOCTL_HW_FREE or closed via
snd_pcm_release_substream() without having entered SNDRV_PCM_STATE_RUNNING
(or if closed from SNDRV_PCM_STATE_SUSPENDED), snd_pcm_do_stop() skips the
SNDRV_PCM_TRIGGER_STOP trigger because snd_pcm_running(substream) is false:

sound/core/pcm_native.c:snd_pcm_do_stop() {
	if (substream->runtime->trigger_master == substream &&
	    snd_pcm_running(substream)) {
		substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
		substream->runtime->stop_operating = true;
	}
	return 0;
}

In that case, imx_rpmsg_terminate_all() is never called, so stream_timer
remains armed across imx_rpmsg_pcm_hw_free(). Even when the stream was
running, imx_rpmsg_terminate_all() runs in atomic trigger context and only
calls non-synchronous timer_delete(), which does not wait if
imx_rpmsg_timer_callback() is already running on another CPU and has not yet
called imx_rpmsg_insert_workqueue(). In addition, SNDRV_PCM_IOCTL_SYNC_PTR
can still invoke imx_rpmsg_pcm_ack() after hw_free while rpmsg->force_lpa
remains set.

Because flush_workqueue() was removed from imx_rpmsg_pcm_close() and
timer_delete() is only called after info->send_message(msg, info) in
imx_rpmsg_pcm_close(), any TX_PERIOD_DONE or RX_PERIOD_DONE work queued after
imx_rpmsg_pcm_hw_free() can run concurrently with fsl_rpmsg_hw_free() gating
rpmsg->mclk and can also be sent after TX_CLOSE or RX_CLOSE.

Should imx_rpmsg_pcm_hw_free() synchronously stop the timer with
timer_delete_sync() and clear rpmsg->force_lpa before calling
flush_workqueue(info->rpmsg_wq)?

> @@ -624,6 +631,7 @@ static const struct snd_soc_component_driver imx_rpmsg_soc_component = {
>  	.open		= imx_rpmsg_pcm_open,
>  	.close		= imx_rpmsg_pcm_close,
>  	.hw_params	= imx_rpmsg_pcm_hw_params,
> +	.hw_free	= imx_rpmsg_pcm_hw_free,
>  	.trigger	= imx_rpmsg_pcm_trigger,
>  	.pointer	= imx_rpmsg_pcm_pointer,
>  	.ack		= imx_rpmsg_pcm_ack,

[Severity: Medium]
What happens if userspace stops a running stream via SNDRV_PCM_IOCTL_DROP
(or drain) and then calls SNDRV_PCM_IOCTL_HW_PARAMS or
SNDRV_PCM_IOCTL_PREPARE without calling hw_free() in between?

When snd_pcm_drop() stops a running stream, imx_rpmsg_pcm_trigger() calls
imx_rpmsg_terminate_all(), which queues TX_TERMINATE or RX_TERMINATE to
info->rpmsg_wq and leaves the substream in SNDRV_PCM_STATE_SETUP. When
snd_pcm_hw_params() or snd_pcm_prepare() is called from SNDRV_PCM_STATE_SETUP,
ALSA does not call .hw_free; instead it relies on snd_pcm_sync_stop() to wait
for asynchronous stop operations:

sound/core/pcm_native.c:snd_pcm_hw_params() {
	...
	snd_pcm_sync_stop(substream, true);
	...
	if (substream->ops->hw_params != NULL) {
		err = substream->ops->hw_params(substream, params);
		if (err < 0)
			goto _error;
	}
	...
}

Because imx_rpmsg_soc_component hooks .hw_free rather than .sync_stop:

1. In snd_pcm_hw_params() -> __soc_pcm_hw_params(), fsl_rpmsg_hw_params()
re-parents the shared audio clock via clk_set_parent() while TX_TERMINATE or
RX_TERMINATE work may still be pending or executing in info->rpmsg_wq, and
imx_rpmsg_pcm_hw_params() synchronously calls info->send_message() for
TX_HW_PARAM or RX_HW_PARAM, which can overtake the queued TERMINATE message.

2. During system suspend, snd_pcm_suspend_all() triggers
SNDRV_PCM_TRIGGER_SUSPEND (queueing PAUSE or TERMINATE work to info->rpmsg_wq)
and calls snd_pcm_sync_stop(substream, false) without calling .hw_free,
allowing imx_rpmsg_pcm_suspend() to synchronously send TX_SUSPEND or
RX_SUSPEND before the workqueue is flushed.

3. In do_hw_free(), snd_pcm_sync_stop() runs before soc_pcm_hw_clean(),
whereas component .hw_free runs only after snd_soc_dai_digital_mute() and
snd_soc_dapm_stream_stop().

Would hooking .sync_stop in imx_rpmsg_soc_component (or in addition to
.hw_free) ensure the workqueue is flushed across all stop transitions before
clock reconfiguration, suspend, or hw_free cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-b4-imx-pcm-rpmsg-concurrent-v1-1-c1fb8c33f4db@nxp.com?part=1