diff mbox series

[04/16] ASoC: rt711: enable pm_runtime in probe, keep status as 'suspended'

Message ID 20230802153629.53576-5-pierre-louis.bossart@linux.intel.com
State Accepted
Commit a8590dd73d9f7fd955ac24a8e210d0721d5c10af
Headers show
Series ASoC: SoundWire codecs: improve pm_runtime handling | expand

Commit Message

Pierre-Louis Bossart Aug. 2, 2023, 3:36 p.m. UTC 
In stress cases involving module insertion/removal followed by
playback/capture, it can happen that capture/playback is started
before the codec enumeration completes.

The codec driver registers its components with the ASoC framework
during the probe stage, so there is currently no way for the card
creation to wait for the codec enumeration/initialization to complete.

In addition, when the capture/playback starts, the ASoC framework uses
pm_runtime_get_sync() to properly refcount and power-manage
devices. This is problematic in the SoundWire case because pm_runtime
is enabled during the enumeration/initialization stage, so
pm_runtime_get_sync() will return -EACCESS which is
ignored. Additional errors will happen when setting the pm_runtime
status as 'active' because the parent is not properly resumed,
resulting in an error such as:

"rt711 sdw:0:025d:0711:00: runtime PM trying to activate child device
sdw:0:025d:0711:00 but parent (sdw-master-0) is not active"

This patch suggests enabling pm_runtime during the probe, but marking
the device as 'active' only after it is enumerated. That will force a
dependency between the card and the codec, pm_runtime_get_sync() will
have to wait for the codec device to resume and hence implicitly wait
for the enumeration/initialization to be completed. In the nominal
case where the codec device is already active the get_sync() would
only perform a ref-count increase.

Closes: https://github.com/thesofproject/linux/issues/4328
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 sound/soc/codecs/rt711-sdw.c |  3 +--
 sound/soc/codecs/rt711.c     | 40 ++++++++++++++++++++++++------------
 2 files changed, 28 insertions(+), 15 deletions(-)
diff mbox series

Patch

diff --git a/sound/soc/codecs/rt711-sdw.c b/sound/soc/codecs/rt711-sdw.c
index 530d1ae32c04..3f5773310ae8 100644
--- a/sound/soc/codecs/rt711-sdw.c
+++ b/sound/soc/codecs/rt711-sdw.c
@@ -466,8 +466,7 @@  static int rt711_sdw_remove(struct sdw_slave *slave)
 		cancel_work_sync(&rt711->calibration_work);
 	}
 
-	if (rt711->first_hw_init)
-		pm_runtime_disable(&slave->dev);
+	pm_runtime_disable(&slave->dev);
 
 	mutex_destroy(&rt711->calibrate_mutex);
 	mutex_destroy(&rt711->disable_irq_lock);
diff --git a/sound/soc/codecs/rt711.c b/sound/soc/codecs/rt711.c
index 0ca955e2f4e7..66eaed13b0d6 100644
--- a/sound/soc/codecs/rt711.c
+++ b/sound/soc/codecs/rt711.c
@@ -462,6 +462,10 @@  static int rt711_set_jack_detect(struct snd_soc_component *component,
 
 	rt711->hs_jack = hs_jack;
 
+	/* we can only resume if the device was initialized at least once */
+	if (!rt711->first_hw_init)
+		return 0;
+
 	ret = pm_runtime_resume_and_get(component->dev);
 	if (ret < 0) {
 		if (ret != -EACCES) {
@@ -941,6 +945,9 @@  static int rt711_probe(struct snd_soc_component *component)
 	rt711_parse_dt(rt711, &rt711->slave->dev);
 	rt711->component = component;
 
+	if (!rt711->first_hw_init)
+		return 0;
+
 	ret = pm_runtime_resume(component->dev);
 	if (ret < 0 && ret != -EACCES)
 		return ret;
@@ -1206,8 +1213,25 @@  int rt711_init(struct device *dev, struct regmap *sdw_regmap,
 				&soc_codec_dev_rt711,
 				rt711_dai,
 				ARRAY_SIZE(rt711_dai));
+	if (ret < 0)
+		return ret;
 
-	dev_dbg(&slave->dev, "%s\n", __func__);
+	/* set autosuspend parameters */
+	pm_runtime_set_autosuspend_delay(dev, 3000);
+	pm_runtime_use_autosuspend(dev);
+
+	/* make sure the device does not suspend immediately */
+	pm_runtime_mark_last_busy(dev);
+
+	pm_runtime_enable(dev);
+
+	/* important note: the device is NOT tagged as 'active' and will remain
+	 * 'suspended' until the hardware is enumerated/initialized. This is required
+	 * to make sure the ASoC framework use of pm_runtime_get_sync() does not silently
+	 * fail with -EACCESS because of race conditions between card creation and enumeration
+	 */
+
+	dev_dbg(dev, "%s\n", __func__);
 
 	return ret;
 }
@@ -1226,22 +1250,12 @@  int rt711_io_init(struct device *dev, struct sdw_slave *slave)
 		regcache_cache_bypass(rt711->regmap, true);
 
 	/*
-	 * PM runtime is only enabled when a Slave reports as Attached
+	 * PM runtime status is marked as 'active' only when a Slave reports as Attached
 	 */
-	if (!rt711->first_hw_init) {
-		/* set autosuspend parameters */
-		pm_runtime_set_autosuspend_delay(&slave->dev, 3000);
-		pm_runtime_use_autosuspend(&slave->dev);
-
+	if (!rt711->first_hw_init)
 		/* update count of parent 'active' children */
 		pm_runtime_set_active(&slave->dev);
 
-		/* make sure the device does not suspend immediately */
-		pm_runtime_mark_last_busy(&slave->dev);
-
-		pm_runtime_enable(&slave->dev);
-	}
-
 	pm_runtime_get_noresume(&slave->dev);
 
 	rt711_reset(rt711->regmap);