| Message ID | 20221209114529.3909192-1-kai.vehmanen@linux.intel.com |
|---|---|
| Headers | show |
| Series | ASoC: SOF: remove unregister calls from shutdown | expand |
Hi kai nit: I think you have an extra " at the end of the subject Thanks for the patch! On Fri, 9 Dec 2022 at 12:46, Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote: > > If system shutdown has not been completed cleanly, it is possible the > DMA stream shutdown has not been done, or was not clean. > > If this is the case, Intel TGL/ADL HDA platforms may fail to shutdown > cleanly due to pending HDA DMA transactions. To avoid this, detect this > scenario in the shutdown callback, and perform an additional controller > reset. This has been tested to unblock S5 entry if this condition is > hit. > > Co-developed-by: Archana Patni <archana.patni@intel.com> > Signed-off-by: Archana Patni <archana.patni@intel.com> > Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> > Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com> > Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Tested-by: Ricardo Ribalda <ribalda@chromium.org> > --- > sound/soc/sof/intel/hda-dsp.c | 72 +++++++++++++++++++++++++++++++++++ > sound/soc/sof/intel/hda.h | 1 + > sound/soc/sof/intel/tgl.c | 2 +- > 3 files changed, 74 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c > index 5fa29df54b42..b4eacae8564c 100644 > --- a/sound/soc/sof/intel/hda-dsp.c > +++ b/sound/soc/sof/intel/hda-dsp.c > @@ -878,6 +878,78 @@ int hda_dsp_suspend(struct snd_sof_dev *sdev, u32 target_state) > return snd_sof_dsp_set_power_state(sdev, &target_dsp_state); > } > > +static unsigned int hda_dsp_check_for_dma_streams(struct snd_sof_dev *sdev) > +{ > + struct hdac_bus *bus = sof_to_bus(sdev); > + struct hdac_stream *s; > + unsigned int active_streams = 0; > + int sd_offset; > + u32 val; > + > + list_for_each_entry(s, &bus->stream_list, list) { > + sd_offset = SOF_STREAM_SD_OFFSET(s); > + val = snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, > + sd_offset); > + if (val & SOF_HDA_SD_CTL_DMA_START) > + active_streams |= BIT(s->index); > + } > + > + return active_streams; > +} > + > +static int hda_dsp_s5_quirk(struct snd_sof_dev *sdev) > +{ > + int ret; > + > + /* > + * Do not assume a certain timing between the prior > + * suspend flow, and running of this quirk function. > + * This is needed if the controller was just put > + * to reset before calling this function. > + */ > + usleep_range(500, 1000); > + > + /* > + * Take controller out of reset to flush DMA > + * transactions. > + */ > + ret = hda_dsp_ctrl_link_reset(sdev, false); > + if (ret < 0) > + return ret; > + > + usleep_range(500, 1000); > + > + /* Restore state for shutdown, back to reset */ > + ret = hda_dsp_ctrl_link_reset(sdev, true); > + if (ret < 0) > + return ret; > + > + return ret; > +} > + > +int hda_dsp_shutdown_dma_flush(struct snd_sof_dev *sdev) > +{ > + unsigned int active_streams; > + int ret, ret2; > + > + /* check if DMA cleanup has been successful */ > + active_streams = hda_dsp_check_for_dma_streams(sdev); > + > + sdev->system_suspend_target = SOF_SUSPEND_S3; > + ret = snd_sof_suspend(sdev->dev); > + > + if (active_streams) { > + dev_warn(sdev->dev, > + "There were active DSP streams (%#x) at shutdown, trying to recover\n", > + active_streams); > + ret2 = hda_dsp_s5_quirk(sdev); > + if (ret2 < 0) > + dev_err(sdev->dev, "shutdown recovery failed (%d)\n", ret2); > + } > + > + return ret; > +} > + > int hda_dsp_shutdown(struct snd_sof_dev *sdev) > { > sdev->system_suspend_target = SOF_SUSPEND_S3; > diff --git a/sound/soc/sof/intel/hda.h b/sound/soc/sof/intel/hda.h > index 022ce80968dd..caccaf8fba9c 100644 > --- a/sound/soc/sof/intel/hda.h > +++ b/sound/soc/sof/intel/hda.h > @@ -592,6 +592,7 @@ int hda_dsp_resume(struct snd_sof_dev *sdev); > int hda_dsp_runtime_suspend(struct snd_sof_dev *sdev); > int hda_dsp_runtime_resume(struct snd_sof_dev *sdev); > int hda_dsp_runtime_idle(struct snd_sof_dev *sdev); > +int hda_dsp_shutdown_dma_flush(struct snd_sof_dev *sdev); > int hda_dsp_shutdown(struct snd_sof_dev *sdev); > int hda_dsp_set_hw_params_upon_resume(struct snd_sof_dev *sdev); > void hda_dsp_dump(struct snd_sof_dev *sdev, u32 flags); > diff --git a/sound/soc/sof/intel/tgl.c b/sound/soc/sof/intel/tgl.c > index 30f2f49ee149..58ac3a46e6a7 100644 > --- a/sound/soc/sof/intel/tgl.c > +++ b/sound/soc/sof/intel/tgl.c > @@ -60,7 +60,7 @@ int sof_tgl_ops_init(struct snd_sof_dev *sdev) > memcpy(&sof_tgl_ops, &sof_hda_common_ops, sizeof(struct snd_sof_dsp_ops)); > > /* probe/remove/shutdown */ > - sof_tgl_ops.shutdown = hda_dsp_shutdown; > + sof_tgl_ops.shutdown = hda_dsp_shutdown_dma_flush; > > if (sdev->pdata->ipc_type == SOF_IPC) { > /* doorbell */ > -- > 2.38.1 >
On Fri, 09 Dec 2022 13:45:27 +0200, Kai Vehmanen wrote: > This patchset is an alternative solution to problems > reported by Ricardo Ribalda <ribalda@chromium.org> and > Zhen Ni <nizhen@uniontech.com>, as discussed in > > - "[PATCH] ALSA: core: Fix deadlock when shutdown a frozen userspace" > https://mailman.alsa-project.org/pipermail/alsa-devel/2022-November/209248.html > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/2] ASoC: SOF: Intel: pci-tgl: unblock S5 entry if DMA stop has failed" commit: 2aa2a5ead0ee0a358bf80a2984a641d1bf2adc2a [2/2] ASoC: SOF: Revert: "core: unregister clients and machine drivers in .shutdown" commit: 44fda61d2bcfb74a942df93959e083a4e8eff75f 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
Hi, this is your Linux kernel regression tracker. On 14.12.22 14:32, Mark Brown wrote: > On Fri, 09 Dec 2022 13:45:27 +0200, Kai Vehmanen wrote: >> This patchset is an alternative solution to problems >> reported by Ricardo Ribalda <ribalda@chromium.org> and >> Zhen Ni <nizhen@uniontech.com>, as discussed in >> >> - "[PATCH] ALSA: core: Fix deadlock when shutdown a frozen userspace" >> https://mailman.alsa-project.org/pipermail/alsa-devel/2022-November/209248.html >> >> [...] > > Applied to > > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next > > Thanks! > > [1/2] ASoC: SOF: Intel: pci-tgl: unblock S5 entry if DMA stop has failed" > commit: 2aa2a5ead0ee0a358bf80a2984a641d1bf2adc2a > [2/2] ASoC: SOF: Revert: "core: unregister clients and machine drivers in .shutdown" > commit: 44fda61d2bcfb74a942df93959e083a4e8eff75f > > 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. > [...] I noticed a regression report in bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=216820 ``` > My laptop started to hang on hibernation (sleep and shutdown are > fine). I bisected it to commit 83bfc7e793b555291785136c3ae86abcdc046887, > which appears to be related to ALSA. ``` That's a commit the second patch from this series reverts. To my untrained eyes it thus looks a lot like these change will resolve the reported issue, which made me wonder: * these patches afaics are not yet in mainline, is the plan to still send it this cycle? * there are no "CC: <stable..." tags in these patches. Is the plan to manually ask for a backport? Or how can we get the regression fixed in older releases? Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) P.S.: As the Linux kernel's regression tracker I deal with a lot of reports and sometimes miss something important when writing mails like this. If that's the case here, don't hesitate to tell me in a public reply, it's in everyone's interest to set the public record straight.
On Mon, Dec 19, 2022 at 10:41:41AM +0100, Thorsten Leemhuis wrote: > * there are no "CC: <stable..." tags in these patches. Is the plan to > manually ask for a backport? Or how can we get the regression fixed in > older releases? Speak to the stable maintainers I guess, or hope their bot picks the commits up.
Hi, On Mon, 19 Dec 2022, Mark Brown wrote: > On Mon, Dec 19, 2022 at 10:41:41AM +0100, Thorsten Leemhuis wrote: > > > * there are no "CC: <stable..." tags in these patches. Is the plan to > > manually ask for a backport? Or how can we get the regression fixed in > > older releases? > > Speak to the stable maintainers I guess, or hope their bot picks the > commits up. thanks Thorsten for the notice. These patches do lack the "Fixes:" tag, so it's possible the bots will not pick these up. I can follow up and send these to stable if this does not happen. Br, Kai
On Tue, Dec 20, 2022 at 01:41:01PM +0200, Kai Vehmanen wrote: > Hi, > > On Mon, 19 Dec 2022, Mark Brown wrote: > > > On Mon, Dec 19, 2022 at 10:41:41AM +0100, Thorsten Leemhuis wrote: > > > > > * there are no "CC: <stable..." tags in these patches. Is the plan to > > > manually ask for a backport? Or how can we get the regression fixed in > > > older releases? > > > > Speak to the stable maintainers I guess, or hope their bot picks the > > commits up. > > thanks Thorsten for the notice. These patches do lack the "Fixes:" tag, so > it's possible the bots will not pick these up. I can follow up and send > these to stable if this does not happen. "Fixes:" guarantees nothing, please NEVER rely on that. As per the kernel documentation for the last 18+ years, you have to tag a commit with the "Cc: stable@..." tag to ensure that it gets picked up properly. thanks, greg k-h
On 20.12.22 12:41, Kai Vehmanen wrote: > > On Mon, 19 Dec 2022, Mark Brown wrote: > >> On Mon, Dec 19, 2022 at 10:41:41AM +0100, Thorsten Leemhuis wrote: >> >>> * there are no "CC: <stable..." tags in these patches. Is the plan to >>> manually ask for a backport? Or how can we get the regression fixed in >>> older releases? >> >> Speak to the stable maintainers I guess, or hope their bot picks the >> commits up. > > thanks Thorsten for the notice. These patches do lack the "Fixes:" tag, so > it's possible the bots will not pick these up. I can follow up and send > these to stable if this does not happen. Thanks, that would be great, I try to stay out of that business, as actual developers of the code in question are in the best position to judge and handle things like this. Ciao, Thorsten
This patchset is an alternative solution to problems reported by Ricardo Ribalda <ribalda@chromium.org> and Zhen Ni <nizhen@uniontech.com>, as discussed in - "[PATCH] ALSA: core: Fix deadlock when shutdown a frozen userspace" https://mailman.alsa-project.org/pipermail/alsa-devel/2022-November/209248.html - "[PATCH] ASoc: SOF: Fix sof-audio-pci-intel-tgl shutdown timeout during hibernation" https://mailman.alsa-project.org/pipermail/alsa-devel/2022-December/209685.html It was raised by Oliver Neukum <oneukum@suse.com> that kernel should not let user-space stall the shutdown process in any scenario and the unregister call in current SOF shutdown code is not right. This series reverts the ASoC SOF patch to unregister clients and the machine drivers at shutdown. To avoid bringing back old bugs, the series includes a patch to fix S5 entry on certain Intel platforms. Kai Vehmanen (2): ASoC: SOF: Intel: pci-tgl: unblock S5 entry if DMA stop has failed" ASoC: SOF: Revert: "core: unregister clients and machine drivers in .shutdown" sound/soc/sof/core.c | 9 ----- sound/soc/sof/intel/hda-dsp.c | 72 +++++++++++++++++++++++++++++++++++ sound/soc/sof/intel/hda.h | 1 + sound/soc/sof/intel/tgl.c | 2 +- 4 files changed, 74 insertions(+), 10 deletions(-) base-commit: e85b1f5a9769ac30f4d2f6fb1cdcd9570c38e0c1