[PATCH 0/2] ASoC: SOF/soundwire: use resume_and_get on component probe
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(-)
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; }
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),
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.
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?
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.
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.
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!
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.
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
participants (2)
-
Mark Brown -
Pierre-Louis Bossart