[PATCH v2 0/6] ASoC: Intel: Skylake: Fix module removal

[PATCH v2 0/6] ASoC: Intel: Skylake: Fix module removal

[Vinod Koul]

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(-)

-- 
1.9.1

[PATCH v2 1/6] ALSA: hda: use list macro for parsing on cleanup

[Vinod Koul]

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);
-- 
1.9.1

[PATCH v2 2/6] ASoC: Intel: Skylake: free codec objects on removal

[Vinod Koul]

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);
-- 
1.9.1

[PATCH v2 3/6] ASoC: Intel: Skylake: Freeup properly on skl_dsp_free

[Vinod Koul]

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);
-- 
1.9.1

[PATCH v2 4/6] ASoC: Intel: Skylake: Unmap the address last

[Vinod Koul]

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);
 
-- 
1.9.1

[PATCH v2 5/6] ASoC: Intel: Skylake: Call i915 exit last

[Vinod Koul]

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);
-- 
1.9.1

[PATCH v2 6/6] ASoC: Intel: Skylake: remove call to pci_dev_put

[Vinod Koul]

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);
-- 
1.9.1

Re: [PATCH v2 6/6] ASoC: Intel: Skylake: remove call to pci_dev_put

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 224 bytes --]

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()?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Applied "ASoC: Intel: Skylake: free codec objects on removal" to the asoc tree

[Mark Brown]

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);
-- 
2.7.0

Re: [PATCH v2 6/6] ASoC: Intel: Skylake: remove call to pci_dev_put

[Vinod Koul]

[-- Attachment #1.1: Type: text/plain, Size: 406 bytes --]

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 :)


-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 6/6] ASoC: Intel: Skylake: remove call to pci_dev_put

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 522 bytes --]

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?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 6/6] ASoC: Intel: Skylake: remove call to pci_dev_put

[Takashi Iwai]

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

Re: [PATCH v2 6/6] ASoC: Intel: Skylake: remove call to pci_dev_put

[Vinod Koul]

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?

-- 
~Vinod

Re: [PATCH v2 6/6] ASoC: Intel: Skylake: remove call to pci_dev_put

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 466 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 6/6] ASoC: Intel: Skylake: remove call to pci_dev_put

[Vinod Koul]

[-- Attachment #1.1: Type: text/plain, Size: 622 bytes --]

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 !

-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Applied "ASoC: Intel: Skylake: remove call to pci_dev_put" to the asoc tree

[Mark Brown]

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);
-- 
2.7.0