diff mbox series

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

Message ID 20230802153629.53576-6-pierre-louis.bossart@linux.intel.com
State Accepted
Commit 0c321fb857707ef68ffdb4f9672beb664e6679cc
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-sdca-sdw.c |  3 +--
 sound/soc/codecs/rt711-sdca.c     | 40 +++++++++++++++++++++----------
 2 files changed, 29 insertions(+), 14 deletions(-)
diff mbox series

Patch

diff --git a/sound/soc/codecs/rt711-sdca-sdw.c b/sound/soc/codecs/rt711-sdca-sdw.c
index 23f23f714b39..935e597022d3 100644
--- a/sound/soc/codecs/rt711-sdca-sdw.c
+++ b/sound/soc/codecs/rt711-sdca-sdw.c
@@ -366,8 +366,7 @@  static int rt711_sdca_sdw_remove(struct sdw_slave *slave)
 		cancel_delayed_work_sync(&rt711->jack_btn_check_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-sdca.c b/sound/soc/codecs/rt711-sdca.c
index bd0f5e05874b..447154cb6010 100644
--- a/sound/soc/codecs/rt711-sdca.c
+++ b/sound/soc/codecs/rt711-sdca.c
@@ -507,6 +507,10 @@  static int rt711_sdca_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) {
@@ -1215,6 +1219,9 @@  static int rt711_sdca_probe(struct snd_soc_component *component)
 	rt711_sdca_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;
@@ -1434,9 +1441,27 @@  int rt711_sdca_init(struct device *dev, struct regmap *regmap,
 			rt711_sdca_dai,
 			ARRAY_SIZE(rt711_sdca_dai));
 
-	dev_dbg(&slave->dev, "%s\n", __func__);
+	if (ret < 0)
+		return ret;
 
-	return ret;
+	/* 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 0;
 }
 
 static void rt711_sdca_vd0_io_init(struct rt711_sdca_priv *rt711)
@@ -1511,20 +1536,11 @@  int rt711_sdca_io_init(struct device *dev, struct sdw_slave *slave)
 		regcache_cache_bypass(rt711->mbq_regmap, true);
 	} else {
 		/*
-		 * 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
 		 */
 
-		/* set autosuspend parameters */
-		pm_runtime_set_autosuspend_delay(&slave->dev, 3000);
-		pm_runtime_use_autosuspend(&slave->dev);
-
 		/* 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);