[PATCH 0/3] ASoC: SOF: Fixes for suspend after firmware crash
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(-)
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 */
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;
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...
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...
Good catch. The original patch in sof-dev did the move: https://github.com/thesofproject/linux/commit/62c1ccce6c6514a3ff8590156fbd7f...
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);
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
participants (4)
-
Amadeusz Sławiński
-
Mark Brown
-
Péter Ujfalusi
-
Ranjani Sridharan