[PATCH 0/3] ASoC: core: Suspend/resume error propagation

[PATCH 0/3] ASoC: core: Suspend/resume error propagation

[Cezary Rojewski]

Currently all erros encountered during suspend/resume are squelched
what limits the error-handling possibilities.

Patches found here combined together allow for those errors to be
propagated up. This aligns ASoC component's behavior with how
PM-framework handles suspend/resume for a device - there's always an
option for a client (driver) to return an error code in case of failure.

Amadeusz Sławiński (3):
  ASoC: component: Propagate result of suspend and resume callbacks
  ASoC: core: Inline resume work back to resume function
  ASoC: core: Propagate component suspend/resume errors

 include/sound/soc-component.h |  4 +-
 include/sound/soc.h           |  3 --
 sound/soc/soc-component.c     | 22 +++++++---
 sound/soc/soc-core.c          | 75 ++++++++++++++++-------------------
 4 files changed, 52 insertions(+), 52 deletions(-)

-- 
2.25.1

[PATCH 1/3] ASoC: component: Propagate result of suspend and resume callbacks

[Cezary Rojewski]

From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

Both component->driver->suspend and ->resume() do return an int value
but it isn't propagated to the core later on. Update
snd_soc_component_suspend() and snd_soc_component_resume() so that the
possible errors are not squelched.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/soc-component.h |  4 ++--
 sound/soc/soc-component.c     | 22 ++++++++++++++++------
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
index c26ffb033777..421f0fc4df3e 100644
--- a/include/sound/soc-component.h
+++ b/include/sound/soc-component.h
@@ -456,8 +456,8 @@ int snd_soc_component_open(struct snd_soc_component *component,
 int snd_soc_component_close(struct snd_soc_component *component,
 			    struct snd_pcm_substream *substream,
 			    int rollback);
-void snd_soc_component_suspend(struct snd_soc_component *component);
-void snd_soc_component_resume(struct snd_soc_component *component);
+int snd_soc_component_suspend(struct snd_soc_component *component);
+int snd_soc_component_resume(struct snd_soc_component *component);
 int snd_soc_component_is_suspended(struct snd_soc_component *component);
 int snd_soc_component_probe(struct snd_soc_component *component);
 void snd_soc_component_remove(struct snd_soc_component *component);
diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index e12f8244242b..27b862ded846 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -318,18 +318,28 @@ int snd_soc_component_close(struct snd_soc_component *component,
 	return soc_component_ret(component, ret);
 }
 
-void snd_soc_component_suspend(struct snd_soc_component *component)
+int snd_soc_component_suspend(struct snd_soc_component *component)
 {
+	int ret = 0;
+
 	if (component->driver->suspend)
-		component->driver->suspend(component);
-	component->suspended = 1;
+		ret = component->driver->suspend(component);
+	if (!ret)
+		component->suspended = 1;
+
+	return soc_component_ret(component, ret);
 }
 
-void snd_soc_component_resume(struct snd_soc_component *component)
+int snd_soc_component_resume(struct snd_soc_component *component)
 {
+	int ret = 0;
+
 	if (component->driver->resume)
-		component->driver->resume(component);
-	component->suspended = 0;
+		ret = component->driver->resume(component);
+	if (!ret)
+		component->suspended = 0;
+
+	return soc_component_ret(component, ret);
 }
 
 int snd_soc_component_is_suspended(struct snd_soc_component *component)
-- 
2.25.1

[PATCH 2/3] ASoC: core: Inline resume work back to resume function

[Cezary Rojewski]

From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

Commit 6ed2597883b1 ("ALSA: ASoC: Don't block system resume") introduced
deferred_resume_work for ASoC subsystem. While this allows for potential
speed up during boot on some slow devices, it doesn't allow to properly
propagate return values in case something failed during system resume.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/soc.h  |  3 ---
 sound/soc/soc-core.c | 48 +++++++++++---------------------------------
 2 files changed, 12 insertions(+), 39 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 37bbfc8b45cb..3465aa075afe 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1005,9 +1005,6 @@ struct snd_soc_card {
 
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *debugfs_card_root;
-#endif
-#ifdef CONFIG_PM_SLEEP
-	struct work_struct deferred_resume_work;
 #endif
 	u32 pop_time;
 
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index a409fbed8f34..5f7e0735f0c1 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -643,17 +643,21 @@ int snd_soc_suspend(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(snd_soc_suspend);
 
-/*
- * deferred resume work, so resume can complete before we finished
- * setting our codec back up, which can be very slow on I2C
- */
-static void soc_resume_deferred(struct work_struct *work)
+/* powers up audio subsystem after a suspend */
+int snd_soc_resume(struct device *dev)
 {
-	struct snd_soc_card *card =
-			container_of(work, struct snd_soc_card,
-				     deferred_resume_work);
+	struct snd_soc_card *card = dev_get_drvdata(dev);
 	struct snd_soc_component *component;
 
+	/* If the card is not initialized yet there is nothing to do */
+	if (!card->instantiated)
+		return 0;
+
+	/* activate pins from sleep state */
+	for_each_card_components(card, component)
+		if (snd_soc_component_active(component))
+			pinctrl_pm_select_default_state(component->dev);
+
 	/*
 	 * our power state is still SNDRV_CTL_POWER_D3hot from suspend time,
 	 * so userspace apps are blocked from touching us
@@ -686,40 +690,14 @@ static void soc_resume_deferred(struct work_struct *work)
 
 	/* userspace can access us now we are back as we were before */
 	snd_power_change_state(card->snd_card, SNDRV_CTL_POWER_D0);
-}
-
-/* powers up audio subsystem after a suspend */
-int snd_soc_resume(struct device *dev)
-{
-	struct snd_soc_card *card = dev_get_drvdata(dev);
-	struct snd_soc_component *component;
-
-	/* If the card is not initialized yet there is nothing to do */
-	if (!card->instantiated)
-		return 0;
-
-	/* activate pins from sleep state */
-	for_each_card_components(card, component)
-		if (snd_soc_component_active(component))
-			pinctrl_pm_select_default_state(component->dev);
-
-	dev_dbg(dev, "ASoC: Scheduling resume work\n");
-	if (!schedule_work(&card->deferred_resume_work))
-		dev_err(dev, "ASoC: resume work item may be lost\n");
 
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_resume);
 
-static void soc_resume_init(struct snd_soc_card *card)
-{
-	/* deferred resume work */
-	INIT_WORK(&card->deferred_resume_work, soc_resume_deferred);
-}
 #else
 #define snd_soc_suspend NULL
 #define snd_soc_resume NULL
-static inline void soc_resume_init(struct snd_soc_card *card) { }
 #endif
 
 static struct device_node
@@ -1968,8 +1946,6 @@ static int snd_soc_bind_card(struct snd_soc_card *card)
 
 	soc_init_card_debugfs(card);
 
-	soc_resume_init(card);
-
 	ret = snd_soc_dapm_new_controls(&card->dapm, card->dapm_widgets,
 					card->num_dapm_widgets);
 	if (ret < 0)
-- 
2.25.1

[PATCH 3/3] ASoC: core: Propagate component suspend/resume errors

[Cezary Rojewski]

From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

In case there is a failure during component suspend/resume, error should
be propagated back to callers.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/soc-core.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 5f7e0735f0c1..931b4dc95234 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -550,6 +550,7 @@ int snd_soc_suspend(struct device *dev)
 	struct snd_soc_card *card = dev_get_drvdata(dev);
 	struct snd_soc_component *component;
 	struct snd_soc_pcm_runtime *rtd;
+	int ret = 0;
 	int i;
 
 	/* If the card is not initialized yet there is nothing to do */
@@ -623,7 +624,14 @@ int snd_soc_suspend(struct device *dev)
 				fallthrough;
 
 			case SND_SOC_BIAS_OFF:
-				snd_soc_component_suspend(component);
+				ret = snd_soc_component_suspend(component);
+				if (ret) {
+					dev_err(component->dev,
+						"ASoC: Suspend component %s failed: %d\n",
+						component->name, ret);
+					goto exit;
+				}
+
 				if (component->regmap)
 					regcache_mark_dirty(component->regmap);
 				/* deactivate pins to sleep state */
@@ -639,7 +647,8 @@ int snd_soc_suspend(struct device *dev)
 
 	snd_soc_card_suspend_post(card);
 
-	return 0;
+exit:
+	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_suspend);
 
@@ -648,6 +657,7 @@ int snd_soc_resume(struct device *dev)
 {
 	struct snd_soc_card *card = dev_get_drvdata(dev);
 	struct snd_soc_component *component;
+	int ret = 0;
 
 	/* If the card is not initialized yet there is nothing to do */
 	if (!card->instantiated)
@@ -671,8 +681,14 @@ int snd_soc_resume(struct device *dev)
 	snd_soc_card_resume_pre(card);
 
 	for_each_card_components(card, component) {
-		if (snd_soc_component_is_suspended(component))
-			snd_soc_component_resume(component);
+		if (snd_soc_component_is_suspended(component)) {
+			ret = snd_soc_component_resume(component);
+			if (ret) {
+				dev_err(component->dev, "ASoC: Resume component %s failed: %d\n",
+					component->name, ret);
+				goto exit;
+			}
+		}
 	}
 
 	soc_dapm_suspend_resume(card, SND_SOC_DAPM_STREAM_RESUME);
@@ -691,7 +707,8 @@ int snd_soc_resume(struct device *dev)
 	/* userspace can access us now we are back as we were before */
 	snd_power_change_state(card->snd_card, SNDRV_CTL_POWER_D0);
 
-	return 0;
+exit:
+	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_resume);
 
-- 
2.25.1

Re: [PATCH 2/3] ASoC: core: Inline resume work back to resume function

[Pierre-Louis Bossart]

On 11/4/22 09:12, Cezary Rojewski wrote:
> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> 
> Commit 6ed2597883b1 ("ALSA: ASoC: Don't block system resume") introduced
> deferred_resume_work for ASoC subsystem. While this allows for potential
> speed up during boot on some slow devices, it doesn't allow to properly
> propagate return values in case something failed during system resume.

Are you suggesting to remove this workqueue that's been there since
2008, which would impact negatively slow devices?

If I follow your logic, we should also remove the workqueue used for
probes for HDaudio devices, on the grounds that probe errors are not
propagated either.

Any time we have deferred processing to avoid blocking the rest of the
system, we incur the risk of not having errors propagated. It's a
compromise between having a system that's usable and a system that's
consistent.
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/sound/soc.h  |  3 ---
>  sound/soc/soc-core.c | 48 +++++++++++---------------------------------
>  2 files changed, 12 insertions(+), 39 deletions(-)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index 37bbfc8b45cb..3465aa075afe 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -1005,9 +1005,6 @@ struct snd_soc_card {
>  
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry *debugfs_card_root;
> -#endif
> -#ifdef CONFIG_PM_SLEEP
> -	struct work_struct deferred_resume_work;
>  #endif
>  	u32 pop_time;
>  
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index a409fbed8f34..5f7e0735f0c1 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -643,17 +643,21 @@ int snd_soc_suspend(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_suspend);
>  
> -/*
> - * deferred resume work, so resume can complete before we finished
> - * setting our codec back up, which can be very slow on I2C
> - */
> -static void soc_resume_deferred(struct work_struct *work)
> +/* powers up audio subsystem after a suspend */
> +int snd_soc_resume(struct device *dev)
>  {
> -	struct snd_soc_card *card =
> -			container_of(work, struct snd_soc_card,
> -				     deferred_resume_work);
> +	struct snd_soc_card *card = dev_get_drvdata(dev);
>  	struct snd_soc_component *component;
>  
> +	/* If the card is not initialized yet there is nothing to do */
> +	if (!card->instantiated)
> +		return 0;
> +
> +	/* activate pins from sleep state */
> +	for_each_card_components(card, component)
> +		if (snd_soc_component_active(component))
> +			pinctrl_pm_select_default_state(component->dev);
> +
>  	/*
>  	 * our power state is still SNDRV_CTL_POWER_D3hot from suspend time,
>  	 * so userspace apps are blocked from touching us
> @@ -686,40 +690,14 @@ static void soc_resume_deferred(struct work_struct *work)
>  
>  	/* userspace can access us now we are back as we were before */
>  	snd_power_change_state(card->snd_card, SNDRV_CTL_POWER_D0);
> -}
> -
> -/* powers up audio subsystem after a suspend */
> -int snd_soc_resume(struct device *dev)
> -{
> -	struct snd_soc_card *card = dev_get_drvdata(dev);
> -	struct snd_soc_component *component;
> -
> -	/* If the card is not initialized yet there is nothing to do */
> -	if (!card->instantiated)
> -		return 0;
> -
> -	/* activate pins from sleep state */
> -	for_each_card_components(card, component)
> -		if (snd_soc_component_active(component))
> -			pinctrl_pm_select_default_state(component->dev);
> -
> -	dev_dbg(dev, "ASoC: Scheduling resume work\n");
> -	if (!schedule_work(&card->deferred_resume_work))
> -		dev_err(dev, "ASoC: resume work item may be lost\n");
>  
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_resume);
>  
> -static void soc_resume_init(struct snd_soc_card *card)
> -{
> -	/* deferred resume work */
> -	INIT_WORK(&card->deferred_resume_work, soc_resume_deferred);
> -}
>  #else
>  #define snd_soc_suspend NULL
>  #define snd_soc_resume NULL
> -static inline void soc_resume_init(struct snd_soc_card *card) { }
>  #endif
>  
>  static struct device_node
> @@ -1968,8 +1946,6 @@ static int snd_soc_bind_card(struct snd_soc_card *card)
>  
>  	soc_init_card_debugfs(card);
>  
> -	soc_resume_init(card);
> -
>  	ret = snd_soc_dapm_new_controls(&card->dapm, card->dapm_widgets,
>  					card->num_dapm_widgets);
>  	if (ret < 0)

Re: [PATCH 1/3] ASoC: component: Propagate result of suspend and resume callbacks

[Pierre-Louis Bossart]

On 11/4/22 09:12, Cezary Rojewski wrote:
> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> 
> Both component->driver->suspend and ->resume() do return an int value
> but it isn't propagated to the core later on. Update
> snd_soc_component_suspend() and snd_soc_component_resume() so that the
> possible errors are not squelched.

This looks alright on paper but could break existing solutions.
There are a number of cases where an error during suspend is not fatal
and you don't want to prevent a system suspend if this is recoverable on
resume.

See for example the errors on clock-stop for SoundWire, which are
squelched on purpose. See also Andy Ross' PR to precisely stop
propagating errors in SOF https://github.com/thesofproject/linux/pull/3863

Maybe a less intrusive change would be to add a WARN_ON or something
visible to make sure solutions are fixed, and only critical issues can
prevent suspend? And in a second step the errors are propagated.

> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
>  include/sound/soc-component.h |  4 ++--
>  sound/soc/soc-component.c     | 22 ++++++++++++++++------
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/include/sound/soc-component.h b/include/sound/soc-component.h
> index c26ffb033777..421f0fc4df3e 100644
> --- a/include/sound/soc-component.h
> +++ b/include/sound/soc-component.h
> @@ -456,8 +456,8 @@ int snd_soc_component_open(struct snd_soc_component *component,
>  int snd_soc_component_close(struct snd_soc_component *component,
>  			    struct snd_pcm_substream *substream,
>  			    int rollback);
> -void snd_soc_component_suspend(struct snd_soc_component *component);
> -void snd_soc_component_resume(struct snd_soc_component *component);
> +int snd_soc_component_suspend(struct snd_soc_component *component);
> +int snd_soc_component_resume(struct snd_soc_component *component);
>  int snd_soc_component_is_suspended(struct snd_soc_component *component);
>  int snd_soc_component_probe(struct snd_soc_component *component);
>  void snd_soc_component_remove(struct snd_soc_component *component);
> diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
> index e12f8244242b..27b862ded846 100644
> --- a/sound/soc/soc-component.c
> +++ b/sound/soc/soc-component.c
> @@ -318,18 +318,28 @@ int snd_soc_component_close(struct snd_soc_component *component,
>  	return soc_component_ret(component, ret);
>  }
>  
> -void snd_soc_component_suspend(struct snd_soc_component *component)
> +int snd_soc_component_suspend(struct snd_soc_component *component)
>  {
> +	int ret = 0;
> +
>  	if (component->driver->suspend)
> -		component->driver->suspend(component);
> -	component->suspended = 1;
> +		ret = component->driver->suspend(component);
> +	if (!ret)
> +		component->suspended = 1;
> +
> +	return soc_component_ret(component, ret);
>  }
>  
> -void snd_soc_component_resume(struct snd_soc_component *component)
> +int snd_soc_component_resume(struct snd_soc_component *component)
>  {
> +	int ret = 0;
> +
>  	if (component->driver->resume)
> -		component->driver->resume(component);
> -	component->suspended = 0;
> +		ret = component->driver->resume(component);
> +	if (!ret)
> +		component->suspended = 0;
> +
> +	return soc_component_ret(component, ret);
>  }
>  
>  int snd_soc_component_is_suspended(struct snd_soc_component *component)

Re: [PATCH 2/3] ASoC: core: Inline resume work back to resume function

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2144 bytes --]

On Fri, Nov 04, 2022 at 09:58:46AM -0400, Pierre-Louis Bossart wrote:
> On 11/4/22 09:12, Cezary Rojewski wrote:

> > Commit 6ed2597883b1 ("ALSA: ASoC: Don't block system resume") introduced
> > deferred_resume_work for ASoC subsystem. While this allows for potential
> > speed up during boot on some slow devices, it doesn't allow to properly
> > propagate return values in case something failed during system resume.

> Are you suggesting to remove this workqueue that's been there since
> 2008, which would impact negatively slow devices?

Indeed, and the definition of fast and slow is a bit of a moving target
- the faster the rest of the system can resume the less desirable it is
to block on for example an I2C register map resync even if the hundreds
of miliseconds of ramp time that were an issue originally.

> If I follow your logic, we should also remove the workqueue used for
> probes for HDaudio devices, on the grounds that probe errors are not
> propagated either.

> Any time we have deferred processing to avoid blocking the rest of the
> system, we incur the risk of not having errors propagated. It's a
> compromise between having a system that's usable and a system that's
> consistent.

The other question is what we'd constructively do about a resume failure
that we can't defer.  It feels like we should at least retain the
ability to defer for devices where this is an issue (older components
tend to be cheap and packaged in easier to assemble packaging and hence
get used with lower end applications even well after they're no longer
competitive at the high end), and if we are going to return some errors
in line it'd be good to understand the benefits and tradeoffs.  I do see
that it is a lot less useful for modern devices where we don't have to
have any delays in startup, though like I say register I/O on slower
buses like I2C could still be a concern.

I'm not keen on moving the support out of the core since there were
originally a bunch of devices trying to open code and it wasn't good,
both from a duplication/complexity point of view and from the point of
view of integrating well with userspace APIs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] ASoC: component: Propagate result of suspend and resume callbacks

[Amadeusz Sławiński]

On 11/4/2022 3:00 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 11/4/22 09:12, Cezary Rojewski wrote:
>> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
>>
>> Both component->driver->suspend and ->resume() do return an int value
>> but it isn't propagated to the core later on. Update
>> snd_soc_component_suspend() and snd_soc_component_resume() so that the
>> possible errors are not squelched.
> 
> This looks alright on paper but could break existing solutions.
> There are a number of cases where an error during suspend is not fatal
> and you don't want to prevent a system suspend if this is recoverable on
> resume.
> 
> See for example the errors on clock-stop for SoundWire, which are
> squelched on purpose. See also Andy Ross' PR to precisely stop
> propagating errors in SOF https://github.com/thesofproject/linux/pull/3863
> 
> Maybe a less intrusive change would be to add a WARN_ON or something
> visible to make sure solutions are fixed, and only critical issues can
> prevent suspend? And in a second step the errors are propagated.
> 

Do note that thread you've pointed out handles device suspend, by which 
I mean, it is modification of sof_suspend(), called by 
snd_sof_runtime_suspend() which is then registered as handler in:
sound/soc/sof/sof-pci-dev.c: 
SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
sound/soc/sof/sof-acpi-dev.c: 
SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
sound/soc/sof/sof-of-dev.c: 
SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
and then taking TGL device for example there is:
static struct pci_driver snd_sof_pci_intel_tgl_driver = {
	(...)
         .driver = {
                 .pm = &sof_pci_pm,
         },
};

And what this patch set changes is handling of .suspend callback present 
in struct snd_soc_component_driver, which as evidenced by followup 
patches is handled in ASoC core while audio is being suspended.
As far as I can tell SOF makes no direct use of this callback.

I'm not negating that maybe there should be a bit of time when only 
warning is emitted, just making sure that we are on the same page, about 
what is being changed.

Re: [PATCH 2/3] ASoC: core: Inline resume work back to resume function

[Cezary Rojewski]

On 2022-11-05 12:54 AM, Mark Brown wrote:
> On Fri, Nov 04, 2022 at 09:58:46AM -0400, Pierre-Louis Bossart wrote:

>> If I follow your logic, we should also remove the workqueue used for
>> probes for HDaudio devices, on the grounds that probe errors are not
>> propagated either.

(save)

>> Any time we have deferred processing to avoid blocking the rest of the
>> system, we incur the risk of not having errors propagated. It's a
>> compromise between having a system that's usable and a system that's
>> consistent.

> The other question is what we'd constructively do about a resume failure
> that we can't defer.  It feels like we should at least retain the
> ability to defer for devices where this is an issue (older components
> tend to be cheap and packaged in easier to assemble packaging and hence
> get used with lower end applications even well after they're no longer
> competitive at the high end), and if we are going to return some errors
> in line it'd be good to understand the benefits and tradeoffs.  I do see
> that it is a lot less useful for modern devices where we don't have to
> have any delays in startup, though like I say register I/O on slower
> buses like I2C could still be a concern.
> 
> I'm not keen on moving the support out of the core since there were
> originally a bunch of devices trying to open code and it wasn't good,
> both from a duplication/complexity point of view and from the point of
> view of integrating well with userspace APIs.

I believe that framework should be supporting both, the deferred and the 
instant resume options. 'void' in front of suspend/resume in ASoC 
hinders developer's options.

(load)
The HDAudio driver is actually a good example of how to do it right - we 
did not modify driver/base/ to have ->probe() return void. It remained 
as is, instead, a developer opt-ins for a delayed probe through a 
workqueue. This way, everyone is satisfied.
Cohesiveness is not to be forgotten too - keeping behavior and 
expectations of the standard set of functionalities aligned with the 
rest of the driver/base makes it easier to hop into ASoC.

We could provide some additional flags so that the ASoC core always 
defers PM-related work for certain components if they choose to.


Regards,
Czarek

Re: [PATCH 1/3] ASoC: component: Propagate result of suspend and resume callbacks

[Pierre-Louis Bossart]

On 11/7/22 02:51, Amadeusz Sławiński wrote:
> On 11/4/2022 3:00 PM, Pierre-Louis Bossart wrote:
>>
>>
>> On 11/4/22 09:12, Cezary Rojewski wrote:
>>> From: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
>>>
>>> Both component->driver->suspend and ->resume() do return an int value
>>> but it isn't propagated to the core later on. Update
>>> snd_soc_component_suspend() and snd_soc_component_resume() so that the
>>> possible errors are not squelched.
>>
>> This looks alright on paper but could break existing solutions.
>> There are a number of cases where an error during suspend is not fatal
>> and you don't want to prevent a system suspend if this is recoverable on
>> resume.
>>
>> See for example the errors on clock-stop for SoundWire, which are
>> squelched on purpose. See also Andy Ross' PR to precisely stop
>> propagating errors in SOF
>> https://github.com/thesofproject/linux/pull/3863
>>
>> Maybe a less intrusive change would be to add a WARN_ON or something
>> visible to make sure solutions are fixed, and only critical issues can
>> prevent suspend? And in a second step the errors are propagated.
>>
> 
> Do note that thread you've pointed out handles device suspend, by which

If by 'that thread' you are referring to PR #3863, then it's an
excellent example of a desire NOT to propage suspend errors and at the
same time an example of a configuration where suspend would not work
without additional changes.

> I mean, it is modification of sof_suspend(), called by
> snd_sof_runtime_suspend() which is then registered as handler in:
> sound/soc/sof/sof-pci-dev.c: SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend,
> snd_sof_runtime_resume,
> sound/soc/sof/sof-acpi-dev.c:
> SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend, snd_sof_runtime_resume,
> sound/soc/sof/sof-of-dev.c: SET_RUNTIME_PM_OPS(snd_sof_runtime_suspend,
> snd_sof_runtime_resume,
> and then taking TGL device for example there is:
> static struct pci_driver snd_sof_pci_intel_tgl_driver = {
>     (...)
>         .driver = {
>                 .pm = &sof_pci_pm,
>         },
> };
> 
> And what this patch set changes is handling of .suspend callback present
> in struct snd_soc_component_driver, which as evidenced by followup
> patches is handled in ASoC core while audio is being suspended.
> As far as I can tell SOF makes no direct use of this callback.
> 
> I'm not negating that maybe there should be a bit of time when only
> warning is emitted, just making sure that we are on the same page, about
> what is being changed.

I don't think there is an impact on SOF indeed.

I was just making the point that well-intended changes to propagate
error status can break platforms. we've had a similar case when trying
to add checks on pm_runtime_get_sync() and saw multiple errors. Adding
more error checks when they were not there from the very beginning is a
difficult thing to achieve without regressions.

Re: [PATCH 2/3] ASoC: core: Inline resume work back to resume function

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On Mon, Nov 07, 2022 at 10:26:14AM +0100, Cezary Rojewski wrote:
> On 2022-11-05 12:54 AM, Mark Brown wrote:

> > The other question is what we'd constructively do about a resume failure
> > that we can't defer.  It feels like we should at least retain the
> > ability to defer for devices where this is an issue (older components

> I believe that framework should be supporting both, the deferred and the
> instant resume options. 'void' in front of suspend/resume in ASoC hinders
> developer's options.

It'd be good to at least have some idea of practical usage as well, the
functions return void because nothing was making any use of the return
values.

> (load)
> The HDAudio driver is actually a good example of how to do it right - we did
> not modify driver/base/ to have ->probe() return void. It remained as is,
> instead, a developer opt-ins for a delayed probe through a workqueue. This
> way, everyone is satisfied.
> Cohesiveness is not to be forgotten too - keeping behavior and expectations
> of the standard set of functionalities aligned with the rest of the
> driver/base makes it easier to hop into ASoC.

There's also an expectation that suspend and resume be fast...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] ASoC: core: Inline resume work back to resume function

[Cezary Rojewski]

On 2022-11-07 3:28 PM, Mark Brown wrote:

> It'd be good to at least have some idea of practical usage as well, the
> functions return void because nothing was making any use of the return
> values.

The problem is mainly connected to a resume operation as even if 
something fails at suspend, there is a chance for the driver to recover 
things up during resume.

Now, if something fails during the resume operation, blindly returning 0 
makes userspace think everything is fine and we can keep going, whereas 
the stream may no longer be operational and requires complete recovery 
(close, reopen).

alsa-utils/aplay/aplay.c does snd_pcm_resume() as long as -EAGAIN is 
returned, then falls back to snd_pcm_prepare() before finally giving up. 
As snd_pcm_prepare() checks pcm's state before invoking ->prepare(), one 
option is to set substream->runtime->status->state to 
SNDRV_PCM_STATE_DISCONNECTED to ensure dai->hw_free() gets called right 
after.

TLDR: it all comes down to some granularity missing. Suspend/resume for 
the AudioDSP drivers do basically entire pcm-lifecycle within what is 
supposed to be a simple TRIGGER_SUSPEND/RESUME operation.

> There's also an expectation that suspend and resume be fast...

You're right. Sound devices should not disturb the PM for the entire system.

Looking at this after thinking the performance/stability argument 
through, indeed deferring work is a good way of dealing with the situation.


Regards,
Czarek