[PATCH 0/2] ASoC: SOF/soundwire: use resume_and_get on component probe

[PATCH 0/2] ASoC: SOF/soundwire: use resume_and_get on component probe

[Pierre-Louis Bossart]

While testing driver bind/unbind sequences, I stumbled on a corner
case where the SoundWire bus can be suspended before the ASoC card
registration happens. During the registration, register accesses would
then lead to timeouts. This does not happen in regular usages where
the card registration happens within the 3-second time window before
suspend.

Adding a simple pm_runtime_resume_and_get() on component probe solves
the issue, but experiments showed it was too invasive to add at the
ASoC core level, with multiple regressions reported by our CI.

This patchset limits the additional resume to the SOF and SoundWire
codec drivers. An additional patch for the soundwire/intel component
will be sent separately.

Pierre-Louis Bossart (2):
  ASoC: SOF: pcm: use pm_resume_and_get() on component probe
  ASoC: codecs: soundwire: call pm_runtime_resume() in component probe

 sound/soc/codecs/max98373.c   | 14 +++++++++++++-
 sound/soc/codecs/rt1308-sdw.c | 12 ++++++++++++
 sound/soc/codecs/rt1316-sdw.c | 12 ++++++++++++
 sound/soc/codecs/rt700.c      |  5 +++++
 sound/soc/codecs/rt711-sdca.c |  5 +++++
 sound/soc/codecs/rt711.c      |  5 +++++
 sound/soc/codecs/rt715-sdca.c | 12 ++++++++++++
 sound/soc/codecs/rt715.c      | 12 ++++++++++++
 sound/soc/sof/pcm.c           | 11 +++++++++++
 9 files changed, 87 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH 1/2] ASoC: SOF: pcm: use pm_resume_and_get() on component probe

[Pierre-Louis Bossart]

Before initiating IPC and/or bus transactions when loading the
topology during a component probe, which happens on card
registration/creation, make sure the device for the SOF driver is
pm_runtime active.

The SOF probe is not necessarily followed by the component probe, such
a timing assumption can be broken in driver bind/unbind tests. This
can be artifially shown if the module for the machine driver is
'blacklisted' and the SOF device becomes pm_runtime_suspended before
manually calling modprobe to register the card.

In an initial experiment, pm_resume_and_get() was called from
soc-component.c, since the current ASoC component model is arguably
missing dependencies between component status and device
status. However this approach proved too invasive and breaks all
existing HDMI playback solutions on Intel platforms.

While this will result in duplication of code, generating pm_runtime
transitions only if strictly required for a given component makes more
sense overall. This patch adds the pm_runtime resume transition for
SOF only.

BugLink: https://github.com/thesofproject/linux/issues/3651
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/pcm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index a76d0b5b2ad95..27504abc5385f 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -604,6 +604,14 @@ static int sof_pcm_probe(struct snd_soc_component *component)
 	const char *tplg_filename;
 	int ret;
 
+	/*
+	 * make sure the device is pm_runtime_active before loading the
+	 * topology and initiating IPC or bus transactions
+	 */
+	ret = pm_runtime_resume_and_get(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
 	/* load the default topology */
 	sdev->component = component;
 
@@ -621,6 +629,9 @@ static int sof_pcm_probe(struct snd_soc_component *component)
 		return ret;
 	}
 
+	pm_runtime_mark_last_busy(component->dev);
+	pm_runtime_put_autosuspend(component->dev);
+
 	return ret;
 }
 
-- 
2.34.1

[PATCH 2/2] ASoC: codecs: soundwire: call pm_runtime_resume() in component probe

[Pierre-Louis Bossart]

Make sure that the bus and codecs are pm_runtime active when the card
is registered/created. This avoid timeouts when accessing registers.

BugLink: https://github.com/thesofproject/linux/issues/3651
BugLink: https://github.com/thesofproject/linux/issues/3650
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/codecs/max98373.c   | 14 +++++++++++++-
 sound/soc/codecs/rt1308-sdw.c | 12 ++++++++++++
 sound/soc/codecs/rt1316-sdw.c | 12 ++++++++++++
 sound/soc/codecs/rt700.c      |  5 +++++
 sound/soc/codecs/rt711-sdca.c |  5 +++++
 sound/soc/codecs/rt711.c      |  5 +++++
 sound/soc/codecs/rt715-sdca.c | 12 ++++++++++++
 sound/soc/codecs/rt715.c      | 12 ++++++++++++
 8 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/max98373.c b/sound/soc/codecs/max98373.c
