[PATCH v2] ASoC: cs35l56: Fix probe deadlock waiting for SoundWire enumeration

[Richard Fitzgerald <rf@opensource.cirrus.com>]

Defer all the parts of cs35l56_component_probe() that require register
access to a work item. This is to prevent a deadlock where
cs35l56_component_probe() is waiting for init_completion to be signaled
but is blocking the code that would signal it.

The deadlock occurs when this is the last component needed for a card.
In that case snd_soc_register_component() directly calls
cs35l56_component_probe() inside the driver probe().
cs35l56_component_probe() is waiting for init_completion but, because the
driver probe() has not returned, the SoundWire bus will not call our
cs35l56_update_status() to report ATTACH state, so init_completion will
not be completed. This is a deadlock until the wait for init_completion
times out.

This fix moves the wait for init_completion, and the code dependent on
that, into a work item that queues dsp_work. Now cs35l56_component_probe()
queues this new work instead of dsp_work and then returns, allowing the
driver probe() to return. The rest of the driver is waiting for dsp_work
to complete to indicate that the amp is now in a usable state, and that is
unchanged. (The extra work was inserted, instead of moving all the code
into dsp_work, to minimize the size of the code change given that this is
a bugfix).

One thing to note about this is that ASoC can call bias_level(STANDBY)
during component_probe(). The flush of dsp_work was previously done
at the STANDBY stage but that would lead to the same deadlock. To avoid
this, the flush_work() has been moved to the PREPARE stage. This has a
benefit that, because it happens later during boot, there is a better
chance that firmware download has already completed.

But note this comment from include/sound/soc-dapm.h:

* @STANDBY: Low power standby state when no playback/capture operations are
*           in progress. NOTE: The transition time between STANDBY and ON
*           should be as fast as possible and no longer than 10ms.

A 10 ms transition time cannot be guaranteed when:
   this is the first PREPARE after probe() or system_resume
   AND
   firmware download has not yet completed.

However, there is a good chance that firmware download has already
completed, and this is better than a deadlock that results in no sound card
at all.

BACKGROUND
==========

The probe() of a SoundWire driver does not have access to device registers.
Register access is only possible after probe() returns and the peripheral
has been enumerated on the bus. But anything that could return EPROBE_DEFER
must be called from probe(), which includes snd_soc_register_component().

This creates a strange state where the component has been created but it is
not yet possible to access registers. Fortunately, the registers for DAPM
widgets that ASoC might attempt to read during component probing all have
defaults and can be accessed while the regmap is still in cache-only mode.
But eventually the driver needs access to the registers.

The way this was handled was a 3-step process:

Step 1:
  cs35l56_component_probe() waits on init_completion.

  cs35l56_sdw_update_status(ATTACH) calls cs35l56_init() which completes
  init_completion.

Step 2:
  cs35l56_component_probe() queues dsp_work to download the DSP firmware.

Step 3:
  cs35l56_bias_level() flushes dsp_work when the bias level changes
  OFF -> STANDBY.

Step 1 is where the deadlock can occur, as described above, because the
core SoundWire subsystem only calls the update_status() callback after
driver probe() has returned.

Fixes: e49611252900 ("ASoC: cs35l56: Add driver for Cirrus Logic CS35L56")
Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
Changes in V2:
- Fixed missing flush_work() in cs35l56_system_suspend() by calling
  cs35l56_wait_dsp_ready() which already does the correct flushes.
  
 sound/soc/codecs/cs35l56.c | 51 ++++++++++++++++++++++++--------------
 sound/soc/codecs/cs35l56.h |  1 +
 2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/sound/soc/codecs/cs35l56.c b/sound/soc/codecs/cs35l56.c
index 0b7b080939a18..00a730d047780 100644
--- a/sound/soc/codecs/cs35l56.c
+++ b/sound/soc/codecs/cs35l56.c
@@ -86,6 +86,7 @@ static int cs35l56_dsp_event(struct snd_soc_dapm_widget *w,
 static void cs35l56_wait_dsp_ready(struct cs35l56_private *cs35l56)
 {
 	/* Wait for patching to complete */
+	flush_work(&cs35l56->deferred_component_init_work);
 	flush_work(&cs35l56->dsp_work);
 }
 
@@ -1359,6 +1360,30 @@ VISIBLE_IF_KUNIT int cs35l56_set_fw_name(struct snd_soc_component *component)
 }
 EXPORT_SYMBOL_IF_KUNIT(cs35l56_set_fw_name);
 
