[PATCH 0/3] ASoC: SOF: Fixes for suspend after firmware crash

[PATCH 0/3] ASoC: SOF: Fixes for suspend after firmware crash

[Ranjani Sridharan]

This series contains 2 patches to fix device suspend after a firmware
crash and another patch to allow reading the FW state from debugfs.

Curtis Malainey (1):
  ASoC: SOF: Add FW state to debugfs

Ranjani Sridharan (2):
  ASoC: SOF: pm: Set target state earlier
  ASoC: SOF: pm: Always tear down pipelines before DSP suspend

 sound/soc/sof/debug.c | 4 +++-
 sound/soc/sof/pm.c    | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/3] ASoC: SOF: pm: Set target state earlier

[Ranjani Sridharan]

If the DSP crashes before the system suspends, the setting of target state
will be skipped because the firmware state will no longer be
SOF_FW_BOOT_COMPLETE. This leads to the incorrect assumption that the
DSP should suspend to D0I3 instead of suspending to D3. To fix this,
set the target_state before we skip to DSP suspend even when the DSP has
crashed.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Curtis Malainey <cujomalainey@chromium.org>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@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/pm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/sof/pm.c b/sound/soc/sof/pm.c
index df740be645e8..5f88c4a01fa3 100644
--- a/sound/soc/sof/pm.c
+++ b/sound/soc/sof/pm.c
@@ -182,7 +182,7 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
 	const struct sof_ipc_pm_ops *pm_ops = sdev->ipc->ops->pm;
 	const struct sof_ipc_tplg_ops *tplg_ops = sdev->ipc->ops->tplg;
 	pm_message_t pm_state;
-	u32 target_state = 0;
+	u32 target_state = snd_sof_dsp_power_target(sdev);
 	int ret;
 
 	/* do nothing if dsp suspend callback is not set */
@@ -206,7 +206,6 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
 		}
 	}
 
-	target_state = snd_sof_dsp_power_target(sdev);
 	pm_state.event = target_state;
 
 	/* Skip to platform-specific suspend if DSP is entering D0 */
-- 
2.25.1

[PATCH 2/3] ASoC: SOF: pm: Always tear down pipelines before DSP suspend

[Ranjani Sridharan]

When the DSP is suspended while the firmware is in the crashed state, we
skip tearing down the pipelines. This means that the widget reference
counts will not get to reset to 0 before suspend. This will lead to
errors with resuming audio after system resume. To fix this, invoke the
tear_down_all_pipelines op before skipping to DSP suspend.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Curtis Malainey <cujomalainey@chromium.org>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@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/pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/sof/pm.c b/sound/soc/sof/pm.c
index 5f88c4a01fa3..f153881db189 100644
--- a/sound/soc/sof/pm.c
+++ b/sound/soc/sof/pm.c
@@ -192,6 +192,9 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
 	if (runtime_suspend && !sof_ops(sdev)->runtime_suspend)
 		return 0;
 
+	if (tplg_ops && tplg_ops->tear_down_all_pipelines)
+		tplg_ops->tear_down_all_pipelines(sdev, false);
+
 	if (sdev->fw_state != SOF_FW_BOOT_COMPLETE)
 		goto suspend;
 
-- 
2.25.1

Re: [PATCH 2/3] ASoC: SOF: pm: Always tear down pipelines before DSP suspend

[Amadeusz Sławiński]

On 12/15/2022 7:53 PM, Ranjani Sridharan wrote:
> When the DSP is suspended while the firmware is in the crashed state, we
> skip tearing down the pipelines. This means that the widget reference
> counts will not get to reset to 0 before suspend. This will lead to
> errors with resuming audio after system resume. To fix this, invoke the
> tear_down_all_pipelines op before skipping to DSP suspend.
> 
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Reviewed-by: Curtis Malainey <cujomalainey@chromium.org>
> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@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/pm.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/sound/soc/sof/pm.c b/sound/soc/sof/pm.c
> index 5f88c4a01fa3..f153881db189 100644
> --- a/sound/soc/sof/pm.c
> +++ b/sound/soc/sof/pm.c
> @@ -192,6 +192,9 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
>   	if (runtime_suspend && !sof_ops(sdev)->runtime_suspend)
>   		return 0;
>   
> +	if (tplg_ops && tplg_ops->tear_down_all_pipelines)
> +		tplg_ops->tear_down_all_pipelines(sdev, false);
> +
>   	if (sdev->fw_state != SOF_FW_BOOT_COMPLETE)
>   		goto suspend;
>   

