[alsa-devel] [PATCH v2 0/6] ASoC: Intel: Skylake: Fix module removal
Skylake driver were crashing on module removal when we run "alsa reload".
The first issues was caused by component framework 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 ensuring driver does proper cleanup and i915 dependency
Changes in v2: - Add acked by Takashi - Remove codec destructor wrapper
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 | 17 ++++++++++------- 3 files changed, 17 insertions(+), 10 deletions(-)
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 --- 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 | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index ab5e25aaeee3..292d51db9a22 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -725,6 +725,10 @@ static void skl_remove(struct pci_dev *pci) if (pci_dev_run_wake(pci)) pm_runtime_get_noresume(&pci->dev); pci_dev_put(pci); + + /* codec removal, invoke bus_device_remove */ + snd_hdac_ext_bus_device_remove(ebus); + skl_platform_unregister(&pci->dev); skl_free_dsp(skl); skl_machine_device_unregister(skl);
The patch
ASoC: Intel: Skylake: free codec objects on removal
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 7373f481dc4098a844a756201e98341bc56baaa2 Mon Sep 17 00:00:00 2001
From: Vinod Koul vinod.koul@intel.com Date: Tue, 15 Mar 2016 16:39:24 +0530 Subject: [PATCH] ASoC: Intel: Skylake: free codec objects on removal
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 Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index ab5e25aaeee3..292d51db9a22 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -725,6 +725,10 @@ static void skl_remove(struct pci_dev *pci) if (pci_dev_run_wake(pci)) pm_runtime_get_noresume(&pci->dev); pci_dev_put(pci); + + /* codec removal, invoke bus_device_remove */ + snd_hdac_ext_bus_device_remove(ebus); + skl_platform_unregister(&pci->dev); skl_free_dsp(skl); skl_machine_device_unregister(skl);
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);
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 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 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 72971dc55c52..07d9bc1cf3cb 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -724,7 +724,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);
/* codec removal, invoke bus_device_remove */ snd_hdac_ext_bus_device_remove(ebus);
On Tue, Mar 15, 2016 at 04:39:28PM +0530, Vinod Koul wrote:
In driver remove we call pci_dev_put(), that is not required as we never call pci_dev_get() so remove that.
Why is the fix for this not to call pci_dev_get()?
On Wed, Mar 16, 2016 at 10:08:29AM +0000, Mark Brown wrote:
On Tue, Mar 15, 2016 at 04:39:28PM +0530, Vinod Koul wrote:
In driver remove we call pci_dev_put(), that is not required as we never call pci_dev_get() so remove that.
Why is the fix for this not to call pci_dev_get()?
Why do I need either, I see no reason why driver should be doing this, so removed :)
On Wed, Mar 16, 2016 at 04:22:44PM +0530, Vinod Koul wrote:
On Wed, Mar 16, 2016 at 10:08:29AM +0000, Mark Brown wrote:
Why is the fix for this not to call pci_dev_get()?
Why do I need either, I see no reason why driver should be doing this, so removed :)
Well, the PCI documentation says that drivers are expected to record a reference to their devices in probe(). This is a bit unusual given that normally the driver core takes a reference to the device for us but presumably there's some reason for this?
On Wed, 16 Mar 2016 12:03:10 +0100, Mark Brown wrote:
On Wed, Mar 16, 2016 at 04:22:44PM +0530, Vinod Koul wrote:
On Wed, Mar 16, 2016 at 10:08:29AM +0000, Mark Brown wrote:
Why is the fix for this not to call pci_dev_get()?
Why do I need either, I see no reason why driver should be doing this, so removed :)
Well, the PCI documentation says that drivers are expected to record a reference to their devices in probe(). This is a bit unusual given that normally the driver core takes a reference to the device for us but presumably there's some reason for this?
Maybe the document is obsoleted. The PCI core, at least the probe / remove via the normal PCI bus, takes pci_dev_get() and pci_dev_put() already there.
Takashi
On Wed, Mar 16, 2016 at 12:35:08PM +0100, Takashi Iwai wrote:
On Wed, 16 Mar 2016 12:03:10 +0100, Mark Brown wrote:
On Wed, Mar 16, 2016 at 04:22:44PM +0530, Vinod Koul wrote:
On Wed, Mar 16, 2016 at 10:08:29AM +0000, Mark Brown wrote:
Why is the fix for this not to call pci_dev_get()?
Why do I need either, I see no reason why driver should be doing this, so removed :)
Well, the PCI documentation says that drivers are expected to record a reference to their devices in probe(). This is a bit unusual given that normally the driver core takes a reference to the device for us but presumably there's some reason for this?
Maybe the document is obsoleted. The PCI core, at least the probe / remove via the normal PCI bus, takes pci_dev_get() and pci_dev_put() already there.
Yes that is my understanding too, that is why we removed this from driver here..
Mark, is this fine now?
On Wed, Mar 16, 2016 at 08:14:05PM +0530, Vinod Koul wrote:
On Wed, Mar 16, 2016 at 12:35:08PM +0100, Takashi Iwai wrote:
Maybe the document is obsoleted. The PCI core, at least the probe / remove via the normal PCI bus, takes pci_dev_get() and pci_dev_put() already there.
Yes that is my understanding too, that is why we removed this from driver here..
Mark, is this fine now?
You might want to put some of this analysis in the changelog.
On Wed, Mar 16, 2016 at 02:57:53PM +0000, Mark Brown wrote:
On Wed, Mar 16, 2016 at 08:14:05PM +0530, Vinod Koul wrote:
On Wed, Mar 16, 2016 at 12:35:08PM +0100, Takashi Iwai wrote:
Maybe the document is obsoleted. The PCI core, at least the probe / remove via the normal PCI bus, takes pci_dev_get() and pci_dev_put() already there.
Yes that is my understanding too, that is why we removed this from driver here..
Mark, is this fine now?
You might want to put some of this analysis in the changelog.
Yup makes sense, v3 on the way then !
The patch
ASoC: Intel: Skylake: remove call to pci_dev_put
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 36e7972c0d3f8819a5d9335c36c5dcd168cd2b72 Mon Sep 17 00:00:00 2001
From: Vinod Koul vinod.koul@intel.com Date: Wed, 16 Mar 2016 21:51:31 +0530 Subject: [PATCH] ASoC: Intel: Skylake: remove call to pci_dev_put
The PCI bus takes pci_dev_get() and pci_dev_put() is also there. So no need for drivers to invoke these. In SKL driver we were calling pci_dev_put() only which is not right, so remove this
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 | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 72971dc55c52..07d9bc1cf3cb 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -724,7 +724,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);
/* codec removal, invoke bus_device_remove */ snd_hdac_ext_bus_device_remove(ebus);
participants (3)
-
Mark Brown -
Takashi Iwai -
Vinod Koul