[RFC] ASoC: SOF: sof-pcm/pm: Stop paused streams before the system suspend

[RFC] ASoC: SOF: sof-pcm/pm: Stop paused streams before the system suspend

[Peter Ujfalusi]

Paused streams will not receive a suspend trigger, they will be marked by
ALSA core as suspended and it's state is saved.
Since the pause stream is not in a fully stopped state, for example DMA
might be still enabled (just the trigger source is removed/disabled) we
need to make sure that the hardware is ready to handle the suspend.

This involves a bit more than just stopping a DMA since we also need to
communicate with the firmware in a delicate sequence to follow IP
programming flows.
To make things a bit more challenging, these flows are different between
IPC versions due to the fact that they use different messages to implement
the same functionality.

To avoid adding yet another path, callbacks and sequencing for handling the
corner case of suspending while a stream is paused, and do this for each
IPC versions and platforms, we can move the stream back to running just to
put it to stopped state.

Explanation of the change:
Streams moved to SUSPENDED state from PAUSED without trigger. If a stream
does not support RESUME then on system resume the RESUME trigger is not
sent, the stream's state and suspended_state remains untouched.
When the user space releases the pause then the core will reject this
because the state of the stream is _not_ PAUSED, it is still SUSPENDED.

From this point user space will do the normal (hw_params) prepare and
START, PAUSE_RELEASE trigger will not be sent by the core after the
system has resumed.

Link: https://github.com/thesofproject/linux/issues/5035
Link: https://github.com/thesofproject/linux/issues/5341
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
Hi,

Please see the problem statement and details of the issue in the commit
message.

I'm not sure if this should be done in ALSA+ASoC level instead. My fear
is that this is changing how things has been working since almost
forever and it really puzzles me why it is not affecting other drivers.
It is true that in SOF the PAUSED state is not equal to STOPED while
it might be so for other vendors (it is for TI stuff for sure).

The main point is that when we do a system suspend and a stream is in
PAUSED state, it will not be triggered (PAUSED == SUSPENDED/STOPPED
assumption?). On resume, if the platform is not supporting RESUME then
nothing will be done for the PAUSED stream, but a PAUSE_RELEASE will
fail and all sorts of state machine assumption will break in SOF/ASoC
stack.

I have a PR open for quite long [1] but we would like to find the best
solution for us and possibly for others facing the same issue as well.

[1] https://github.com/thesofproject/linux/pull/5058

Thank you,
Peter
---
 sound/soc/sof/pcm.c      | 76 ++++++++++++++++++++++++++++++++++++++++
 sound/soc/sof/pm.c       | 11 ++++++
 sound/soc/sof/sof-priv.h |  2 ++
 3 files changed, 89 insertions(+)

diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index d584a72e6f52..0b78aa585cd5 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -760,6 +760,82 @@ static snd_pcm_sframes_t sof_pcm_delay(struct snd_soc_component *component,
 	return 0;
 }
 
