[PATCH 0/3] ASoC: Intel/SOF: simplify S3 resume flows
All Intel drivers for cAVS platforms contain a sequence for S3 resume which doesn't seem justified nor necessary. Forensic Git investigation in internal repositories did not provide any rationale for the implementation, and tests show no impact when those sequences are removed.
This sequence was identified as problematic during a large HDaudio cleanup where all programming sequences were revisited before extensions are added.
Pierre-Louis Bossart (3): ASoC: Intel: Skylake: simplify S3 resume flows ASoC: Intel: avs: simplify S3 resume flows ASoC: SOF: Intel: hda-dsp: simplify S3 resume flows
sound/soc/intel/avs/core.c | 11 ----------- sound/soc/intel/skylake/skl.c | 9 --------- sound/soc/sof/intel/hda-dsp.c | 14 -------------- 3 files changed, 34 deletions(-)
Commit cce6c149eba3a ("ASoC: Intel: Skylake: add link management") added a perfectly logical/symmetrical link handling for 'suspend_active' aka S0ix
However that commit also added a less obvious part, where during S3 resume the code will "turn off the links which are off before suspend" as well as stop the cmd_io which is not started.
This sequence looks completely unnecessary and possibly wrong, remove it.
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 Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/intel/skylake/skl.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index bbba2df33aaf0..1cfdb04f589fe 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -387,15 +387,6 @@ static int skl_resume(struct device *dev) snd_hdac_bus_init_cmd_io(bus); } else { ret = _skl_resume(bus); - - /* turn off the links which are off before suspend */ - list_for_each_entry(hlink, &bus->hlink_list, list) { - if (!hlink->ref_count) - snd_hdac_ext_bus_link_power_down(hlink); - } - - if (!bus->cmd_dma_state) - snd_hdac_bus_stop_cmd_io(bus); }
return ret;
The same code was directly copied from the skylake driver where it was already questionable. Remove and simplify the flow.
Tested-by: Cezary Rojewski cezary.rojewski@intel.com 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 Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/intel/avs/core.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/sound/soc/intel/avs/core.c b/sound/soc/intel/avs/core.c index bb0719c58ca49..58a51361c4b11 100644 --- a/sound/soc/intel/avs/core.c +++ b/sound/soc/intel/avs/core.c @@ -580,7 +580,6 @@ static int __maybe_unused avs_suspend_common(struct avs_dev *adev) static int __maybe_unused avs_resume_common(struct avs_dev *adev, bool purge) { struct hdac_bus *bus = &adev->base.core; - struct hdac_ext_link *hlink; int ret;
snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, true); @@ -595,16 +594,6 @@ static int __maybe_unused avs_resume_common(struct avs_dev *adev, bool purge) return ret; }
- /* turn off the links that were off before suspend */ - list_for_each_entry(hlink, &bus->hlink_list, list) { - if (!hlink->ref_count) - snd_hdac_ext_bus_link_power_down(hlink); - } - - /* check dma status and clean up CORB/RIRB buffers */ - if (!bus->cmd_dma_state) - snd_hdac_bus_stop_cmd_io(bus); - return 0; }
The flow contains surprising parts that seem to have been inspired by the Skylake driver.
During a resume from S3, the state of the links prior to suspend should not matter. One would have to assume that the links are powered down anyways prior to suspend, and there is also no specific code that turns links on unconditionally on resume.
Likewise, the snd_hdac_init_chip() routine will start the cmd_io handling, and it's not clear why we should care about the state of the DMA prior to suspend.
This patch removes these two sequences to simplify the flow.
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 Reviewed-by: Péter Ujfalusi peter.ujfalusi@linux.intel.com --- sound/soc/sof/intel/hda-dsp.c | 14 -------------- 1 file changed, 14 deletions(-)
diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index 3c76f843454b6..799c50fe24dab 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -677,10 +677,6 @@ static int hda_suspend(struct snd_sof_dev *sdev, bool runtime_suspend)
static int hda_resume(struct snd_sof_dev *sdev, bool runtime_resume) { -#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) - struct hdac_bus *bus = sof_to_bus(sdev); - struct hdac_ext_link *hlink = NULL; -#endif int ret;
/* display codec must be powered before link reset */ @@ -707,16 +703,6 @@ static int hda_resume(struct snd_sof_dev *sdev, bool runtime_resume) if (sdev->system_suspend_target == SOF_SUSPEND_NONE) hda_codec_jack_check(sdev); } - - /* turn off the links that were off before suspend */ - list_for_each_entry(hlink, &bus->hlink_list, list) { - if (!hlink->ref_count) - snd_hdac_ext_bus_link_power_down(hlink); - } - - /* check dma status and clean up CORB/RIRB buffers */ - if (!bus->cmd_dma_state) - snd_hdac_bus_stop_cmd_io(bus); #endif
/* enable ppcap interrupt */
On 2022-10-17 10:49 PM, Pierre-Louis Bossart wrote:
All Intel drivers for cAVS platforms contain a sequence for S3 resume which doesn't seem justified nor necessary. Forensic Git investigation in internal repositories did not provide any rationale for the implementation, and tests show no impact when those sequences are removed.
This sequence was identified as problematic during a large HDaudio cleanup where all programming sequences were revisited before extensions are added.
This is a good finding as discussed earlier in the review. Tested-by is already there by the series-wide reviewed-by won't hurt:
Reviewed-by: Cezary Rojewski cezary.rojewski@intel.com
On Mon, 17 Oct 2022 15:49:43 -0500, Pierre-Louis Bossart wrote:
All Intel drivers for cAVS platforms contain a sequence for S3 resume which doesn't seem justified nor necessary. Forensic Git investigation in internal repositories did not provide any rationale for the implementation, and tests show no impact when those sequences are removed.
This sequence was identified as problematic during a large HDaudio cleanup where all programming sequences were revisited before extensions are added.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/3] ASoC: Intel: Skylake: simplify S3 resume flows commit: fac33cb5c12c58e031a5e2f8e3e8c7de8604a764 [2/3] ASoC: Intel: avs: simplify S3 resume flows commit: 8e1ae6f62c7e8f904949e9c60a4a38715c8c0aff [3/3] ASoC: SOF: Intel: hda-dsp: simplify S3 resume flows commit: 9f68d6e64f51bf62f8d2f7d82a425470e9aa3b24
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 (3)
-
Cezary Rojewski -
Mark Brown -
Pierre-Louis Bossart