Can tplg_ops even be null? Rest of SOF code seems to skip this check and 
only check for callback presence.

Also won't tearing down pipelines few lines later become unnecessary then?
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/sof/pm.c?id=a12a383e59ce486abd719b6bda33c353a3b385e7#n220

Re: [PATCH 2/3] ASoC: SOF: pm: Always tear down pipelines before DSP suspend

[Péter Ujfalusi]

On 16/12/2022 11:06, Amadeusz Sławiński wrote:
> On 12/15/2022 7:53 PM, Ranjani Sridharan wrote:
>> When the DSP is suspended while the firmware is in the crashed state, we
>> skip tearing down the pipelines. This means that the widget reference
>> counts will not get to reset to 0 before suspend. This will lead to
>> errors with resuming audio after system resume. To fix this, invoke the
>> tear_down_all_pipelines op before skipping to DSP suspend.
>>
>> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
>> Reviewed-by: Curtis Malainey <cujomalainey@chromium.org>
>> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@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/pm.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/sound/soc/sof/pm.c b/sound/soc/sof/pm.c
>> index 5f88c4a01fa3..f153881db189 100644
>> --- a/sound/soc/sof/pm.c
>> +++ b/sound/soc/sof/pm.c
>> @@ -192,6 +192,9 @@ static int sof_suspend(struct device *dev, bool
>> runtime_suspend)
>>       if (runtime_suspend && !sof_ops(sdev)->runtime_suspend)
>>           return 0;
>>   +    if (tplg_ops && tplg_ops->tear_down_all_pipelines)
>> +        tplg_ops->tear_down_all_pipelines(sdev, false);
>> +
>>       if (sdev->fw_state != SOF_FW_BOOT_COMPLETE)
>>           goto suspend;
>>   
> 
> Can tplg_ops even be null? Rest of SOF code seems to skip this check and
> only check for callback presence.

We have another series not yet sent which will re-visit these ops and
their optionality. This patch was created on top of that work in SOF's
sof-dev branch.

> Also won't tearing down pipelines few lines later become unnecessary then?
> https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/sof/pm.c?id=a12a383e59ce486abd719b6bda33c353a3b385e7#n220

Good catch. The original patch in sof-dev did the move:
https://github.com/thesofproject/linux/commit/62c1ccce6c6514a3ff8590156fbd7fff9d24586f

-- 
Péter

[PATCH 3/3] ASoC: SOF: Add FW state to debugfs

[Ranjani Sridharan]

From: Curtis Malainey <cujomalainey@chromium.org>

Allow system health detection mechanisms to check the FW state, this
will allow them to check if the FW is in its "crashed" state going
forward to help automatically diagnose driver state.

Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/debug.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sof/debug.c b/sound/soc/sof/debug.c
index d9a3ce7b69e1..ade0507328af 100644
--- a/sound/soc/sof/debug.c
+++ b/sound/soc/sof/debug.c
@@ -353,7 +353,9 @@ int snd_sof_dbg_init(struct snd_sof_dev *sdev)
 			return err;
 	}
 
-	return 0;
+	return snd_sof_debugfs_buf_item(sdev, &sdev->fw_state,
+					sizeof(sdev->fw_state),
+					"fw_state", 0444);
 }
 EXPORT_SYMBOL_GPL(snd_sof_dbg_init);
 
-- 
2.25.1

Re: [PATCH 0/3] ASoC: SOF: Fixes for suspend after firmware crash

[Mark Brown]

On Thu, 15 Dec 2022 10:53:44 -0800, Ranjani Sridharan wrote:
> This series contains 2 patches to fix device suspend after a firmware
> crash and another patch to allow reading the FW state from debugfs.
> 
> Curtis Malainey (1):
>   ASoC: SOF: Add FW state to debugfs
> 
> Ranjani Sridharan (2):
>   ASoC: SOF: pm: Set target state earlier
>   ASoC: SOF: pm: Always tear down pipelines before DSP suspend
> 
> [...]

Applied to

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

Thanks!

[1/3] ASoC: SOF: pm: Set target state earlier
      commit: 6f95eec6fb89e195dbdf30de65553c7fc57d9372
[2/3] ASoC: SOF: pm: Always tear down pipelines before DSP suspend
      commit: d185e0689abc98ef55fb7a7d75aa0c48a0ed5838
[3/3] ASoC: SOF: Add FW state to debugfs
      commit: 9a9134fd56f6ba614ff7b2b3b0bac0bf1d0dc0c9

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