+static int sof_pcm_trigger_suspended_paused_streams(struct snd_sof_dev *sdev,
+						    int cmd)
+{
+	struct snd_pcm_substream *substream;
+	struct snd_pcm_runtime *runtime;
+	struct snd_sof_pcm *spcm;
+	int dir, ret;
+
+	list_for_each_entry(spcm, &sdev->pcm_list, list) {
+		for_each_pcm_streams(dir) {
+			substream = spcm->stream[dir].substream;
+			if (!substream || !substream->runtime)
+				continue;
+
+			/*
+			 * The stream supports RESUME, it is expected that it
+			 * is handling the corner case of suspending while
+			 * a stream is paused
+			 */
+			runtime = substream->runtime;
+			if (runtime->info & SNDRV_PCM_INFO_RESUME)
+				continue;
+
+			/* Only send the trigger to a paused and suspended stream */
+			if (runtime->state != SNDRV_PCM_STATE_SUSPENDED ||
+			    runtime->suspended_state != SNDRV_PCM_STATE_PAUSED)
+				continue;
+
+			ret = substream->ops->trigger(substream, cmd);
+			if (ret) {
+				spcm_err(spcm, substream->stream,
+					 "trigger %d failed\n", cmd);
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
+int sof_pcm_stop_paused_on_suspend(struct snd_sof_dev *sdev)
+{
+	int ret;
+
+	/*
+	 * Handle the corner case of system suspend while at least one stream is
+	 * paused.
+	 * Paused streams will not receive the SUSPEND triggers, they are
+	 * 'silently' moved to SUSPENDED state.
+	 *
+	 * The workaround for the corner case is applicable for streams not
+	 * supporting RESUME.
+	 *
+	 * First we need to move (trigger) the paused streams to RUNNING state,
+	 * then we need to stop them
+	 *
+	 * Explanation: Streams moved to SUSPENDED state from PAUSED without
+	 * trigger. If a stream does not support RESUME then on system resume
+	 * the RESUME trigger is not sent, the stream's state and suspended_state
+	 * remains untouched. When the user space releases the pause then the
+	 * core will reject this because the state of the stream is _not_ PAUSED,
+	 * it is still SUSPENDED.
+	 * From this point user space will do the normal (hw_params) prepare and
+	 * START, PAUSE_RELEASE trigger will not be sent by the core after the
+	 * system has resumed.
+	 */
+	ret = sof_pcm_trigger_suspended_paused_streams(sdev,
+						       SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+	if (ret)
+		return ret;
+
+	return sof_pcm_trigger_suspended_paused_streams(sdev,
+							SNDRV_PCM_TRIGGER_STOP);
+}
+EXPORT_SYMBOL(sof_pcm_stop_paused_on_suspend);
+
 void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
 {
 	struct snd_soc_component_driver *pd = &sdev->plat_drv;
diff --git a/sound/soc/sof/pm.c b/sound/soc/sof/pm.c
index 8e3bcf602beb..e257d23d9d19 100644
--- a/sound/soc/sof/pm.c
+++ b/sound/soc/sof/pm.c
@@ -210,6 +210,17 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
 	if (runtime_suspend && !sof_ops(sdev)->runtime_suspend)
 		return 0;
 
+	/*
+	 * On system suspend we need special handling of paused streams
+	 * For more details, see the comment section in
+	 * sof_pcm_stop_paused_on_suspend)(
+	 */
+	if (!runtime_suspend) {
+		ret = sof_pcm_stop_paused_on_suspend(sdev);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* we need to tear down pipelines only if the DSP hardware is
 	 * active, which happens for PCI devices. if the device is
 	 * suspended, it is brought back to full power and then
diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
index abbb5ee7e08c..5750d9038647 100644
--- a/sound/soc/sof/sof-priv.h
+++ b/sound/soc/sof/sof-priv.h
@@ -706,6 +706,8 @@ void snd_sof_complete(struct device *dev);
 
 void snd_sof_new_platform_drv(struct snd_sof_dev *sdev);
 
+int sof_pcm_stop_paused_on_suspend(struct snd_sof_dev *sdev);
+
 /*
  * Compress support
  */
-- 
2.49.0

Re: [RFC] ASoC: SOF: sof-pcm/pm: Stop paused streams before the system suspend

[Takashi Iwai]

On Mon, 31 Mar 2025 12:56:31 +0200,
Peter Ujfalusi wrote:
> 
> Paused streams will not receive a suspend trigger, they will be marked by
> ALSA core as suspended and it's state is saved.
> Since the pause stream is not in a fully stopped state, for example DMA
> might be still enabled (just the trigger source is removed/disabled) we
> need to make sure that the hardware is ready to handle the suspend.
> 
> This involves a bit more than just stopping a DMA since we also need to
> communicate with the firmware in a delicate sequence to follow IP
> programming flows.
> To make things a bit more challenging, these flows are different between
> IPC versions due to the fact that they use different messages to implement
> the same functionality.
> 
> To avoid adding yet another path, callbacks and sequencing for handling the
> corner case of suspending while a stream is paused, and do this for each
> IPC versions and platforms, we can move the stream back to running just to
> put it to stopped state.
> 
> Explanation of the change:
> Streams moved to SUSPENDED state from PAUSED without trigger. If a stream
> does not support RESUME then on system resume the RESUME trigger is not
> sent, the stream's state and suspended_state remains untouched.
> When the user space releases the pause then the core will reject this
> because the state of the stream is _not_ PAUSED, it is still SUSPENDED.
> 
> From this point user space will do the normal (hw_params) prepare and
> START, PAUSE_RELEASE trigger will not be sent by the core after the
> system has resumed.
> 
> Link: https://github.com/thesofproject/linux/issues/5035
> Link: https://github.com/thesofproject/linux/issues/5341
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> ---
> Hi,
> 
> Please see the problem statement and details of the issue in the commit
> message.
> 
> I'm not sure if this should be done in ALSA+ASoC level instead. My fear
> is that this is changing how things has been working since almost
> forever and it really puzzles me why it is not affecting other drivers.
> It is true that in SOF the PAUSED state is not equal to STOPED while
> it might be so for other vendors (it is for TI stuff for sure).
> 
> The main point is that when we do a system suspend and a stream is in
> PAUSED state, it will not be triggered (PAUSED == SUSPENDED/STOPPED
> assumption?). On resume, if the platform is not supporting RESUME then
> nothing will be done for the PAUSED stream, but a PAUSE_RELEASE will
> fail and all sorts of state machine assumption will break in SOF/ASoC
> stack.
> 
> I have a PR open for quite long [1] but we would like to find the best
> solution for us and possibly for others facing the same issue as well.
> 
> [1] https://github.com/thesofproject/linux/pull/5058

IMO, this kind of thing should be handled in ALSA core side.
If we want to avoid possible breakage, a flag can be introduced and
perform this conditionally, too.  It'll become a bit complex, but
that's because of the subtle hardware behavior differences,
unfortunately.

AFAIK, dmaengine PCM code assumes that the paused device can be safely
suspended / resumed as is.  There is a special handling to do pause at
snd_dmaengine_pcm_trigger(), for example.

Also, the suspend/resume after pause worked on other legacy devices,
as it seems, too.


thanks,

Takashi

> 
> Thank you,
> Peter
> ---
>  sound/soc/sof/pcm.c      | 76 ++++++++++++++++++++++++++++++++++++++++
>  sound/soc/sof/pm.c       | 11 ++++++
>  sound/soc/sof/sof-priv.h |  2 ++
>  3 files changed, 89 insertions(+)
> 
> diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
> index d584a72e6f52..0b78aa585cd5 100644
> --- a/sound/soc/sof/pcm.c
> +++ b/sound/soc/sof/pcm.c
> @@ -760,6 +760,82 @@ static snd_pcm_sframes_t sof_pcm_delay(struct snd_soc_component *component,
>  	return 0;
>  }
>  
> +static int sof_pcm_trigger_suspended_paused_streams(struct snd_sof_dev *sdev,
> +						    int cmd)
> +{
> +	struct snd_pcm_substream *substream;
> +	struct snd_pcm_runtime *runtime;
> +	struct snd_sof_pcm *spcm;
> +	int dir, ret;
> +
> +	list_for_each_entry(spcm, &sdev->pcm_list, list) {
> +		for_each_pcm_streams(dir) {
> +			substream = spcm->stream[dir].substream;
> +			if (!substream || !substream->runtime)
> +				continue;
> +
> +			/*
> +			 * The stream supports RESUME, it is expected that it
> +			 * is handling the corner case of suspending while
> +			 * a stream is paused
> +			 */
> +			runtime = substream->runtime;
> +			if (runtime->info & SNDRV_PCM_INFO_RESUME)
> +				continue;
> +
> +			/* Only send the trigger to a paused and suspended stream */
> +			if (runtime->state != SNDRV_PCM_STATE_SUSPENDED ||
> +			    runtime->suspended_state != SNDRV_PCM_STATE_PAUSED)
> +				continue;
> +
> +			ret = substream->ops->trigger(substream, cmd);
> +			if (ret) {
> +				spcm_err(spcm, substream->stream,
> +					 "trigger %d failed\n", cmd);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +int sof_pcm_stop_paused_on_suspend(struct snd_sof_dev *sdev)
> +{
> +	int ret;
> +
> +	/*
> +	 * Handle the corner case of system suspend while at least one stream is
> +	 * paused.
> +	 * Paused streams will not receive the SUSPEND triggers, they are
> +	 * 'silently' moved to SUSPENDED state.
> +	 *
> +	 * The workaround for the corner case is applicable for streams not
> +	 * supporting RESUME.
> +	 *
> +	 * First we need to move (trigger) the paused streams to RUNNING state,
> +	 * then we need to stop them
> +	 *
> +	 * Explanation: Streams moved to SUSPENDED state from PAUSED without
> +	 * trigger. If a stream does not support RESUME then on system resume
> +	 * the RESUME trigger is not sent, the stream's state and suspended_state
> +	 * remains untouched. When the user space releases the pause then the
> +	 * core will reject this because the state of the stream is _not_ PAUSED,
> +	 * it is still SUSPENDED.
> +	 * From this point user space will do the normal (hw_params) prepare and
> +	 * START, PAUSE_RELEASE trigger will not be sent by the core after the
> +	 * system has resumed.
> +	 */
> +	ret = sof_pcm_trigger_suspended_paused_streams(sdev,
> +						       SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> +	if (ret)
> +		return ret;
> +
> +	return sof_pcm_trigger_suspended_paused_streams(sdev,
> +							SNDRV_PCM_TRIGGER_STOP);
> +}
> +EXPORT_SYMBOL(sof_pcm_stop_paused_on_suspend);
> +
>  void snd_sof_new_platform_drv(struct snd_sof_dev *sdev)
>  {
>  	struct snd_soc_component_driver *pd = &sdev->plat_drv;
> diff --git a/sound/soc/sof/pm.c b/sound/soc/sof/pm.c
> index 8e3bcf602beb..e257d23d9d19 100644
> --- a/sound/soc/sof/pm.c
> +++ b/sound/soc/sof/pm.c
> @@ -210,6 +210,17 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
>  	if (runtime_suspend && !sof_ops(sdev)->runtime_suspend)
>  		return 0;
>  
> +	/*
> +	 * On system suspend we need special handling of paused streams
> +	 * For more details, see the comment section in
> +	 * sof_pcm_stop_paused_on_suspend)(
> +	 */
> +	if (!runtime_suspend) {
> +		ret = sof_pcm_stop_paused_on_suspend(sdev);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	/* we need to tear down pipelines only if the DSP hardware is
>  	 * active, which happens for PCI devices. if the device is
>  	 * suspended, it is brought back to full power and then
> diff --git a/sound/soc/sof/sof-priv.h b/sound/soc/sof/sof-priv.h
> index abbb5ee7e08c..5750d9038647 100644
> --- a/sound/soc/sof/sof-priv.h
> +++ b/sound/soc/sof/sof-priv.h
> @@ -706,6 +706,8 @@ void snd_sof_complete(struct device *dev);
>  
>  void snd_sof_new_platform_drv(struct snd_sof_dev *sdev);
>  
> +int sof_pcm_stop_paused_on_suspend(struct snd_sof_dev *sdev);
> +
>  /*
>   * Compress support
>   */
> -- 
> 2.49.0
> 

Re: [RFC] ASoC: SOF: sof-pcm/pm: Stop paused streams before the system suspend

[Péter Ujfalusi]

On 31/03/2025 14:09, Takashi Iwai wrote:
> On Mon, 31 Mar 2025 12:56:31 +0200,
> Peter Ujfalusi wrote:
>>
>> Paused streams will not receive a suspend trigger, they will be marked by
>> ALSA core as suspended and it's state is saved.
>> Since the pause stream is not in a fully stopped state, for example DMA
>> might be still enabled (just the trigger source is removed/disabled) we
>> need to make sure that the hardware is ready to handle the suspend.
>>
>> This involves a bit more than just stopping a DMA since we also need to
>> communicate with the firmware in a delicate sequence to follow IP
>> programming flows.
>> To make things a bit more challenging, these flows are different between
>> IPC versions due to the fact that they use different messages to implement
>> the same functionality.
>>
>> To avoid adding yet another path, callbacks and sequencing for handling the
>> corner case of suspending while a stream is paused, and do this for each
>> IPC versions and platforms, we can move the stream back to running just to
>> put it to stopped state.
>>
>> Explanation of the change:
>> Streams moved to SUSPENDED state from PAUSED without trigger. If a stream
>> does not support RESUME then on system resume the RESUME trigger is not
>> sent, the stream's state and suspended_state remains untouched.
>> When the user space releases the pause then the core will reject this
>> because the state of the stream is _not_ PAUSED, it is still SUSPENDED.
>>
>> From this point user space will do the normal (hw_params) prepare and
>> START, PAUSE_RELEASE trigger will not be sent by the core after the
>> system has resumed.
>>
>> Link: https://github.com/thesofproject/linux/issues/5035
>> Link: https://github.com/thesofproject/linux/issues/5341
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
>> ---
>> Hi,
>>
>> Please see the problem statement and details of the issue in the commit
>> message.
>>
>> I'm not sure if this should be done in ALSA+ASoC level instead. My fear
>> is that this is changing how things has been working since almost
>> forever and it really puzzles me why it is not affecting other drivers.
>> It is true that in SOF the PAUSED state is not equal to STOPED while
>> it might be so for other vendors (it is for TI stuff for sure).
>>
>> The main point is that when we do a system suspend and a stream is in
>> PAUSED state, it will not be triggered (PAUSED == SUSPENDED/STOPPED
>> assumption?). On resume, if the platform is not supporting RESUME then
>> nothing will be done for the PAUSED stream, but a PAUSE_RELEASE will
>> fail and all sorts of state machine assumption will break in SOF/ASoC
>> stack.
>>
>> I have a PR open for quite long [1] but we would like to find the best
>> solution for us and possibly for others facing the same issue as well.
>>
>> [1] https://github.com/thesofproject/linux/pull/5058
> 
> IMO, this kind of thing should be handled in ALSA core side.
> If we want to avoid possible breakage, a flag can be introduced and
> perform this conditionally, too.  It'll become a bit complex, but
> that's because of the subtle hardware behavior differences,
> unfortunately.

I had hard time to define that flag to act upon for months ;)

It is something of a mix of conditions that needs to be present and
_might_ not really need a new flag:

The PCM device must support SNDRV_PCM_INFO_PAUSE and must not support
SNDRV_PCM_INFO_RESUME.
Given that ALSA core will not trigger the PAUSED streams on suspend,
they just moved to SUSPENDED.
On resume the RESUME is going to be skipped as well as it is not
supported, the PAUSED stream remains SUSPENDED.
If the RESUME is supported then the driver will receive the trigger and
_might_ be able to move things around to match the paused state it was
before suspend.

This patch in a way plays with these rules knowing that on resume the
paused stream is going to fail to release and we will go to a new start,
so internally it would bluntly stops anything which is paused, they will
never going to PAUSE_RELEASE, they will be started w/o the driver's
knowledge of a skipped pause release.

What I'm not sure is how this can be done legitimately in core. Move the
stream from PAUSED to STOPPED without user space knowing it? So after
resume the it thinks that the stream is paused, but the kernel has moved
it to stopped?

In SOF we need a bit higher level triggers to have the delicate
sequencing right for the BE/FE and for the IPC versions.

I think this is where I thought that this is a bit more complicated than
just add a flag and do the trigger.

Note: this workaround do have a small drawback due to what it does: it
will release the pause on the stream for a short period, which might or
might not have audible consequence, but that is still better than broken
audio stack.
> AFAIK, dmaengine PCM code assumes that the paused device can be safely
> suspended / resumed as is.

Yes, but that is a bit different as the DMAengine drivers will handle
the suspend independently and save context, with SOF (on Intel/AMD at
least) we don't use DMAengine for HD-DMA...

> There is a special handling to do pause at snd_dmaengine_pcm_trigger(), for example.

Yes, DMAengine have dmaengine_pause(), but that is optional, not all DMA
drivers have it implemented / supported.
Some DMA driver I still maintain the pause is either a stop or a a
genuine pause and on suspend the context is saved to be restored on resume.
> Also, the suspend/resume after pause worked on other legacy devices,
> as it seems, too.

Yep, they do, but we have the DSP and firmware with it's own state and
BE/FE triggering fun with the addition of the difference of sequencing
IPCs and host operations between IPC versions.

I was surprised when I invested time to figure out why this is happening
to be honest.

---
Péter

Re: [RFC] ASoC: SOF: sof-pcm/pm: Stop paused streams before the system suspend

[Péter Ujfalusi]

On 01/04/2025 14:28, Péter Ujfalusi wrote:
> 
> 
> On 31/03/2025 14:09, Takashi Iwai wrote:
>> On Mon, 31 Mar 2025 12:56:31 +0200,
>> Peter Ujfalusi wrote:
>>>
>>> Paused streams will not receive a suspend trigger, they will be marked by
>>> ALSA core as suspended and it's state is saved.
>>> Since the pause stream is not in a fully stopped state, for example DMA
>>> might be still enabled (just the trigger source is removed/disabled) we
>>> need to make sure that the hardware is ready to handle the suspend.
>>>
>>> This involves a bit more than just stopping a DMA since we also need to
>>> communicate with the firmware in a delicate sequence to follow IP
>>> programming flows.
>>> To make things a bit more challenging, these flows are different between
>>> IPC versions due to the fact that they use different messages to implement
>>> the same functionality.
>>>
>>> To avoid adding yet another path, callbacks and sequencing for handling the
>>> corner case of suspending while a stream is paused, and do this for each
>>> IPC versions and platforms, we can move the stream back to running just to
>>> put it to stopped state.
>>>
>>> Explanation of the change:
>>> Streams moved to SUSPENDED state from PAUSED without trigger. If a stream
>>> does not support RESUME then on system resume the RESUME trigger is not
>>> sent, the stream's state and suspended_state remains untouched.
>>> When the user space releases the pause then the core will reject this
>>> because the state of the stream is _not_ PAUSED, it is still SUSPENDED.
>>>
>>> From this point user space will do the normal (hw_params) prepare and
>>> START, PAUSE_RELEASE trigger will not be sent by the core after the
>>> system has resumed.
>>>
>>> Link: https://github.com/thesofproject/linux/issues/5035
>>> Link: https://github.com/thesofproject/linux/issues/5341
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
>>> ---
>>> Hi,
>>>
>>> Please see the problem statement and details of the issue in the commit
>>> message.
>>>
>>> I'm not sure if this should be done in ALSA+ASoC level instead. My fear
>>> is that this is changing how things has been working since almost
>>> forever and it really puzzles me why it is not affecting other drivers.
>>> It is true that in SOF the PAUSED state is not equal to STOPED while
>>> it might be so for other vendors (it is for TI stuff for sure).
>>>
>>> The main point is that when we do a system suspend and a stream is in
>>> PAUSED state, it will not be triggered (PAUSED == SUSPENDED/STOPPED
>>> assumption?). On resume, if the platform is not supporting RESUME then
>>> nothing will be done for the PAUSED stream, but a PAUSE_RELEASE will
>>> fail and all sorts of state machine assumption will break in SOF/ASoC
>>> stack.
>>>
>>> I have a PR open for quite long [1] but we would like to find the best
>>> solution for us and possibly for others facing the same issue as well.
>>>
>>> [1] https://github.com/thesofproject/linux/pull/5058
>>
>> IMO, this kind of thing should be handled in ALSA core side.
>> If we want to avoid possible breakage, a flag can be introduced and
>> perform this conditionally, too.  It'll become a bit complex, but
>> that's because of the subtle hardware behavior differences,
>> unfortunately.
> 
> I had hard time to define that flag to act upon for months ;)
> 
> It is something of a mix of conditions that needs to be present and
> _might_ not really need a new flag:
> 
> The PCM device must support SNDRV_PCM_INFO_PAUSE and must not support
> SNDRV_PCM_INFO_RESUME.
> Given that ALSA core will not trigger the PAUSED streams on suspend,
> they just moved to SUSPENDED.
> On resume the RESUME is going to be skipped as well as it is not
> supported, the PAUSED stream remains SUSPENDED.
> If the RESUME is supported then the driver will receive the trigger and
> _might_ be able to move things around to match the paused state it was
> before suspend.
> 
> This patch in a way plays with these rules knowing that on resume the
> paused stream is going to fail to release and we will go to a new start,
> so internally it would bluntly stops anything which is paused, they will
> never going to PAUSE_RELEASE, they will be started w/o the driver's
> knowledge of a skipped pause release.
> 
> What I'm not sure is how this can be done legitimately in core. Move the
> stream from PAUSED to STOPPED without user space knowing it? So after
> resume the it thinks that the stream is paused, but the kernel has moved
> it to stopped?
> 
> In SOF we need a bit higher level triggers to have the delicate
> sequencing right for the BE/FE and for the IPC versions.
> 
> I think this is where I thought that this is a bit more complicated than
> just add a flag and do the trigger.

Having said all of this, the following small diff works well with SOF
stack, for aplay this is fine at least ;)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 6c2b6a62d9d2..6d8389642e37 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1694,8 +1694,14 @@ static int snd_pcm_do_suspend(struct
snd_pcm_substream *substream,
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	if (runtime->trigger_master != substream)
 		return 0;
-	if (! snd_pcm_running(substream))
-		return 0;
+	if (! snd_pcm_running(substream)) {
+		if (runtime->info & SNDRV_PCM_INFO_RESUME ||
+		    runtime->state != SNDRV_PCM_STATE_PAUSED)
+			return 0;
+
+		/* release the paused stream to suspend */
+		snd_pcm_pause(substream, false);
+	}
 	substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
 	runtime->stop_operating = true;
 	return 0; /* suspend unconditionally */

So, if the PCM device does not support resuming, then release the pause
before going the suspend.

-- 
Péter

Re: [RFC] ASoC: SOF: sof-pcm/pm: Stop paused streams before the system suspend

[Takashi Iwai]

On Tue, 01 Apr 2025 14:20:14 +0200,
Péter Ujfalusi wrote:
> 
> 
> 
> On 01/04/2025 14:28, Péter Ujfalusi wrote:
> > 
> > 
> > On 31/03/2025 14:09, Takashi Iwai wrote:
> >> On Mon, 31 Mar 2025 12:56:31 +0200,
> >> Peter Ujfalusi wrote:
> >>>
> >>> Paused streams will not receive a suspend trigger, they will be marked by
> >>> ALSA core as suspended and it's state is saved.
> >>> Since the pause stream is not in a fully stopped state, for example DMA
> >>> might be still enabled (just the trigger source is removed/disabled) we
> >>> need to make sure that the hardware is ready to handle the suspend.
> >>>
> >>> This involves a bit more than just stopping a DMA since we also need to
> >>> communicate with the firmware in a delicate sequence to follow IP
> >>> programming flows.
> >>> To make things a bit more challenging, these flows are different between
> >>> IPC versions due to the fact that they use different messages to implement
> >>> the same functionality.
> >>>
> >>> To avoid adding yet another path, callbacks and sequencing for handling the
> >>> corner case of suspending while a stream is paused, and do this for each
> >>> IPC versions and platforms, we can move the stream back to running just to
> >>> put it to stopped state.
> >>>
> >>> Explanation of the change:
> >>> Streams moved to SUSPENDED state from PAUSED without trigger. If a stream
> >>> does not support RESUME then on system resume the RESUME trigger is not
> >>> sent, the stream's state and suspended_state remains untouched.
> >>> When the user space releases the pause then the core will reject this
> >>> because the state of the stream is _not_ PAUSED, it is still SUSPENDED.
> >>>
> >>> From this point user space will do the normal (hw_params) prepare and
> >>> START, PAUSE_RELEASE trigger will not be sent by the core after the
> >>> system has resumed.
> >>>
> >>> Link: https://github.com/thesofproject/linux/issues/5035
> >>> Link: https://github.com/thesofproject/linux/issues/5341
> >>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
> >>> ---
> >>> Hi,
> >>>
> >>> Please see the problem statement and details of the issue in the commit
> >>> message.
> >>>
> >>> I'm not sure if this should be done in ALSA+ASoC level instead. My fear
> >>> is that this is changing how things has been working since almost
> >>> forever and it really puzzles me why it is not affecting other drivers.
> >>> It is true that in SOF the PAUSED state is not equal to STOPED while
> >>> it might be so for other vendors (it is for TI stuff for sure).
> >>>
> >>> The main point is that when we do a system suspend and a stream is in
> >>> PAUSED state, it will not be triggered (PAUSED == SUSPENDED/STOPPED
> >>> assumption?). On resume, if the platform is not supporting RESUME then
> >>> nothing will be done for the PAUSED stream, but a PAUSE_RELEASE will
> >>> fail and all sorts of state machine assumption will break in SOF/ASoC
> >>> stack.
> >>>
> >>> I have a PR open for quite long [1] but we would like to find the best
> >>> solution for us and possibly for others facing the same issue as well.
> >>>
> >>> [1] https://github.com/thesofproject/linux/pull/5058
> >>
> >> IMO, this kind of thing should be handled in ALSA core side.
> >> If we want to avoid possible breakage, a flag can be introduced and
> >> perform this conditionally, too.  It'll become a bit complex, but
> >> that's because of the subtle hardware behavior differences,
> >> unfortunately.
> > 
> > I had hard time to define that flag to act upon for months ;)
> > 
> > It is something of a mix of conditions that needs to be present and
> > _might_ not really need a new flag:
> > 
> > The PCM device must support SNDRV_PCM_INFO_PAUSE and must not support
> > SNDRV_PCM_INFO_RESUME.
> > Given that ALSA core will not trigger the PAUSED streams on suspend,
> > they just moved to SUSPENDED.
> > On resume the RESUME is going to be skipped as well as it is not
> > supported, the PAUSED stream remains SUSPENDED.
> > If the RESUME is supported then the driver will receive the trigger and
> > _might_ be able to move things around to match the paused state it was
> > before suspend.
> > 
> > This patch in a way plays with these rules knowing that on resume the
> > paused stream is going to fail to release and we will go to a new start,
> > so internally it would bluntly stops anything which is paused, they will
> > never going to PAUSE_RELEASE, they will be started w/o the driver's
> > knowledge of a skipped pause release.
> > 
> > What I'm not sure is how this can be done legitimately in core. Move the
> > stream from PAUSED to STOPPED without user space knowing it? So after
> > resume the it thinks that the stream is paused, but the kernel has moved
> > it to stopped?
> > 
> > In SOF we need a bit higher level triggers to have the delicate
> > sequencing right for the BE/FE and for the IPC versions.
> > 
> > I think this is where I thought that this is a bit more complicated than
> > just add a flag and do the trigger.
> 
> Having said all of this, the following small diff works well with SOF
> stack, for aplay this is fine at least ;)
> 
> diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
> index 6c2b6a62d9d2..6d8389642e37 100644
> --- a/sound/core/pcm_native.c
> +++ b/sound/core/pcm_native.c
> @@ -1694,8 +1694,14 @@ static int snd_pcm_do_suspend(struct
> snd_pcm_substream *substream,
>  	struct snd_pcm_runtime *runtime = substream->runtime;
>  	if (runtime->trigger_master != substream)
>  		return 0;
> -	if (! snd_pcm_running(substream))
> -		return 0;
> +	if (! snd_pcm_running(substream)) {
> +		if (runtime->info & SNDRV_PCM_INFO_RESUME ||
> +		    runtime->state != SNDRV_PCM_STATE_PAUSED)
> +			return 0;
> +
> +		/* release the paused stream to suspend */
> +		snd_pcm_pause(substream, false);
> +	}
>  	substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_SUSPEND);
>  	runtime->stop_operating = true;
>  	return 0; /* suspend unconditionally */
> 
> So, if the PCM device does not support resuming, then release the pause
> before going the suspend.

Yeah, I had this kind of change in mind, too.  I thought a bit
different conditional, but the check of SNDRV_PCM_INFO_RESUME bit
should be enough.

But, calling snd_pcm_pause() from that point is dangerous; it's in a
messy loop of linked substream traversals.  I'd call in
snd_pcm_suspend() before the suspend action instead, something like:

--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -1731,6 +1731,9 @@ static const struct action_ops snd_pcm_action_suspend = {
 static int snd_pcm_suspend(struct snd_pcm_substream *substream)
 {
 	guard(pcm_stream_lock_irqsave)(substream);
+	if (runtime->state == SNDRV_PCM_STATE_PAUSED &&
+	    !(runtime->info & SNDRV_PCM_INFO_RESUME))
+		snd_pcm_pause(substream, false);
 	return snd_pcm_action(&snd_pcm_action_suspend, substream,
 			      ACTION_ARG_IGNORE);
 }


thanks,

Takashi