+static void cs35l56_deferred_component_init_work(struct work_struct *work)
+{
+	struct cs35l56_private *cs35l56 = container_of(work,
+						       struct cs35l56_private,
+						       deferred_component_init_work);
+	int ret;
+
+	if (!wait_for_completion_timeout(&cs35l56->init_completion,
+					 msecs_to_jiffies(5000))) {
+		dev_err(cs35l56->base.dev, "%s: init_completion timed out\n", __func__);
+		return;
+	}
+
+	ret = cs35l56_set_fw_name(cs35l56->component);
+	if (ret)
+		return;
+
+	ret = cs35l56_set_fw_suffix(cs35l56);
+	if (ret)
+		return;
+
+	queue_work(cs35l56->dsp_wq, &cs35l56->dsp_work);
+}
+
 static int _cs35l56_component_probe(struct snd_soc_component *component)
 {
 	struct snd_soc_dapm_context *dapm = snd_soc_component_to_dapm(component);
@@ -1368,24 +1393,11 @@ static int _cs35l56_component_probe(struct snd_soc_component *component)
 
 	BUILD_BUG_ON(ARRAY_SIZE(cs35l56_tx_input_texts) != ARRAY_SIZE(cs35l56_tx_input_values));
 
-	if (!wait_for_completion_timeout(&cs35l56->init_completion,
-					 msecs_to_jiffies(5000))) {
-		dev_err(cs35l56->base.dev, "%s: init_completion timed out\n", __func__);
-		return -ENODEV;
-	}
-
 	cs35l56->dsp.part = kasprintf(GFP_KERNEL, "cs35l%02x", cs35l56->base.type);
 	if (!cs35l56->dsp.part)
 		return -ENOMEM;
 
 	cs35l56->component = component;
-	ret = cs35l56_set_fw_name(component);
-	if (ret)
-		return ret;
-
-	ret = cs35l56_set_fw_suffix(cs35l56);
-	if (ret)
-		return ret;
 
 	wm_adsp2_component_probe(&cs35l56->dsp, component);
 
@@ -1433,7 +1445,7 @@ static int _cs35l56_component_probe(struct snd_soc_component *component)
 	if (IS_ENABLED(CONFIG_SND_SOC_CS35L56_CAL_DEBUGFS))
 		cs35l56_create_cal_debugfs(&cs35l56->base, &cs35l56_cal_debugfs_fops);
 
-	queue_work(cs35l56->dsp_wq, &cs35l56->dsp_work);
+	queue_work(cs35l56->dsp_wq, &cs35l56->deferred_component_init_work);
 
 	return 0;
 }
@@ -1442,6 +1454,7 @@ static void cs35l56_component_remove(struct snd_soc_component *component)
 {
 	struct cs35l56_private *cs35l56 = snd_soc_component_get_drvdata(component);
 
+	cancel_work_sync(&cs35l56->deferred_component_init_work);
 	cancel_work_sync(&cs35l56->dsp_work);
 
 	cs35l56_remove_cal_debugfs(&cs35l56->base);
@@ -1478,12 +1491,12 @@ static int cs35l56_set_bias_level(struct snd_soc_component *component,
 	struct snd_soc_dapm_context *dapm = snd_soc_component_to_dapm(component);
 
 	switch (level) {
-	case SND_SOC_BIAS_STANDBY:
+	case SND_SOC_BIAS_PREPARE:
 		/*
 		 * Wait for patching to complete when transitioning from
-		 * BIAS_OFF to BIAS_STANDBY
+		 * BIAS_STANDBY.
 		 */
-		if (snd_soc_dapm_get_bias_level(dapm) == SND_SOC_BIAS_OFF)
+		if (snd_soc_dapm_get_bias_level(dapm) == SND_SOC_BIAS_STANDBY)
 			cs35l56_wait_dsp_ready(cs35l56);
 
 		break;
@@ -1530,7 +1543,7 @@ int cs35l56_system_suspend(struct device *dev)
 	dev_dbg(dev, "system_suspend\n");
 
 	if (cs35l56->component)
-		flush_work(&cs35l56->dsp_work);
+		cs35l56_wait_dsp_ready(cs35l56);
 
 	/*
 	 * The interrupt line is normally shared, but after we start suspending
@@ -1689,6 +1702,8 @@ static int cs35l56_dsp_init(struct cs35l56_private *cs35l56)
 	if (!cs35l56->dsp_wq)
 		return -ENOMEM;
 
+	INIT_WORK(&cs35l56->deferred_component_init_work,
+		  cs35l56_deferred_component_init_work);
 	INIT_WORK(&cs35l56->dsp_work, cs35l56_dsp_work);
 
 	dsp = &cs35l56->dsp;
diff --git a/sound/soc/codecs/cs35l56.h b/sound/soc/codecs/cs35l56.h
index 9acd2e7e17c93..1ff6ffdc12e26 100644
--- a/sound/soc/codecs/cs35l56.h
+++ b/sound/soc/codecs/cs35l56.h
@@ -32,6 +32,7 @@ struct sdw_slave;
 struct cs35l56_private {
 	struct wm_adsp dsp; /* must be first member */
 	struct cs35l56_base base;
+	struct work_struct deferred_component_init_work;
 	struct work_struct dsp_work;
 	struct workqueue_struct *dsp_wq;
 	struct snd_soc_component *component;
-- 
2.47.3