index e14fe98349a5e..1517c47afbf14 100644
--- a/sound/soc/codecs/max98373.c
+++ b/sound/soc/codecs/max98373.c
@@ -5,6 +5,7 @@
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/cdev.h>
@@ -440,8 +441,19 @@ const struct snd_soc_component_driver soc_codec_dev_max98373 = {
 };
 EXPORT_SYMBOL_GPL(soc_codec_dev_max98373);
 
+static int max98373_sdw_probe(struct snd_soc_component *component)
+{
+	int ret;
+
+	ret = pm_runtime_resume(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
+	return 0;
+}
+
 const struct snd_soc_component_driver soc_codec_dev_max98373_sdw = {
-	.probe			= NULL,
+	.probe			= max98373_sdw_probe,
 	.controls		= max98373_snd_controls,
 	.num_controls		= ARRAY_SIZE(max98373_snd_controls),
 	.dapm_widgets		= max98373_dapm_widgets,
diff --git a/sound/soc/codecs/rt1308-sdw.c b/sound/soc/codecs/rt1308-sdw.c
index 72f673f278eec..0be6e72ff5a9a 100644
--- a/sound/soc/codecs/rt1308-sdw.c
+++ b/sound/soc/codecs/rt1308-sdw.c
@@ -608,7 +608,19 @@ static const struct sdw_slave_ops rt1308_slave_ops = {
 	.bus_config = rt1308_bus_config,
 };
 
+static int rt1308_sdw_component_probe(struct snd_soc_component *component)
+{
+	int ret;
+
+	ret = pm_runtime_resume(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
+	return 0;
+}
+
 static const struct snd_soc_component_driver soc_component_sdw_rt1308 = {
+	.probe = rt1308_sdw_component_probe,
 	.controls = rt1308_snd_controls,
 	.num_controls = ARRAY_SIZE(rt1308_snd_controls),
 	.dapm_widgets = rt1308_dapm_widgets,
diff --git a/sound/soc/codecs/rt1316-sdw.c b/sound/soc/codecs/rt1316-sdw.c
index 2d6b5f9d4d770..e53396606a1c6 100644
--- a/sound/soc/codecs/rt1316-sdw.c
+++ b/sound/soc/codecs/rt1316-sdw.c
@@ -590,7 +590,19 @@ static struct sdw_slave_ops rt1316_slave_ops = {
 	.update_status = rt1316_update_status,
 };
 
+static int rt1316_sdw_component_probe(struct snd_soc_component *component)
+{
+	int ret;
+
+	ret = pm_runtime_resume(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
+	return 0;
+}
+
 static const struct snd_soc_component_driver soc_component_sdw_rt1316 = {
+	.probe = rt1316_sdw_component_probe,
 	.controls = rt1316_snd_controls,
 	.num_controls = ARRAY_SIZE(rt1316_snd_controls),
 	.dapm_widgets = rt1316_dapm_widgets,
diff --git a/sound/soc/codecs/rt700.c b/sound/soc/codecs/rt700.c
index 9bceeeb830b15..055c3ae974d80 100644
--- a/sound/soc/codecs/rt700.c
+++ b/sound/soc/codecs/rt700.c
@@ -818,9 +818,14 @@ static const struct snd_soc_dapm_route rt700_audio_map[] = {
 static int rt700_probe(struct snd_soc_component *component)
 {
 	struct rt700_priv *rt700 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	rt700->component = component;
 
+	ret = pm_runtime_resume(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
 	return 0;
 }
 
diff --git a/sound/soc/codecs/rt711-sdca.c b/sound/soc/codecs/rt711-sdca.c
index dfe3c9299ebde..9d226b1cb7e92 100644
--- a/sound/soc/codecs/rt711-sdca.c
+++ b/sound/soc/codecs/rt711-sdca.c
@@ -1194,10 +1194,15 @@ static int rt711_sdca_parse_dt(struct rt711_sdca_priv *rt711, struct device *dev
 static int rt711_sdca_probe(struct snd_soc_component *component)
 {
 	struct rt711_sdca_priv *rt711 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	rt711_sdca_parse_dt(rt711, &rt711->slave->dev);
 	rt711->component = component;
 
+	ret = pm_runtime_resume(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
 	return 0;
 }
 
diff --git a/sound/soc/codecs/rt711.c b/sound/soc/codecs/rt711.c
index 9df800abfc2d8..1bf6180891942 100644
--- a/sound/soc/codecs/rt711.c
+++ b/sound/soc/codecs/rt711.c
@@ -935,10 +935,15 @@ static int rt711_parse_dt(struct rt711_priv *rt711, struct device *dev)
 static int rt711_probe(struct snd_soc_component *component)
 {
 	struct rt711_priv *rt711 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	rt711_parse_dt(rt711, &rt711->slave->dev);
 	rt711->component = component;
 
+	ret = pm_runtime_resume(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
 	return 0;
 }
 
diff --git a/sound/soc/codecs/rt715-sdca.c b/sound/soc/codecs/rt715-sdca.c
index 5857d08663073..ce8bbc76199a8 100644
--- a/sound/soc/codecs/rt715-sdca.c
+++ b/sound/soc/codecs/rt715-sdca.c
@@ -758,7 +758,19 @@ static const struct snd_soc_dapm_route rt715_sdca_audio_map[] = {
 	{"ADC 25 Mux", "DMIC4", "DMIC4"},
 };
 
+static int rt715_sdca_probe(struct snd_soc_component *component)
+{
+	int ret;
+
+	ret = pm_runtime_resume(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
+	return 0;
+}
+
 static const struct snd_soc_component_driver soc_codec_dev_rt715_sdca = {
+	.probe = rt715_sdca_probe,
 	.controls = rt715_sdca_snd_controls,
 	.num_controls = ARRAY_SIZE(rt715_sdca_snd_controls),
 	.dapm_widgets = rt715_sdca_dapm_widgets,
diff --git a/sound/soc/codecs/rt715.c b/sound/soc/codecs/rt715.c
index 418e006b19ef5..e93240521c74e 100644
--- a/sound/soc/codecs/rt715.c
+++ b/sound/soc/codecs/rt715.c
@@ -737,7 +737,19 @@ static int rt715_set_bias_level(struct snd_soc_component *component,
 	return 0;
 }
 
+static int rt715_probe(struct snd_soc_component *component)
+{
+	int ret;
+
+	ret = pm_runtime_resume(component->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+
+	return 0;
+}
+
 static const struct snd_soc_component_driver soc_codec_dev_rt715 = {
+	.probe = rt715_probe,
 	.set_bias_level = rt715_set_bias_level,
 	.controls = rt715_snd_controls,
 	.num_controls = ARRAY_SIZE(rt715_snd_controls),
-- 
2.34.1

Re: [PATCH 2/2] ASoC: codecs: soundwire: call pm_runtime_resume() in component probe

[Mark Brown]

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

On Thu, Jun 16, 2022 at 04:08:25PM -0500, Pierre-Louis Bossart wrote:

> Make sure that the bus and codecs are pm_runtime active when the card
> is registered/created. This avoid timeouts when accessing registers.

> +static int max98373_sdw_probe(struct snd_soc_component *component)
> +{
> +	int ret;
> +
> +	ret = pm_runtime_resume(component->dev);
> +	if (ret < 0 && ret != -EACCES)
> +		return ret;

I'm not clear what the issue is here.  Is something that's accessing the
registers forgetting to do a pm_runtime_get(), or doing that rather than
using pm_runtime_get_sync()?  This doesn't feel safe or robust.

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

Re: [PATCH 2/2] ASoC: codecs: soundwire: call pm_runtime_resume() in component probe

[Pierre-Louis Bossart]

On 6/17/22 04:44, Mark Brown wrote:
> On Thu, Jun 16, 2022 at 04:08:25PM -0500, Pierre-Louis Bossart wrote:
> 
>> Make sure that the bus and codecs are pm_runtime active when the card
>> is registered/created. This avoid timeouts when accessing registers.
> 
>> +static int max98373_sdw_probe(struct snd_soc_component *component)
>> +{
>> +	int ret;
>> +
>> +	ret = pm_runtime_resume(component->dev);
>> +	if (ret < 0 && ret != -EACCES)
>> +		return ret;
> 
> I'm not clear what the issue is here.  Is something that's accessing the
> registers forgetting to do a pm_runtime_get(), or doing that rather than
> using pm_runtime_get_sync()?  This doesn't feel safe or robust.

The context is that I have been trying to remove all timing dependencies
between components, and make sure that you can bind/unbind drivers in
any order, with the deferred probe making sure that all required
components are already probed. I started this after seeing reports of
kernel oopses when the machine driver was removed, and realizing that
the SoundWire bus itself didn't support bind/unbind tests by design.

In the case where you bind the machine driver after a delay, then the
bus might be suspended already, and there are cases where we see
timeouts for registers that are not regmap-managed (usually
vendor-specific stuff with an indirection mechanism), and even for
regmap the register read-write are cache-based when the bus is suspended.

What this patch does it make sure that the bus is operation when the
card is created. In usual cases, this is a no-op, this just helps with
corner test cases. It's not plugging a major hole in the pm_runtime
support, just fixing a programming sequence that was not tested before.

One possible objection is that we don't keep the reference and the bus
active until all components are probed. I tried doing this at the ASoC
core level, but that breaks all kinds of devices that have their own
quirky way of dealing with pm_runtime - specifically HDaudio and HDMI.
That's why I added this resume here.

Makes sense?

Re: [PATCH 2/2] ASoC: codecs: soundwire: call pm_runtime_resume() in component probe

[Mark Brown]

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

On Fri, Jun 17, 2022 at 09:35:26AM -0500, Pierre-Louis Bossart wrote:

> What this patch does it make sure that the bus is operation when the
> card is created. In usual cases, this is a no-op, this just helps with
> corner test cases. It's not plugging a major hole in the pm_runtime
> support, just fixing a programming sequence that was not tested before.

> One possible objection is that we don't keep the reference and the bus
> active until all components are probed. I tried doing this at the ASoC
> core level, but that breaks all kinds of devices that have their own
> quirky way of dealing with pm_runtime - specifically HDaudio and HDMI.
> That's why I added this resume here.

> Makes sense?

Ish.  Ugh, right.  So it's not fixing anything really, it's mainly
papering over cracks where things are being missed.  In any case it's
not doing any harm and it helps things for now.

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

Re: [PATCH 2/2] ASoC: codecs: soundwire: call pm_runtime_resume() in component probe

[Pierre-Louis Bossart]

On 6/17/22 12:37, Mark Brown wrote:
> On Fri, Jun 17, 2022 at 09:35:26AM -0500, Pierre-Louis Bossart wrote:
> 
>> What this patch does it make sure that the bus is operation when the
>> card is created. In usual cases, this is a no-op, this just helps with
>> corner test cases. It's not plugging a major hole in the pm_runtime
>> support, just fixing a programming sequence that was not tested before.
> 
>> One possible objection is that we don't keep the reference and the bus
>> active until all components are probed. I tried doing this at the ASoC
>> core level, but that breaks all kinds of devices that have their own
>> quirky way of dealing with pm_runtime - specifically HDaudio and HDMI.
>> That's why I added this resume here.
> 
>> Makes sense?
> 
> Ish.  Ugh, right.  So it's not fixing anything really, it's mainly
> papering over cracks where things are being missed.  In any case it's
> not doing any harm and it helps things for now.

You got it right. There are additional patches that were sent to use
pm_runtime_resume_and_get() on set_jack, and other clear cases that were
missed, but this is more of a blanket "do not harm" resume in case codec
drivers are missing something.

Re: [PATCH 2/2] ASoC: codecs: soundwire: call pm_runtime_resume() in component probe

[Pierre-Louis Bossart]

>>> What this patch does it make sure that the bus is operation when the
>>> card is created. In usual cases, this is a no-op, this just helps with
>>> corner test cases. It's not plugging a major hole in the pm_runtime
>>> support, just fixing a programming sequence that was not tested before.
>>
>>> One possible objection is that we don't keep the reference and the bus
>>> active until all components are probed. I tried doing this at the ASoC
>>> core level, but that breaks all kinds of devices that have their own
>>> quirky way of dealing with pm_runtime - specifically HDaudio and HDMI.
>>> That's why I added this resume here.
>>
>>> Makes sense?
>>
>> Ish.  Ugh, right.  So it's not fixing anything really, it's mainly
>> papering over cracks where things are being missed.  In any case it's
>> not doing any harm and it helps things for now.
> 
> You got it right. There are additional patches that were sent to use
> pm_runtime_resume_and_get() on set_jack, and other clear cases that were
> missed, but this is more of a blanket "do not harm" resume in case codec
> drivers are missing something.

please wait for merges, we're chasing two regressions with nonsensical
mixer values

numid=5,iface=MIXER,name='PGA4.0 4 Master Capture Volume'
  ; type=BOOLEAN,access=rw---R--,values=2
  : values=on,on
  | dBscale-min=-50.00dB,step=1.00dB,mute=1

and a spurious log that we missed:

snd-soc-dummy snd-soc-dummy: Runtime PM usage count underflow!

Re: [PATCH 2/2] ASoC: codecs: soundwire: call pm_runtime_resume() in component probe

[Pierre-Louis Bossart]

On 6/17/22 14:05, Pierre-Louis Bossart wrote:
> 
>>>> What this patch does it make sure that the bus is operation when the
>>>> card is created. In usual cases, this is a no-op, this just helps with
>>>> corner test cases. It's not plugging a major hole in the pm_runtime
>>>> support, just fixing a programming sequence that was not tested before.
>>>
>>>> One possible objection is that we don't keep the reference and the bus
>>>> active until all components are probed. I tried doing this at the ASoC
>>>> core level, but that breaks all kinds of devices that have their own
>>>> quirky way of dealing with pm_runtime - specifically HDaudio and HDMI.
>>>> That's why I added this resume here.
>>>
>>>> Makes sense?
>>>
>>> Ish.  Ugh, right.  So it's not fixing anything really, it's mainly
>>> papering over cracks where things are being missed.  In any case it's
>>> not doing any harm and it helps things for now.
>>
>> You got it right. There are additional patches that were sent to use
>> pm_runtime_resume_and_get() on set_jack, and other clear cases that were
>> missed, but this is more of a blanket "do not harm" resume in case codec
>> drivers are missing something.
> 
> please wait for merges, we're chasing two regressions with nonsensical
> mixer values
> 
> numid=5,iface=MIXER,name='PGA4.0 4 Master Capture Volume'
>   ; type=BOOLEAN,access=rw---R--,values=2
>   : values=on,on
>   | dBscale-min=-50.00dB,step=1.00dB,mute=1
> 
> and a spurious log that we missed:
> 
> snd-soc-dummy snd-soc-dummy: Runtime PM usage count underflow!

The two regressions are not caused by this series.

The mixer issue already exists and is fixed with Sameer Pujar's "ASoC:
ops: Fix multiple value control type" patch.

The "Runtime PM usage count underflow" is a mistake on my side, the
patch "ASoC: soc-component: use pm_runtime_resume_and_get()" is invalid
and shall not be merged.

No other problem detected.

Re: [PATCH 0/2] ASoC: SOF/soundwire: use resume_and_get on component probe

[Mark Brown]

On Thu, 16 Jun 2022 16:08:23 -0500, Pierre-Louis Bossart wrote:
> While testing driver bind/unbind sequences, I stumbled on a corner
> case where the SoundWire bus can be suspended before the ASoC card
> registration happens. During the registration, register accesses would
> then lead to timeouts. This does not happen in regular usages where
> the card registration happens within the 3-second time window before
> suspend.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] ASoC: SOF: pcm: use pm_resume_and_get() on component probe
      commit: 4ea3bfd13a2484b5f1c19f60b1dc7494f261f0a4
[2/2] ASoC: codecs: soundwire: call pm_runtime_resume() in component probe
      commit: 011e397f5c9c96e533d4a244af84e74c9caefb83

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark