[alsa-devel] [PATCH 0/6] ASoC: Intel: Skylake: Fix module removal
Skyalke driver were crashing on module removal when we run "alsa reload".
The first issues was caused by component framwork regression which Russell fixed and in in 4.5. In driver there were still some missing order which needs to be fixed. This series first update the list parsing in core, then ensure proper freeup of code objects, and then esnureing driver does proper cleanup and i915 dependency
Vinod Koul (6): ALSA: hda: use list macro for parsing on cleanup ASoC: Intel: Skylake: free codec objects on removal ASoC: Intel: Skylake: Freeup properly on skl_dsp_free ASoC: Intel: Skylake: Unmap the address last ASoC: Intel: Skylake: Call i915 exit last ASoC: Intel: Skylake: remove call to pci_dev_put
sound/hda/ext/hdac_ext_stream.c | 5 ++--- sound/soc/intel/skylake/skl-sst-dsp.c | 5 +++++ sound/soc/intel/skylake/skl.c | 22 +++++++++++++++------- 3 files changed, 22 insertions(+), 10 deletions(-)
Thanks
It is always better to use list_for_each_entry_safe() while doing cleanup. So use this instead of open coding this in list in snd_hdac_stream_free_all()
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/hda/ext/hdac_ext_stream.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c index 023cc4cad5c1..626f3bb24c55 100644 --- a/sound/hda/ext/hdac_ext_stream.c +++ b/sound/hda/ext/hdac_ext_stream.c @@ -104,12 +104,11 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_stream_init_all); */ void snd_hdac_stream_free_all(struct hdac_ext_bus *ebus) { - struct hdac_stream *s; + struct hdac_stream *s, *_s; struct hdac_ext_stream *stream; struct hdac_bus *bus = ebus_to_hbus(ebus);
- while (!list_empty(&bus->stream_list)) { - s = list_first_entry(&bus->stream_list, struct hdac_stream, list); + list_for_each_entry_safe(s, _s, &bus->stream_list, list) { stream = stream_to_hdac_ext_stream(s); snd_hdac_ext_stream_decouple(ebus, stream, false); list_del(&s->list);
On Tue, 15 Mar 2016 10:57:11 +0100, Vinod Koul wrote:
It is always better to use list_for_each_entry_safe() while doing cleanup. So use this instead of open coding this in list in snd_hdac_stream_free_all()
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com
While this change is fine, we shouldn't trust blindly list_for_each_safe() as always safe. It assumes that the list removal is done only for the current item. But it's not always true. The loop in the current code is one of standard idiom in such a case.
In anyway, take my ack when Mark applies it: Acked-by: Takashi Iwai tiwai@suse.de
Takashi
sound/hda/ext/hdac_ext_stream.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c index 023cc4cad5c1..626f3bb24c55 100644 --- a/sound/hda/ext/hdac_ext_stream.c +++ b/sound/hda/ext/hdac_ext_stream.c @@ -104,12 +104,11 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_stream_init_all); */ void snd_hdac_stream_free_all(struct hdac_ext_bus *ebus) {
- struct hdac_stream *s;
- struct hdac_stream *s, *_s; struct hdac_ext_stream *stream; struct hdac_bus *bus = ebus_to_hbus(ebus);
- while (!list_empty(&bus->stream_list)) {
s = list_first_entry(&bus->stream_list, struct hdac_stream, list);
- list_for_each_entry_safe(s, _s, &bus->stream_list, list) { stream = stream_to_hdac_ext_stream(s); snd_hdac_ext_stream_decouple(ebus, stream, false); list_del(&s->list);
-- 1.9.1
On Tue, Mar 15, 2016 at 11:00:53AM +0100, Takashi Iwai wrote:
On Tue, 15 Mar 2016 10:57:11 +0100, Vinod Koul wrote:
It is always better to use list_for_each_entry_safe() while doing cleanup. So use this instead of open coding this in list in snd_hdac_stream_free_all()
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com
While this change is fine, we shouldn't trust blindly list_for_each_safe() as always safe. It assumes that the list removal is done only for the current item. But it's not always true. The loop in the current code is one of standard idiom in such a case.
Yes thanks for the warning :)
In anyway, take my ack when Mark applies it: Acked-by: Takashi Iwai tiwai@suse.de
Will add this for v2
--
~Vinod
The patch
ALSA: hda: use list macro for parsing on cleanup
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
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
From 4a6c5e6a8d29e4d33858227db9179e91aa8a7407 Mon Sep 17 00:00:00 2001
From: Vinod Koul vinod.koul@intel.com Date: Tue, 15 Mar 2016 16:39:23 +0530 Subject: [PATCH] ALSA: hda: use list macro for parsing on cleanup
It is always better to use list_for_each_entry_safe() while doing cleanup. So use this instead of open coding this in list in snd_hdac_stream_free_all()
Signed-off-by: Jeeja KP jeeja.kp@intel.com Acked-by: Takashi Iwai tiwai@suse.de Signed-off-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/hda/ext/hdac_ext_stream.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/hda/ext/hdac_ext_stream.c b/sound/hda/ext/hdac_ext_stream.c index 023cc4cad5c1..626f3bb24c55 100644 --- a/sound/hda/ext/hdac_ext_stream.c +++ b/sound/hda/ext/hdac_ext_stream.c @@ -104,12 +104,11 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_stream_init_all); */ void snd_hdac_stream_free_all(struct hdac_ext_bus *ebus) { - struct hdac_stream *s; + struct hdac_stream *s, *_s; struct hdac_ext_stream *stream; struct hdac_bus *bus = ebus_to_hbus(ebus);
- while (!list_empty(&bus->stream_list)) { - s = list_first_entry(&bus->stream_list, struct hdac_stream, list); + list_for_each_entry_safe(s, _s, &bus->stream_list, list) { stream = stream_to_hdac_ext_stream(s); snd_hdac_ext_stream_decouple(ebus, stream, false); list_del(&s->list);
On driver removal we should ask the core to remove the device objects as well, so invoke snd_hdac_ext_bus_device_remove() in remove.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index ab5e25aaeee3..371df495dd14 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -454,6 +454,14 @@ static int skl_codec_create(struct hdac_ext_bus *ebus) return 0; }
+/* + * codec destructor + */ +static void skl_codecs_remove(struct hdac_ext_bus *ebus) +{ + snd_hdac_ext_bus_device_remove(ebus); +} + static const struct hdac_bus_ops bus_core_ops = { .command = snd_hdac_bus_send_cmd, .get_response = snd_hdac_bus_get_response, @@ -725,6 +733,7 @@ static void skl_remove(struct pci_dev *pci) if (pci_dev_run_wake(pci)) pm_runtime_get_noresume(&pci->dev); pci_dev_put(pci); + skl_codecs_remove(ebus); skl_platform_unregister(&pci->dev); skl_free_dsp(skl); skl_machine_device_unregister(skl);
On Tue, Mar 15, 2016 at 03:27:12PM +0530, Vinod Koul wrote:
+/*
- codec destructor
- */
+static void skl_codecs_remove(struct hdac_ext_bus *ebus) +{
- snd_hdac_ext_bus_device_remove(ebus);
+}
- skl_codecs_remove(ebus);
Why the wrapper here?
On Tue, Mar 15, 2016 at 10:07:43AM +0000, Mark Brown wrote:
On Tue, Mar 15, 2016 at 03:27:12PM +0530, Vinod Koul wrote:
+/*
- codec destructor
- */
+static void skl_codecs_remove(struct hdac_ext_bus *ebus) +{
- snd_hdac_ext_bus_device_remove(ebus);
+}
- skl_codecs_remove(ebus);
Why the wrapper here?
IIRC Shameless copy of old HDA code, but yes I don't think we need this, I will update this one.
Thanks for pointing out.
We are supposed to freeup the Code loader DMA allocation and ensure all interrupts are disabled before we disable dsp cores. So invoke these to ensure DSP shuts down properly.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl-sst-dsp.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-sst-dsp.c b/sound/soc/intel/skylake/skl-sst-dsp.c index a5267e8a96e0..2962ef22fc84 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.c +++ b/sound/soc/intel/skylake/skl-sst-dsp.c @@ -336,6 +336,11 @@ void skl_dsp_free(struct sst_dsp *dsp) skl_ipc_int_disable(dsp);
free_irq(dsp->irq, dsp); + dsp->cl_dev.ops.cl_cleanup_controller(dsp); + skl_cldma_int_disable(dsp); + skl_ipc_op_int_disable(dsp); + skl_ipc_int_disable(dsp); + skl_dsp_disable_core(dsp); } EXPORT_SYMBOL_GPL(skl_dsp_free);
The patch
ASoC: Intel: Skylake: Freeup properly on skl_dsp_free
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
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
From 3f7f8489e25b180cf8de8a3ae3896b3f18fc4aa5 Mon Sep 17 00:00:00 2001
From: Vinod Koul vinod.koul@intel.com Date: Tue, 15 Mar 2016 16:39:25 +0530 Subject: [PATCH] ASoC: Intel: Skylake: Freeup properly on skl_dsp_free
We are supposed to freeup the Code loader DMA allocation and ensure all interrupts are disabled before we disable dsp cores. So invoke these to ensure DSP shuts down properly.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl-sst-dsp.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/sound/soc/intel/skylake/skl-sst-dsp.c b/sound/soc/intel/skylake/skl-sst-dsp.c index a5267e8a96e0..2962ef22fc84 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.c +++ b/sound/soc/intel/skylake/skl-sst-dsp.c @@ -336,6 +336,11 @@ void skl_dsp_free(struct sst_dsp *dsp) skl_ipc_int_disable(dsp);
free_irq(dsp->irq, dsp); + dsp->cl_dev.ops.cl_cleanup_controller(dsp); + skl_cldma_int_disable(dsp); + skl_ipc_op_int_disable(dsp); + skl_ipc_int_disable(dsp); + skl_dsp_disable_core(dsp); } EXPORT_SYMBOL_GPL(skl_dsp_free);
In Skylake destructor we unmap the hardware address and then free links and streams. The stream free accesses hardware to write to registers and predictably causes oops.
So change the order and unmap last in destructor.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 371df495dd14..06887eacffa7 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -316,12 +316,13 @@ static int skl_free(struct hdac_ext_bus *ebus)
if (bus->irq >= 0) free_irq(bus->irq, (void *)bus); - if (bus->remap_addr) - iounmap(bus->remap_addr); - snd_hdac_bus_free_stream_pages(bus); snd_hdac_stream_free_all(ebus); snd_hdac_link_free_all(ebus); + + if (bus->remap_addr) + iounmap(bus->remap_addr); + pci_release_regions(skl->pci); pci_disable_device(skl->pci);
The patch
ASoC: Intel: Skylake: Unmap the address last
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
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
From 077411e5eb8872736fdc5f3e7277719160918dde Mon Sep 17 00:00:00 2001
From: Vinod Koul vinod.koul@intel.com Date: Tue, 15 Mar 2016 16:39:26 +0530 Subject: [PATCH] ASoC: Intel: Skylake: Unmap the address last
In Skylake destructor we unmap the hardware address and then free links and streams. The stream free accesses hardware to write to registers and predictably causes oops.
So change the order and unmap last in destructor.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 292d51db9a22..6e916c3c3a4b 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -316,12 +316,13 @@ static int skl_free(struct hdac_ext_bus *ebus)
if (bus->irq >= 0) free_irq(bus->irq, (void *)bus); - if (bus->remap_addr) - iounmap(bus->remap_addr); - snd_hdac_bus_free_stream_pages(bus); snd_hdac_stream_free_all(ebus); snd_hdac_link_free_all(ebus); + + if (bus->remap_addr) + iounmap(bus->remap_addr); + pci_release_regions(skl->pci); pci_disable_device(skl->pci);
The Skylake driver uses i915 component APIs to talk to display. On remove we should free up by invoking snd_hdac_i915_exit() but that should be last thing in remove routine, so move it to last in skl_free()
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 06887eacffa7..fdf345b1ace5 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -328,6 +328,8 @@ static int skl_free(struct hdac_ext_bus *ebus)
snd_hdac_ext_bus_exit(ebus);
+ if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) + snd_hdac_i915_exit(&ebus->bus); return 0; }
@@ -728,9 +730,6 @@ static void skl_remove(struct pci_dev *pci) if (skl->tplg) release_firmware(skl->tplg);
- if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) - snd_hdac_i915_exit(&ebus->bus); - if (pci_dev_run_wake(pci)) pm_runtime_get_noresume(&pci->dev); pci_dev_put(pci);
The patch
ASoC: Intel: Skylake: Call i915 exit last
has been applied to the asoc tree at
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
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
From 5b2fe89856b2d0faaaea9e4b4b2c4920de7a600c Mon Sep 17 00:00:00 2001
From: Vinod Koul vinod.koul@intel.com Date: Tue, 15 Mar 2016 16:39:27 +0530 Subject: [PATCH] ASoC: Intel: Skylake: Call i915 exit last
The Skylake driver uses i915 component APIs to talk to display. On remove we should free up by invoking snd_hdac_i915_exit() but that should be last thing in remove routine, so move it to last in skl_free()
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 6e916c3c3a4b..72971dc55c52 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -328,6 +328,8 @@ static int skl_free(struct hdac_ext_bus *ebus)
snd_hdac_ext_bus_exit(ebus);
+ if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) + snd_hdac_i915_exit(&ebus->bus); return 0; }
@@ -720,9 +722,6 @@ static void skl_remove(struct pci_dev *pci) if (skl->tplg) release_firmware(skl->tplg);
- if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) - snd_hdac_i915_exit(&ebus->bus); - if (pci_dev_run_wake(pci)) pm_runtime_get_noresume(&pci->dev); pci_dev_put(pci);
In driver remove we call pci_dev_put(), that is not required as we never call pci_dev_get() so remove that.
Signed-off-by: Jeeja KP jeeja.kp@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com --- sound/soc/intel/skylake/skl.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index fdf345b1ace5..c8ab21245177 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -732,7 +732,6 @@ static void skl_remove(struct pci_dev *pci)
if (pci_dev_run_wake(pci)) pm_runtime_get_noresume(&pci->dev); - pci_dev_put(pci); skl_codecs_remove(ebus); skl_platform_unregister(&pci->dev); skl_free_dsp(skl);
participants (3)
-
Mark Brown -
Takashi Iwai -
Vinod Koul