[PATCH v2 00/11] Fix driver reload issues

[PATCH v2 00/11] Fix driver reload issues

[Amadeusz Sławiński]

Hi,

This series of patches introduces fixes to various issues found while
trying to unload all snd* modules and then loading them again. This
allows for modules to be really _modules_ and be unloaded and loaded on
demand, making it easier to develop and test them without constant
system reboots.

There are some fixes in flow, either we don't initialize things before
cleaning them up, clean up in wrong places or don't clean up at all.
Other patches fix memory management problems, mostly things are not
being freed. And finally there is few miscellaneous patches, please
refer to specific patches to see what they do.

This series was tested on SKL, BXT, GLK & KBL.

Changes from previous patchset:
  * followed suggetion by Pierre in "ALSA: hdac: Fix codec name after
machine driver is unloaded and reloaded"
  * dropped patches which were merged

Amadeusz Sławiński (11):
  ASoC: Intel: Skylake: Initialize lists before access so they are safe
    to use
  ALSA: hdac: Fix codec name after machine driver is unloaded and
    reloaded
  ASoC: compress: Fix memory leak from snd_soc_new_compress
  ASoC: Intel: Skylake: Don't return failure on machine driver reload
  ASoC: Intel: Skylake: Remove static table index when parsing topology
  ASoC: Intel: Skylake: Add function to cleanup debugfs interface
  ASoC: Intel: Skylake: Properly cleanup on component removal
  ASoC: Intel: Skylake: Fix NULL ptr dereference when unloading clk dev
  ASoC: Intel: hdac_hdmi: Set ops to NULL on remove
  ASoC: topology: Consolidate how dtexts and dvalues are freed
  ASoC: topology: Consolidate and fix asoc_tplg_dapm_widget_*_create
    flow

 sound/hda/ext/hdac_ext_bus.c           |   8 +-
 sound/soc/codecs/hdac_hdmi.c           |   6 ++
 sound/soc/intel/skylake/skl-debug.c    |   9 ++
 sound/soc/intel/skylake/skl-pcm.c      |  16 ++--
 sound/soc/intel/skylake/skl-ssp-clk.c  |  16 ++--
 sound/soc/intel/skylake/skl-topology.c |  50 ++++++-----
 sound/soc/intel/skylake/skl-topology.h |   2 +
 sound/soc/intel/skylake/skl.c          |   7 +-
 sound/soc/intel/skylake/skl.h          |   5 ++
 sound/soc/soc-compress.c               |  17 ++--
 sound/soc/soc-topology.c               | 114 ++++++++++++-------------
 11 files changed, 136 insertions(+), 114 deletions(-)

-- 
2.17.1

[PATCH v2 01/11] ASoC: Intel: Skylake: Initialize lists before access so they are safe to use

[Amadeusz Sławiński]

If skl_probe_work() was not run driver ends up dereferencing NULL
pointer when operating on lists in skl_platform_unregister().
To fix this initialize lists in skl_create(). Also run
cancel_work_sync() before all cleanup functions, so we don't end up
unnecessarily running probe work.

Easily reproducible with:
while true; do modprobe snd_soc_skl; rmmod snd_soc_skl; done
(with the assumption that relevant drivers are added to blacklist on
system boot)

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c | 3 ---
 sound/soc/intel/skylake/skl.c     | 5 ++++-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 8b7232d3ffee..489ecef311ad 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -1477,9 +1477,6 @@ int skl_platform_register(struct device *dev)
 	struct hdac_bus *bus = dev_get_drvdata(dev);
 	struct skl *skl = bus_to_skl(bus);
 
-	INIT_LIST_HEAD(&skl->ppl_list);
-	INIT_LIST_HEAD(&skl->bind_list);
-
 	skl->dais = kmemdup(skl_platform_dai, sizeof(skl_platform_dai),
 			    GFP_KERNEL);
 	if (!skl->dais) {
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index fc79401cd474..cae97c65eef8 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -430,7 +430,6 @@ static int skl_free(struct hdac_bus *bus)
 
 	snd_hdac_ext_bus_exit(bus);
 
-	cancel_work_sync(&skl->probe_work);
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
 		snd_hdac_display_power(bus, HDA_CODEC_IDX_CONTROLLER, false);
 		snd_hdac_i915_exit(bus);
@@ -859,6 +858,9 @@ static int skl_create(struct pci_dev *pci,
 	hbus = skl_to_hbus(skl);
 	bus = skl_to_bus(skl);
 
+	INIT_LIST_HEAD(&skl->ppl_list);
+	INIT_LIST_HEAD(&skl->bind_list);
+
 #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC)
 	ext_ops = snd_soc_hdac_hda_get_ops();
 #endif
@@ -1108,6 +1110,7 @@ static void skl_remove(struct pci_dev *pci)
 	struct hdac_bus *bus = pci_get_drvdata(pci);
 	struct skl *skl = bus_to_skl(bus);
 
+	cancel_work_sync(&skl->probe_work);
 	release_firmware(skl->tplg);
 
 	pm_runtime_get_noresume(&pci->dev);
-- 
2.17.1

[PATCH v2 02/11] ALSA: hdac: Fix codec name after machine driver is unloaded and reloaded

[Amadeusz Sławiński]

Currently on each driver reload internal counter is being increased. It
causes failure to enumerate driver devices, as they have hardcoded:
.codec_name = "ehdaudio0D2",
As there is currently no devices with multiple hda codecs and there is
currently no established way to reliably differentiate, between them,
always assign bus->idx = 0;

This fixes a problem when we unload and reload machine driver idx gets
incremented, so .codec_name would've needed to be set to "ehdaudio1D2"
after first reload and so on.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/hda/ext/hdac_ext_bus.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c
index a3a113ef5d56..4f9f1d2a2ec5 100644
--- a/sound/hda/ext/hdac_ext_bus.c
+++ b/sound/hda/ext/hdac_ext_bus.c
@@ -85,7 +85,6 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
 			const struct hdac_ext_bus_ops *ext_ops)
 {
 	int ret;
-	static int idx;
 
 	/* check if io ops are provided, if not load the defaults */
 	if (io_ops == NULL)
@@ -96,7 +95,12 @@ int snd_hdac_ext_bus_init(struct hdac_bus *bus, struct device *dev,
 		return ret;
 
 	bus->ext_ops = ext_ops;
-	bus->idx = idx++;
+	/* FIXME:
+	 * Currently only one bus is supported, if there is device with more
+	 * buses, bus->idx should be greater than 0, but there needs to be a
+	 * reliable way to always assign same number.
+	 */
+	bus->idx = 0;
 	bus->cmd_dma_state = true;
 
 	return 0;
-- 
2.17.1

[PATCH v2 03/11] ASoC: compress: Fix memory leak from snd_soc_new_compress

[Amadeusz Sławiński]

Change kzalloc to devm_kzalloc, so compr gets automatically freed when
it's no longer needed.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-compress.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 03d5b9ccd3fc..ddef4ff677ce 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -896,16 +896,14 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 	else
 		direction = SND_COMPRESS_CAPTURE;
 
-	compr = kzalloc(sizeof(*compr), GFP_KERNEL);
+	compr = devm_kzalloc(rtd->card->dev, sizeof(*compr), GFP_KERNEL);
 	if (!compr)
 		return -ENOMEM;
 
 	compr->ops = devm_kzalloc(rtd->card->dev, sizeof(soc_compr_ops),
 				  GFP_KERNEL);
-	if (!compr->ops) {
-		ret = -ENOMEM;
-		goto compr_err;
-	}
+	if (!compr->ops)
+		return -ENOMEM;
 
 	if (rtd->dai_link->dynamic) {
 		snprintf(new_name, sizeof(new_name), "(%s)",
@@ -918,7 +916,7 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 			dev_err(rtd->card->dev,
 				"Compress ASoC: can't create compressed for %s: %d\n",
 				rtd->dai_link->name, ret);
-			goto compr_err;
+			return ret;
 		}
 
 		rtd->pcm = be_pcm;
@@ -954,7 +952,7 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 		dev_err(component->dev,
 			"Compress ASoC: can't create compress for codec %s: %d\n",
 			component->name, ret);
-		goto compr_err;
+		return ret;
 	}
 
 	/* DAPM dai link stream work */
@@ -965,10 +963,7 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
 
 	dev_info(rtd->card->dev, "Compress ASoC: %s <-> %s mapping ok\n",
 		 codec_dai->name, cpu_dai->name);
-	return ret;
 
-compr_err:
-	kfree(compr);
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_new_compress);
-- 
2.17.1

[PATCH v2 04/11] ASoC: Intel: Skylake: Don't return failure on machine driver reload

[Amadeusz Sławiński]

When we unload and reload machine driver, we shouldn't return that we
failed to initialize. This allows to reload machine driver, without
having to unload whole stack.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 489ecef311ad..5e3421cc0feb 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -1418,11 +1418,6 @@ static int skl_platform_soc_probe(struct snd_soc_component *component)
 		if (!ops)
 			return -EIO;
 
-		if (!skl->skl_sst->is_first_boot) {
-			dev_err(component->dev, "DSP reports first boot done!!!\n");
-			return -EIO;
-		}
-
 		/*
 		 * Disable dynamic clock and power gating during firmware
 		 * and library download
-- 
2.17.1

[PATCH v2 05/11] ASoC: Intel: Skylake: Remove static table index when parsing topology

[Amadeusz Sławiński]

Currently when we remove and reload driver we use previous ref_count
value to start iterating over skl->modules which leads to out of table
access. To fix this just inline the function and calculate indexes
everytime we parse UUID token.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/intel/skylake/skl-topology.c | 35 ++++++++++----------------
 1 file changed, 13 insertions(+), 22 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index c69d999d7bf1..44f3b29a7210 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -3477,25 +3477,6 @@ static int skl_tplg_get_int_tkn(struct device *dev,
 	return tkn_count;
 }
 
-static int skl_tplg_get_manifest_uuid(struct device *dev,
-				struct skl *skl,
-				struct snd_soc_tplg_vendor_uuid_elem *uuid_tkn)
-{
-	static int ref_count;
-	struct skl_module *mod;
-
-	if (uuid_tkn->token == SKL_TKN_UUID) {
-		mod = skl->modules[ref_count];
-		memcpy(&mod->uuid, &uuid_tkn->uuid, sizeof(uuid_tkn->uuid));
-		ref_count++;
-	} else {
-		dev_err(dev, "Not an UUID token tkn %d\n", uuid_tkn->token);
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 /*
  * Fill the manifest structure by parsing the tokens based on the
  * type.
@@ -3506,6 +3487,7 @@ static int skl_tplg_get_manifest_tkn(struct device *dev,
 {
 	int tkn_count = 0, ret;
 	int off = 0, tuple_size = 0;
+	u8 uuid_index = 0;
 	struct snd_soc_tplg_vendor_array *array;
 	struct snd_soc_tplg_vendor_value_elem *tkn_elem;
 
@@ -3528,9 +3510,18 @@ static int skl_tplg_get_manifest_tkn(struct device *dev,
 			continue;
 
 		case SND_SOC_TPLG_TUPLE_TYPE_UUID:
-			ret = skl_tplg_get_manifest_uuid(dev, skl, array->uuid);
-			if (ret < 0)
-				return ret;
+			if (array->uuid->token != SKL_TKN_UUID) {
+				dev_err(dev, "Not an UUID token: %d\n",
+					array->uuid->token);
+				return -EINVAL;
+			}
+			if (uuid_index >= skl->nr_modules) {
+				dev_err(dev, "Too many UUID tokens\n");
+				return -EINVAL;
+			}
+			memcpy(&skl->modules[uuid_index++]->uuid,
+			       &array->uuid->uuid,
+			       sizeof(array->uuid->uuid));
 
 			tuple_size += sizeof(*array->uuid);
 			continue;
-- 
2.17.1

[PATCH v2 06/11] ASoC: Intel: Skylake: Add function to cleanup debugfs interface

[Amadeusz Sławiński]

Currently debugfs has no cleanup function. Add skl_debufs_exit function
so we can clean after ourselves properly.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/intel/skylake/skl-debug.c | 9 +++++++++
 sound/soc/intel/skylake/skl.h       | 5 +++++
 2 files changed, 14 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-debug.c b/sound/soc/intel/skylake/skl-debug.c
index 69cbe9eb026b..b9b4a72a4334 100644
--- a/sound/soc/intel/skylake/skl-debug.c
+++ b/sound/soc/intel/skylake/skl-debug.c
@@ -251,3 +251,12 @@ struct skl_debug *skl_debugfs_init(struct skl *skl)
 	debugfs_remove_recursive(d->fs);
 	return NULL;
 }
+
+void skl_debugfs_exit(struct skl *skl)
+{
+	struct skl_debug *d = skl->debugfs;
+
+	debugfs_remove_recursive(d->fs);
+
+	d = NULL;
+}
diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
index e7870ec81a9b..6fab1a2e133a 100644
--- a/sound/soc/intel/skylake/skl.h
+++ b/sound/soc/intel/skylake/skl.h
@@ -155,6 +155,7 @@ struct skl_module_cfg;
 
 #ifdef CONFIG_DEBUG_FS
 struct skl_debug *skl_debugfs_init(struct skl *skl);
+void skl_debugfs_exit(struct skl *skl);
 void skl_debug_init_module(struct skl_debug *d,
 			struct snd_soc_dapm_widget *w,
 			struct skl_module_cfg *mconfig);
@@ -163,6 +164,10 @@ static inline struct skl_debug *skl_debugfs_init(struct skl *skl)
 {
 	return NULL;
 }
+
+static inline void skl_debugfs_exit(struct skl *skl)
+{}
+
 static inline void skl_debug_init_module(struct skl_debug *d,
 					 struct snd_soc_dapm_widget *w,
 					 struct skl_module_cfg *mconfig)
-- 
2.17.1

[PATCH v2 07/11] ASoC: Intel: Skylake: Properly cleanup on component removal

[Amadeusz Sławiński]

When we remove component we need to reverse things which were done on
init, this consists of topology cleanup, lists cleanup and releasing
firmware.

Currently cleanup handlers are put in wrong places or otherwise missing.
So add proper component cleanup function and perform cleanups in it.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/intel/skylake/skl-pcm.c      |  8 ++++++--
 sound/soc/intel/skylake/skl-topology.c | 15 +++++++++++++++
 sound/soc/intel/skylake/skl-topology.h |  2 ++
 sound/soc/intel/skylake/skl.c          |  2 --
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 5e3421cc0feb..af479e9c8d0b 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -1450,8 +1450,12 @@ static int skl_platform_soc_probe(struct snd_soc_component *component)
 
 static void skl_pcm_remove(struct snd_soc_component *component)
 {
-	/* remove topology */
-	snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL);
+	struct hdac_bus *bus = dev_get_drvdata(component->dev);
+	struct skl *skl = bus_to_skl(bus);
+
+	skl_tplg_exit(component, bus);
+
+	skl_debugfs_exit(skl);
 }
 
 static const struct snd_soc_component_driver skl_component  = {
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index 44f3b29a7210..3964262109ac 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -3748,3 +3748,18 @@ int skl_tplg_init(struct snd_soc_component *component, struct hdac_bus *bus)
 
 	return 0;
 }
+
+void skl_tplg_exit(struct snd_soc_component *component, struct hdac_bus *bus)
+{
+	struct skl *skl = bus_to_skl(bus);
+	struct skl_pipeline *ppl, *tmp;
+
+	if (!list_empty(&skl->ppl_list))
+		list_for_each_entry_safe(ppl, tmp, &skl->ppl_list, node)
+			list_del(&ppl->node);
+
+	/* clean up topology */
+	snd_soc_tplg_component_remove(component, SND_SOC_TPLG_INDEX_ALL);
+
+	release_firmware(skl->tplg);
+}
diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h
index b66e3a728853..a8ecb615d24b 100644
--- a/sound/soc/intel/skylake/skl-topology.h
+++ b/sound/soc/intel/skylake/skl-topology.h
@@ -462,6 +462,8 @@ void skl_tplg_set_be_dmic_config(struct snd_soc_dai *dai,
 	struct skl_pipe_params *params, int stream);
 int skl_tplg_init(struct snd_soc_component *component,
 				struct hdac_bus *ebus);
+void skl_tplg_exit(struct snd_soc_component *component,
+				struct hdac_bus *bus);
 struct skl_module_cfg *skl_tplg_fe_get_cpr_module(
 		struct snd_soc_dai *dai, int stream);
 int skl_tplg_update_pipe_params(struct device *dev,
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index cae97c65eef8..7e093fb13c92 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -1111,14 +1111,12 @@ static void skl_remove(struct pci_dev *pci)
 	struct skl *skl = bus_to_skl(bus);
 
 	cancel_work_sync(&skl->probe_work);
-	release_firmware(skl->tplg);
 
 	pm_runtime_get_noresume(&pci->dev);
 
 	/* codec removal, invoke bus_device_remove */
 	snd_hdac_ext_bus_device_remove(bus);
 
-	skl->debugfs = NULL;
 	skl_platform_unregister(&pci->dev);
 	skl_free_dsp(skl);
 	skl_machine_device_unregister(skl);
-- 
2.17.1

[PATCH v2 08/11] ASoC: Intel: Skylake: Fix NULL ptr dereference when unloading clk dev

[Amadeusz Sławiński]

When driver is probed, we iterate over NHLT and check if clk entries are
present. For each such entry we call register_skl_clk and keep the
result in data->clk[].
Currently data->clk is sparsely indexed using NHLT table iterator, while
when freeing we use number of registered entries. Let's just use
data->avail_clk_cnt as index, so it can be reset back in
unregister_src_clk.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/intel/skylake/skl-ssp-clk.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c b/sound/soc/intel/skylake/skl-ssp-clk.c
index cda1b5fa7436..5bb6e40d4d3e 100644
--- a/sound/soc/intel/skylake/skl-ssp-clk.c
+++ b/sound/soc/intel/skylake/skl-ssp-clk.c
@@ -276,10 +276,8 @@ static void unregister_parent_src_clk(struct skl_clk_parent *pclk,
 
 static void unregister_src_clk(struct skl_clk_data *dclk)
 {
-	u8 cnt = dclk->avail_clk_cnt;
-
-	while (cnt--)
-		clkdev_drop(dclk->clk[cnt]->lookup);
+	while (dclk->avail_clk_cnt--)
+		clkdev_drop(dclk->clk[dclk->avail_clk_cnt]->lookup);
 }
 
 static int skl_register_parent_clks(struct device *dev,
@@ -381,13 +379,13 @@ static int skl_clk_dev_probe(struct platform_device *pdev)
 		if (clks[i].rate_cfg[0].rate == 0)
 			continue;
 
-		data->clk[i] = register_skl_clk(dev, &clks[i], clk_pdata, i);
-		if (IS_ERR(data->clk[i])) {
-			ret = PTR_ERR(data->clk[i]);
+		data->clk[data->avail_clk_cnt] = register_skl_clk(dev,
+				&clks[i], clk_pdata, i);
+
+		if (IS_ERR(data->clk[data->avail_clk_cnt])) {
+			ret = PTR_ERR(data->clk[data->avail_clk_cnt++]);
 			goto err_unreg_skl_clk;
 		}
-
-		data->avail_clk_cnt++;
 	}
 
 	platform_set_drvdata(pdev, data);
-- 
2.17.1

[PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Amadeusz Sławiński]

When we unload Skylake driver we may end up calling
hdac_component_master_unbind(), it uses acomp->audio_ops, which we set
in hdmi_codec_probe(), so we need to set it to NULL in hdmi_codec_remove(),
otherwise we will dereference no longer existing pointer.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 911bb6e2a1ac..9ee1bff548d8 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -1890,6 +1890,12 @@ static void hdmi_codec_remove(struct snd_soc_component *component)
 {
 	struct hdac_hdmi_priv *hdmi = snd_soc_component_get_drvdata(component);
 	struct hdac_device *hdev = hdmi->hdev;
+	int ret;
+
+	ret = snd_hdac_acomp_register_notifier(hdev->bus, NULL);
+	if (ret < 0)
+		dev_err(&hdev->dev, "notifier unregister failed: err: %d\n",
+				ret);
 
 	pm_runtime_disable(&hdev->dev);
 }
-- 
2.17.1

[PATCH v2 10/11] ASoC: topology: Consolidate how dtexts and dvalues are freed

[Amadeusz Sławiński]

Provide helper functions and use them to free dtexts and dvalues in
topology. This is followup cleanup after related changes in this area as
suggested in:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-January/144761.html

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-topology.c | 41 +++++++++++++++++++++++-----------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index b538412e4bcf..a926c2afbe05 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -86,6 +86,8 @@ snd_soc_dapm_new_control_unlocked(struct snd_soc_dapm_context *dapm,
 struct snd_soc_dapm_widget *
 snd_soc_dapm_new_control(struct snd_soc_dapm_context *dapm,
 			 const struct snd_soc_dapm_widget *widget);
+static void soc_tplg_denum_remove_texts(struct soc_enum *se);
+static void soc_tplg_denum_remove_values(struct soc_enum *se);
 
 /* check we dont overflow the data for this control chunk */
 static int soc_tplg_check_elem_count(struct soc_tplg *tplg, size_t elem_size,
@@ -398,7 +400,6 @@ static void remove_enum(struct snd_soc_component *comp,
 {
 	struct snd_card *card = comp->card->snd_card;
 	struct soc_enum *se = container_of(dobj, struct soc_enum, dobj);
-	int i;
 
 	if (pass != SOC_TPLG_PASS_MIXER)
 		return;
@@ -409,10 +410,8 @@ static void remove_enum(struct snd_soc_component *comp,
 	snd_ctl_remove(card, dobj->control.kcontrol);
 	list_del(&dobj->list);
 
-	kfree(dobj->control.dvalues);
-	for (i = 0; i < se->items; i++)
-		kfree(dobj->control.dtexts[i]);
-	kfree(dobj->control.dtexts);
+	soc_tplg_denum_remove_values(se);
+	soc_tplg_denum_remove_texts(se);
 	kfree(se);
 }
 
@@ -480,15 +479,12 @@ static void remove_widget(struct snd_soc_component *comp,
 			struct snd_kcontrol *kcontrol = w->kcontrols[i];
 			struct soc_enum *se =
 				(struct soc_enum *)kcontrol->private_value;
-			int j;
 
 			snd_ctl_remove(card, kcontrol);
 
 			/* free enum kcontrol's dvalues and dtexts */
-			kfree(se->dobj.control.dvalues);
-			for (j = 0; j < se->items; j++)
-				kfree(se->dobj.control.dtexts[j]);
-			kfree(se->dobj.control.dtexts);
+			soc_tplg_denum_remove_values(se);
+			soc_tplg_denum_remove_texts(se);
 
 			kfree(se);
 			kfree(w->kcontrol_news[i].name);
@@ -956,14 +952,23 @@ static int soc_tplg_denum_create_texts(struct soc_enum *se,
 		}
 	}
 
+	se->items = le32_to_cpu(ec->items);
 	se->texts = (const char * const *)se->dobj.control.dtexts;
 	return 0;
 
 err:
+	se->items = i;
+	soc_tplg_denum_remove_texts(se);
+	return ret;
+}
+
+static inline void soc_tplg_denum_remove_texts(struct soc_enum *se)
+{
+	int i = se->items;
+
 	for (--i; i >= 0; i--)
 		kfree(se->dobj.control.dtexts[i]);
 	kfree(se->dobj.control.dtexts);
-	return ret;
 }
 
 static int soc_tplg_denum_create_values(struct soc_enum *se,
@@ -988,6 +993,11 @@ static int soc_tplg_denum_create_values(struct soc_enum *se,
 	return 0;
 }
 
+static inline void soc_tplg_denum_remove_values(struct soc_enum *se)
+{
+	kfree(se->dobj.control.dvalues);
+}
+
 static int soc_tplg_denum_create(struct soc_tplg *tplg, unsigned int count,
 	size_t size)
 {
@@ -1035,7 +1045,6 @@ static int soc_tplg_denum_create(struct soc_tplg *tplg, unsigned int count,
 		se->shift_r = tplc_chan_get_shift(tplg, ec->channel,
 			SNDRV_CHMAP_FL);
 
-		se->items = le32_to_cpu(ec->items);
 		se->mask = le32_to_cpu(ec->mask);
 		se->dobj.index = tplg->index;
 		se->dobj.type = SND_SOC_DOBJ_ENUM;
@@ -1381,7 +1390,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
 	struct snd_kcontrol_new *kc;
 	struct snd_soc_tplg_enum_control *ec;
 	struct soc_enum *se;
-	int i, j, err;
+	int i, err;
 
 	kc = kcalloc(num_kcontrols, sizeof(*kc), GFP_KERNEL);
 	if (kc == NULL)
@@ -1476,10 +1485,8 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
 		if (!se)
 			continue;
 
-		kfree(se->dobj.control.dvalues);
-		for (j = 0; j < ec->items; j++)
-			kfree(se->dobj.control.dtexts[j]);
-		kfree(se->dobj.control.dtexts);
+		soc_tplg_denum_remove_values(se);
+		soc_tplg_denum_remove_texts(se);
 
 		kfree(se);
 		kfree(kc[i].name);
-- 
2.17.1

[PATCH v2 11/11] ASoC: topology: Consolidate and fix asoc_tplg_dapm_widget_*_create flow

[Amadeusz Sławiński]

There are a few soc_tplg_dapm_widget_*_create functions with similar
content, but slightly different flow, unify their flow and make sure
that we go to error handler and free memory in case of failure.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-topology.c | 77 ++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 42 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index a926c2afbe05..fc1f1d6f9e92 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1310,14 +1310,15 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
 
 	for (i = 0; i < num_kcontrols; i++) {
 		mc = (struct snd_soc_tplg_mixer_control *)tplg->pos;
-		sm = kzalloc(sizeof(*sm), GFP_KERNEL);
-		if (sm == NULL)
-			goto err;
 
 		/* validate kcontrol */
 		if (strnlen(mc->hdr.name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
 			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			goto err_str;
+			goto err_sm;
+
+		sm = kzalloc(sizeof(*sm), GFP_KERNEL);
+		if (sm == NULL)
+			goto err_sm;
 
 		tplg->pos += (sizeof(struct snd_soc_tplg_mixer_control) +
 			      le32_to_cpu(mc->priv.size));
@@ -1327,7 +1328,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
 
 		kc[i].name = kstrdup(mc->hdr.name, GFP_KERNEL);
 		if (kc[i].name == NULL)
-			goto err_str;
+			goto err_sm;
 		kc[i].private_value = (long)sm;
 		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc[i].access = mc->hdr.access;
@@ -1353,8 +1354,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
 		err = soc_tplg_kcontrol_bind_io(&mc->hdr, &kc[i], tplg);
 		if (err) {
 			soc_control_err(tplg, &mc->hdr, mc->hdr.name);
-			kfree(sm);
-			continue;
+			goto err_sm;
 		}
 
 		/* create any TLV data */
@@ -1367,20 +1367,19 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
 			dev_err(tplg->dev, "ASoC: failed to init %s\n",
 				mc->hdr.name);
 			soc_tplg_free_tlv(tplg, &kc[i]);
-			kfree(sm);
-			continue;
+			goto err_sm;
 		}
 	}
 	return kc;
 
-err_str:
-	kfree(sm);
-err:
-	for (--i; i >= 0; i--) {
-		kfree((void *)kc[i].private_value);
+err_sm:
+	for (; i >= 0; i--) {
+		sm = (struct soc_mixer_control *)kc[i].private_value;
+		kfree(sm);
 		kfree(kc[i].name);
 	}
 	kfree(kc);
+
 	return NULL;
 }
 
@@ -1401,11 +1400,11 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
 		/* validate kcontrol */
 		if (strnlen(ec->hdr.name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
 			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			goto err;
+			goto err_se;
 
 		se = kzalloc(sizeof(*se), GFP_KERNEL);
 		if (se == NULL)
-			goto err;
+			goto err_se;
 
 		tplg->pos += (sizeof(struct snd_soc_tplg_enum_control) +
 				ec->priv.size);
@@ -1414,10 +1413,8 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
 			ec->hdr.name);
 
 		kc[i].name = kstrdup(ec->hdr.name, GFP_KERNEL);
-		if (kc[i].name == NULL) {
-			kfree(se);
+		if (kc[i].name == NULL)
 			goto err_se;
-		}
 		kc[i].private_value = (long)se;
 		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc[i].access = ec->hdr.access;
@@ -1482,44 +1479,43 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
 	for (; i >= 0; i--) {
 		/* free values and texts */
 		se = (struct soc_enum *)kc[i].private_value;
-		if (!se)
-			continue;
 
-		soc_tplg_denum_remove_values(se);
-		soc_tplg_denum_remove_texts(se);
+		if (se) {
+			soc_tplg_denum_remove_values(se);
+			soc_tplg_denum_remove_texts(se);
+		}
 
 		kfree(se);
 		kfree(kc[i].name);
 	}
-err:
 	kfree(kc);
 
 	return NULL;
 }
 
 static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
-	struct soc_tplg *tplg, int count)
+	struct soc_tplg *tplg, int num_kcontrols)
 {
 	struct snd_soc_tplg_bytes_control *be;
-	struct soc_bytes_ext  *sbe;
+	struct soc_bytes_ext *sbe;
 	struct snd_kcontrol_new *kc;
 	int i, err;
 
-	kc = kcalloc(count, sizeof(*kc), GFP_KERNEL);
+	kc = kcalloc(num_kcontrols, sizeof(*kc), GFP_KERNEL);
 	if (!kc)
 		return NULL;
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < num_kcontrols; i++) {
 		be = (struct snd_soc_tplg_bytes_control *)tplg->pos;
 
 		/* validate kcontrol */
 		if (strnlen(be->hdr.name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
 			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			goto err;
+			goto err_sbe;
 
 		sbe = kzalloc(sizeof(*sbe), GFP_KERNEL);
 		if (sbe == NULL)
-			goto err;
+			goto err_sbe;
 
 		tplg->pos += (sizeof(struct snd_soc_tplg_bytes_control) +
 			      le32_to_cpu(be->priv.size));
@@ -1529,10 +1525,8 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
 			be->hdr.name, be->hdr.access);
 
 		kc[i].name = kstrdup(be->hdr.name, GFP_KERNEL);
-		if (kc[i].name == NULL) {
-			kfree(sbe);
-			goto err;
-		}
+		if (kc[i].name == NULL)
+			goto err_sbe;
 		kc[i].private_value = (long)sbe;
 		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc[i].access = be->hdr.access;
@@ -1544,8 +1538,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
 		err = soc_tplg_kcontrol_bind_io(&be->hdr, &kc[i], tplg);
 		if (err) {
 			soc_control_err(tplg, &be->hdr, be->hdr.name);
-			kfree(sbe);
-			continue;
+			goto err_sbe;
 		}
 
 		/* pass control to driver for optional further init */
@@ -1554,20 +1547,20 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
 		if (err < 0) {
 			dev_err(tplg->dev, "ASoC: failed to init %s\n",
 				be->hdr.name);
-			kfree(sbe);
-			continue;
+			goto err_sbe;
 		}
 	}
 
 	return kc;
 
-err:
-	for (--i; i >= 0; i--) {
-		kfree((void *)kc[i].private_value);
+err_sbe:
+	for (; i >= 0; i--) {
+		sbe = (struct soc_bytes_ext *)kc[i].private_value;
+		kfree(sbe);
 		kfree(kc[i].name);
 	}
-
 	kfree(kc);
+
 	return NULL;
 }
 
-- 
2.17.1

Re: [PATCH v2 02/11] ALSA: hdac: Fix codec name after machine driver is unloaded and reloaded

[Takashi Iwai]

On Mon, 17 Jun 2019 13:36:35 +0200,
Amadeusz Sławiński wrote:
> 
> Currently on each driver reload internal counter is being increased. It
> causes failure to enumerate driver devices, as they have hardcoded:
> .codec_name = "ehdaudio0D2",
> As there is currently no devices with multiple hda codecs and there is
> currently no established way to reliably differentiate, between them,
> always assign bus->idx = 0;
> 
> This fixes a problem when we unload and reload machine driver idx gets
> incremented, so .codec_name would've needed to be set to "ehdaudio1D2"
> after first reload and so on.
> 
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

Acked-by: Takashi Iwai <tiwai@suse.de>


Takashi

Re: [PATCH v2 02/11] ALSA: hdac: Fix codec name after machine driver is unloaded and reloaded

[Takashi Iwai]

On Mon, 17 Jun 2019 13:36:35 +0200,
Amadeusz Sławiński wrote:
> 
> Currently on each driver reload internal counter is being increased. It
> causes failure to enumerate driver devices, as they have hardcoded:
> .codec_name = "ehdaudio0D2",
> As there is currently no devices with multiple hda codecs and there is
> currently no established way to reliably differentiate, between them,
> always assign bus->idx = 0;
> 
> This fixes a problem when we unload and reload machine driver idx gets
> incremented, so .codec_name would've needed to be set to "ehdaudio1D2"
> after first reload and so on.
> 
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>

Acked-by: Takashi Iwai <tiwai@suse.de>


Takashi

Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Ranjani Sridharan]

On Mon, 2019-06-17 at 13:36 +0200, Amadeusz Sławiński wrote:
> When we unload Skylake driver we may end up calling
> hdac_component_master_unbind(), it uses acomp->audio_ops, which we
> set
> in hdmi_codec_probe(), so we need to set it to NULL in
> hdmi_codec_remove(),
> otherwise we will dereference no longer existing pointer.

Hi Amadeusz,

It looks like the audio_ops should be deleted snd_hdac_acomp_exit().
Also, this doesnt seem to be the case with when the SOF driver is
removed.
Could you please give a bit more context on what error you see when
this happens?

Thanks,
Ranjani
> 
> Signed-off-by: Amadeusz Sławiński <
> amadeuszx.slawinski@linux.intel.com>
> ---
>  sound/soc/codecs/hdac_hdmi.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c
> b/sound/soc/codecs/hdac_hdmi.c
> index 911bb6e2a1ac..9ee1bff548d8 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -1890,6 +1890,12 @@ static void hdmi_codec_remove(struct
> snd_soc_component *component)
>  {
>  	struct hdac_hdmi_priv *hdmi =
> snd_soc_component_get_drvdata(component);
>  	struct hdac_device *hdev = hdmi->hdev;
> +	int ret;
> +
> +	ret = snd_hdac_acomp_register_notifier(hdev->bus, NULL);
> +	if (ret < 0)
> +		dev_err(&hdev->dev, "notifier unregister failed: err:
> %d\n",
> +				ret);
>  
>  	pm_runtime_disable(&hdev->dev);
>  }

Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Takashi Iwai]

On Mon, 17 Jun 2019 22:51:42 +0200,
Ranjani Sridharan wrote:
> 
> On Mon, 2019-06-17 at 13:36 +0200, Amadeusz Sławiński wrote:
> > When we unload Skylake driver we may end up calling
> > hdac_component_master_unbind(), it uses acomp->audio_ops, which we
> > set
> > in hdmi_codec_probe(), so we need to set it to NULL in
> > hdmi_codec_remove(),
> > otherwise we will dereference no longer existing pointer.
> 
> Hi Amadeusz,
> 
> It looks like the audio_ops should be deleted snd_hdac_acomp_exit().

It's a different one.  The codec driver registers / de-registers the
notifier at its probe/remove, while the controller driver takes care
of snd_hdac_acomp_init().  snd_hdac_acomp_exit() is usually not needed
to be called explicitly, as it's managed via devres.


Takashi

> Also, this doesnt seem to be the case with when the SOF driver is
> removed.
> Could you please give a bit more context on what error you see when
> this happens?
> 
> Thanks,
> Ranjani
> > 
> > Signed-off-by: Amadeusz Sławiński <
> > amadeuszx.slawinski@linux.intel.com>
> > ---
> >  sound/soc/codecs/hdac_hdmi.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/sound/soc/codecs/hdac_hdmi.c
> > b/sound/soc/codecs/hdac_hdmi.c
> > index 911bb6e2a1ac..9ee1bff548d8 100644
> > --- a/sound/soc/codecs/hdac_hdmi.c
> > +++ b/sound/soc/codecs/hdac_hdmi.c
> > @@ -1890,6 +1890,12 @@ static void hdmi_codec_remove(struct
> > snd_soc_component *component)
> >  {
> >  	struct hdac_hdmi_priv *hdmi =
> > snd_soc_component_get_drvdata(component);
> >  	struct hdac_device *hdev = hdmi->hdev;
> > +	int ret;
> > +
> > +	ret = snd_hdac_acomp_register_notifier(hdev->bus, NULL);
> > +	if (ret < 0)
> > +		dev_err(&hdev->dev, "notifier unregister failed: err:
> > %d\n",
> > +				ret);
> >  
> >  	pm_runtime_disable(&hdev->dev);
> >  }
> 
> 

Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Ranjani Sridharan]

On Mon, 2019-06-17 at 23:36 +0200, Takashi Iwai wrote:
> On Mon, 17 Jun 2019 22:51:42 +0200,
> Ranjani Sridharan wrote:
> > 
> > On Mon, 2019-06-17 at 13:36 +0200, Amadeusz Sławiński wrote:
> > > When we unload Skylake driver we may end up calling
> > > hdac_component_master_unbind(), it uses acomp->audio_ops, which
> > > we
> > > set
> > > in hdmi_codec_probe(), so we need to set it to NULL in
> > > hdmi_codec_remove(),
> > > otherwise we will dereference no longer existing pointer.
> > 
> > Hi Amadeusz,
> > 
> > It looks like the audio_ops should be deleted
> > snd_hdac_acomp_exit().
> 
> It's a different one.  The codec driver registers / de-registers the
> notifier at its probe/remove, while the controller driver takes care
> of snd_hdac_acomp_init().  snd_hdac_acomp_exit() is usually not
> needed
> to be called explicitly, as it's managed via devres.

Makes sense, thanks Takashi. But I am still wondering why we havent
seen this issue with SOF yet. We run the module unload/reload stress
test as well and havent seen this yet. 

Thanks,
Ranjani
> 
> 
> Takashi
> 
> > Also, this doesnt seem to be the case with when the SOF driver is
> > removed.
> > Could you please give a bit more context on what error you see when
> > this happens?
> > 
> > Thanks,
> > Ranjani
> > > 
> > > Signed-off-by: Amadeusz Sławiński <
> > > amadeuszx.slawinski@linux.intel.com>
> > > ---
> > >  sound/soc/codecs/hdac_hdmi.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/sound/soc/codecs/hdac_hdmi.c
> > > b/sound/soc/codecs/hdac_hdmi.c
> > > index 911bb6e2a1ac..9ee1bff548d8 100644
> > > --- a/sound/soc/codecs/hdac_hdmi.c
> > > +++ b/sound/soc/codecs/hdac_hdmi.c
> > > @@ -1890,6 +1890,12 @@ static void hdmi_codec_remove(struct
> > > snd_soc_component *component)
> > >  {
> > >  	struct hdac_hdmi_priv *hdmi =
> > > snd_soc_component_get_drvdata(component);
> > >  	struct hdac_device *hdev = hdmi->hdev;
> > > +	int ret;
> > > +
> > > +	ret = snd_hdac_acomp_register_notifier(hdev->bus, NULL);
> > > +	if (ret < 0)
> > > +		dev_err(&hdev->dev, "notifier unregister failed: err:
> > > %d\n",
> > > +				ret);
> > >  
> > >  	pm_runtime_disable(&hdev->dev);
> > >  }
> > 
> > 

Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Takashi Iwai]

On Tue, 18 Jun 2019 06:19:15 +0200,
Ranjani Sridharan wrote:
> 
> On Mon, 2019-06-17 at 23:36 +0200, Takashi Iwai wrote:
> > On Mon, 17 Jun 2019 22:51:42 +0200,
> > Ranjani Sridharan wrote:
> > > 
> > > On Mon, 2019-06-17 at 13:36 +0200, Amadeusz Sławiński wrote:
> > > > When we unload Skylake driver we may end up calling
> > > > hdac_component_master_unbind(), it uses acomp->audio_ops, which
> > > > we
> > > > set
> > > > in hdmi_codec_probe(), so we need to set it to NULL in
> > > > hdmi_codec_remove(),
> > > > otherwise we will dereference no longer existing pointer.
> > > 
> > > Hi Amadeusz,
> > > 
> > > It looks like the audio_ops should be deleted
> > > snd_hdac_acomp_exit().
> > 
> > It's a different one.  The codec driver registers / de-registers the
> > notifier at its probe/remove, while the controller driver takes care
> > of snd_hdac_acomp_init().  snd_hdac_acomp_exit() is usually not
> > needed
> > to be called explicitly, as it's managed via devres.
> 
> Makes sense, thanks Takashi. But I am still wondering why we havent
> seen this issue with SOF yet. We run the module unload/reload stress
> test as well and havent seen this yet. 

Usually this isn't a problem as the controller is removed at first.
But if the codec is unbound at first by some reason, it can be a
problem without unregistering, I guess.


Takashi

> 
> Thanks,
> Ranjani
> > 
> > 
> > Takashi
> > 
> > > Also, this doesnt seem to be the case with when the SOF driver is
> > > removed.
> > > Could you please give a bit more context on what error you see when
> > > this happens?
> > > 
> > > Thanks,
> > > Ranjani
> > > > 
> > > > Signed-off-by: Amadeusz Sławiński <
> > > > amadeuszx.slawinski@linux.intel.com>
> > > > ---
> > > >  sound/soc/codecs/hdac_hdmi.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/sound/soc/codecs/hdac_hdmi.c
> > > > b/sound/soc/codecs/hdac_hdmi.c
> > > > index 911bb6e2a1ac..9ee1bff548d8 100644
> > > > --- a/sound/soc/codecs/hdac_hdmi.c
> > > > +++ b/sound/soc/codecs/hdac_hdmi.c
> > > > @@ -1890,6 +1890,12 @@ static void hdmi_codec_remove(struct
> > > > snd_soc_component *component)
> > > >  {
> > > >  	struct hdac_hdmi_priv *hdmi =
> > > > snd_soc_component_get_drvdata(component);
> > > >  	struct hdac_device *hdev = hdmi->hdev;
> > > > +	int ret;
> > > > +
> > > > +	ret = snd_hdac_acomp_register_notifier(hdev->bus, NULL);
> > > > +	if (ret < 0)
> > > > +		dev_err(&hdev->dev, "notifier unregister failed: err:
> > > > %d\n",
> > > > +				ret);
> > > >  
> > > >  	pm_runtime_disable(&hdev->dev);
> > > >  }
> > > 
> > > 
> 

Re: [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Takashi Iwai]

On Tue, 18 Jun 2019 06:19:15 +0200,
Ranjani Sridharan wrote:
> 
> On Mon, 2019-06-17 at 23:36 +0200, Takashi Iwai wrote:
> > On Mon, 17 Jun 2019 22:51:42 +0200,
> > Ranjani Sridharan wrote:
> > > 
> > > On Mon, 2019-06-17 at 13:36 +0200, Amadeusz Sławiński wrote:
> > > > When we unload Skylake driver we may end up calling
> > > > hdac_component_master_unbind(), it uses acomp->audio_ops, which
> > > > we
> > > > set
> > > > in hdmi_codec_probe(), so we need to set it to NULL in
> > > > hdmi_codec_remove(),
> > > > otherwise we will dereference no longer existing pointer.
> > > 
> > > Hi Amadeusz,
> > > 
> > > It looks like the audio_ops should be deleted
> > > snd_hdac_acomp_exit().
> > 
> > It's a different one.  The codec driver registers / de-registers the
> > notifier at its probe/remove, while the controller driver takes care
> > of snd_hdac_acomp_init().  snd_hdac_acomp_exit() is usually not
> > needed
> > to be called explicitly, as it's managed via devres.
> 
> Makes sense, thanks Takashi. But I am still wondering why we havent
> seen this issue with SOF yet. We run the module unload/reload stress
> test as well and havent seen this yet. 

Usually this isn't a problem as the controller is removed at first.
But if the codec is unbound at first by some reason, it can be a
problem without unregistering, I guess.


Takashi

> 
> Thanks,
> Ranjani
> > 
> > 
> > Takashi
> > 
> > > Also, this doesnt seem to be the case with when the SOF driver is
> > > removed.
> > > Could you please give a bit more context on what error you see when
> > > this happens?
> > > 
> > > Thanks,
> > > Ranjani
> > > > 
> > > > Signed-off-by: Amadeusz Sławiński <
> > > > amadeuszx.slawinski@linux.intel.com>
> > > > ---
> > > >  sound/soc/codecs/hdac_hdmi.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/sound/soc/codecs/hdac_hdmi.c
> > > > b/sound/soc/codecs/hdac_hdmi.c
> > > > index 911bb6e2a1ac..9ee1bff548d8 100644
> > > > --- a/sound/soc/codecs/hdac_hdmi.c
> > > > +++ b/sound/soc/codecs/hdac_hdmi.c
> > > > @@ -1890,6 +1890,12 @@ static void hdmi_codec_remove(struct
> > > > snd_soc_component *component)
> > > >  {
> > > >  	struct hdac_hdmi_priv *hdmi =
> > > > snd_soc_component_get_drvdata(component);
> > > >  	struct hdac_device *hdev = hdmi->hdev;
> > > > +	int ret;
> > > > +
> > > > +	ret = snd_hdac_acomp_register_notifier(hdev->bus, NULL);
> > > > +	if (ret < 0)
> > > > +		dev_err(&hdev->dev, "notifier unregister failed: err:
> > > > %d\n",
> > > > +				ret);
> > > >  
> > > >  	pm_runtime_disable(&hdev->dev);
> > > >  }
> > > 
> > > 
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Amadeusz Sławiński]

On Mon, 17 Jun 2019 13:51:42 -0700
Ranjani Sridharan <ranjani.sridharan@linux.intel.com> wrote:

> On Mon, 2019-06-17 at 13:36 +0200, Amadeusz Sławiński wrote:
> > When we unload Skylake driver we may end up calling
> > hdac_component_master_unbind(), it uses acomp->audio_ops, which we
> > set
> > in hdmi_codec_probe(), so we need to set it to NULL in
> > hdmi_codec_remove(),
> > otherwise we will dereference no longer existing pointer.  
> 
> Hi Amadeusz,
> 
> It looks like the audio_ops should be deleted snd_hdac_acomp_exit().
> Also, this doesnt seem to be the case with when the SOF driver is
> removed.
> Could you please give a bit more context on what error you see when
> this happens?

Hi,

I get Oops. This is what happens with all other patches in this series and only this one reverted:

root@APL:~# rmmod snd_soc_sst_bxt_rt298
root@APL:~# rmmod snd_soc_hdac_hdmi
root@APL:~# rmmod snd_soc_skl
Killed

[   57.007783] BUG: unable to handle page fault for address: fffffbfff4067038
[   57.007956] #PF: supervisor read access in kernel mode
[   57.008065] #PF: error_code(0x0000) - not-present page
[   57.008173] PGD 268266067 P4D 268266067 PUD 23809a067 PMD 22b545067 PTE 0
[   57.008322] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC KASAN PTI
[   57.008453] CPU: 3 PID: 1045 Comm: rmmod Tainted: G                T 5.2.0-rc4-dev #824
[   57.008617] Hardware name: Intel Corp. Broxton P/Apollolake RVP1C, BIOS APLKRVPA.X64.0151.B25.1609151411 09/15/2016
[   57.008834] RIP: 0010:__asan_load8+0x39/0x90
[   57.008931] Code: ff ff ff ff 7f ff ff 48 39 c3 76 40 48 8d 43 07 48 89 c2 83 e2 07 48 83 fa 07 75 19 48 ba 00 00 00 00 00 fc ff df 48 c1 e8 03 <0f> b6 04 10 84 c0 75 2c 5b 5d c3 48 be 00 00 00 00 00 f
c ff df 48
[   57.009299] RSP: 0018:ffff88822431fa68 EFLAGS: 00010203
[   57.009411] RAX: 1ffffffff4067038 RBX: ffffffffa03381c0 RCX: ffffffffa01bd8a4
[   57.009557] RDX: dffffc0000000000 RSI: dffffc0000000000 RDI: ffffffffa03381c0
[   57.009704] RBP: ffff88822431fa70 R08: ffffed1046a6d8f3 R09: ffffed1046a6d8f3
[   57.009851] R10: ffffed1046a6d8f3 R11: 0000000000000000 R12: ffff88823536c4b0
[   57.009998] R13: ffffffffa03381a0 R14: ffffffffa01bd860 R15: ffff888223108538
[   57.010147] FS:  00007fedb579f540(0000) GS:ffff888237780000(0000) knlGS:0000000000000000
[   57.010312] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   57.010433] CR2: fffffbfff4067038 CR3: 000000022260a000 CR4: 00000000003406e0
[   57.010580] Call Trace:
[   57.010667]  hdac_component_master_unbind+0x44/0xb0 [snd_hda_core]
[   57.010822]  ? snd_hdac_acomp_exit+0x130/0x130 [snd_hda_core]
[   57.010949]  take_down_master+0x53/0x80
[   57.011037]  component_master_del+0x76/0xa0
[   57.011144]  snd_hdac_acomp_exit+0x97/0x130 [snd_hda_core]
[   57.011275]  ? snd_hdac_display_power+0x12e/0x1d0 [snd_hda_core]
[   57.011414]  skl_free+0xbf/0xd0 [snd_soc_skl]
[   57.011519]  skl_remove+0xf1/0x110 [snd_soc_skl]
[   57.011623]  pci_device_remove+0xd9/0x1f0
[   57.011714]  ? pcibios_free_irq+0x10/0x10
[   57.011806]  ? preempt_count_sub+0x18/0xd0
[   57.011898]  ? _raw_spin_unlock_irqrestore+0x26/0x40
[   57.012009]  device_release_driver_internal+0x140/0x270
[   57.012124]  driver_detach+0x7a/0xe0
[   57.012207]  bus_remove_driver+0x95/0x160
[   57.012303]  driver_unregister+0x43/0x60
[   57.012392]  pci_unregister_driver+0x29/0x110
[   57.012501]  skl_driver_exit+0x10/0x1b [snd_soc_skl]
[   57.012610]  __x64_sys_delete_module+0x235/0x3d0
[   57.012712]  ? free_module+0x380/0x380
[   57.012804]  do_syscall_64+0xcd/0x650
[   57.012887]  ? syscall_return_slowpath+0x1e0/0x1e0
[   57.012998]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[   57.013107] RIP: 0033:0x7fedb52bc1b7
[   57.013189] Code: 73 01 c3 48 8b 0d d1 8c 2c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 b8 b0 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d a1 8c 2c 00 f7 d8 64 89 01 48
[   57.013556] RSP: 002b:00007ffcfc17ce18 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
[   57.013712] RAX: ffffffffffffffda RBX: 00007ffcfc17ce78 RCX: 00007fedb52bc1b7
[   57.013858] RDX: 000000000000000a RSI: 0000000000000800 RDI: 00005649a5309a98
[   57.014004] RBP: 00005649a5309a30 R08: 00007ffcfc17bd91 R09: 0000000000000000
[   57.014149] R10: 00007fedb5338cc0 R11: 0000000000000206 R12: 00007ffcfc17d040
[   57.014294] R13: 00007ffcfc17e79b R14: 00005649a5309260 R15: 00005649a5309a30
[   57.014446] Modules linked in: i2c_designware_platform i2c_designware_core snd_soc_dmic joydev x86_pkg_temp_thermal intel_powerclamp coretemp crc32c_intel serio_raw pwm_lpss_pci pwm_lpss intel_lpss_pci intel_lpss snd_soc_rt298 mei_me mei snd_soc_rt286 snd_soc_rl6347a snd_soc_skl(-) snd_soc_skl_ipc snd_soc_sst_ipc snd_soc_sst_dsp snd_hda_ext_core snd_hda_core snd_soc_acpi_intel_match snd_soc_acpi snd_soc_core snd_compress snd_pcm_dmaengine snd_pcm snd_timer parport_pc lp parport ip_tables x_tables igb dca pinctrl_broxton pinctrl_intel [last unloaded: snd_soc_hdac_hdmi]
[   57.015477] CR2: fffffbfff4067038
[   57.015556] ---[ end trace 794bf9fb0862965b ]---

Amadeusz

Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Ranjani Sridharan]

On Tue, 2019-06-18 at 13:00 +0200, Amadeusz Sławiński wrote:
> On Mon, 17 Jun 2019 13:51:42 -0700
> Ranjani Sridharan <ranjani.sridharan@linux.intel.com> wrote:
> 
> > On Mon, 2019-06-17 at 13:36 +0200, Amadeusz Sławiński wrote:
> > > When we unload Skylake driver we may end up calling
> > > hdac_component_master_unbind(), it uses acomp->audio_ops, which
> > > we
> > > set
> > > in hdmi_codec_probe(), so we need to set it to NULL in
> > > hdmi_codec_remove(),
> > > otherwise we will dereference no longer existing pointer.  
> > 
> > Hi Amadeusz,
> > 
> > It looks like the audio_ops should be deleted
> > snd_hdac_acomp_exit().
> > Also, this doesnt seem to be the case with when the SOF driver is
> > removed.
> > Could you please give a bit more context on what error you see when
> > this happens?
> 
> Hi,
> 
> I get Oops. This is what happens with all other patches in this
> series and only this one reverted:
> 
> root@APL:~# rmmod snd_soc_sst_bxt_rt298
> root@APL:~# rmmod snd_soc_hdac_hdmi
> root@APL:~# rmmod snd_soc_skl

Thanks, Amadeusz. I think the order in which the drivers are removed is
what's causing the oops in your case. With SOF, the order we remove is

1. rmmod sof_pci_dev
2. rmmod snd_soc_sst_bxt_rt298
3. rmmod snd_soc_hdac_hdmi

Thanks,
Ranjani

Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Amadeusz Sławiński]

On Tue, 18 Jun 2019 08:58:22 -0700
Ranjani Sridharan <ranjani.sridharan@linux.intel.com> wrote:

> On Tue, 2019-06-18 at 13:00 +0200, Amadeusz Sławiński wrote:
> > On Mon, 17 Jun 2019 13:51:42 -0700
> > Ranjani Sridharan <ranjani.sridharan@linux.intel.com> wrote:
> >   
> > > On Mon, 2019-06-17 at 13:36 +0200, Amadeusz Sławiński wrote:  
> > > > When we unload Skylake driver we may end up calling
> > > > hdac_component_master_unbind(), it uses acomp->audio_ops, which
> > > > we
> > > > set
> > > > in hdmi_codec_probe(), so we need to set it to NULL in
> > > > hdmi_codec_remove(),
> > > > otherwise we will dereference no longer existing pointer.    
> > > 
> > > Hi Amadeusz,
> > > 
> > > It looks like the audio_ops should be deleted
> > > snd_hdac_acomp_exit().
> > > Also, this doesnt seem to be the case with when the SOF driver is
> > > removed.
> > > Could you please give a bit more context on what error you see
> > > when this happens?  
> > 
> > Hi,
> > 
> > I get Oops. This is what happens with all other patches in this
> > series and only this one reverted:
> > 
> > root@APL:~# rmmod snd_soc_sst_bxt_rt298
> > root@APL:~# rmmod snd_soc_hdac_hdmi
> > root@APL:~# rmmod snd_soc_skl  
> 
> Thanks, Amadeusz. I think the order in which the drivers are removed
> is what's causing the oops in your case. With SOF, the order we
> remove is
> 
> 1. rmmod sof_pci_dev
> 2. rmmod snd_soc_sst_bxt_rt298
> 3. rmmod snd_soc_hdac_hdmi
> 

Well, there is nothing enforcing the order in which modules can be
unloaded (and I see no reason to force it), as you can see from
following excerpt, you can either start unloading from
snd_soc_sst_bxt_rt298 or snd_soc_skl, and yes if you start from
snd_soc_skl, there is no problem.

snd_soc_sst_bxt_rt298    40960  0
snd_soc_hdac_hdmi      45056  1 snd_soc_sst_bxt_rt298
snd_soc_dmic           16384  1
snd_soc_rt298          65536  2 snd_soc_sst_bxt_rt298
snd_soc_rt286          61440  0
snd_soc_rl6347a        16384  2 snd_soc_rt298,snd_soc_rt286
snd_soc_skl           372736  0
snd_soc_sst_ipc        20480  1 snd_soc_skl
snd_soc_sst_dsp        36864  1 snd_soc_skl
snd_hda_ext_core       28672  2 snd_soc_hdac_hdmi,snd_soc_skl
snd_hda_core          139264  3
snd_hda_ext_core,snd_soc_hdac_hdmi,snd_soc_skl
snd_soc_acpi_intel_match    53248  1 snd_soc_skl snd_soc_acpi
16384  2 snd_soc_acpi_intel_match,snd_soc_skl snd_soc_core
405504  6
snd_soc_rt298,snd_soc_rt286,snd_soc_hdac_hdmi,snd_soc_skl,snd_soc_dmic,snd_soc_sst_bxt_rt298
snd_compress           36864  2 snd_soc_core,snd_soc_skl
snd_pcm_dmaengine      16384  1 snd_soc_core snd_pcm
163840  10
snd_soc_rt298,snd_soc_rt286,snd_hda_ext_core,snd_soc_hdac_hdmi,snd_compress,snd_soc_core,snd_soc_skl,snd_hda_core,snd_soc_sst_bxt_rt298,snd_pcm_dmaengine
snd_timer              53248  1 snd_pcm

Amadeusz

Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Ranjani Sridharan]

On Wed, 2019-06-19 at 10:38 +0200, Amadeusz Sławiński wrote:
> On Tue, 18 Jun 2019 08:58:22 -0700
> Ranjani Sridharan <ranjani.sridharan@linux.intel.com> wrote:
> 
> > On Tue, 2019-06-18 at 13:00 +0200, Amadeusz Sławiński wrote:
> > > On Mon, 17 Jun 2019 13:51:42 -0700
> > > Ranjani Sridharan <ranjani.sridharan@linux.intel.com> wrote:
> > >   
> > > > On Mon, 2019-06-17 at 13:36 +0200, Amadeusz Sławiński wrote:  
> > > > > When we unload Skylake driver we may end up calling
> > > > > hdac_component_master_unbind(), it uses acomp->audio_ops,
> > > > > which
> > > > > we
> > > > > set
> > > > > in hdmi_codec_probe(), so we need to set it to NULL in
> > > > > hdmi_codec_remove(),
> > > > > otherwise we will dereference no longer existing pointer.    
> > > > 
> > > > Hi Amadeusz,
> > > > 
> > > > It looks like the audio_ops should be deleted
> > > > snd_hdac_acomp_exit().
> > > > Also, this doesnt seem to be the case with when the SOF driver
> > > > is
> > > > removed.
> > > > Could you please give a bit more context on what error you see
> > > > when this happens?  
> > > 
> > > Hi,
> > > 
> > > I get Oops. This is what happens with all other patches in this
> > > series and only this one reverted:
> > > 
> > > root@APL:~# rmmod snd_soc_sst_bxt_rt298
> > > root@APL:~# rmmod snd_soc_hdac_hdmi
> > > root@APL:~# rmmod snd_soc_skl  
> > 
> > Thanks, Amadeusz. I think the order in which the drivers are
> > removed
> > is what's causing the oops in your case. With SOF, the order we
> > remove is
> > 
> > 1. rmmod sof_pci_dev
> > 2. rmmod snd_soc_sst_bxt_rt298
> > 3. rmmod snd_soc_hdac_hdmi
> > 
> 
> Well, there is nothing enforcing the order in which modules can be
> unloaded (and I see no reason to force it), as you can see from
> following excerpt, you can either start unloading from
> snd_soc_sst_bxt_rt298 or snd_soc_skl, and yes if you start from
> snd_soc_skl, there is no problem.
> 
I am good with this patch. I just wanted to understand why we werent
seeing this error with SOF. Sure, there's nothing enforcing the order
in which modules are unloaded  but there must be a logical order for
testing purposes. 

Pierre, can you please comment on it. I vaguely remember discussing
this with you last year.

Thanks,
Ranjani

Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Pierre-Louis Bossart]

>>>>> Could you please give a bit more context on what error you see
>>>>> when this happens?
>>>>
>>>> Hi,
>>>>
>>>> I get Oops. This is what happens with all other patches in this
>>>> series and only this one reverted:
>>>>
>>>> root@APL:~# rmmod snd_soc_sst_bxt_rt298
>>>> root@APL:~# rmmod snd_soc_hdac_hdmi
>>>> root@APL:~# rmmod snd_soc_skl
>>>
>>> Thanks, Amadeusz. I think the order in which the drivers are
>>> removed
>>> is what's causing the oops in your case. With SOF, the order we
>>> remove is
>>>
>>> 1. rmmod sof_pci_dev
>>> 2. rmmod snd_soc_sst_bxt_rt298
>>> 3. rmmod snd_soc_hdac_hdmi
>>>
>>
>> Well, there is nothing enforcing the order in which modules can be
>> unloaded (and I see no reason to force it), as you can see from
>> following excerpt, you can either start unloading from
>> snd_soc_sst_bxt_rt298 or snd_soc_skl, and yes if you start from
>> snd_soc_skl, there is no problem.

there is a fundamental dependency that you are ignoring: the module 
snd_soc_sst_bxt_rt298 is a machine driver which will be probed when 
snd_soc_skl creates a platform_device.
Sure you can remove modules in a different order, but that's a bit of an 
artificial/academic exercise isn't it?

>>
> I am good with this patch. I just wanted to understand why we werent
> seeing this error with SOF. Sure, there's nothing enforcing the order
> in which modules are unloaded  but there must be a logical order for
> testing purposes.
> 
> Pierre, can you please comment on it. I vaguely remember discussing
> this with you last year.

Our tests remove the modules by taking care of dependencies and it's 
already unveiled dozens of issues.
We could add a sequence similar to Amadeusz and unbind the modules which 
are loaded with the creation of a platform_device (machine driver, 
dmic), I am just not sure how of useful this would be.

Re: [alsa-devel] [PATCH v2 09/11] ASoC: Intel: hdac_hdmi: Set ops to NULL on remove

[Amadeusz Sławiński]

On Thu, 20 Jun 2019 08:17:33 +0200
Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:

> >>>>> Could you please give a bit more context on what error you see
> >>>>> when this happens?  
> >>>>
> >>>> Hi,
> >>>>
> >>>> I get Oops. This is what happens with all other patches in this
> >>>> series and only this one reverted:
> >>>>
> >>>> root@APL:~# rmmod snd_soc_sst_bxt_rt298
> >>>> root@APL:~# rmmod snd_soc_hdac_hdmi
> >>>> root@APL:~# rmmod snd_soc_skl  
> >>>
> >>> Thanks, Amadeusz. I think the order in which the drivers are
> >>> removed
> >>> is what's causing the oops in your case. With SOF, the order we
> >>> remove is
> >>>
> >>> 1. rmmod sof_pci_dev
> >>> 2. rmmod snd_soc_sst_bxt_rt298
> >>> 3. rmmod snd_soc_hdac_hdmi
> >>>  
> >>
> >> Well, there is nothing enforcing the order in which modules can be
> >> unloaded (and I see no reason to force it), as you can see from
> >> following excerpt, you can either start unloading from
> >> snd_soc_sst_bxt_rt298 or snd_soc_skl, and yes if you start from
> >> snd_soc_skl, there is no problem.  
> 
> there is a fundamental dependency that you are ignoring: the module 
> snd_soc_sst_bxt_rt298 is a machine driver which will be probed when 
> snd_soc_skl creates a platform_device.
> Sure you can remove modules in a different order, but that's a bit of
> an artificial/academic exercise isn't it?
> 
> >>  
> > I am good with this patch. I just wanted to understand why we werent
> > seeing this error with SOF. Sure, there's nothing enforcing the
> > order in which modules are unloaded  but there must be a logical
> > order for testing purposes.
> > 
> > Pierre, can you please comment on it. I vaguely remember discussing
> > this with you last year.  
> 
> Our tests remove the modules by taking care of dependencies and it's 
> already unveiled dozens of issues.
> We could add a sequence similar to Amadeusz and unbind the modules
> which are loaded with the creation of a platform_device (machine
> driver, dmic), I am just not sure how of useful this would be.

You work under the assumption that users will remove modules in
"correct" order. Because it is not enforced by modules dependencies you
can expect users to do everything possible at some point in time. In
this case unloading modules in not expected order will lead to kernel
Oops, which is not what should happen.

Re: [alsa-devel] [PATCH v2 11/11] ASoC: topology: Consolidate and fix asoc_tplg_dapm_widget_*_create flow

[Ranjani Sridharan]

On Mon, 2019-06-17 at 13:36 +0200, Amadeusz Sławiński wrote:
> There are a few soc_tplg_dapm_widget_*_create functions with similar
> content, but slightly different flow, unify their flow and make sure
> that we go to error handler and free memory in case of failure.
> 
> Signed-off-by: Amadeusz Sławiński <
> amadeuszx.slawinski@linux.intel.com>
Acked-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

I'm good with all the patches in the series.

Thanks,
Ranjani

> ---
>  sound/soc/soc-topology.c | 77 ++++++++++++++++++------------------
> ----
>  1 file changed, 35 insertions(+), 42 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index a926c2afbe05..fc1f1d6f9e92 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -1310,14 +1310,15 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_dmixer_create(
>  
>  	for (i = 0; i < num_kcontrols; i++) {
>  		mc = (struct snd_soc_tplg_mixer_control *)tplg->pos;
> -		sm = kzalloc(sizeof(*sm), GFP_KERNEL);
> -		if (sm == NULL)
> -			goto err;
>  
>  		/* validate kcontrol */
>  		if (strnlen(mc->hdr.name,
> SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
>  			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
> -			goto err_str;
> +			goto err_sm;
> +
> +		sm = kzalloc(sizeof(*sm), GFP_KERNEL);
> +		if (sm == NULL)
> +			goto err_sm;
>  
>  		tplg->pos += (sizeof(struct snd_soc_tplg_mixer_control)
> +
>  			      le32_to_cpu(mc->priv.size));
> @@ -1327,7 +1328,7 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_dmixer_create(
>  
>  		kc[i].name = kstrdup(mc->hdr.name, GFP_KERNEL);
>  		if (kc[i].name == NULL)
> -			goto err_str;
> +			goto err_sm;
>  		kc[i].private_value = (long)sm;
>  		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  		kc[i].access = mc->hdr.access;
> @@ -1353,8 +1354,7 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_dmixer_create(
>  		err = soc_tplg_kcontrol_bind_io(&mc->hdr, &kc[i],
> tplg);
>  		if (err) {
>  			soc_control_err(tplg, &mc->hdr, mc->hdr.name);
> -			kfree(sm);
> -			continue;
> +			goto err_sm;
>  		}
>  
>  		/* create any TLV data */
> @@ -1367,20 +1367,19 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_dmixer_create(
>  			dev_err(tplg->dev, "ASoC: failed to init %s\n",
>  				mc->hdr.name);
>  			soc_tplg_free_tlv(tplg, &kc[i]);
> -			kfree(sm);
> -			continue;
> +			goto err_sm;
>  		}
>  	}
>  	return kc;
>  
> -err_str:
> -	kfree(sm);
> -err:
> -	for (--i; i >= 0; i--) {
> -		kfree((void *)kc[i].private_value);
> +err_sm:
> +	for (; i >= 0; i--) {
> +		sm = (struct soc_mixer_control *)kc[i].private_value;
> +		kfree(sm);
>  		kfree(kc[i].name);
>  	}
>  	kfree(kc);
> +
>  	return NULL;
>  }
>  
> @@ -1401,11 +1400,11 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_denum_create(
>  		/* validate kcontrol */
>  		if (strnlen(ec->hdr.name,
> SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
>  			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
> -			goto err;
> +			goto err_se;
>  
>  		se = kzalloc(sizeof(*se), GFP_KERNEL);
>  		if (se == NULL)
> -			goto err;
> +			goto err_se;
>  
>  		tplg->pos += (sizeof(struct snd_soc_tplg_enum_control)
> +
>  				ec->priv.size);
> @@ -1414,10 +1413,8 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_denum_create(
>  			ec->hdr.name);
>  
>  		kc[i].name = kstrdup(ec->hdr.name, GFP_KERNEL);
> -		if (kc[i].name == NULL) {
> -			kfree(se);
> +		if (kc[i].name == NULL)
>  			goto err_se;
> -		}
>  		kc[i].private_value = (long)se;
>  		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  		kc[i].access = ec->hdr.access;
> @@ -1482,44 +1479,43 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_denum_create(
>  	for (; i >= 0; i--) {
>  		/* free values and texts */
>  		se = (struct soc_enum *)kc[i].private_value;
> -		if (!se)
> -			continue;
>  
> -		soc_tplg_denum_remove_values(se);
> -		soc_tplg_denum_remove_texts(se);
> +		if (se) {
> +			soc_tplg_denum_remove_values(se);
> +			soc_tplg_denum_remove_texts(se);
> +		}
>  
>  		kfree(se);
>  		kfree(kc[i].name);
>  	}
> -err:
>  	kfree(kc);
>  
>  	return NULL;
>  }
>  
>  static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
> -	struct soc_tplg *tplg, int count)
> +	struct soc_tplg *tplg, int num_kcontrols)
>  {
>  	struct snd_soc_tplg_bytes_control *be;
> -	struct soc_bytes_ext  *sbe;
> +	struct soc_bytes_ext *sbe;
>  	struct snd_kcontrol_new *kc;
>  	int i, err;
>  
> -	kc = kcalloc(count, sizeof(*kc), GFP_KERNEL);
> +	kc = kcalloc(num_kcontrols, sizeof(*kc), GFP_KERNEL);
>  	if (!kc)
>  		return NULL;
>  
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < num_kcontrols; i++) {
>  		be = (struct snd_soc_tplg_bytes_control *)tplg->pos;
>  
>  		/* validate kcontrol */
>  		if (strnlen(be->hdr.name,
> SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
>  			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
> -			goto err;
> +			goto err_sbe;
>  
>  		sbe = kzalloc(sizeof(*sbe), GFP_KERNEL);
>  		if (sbe == NULL)
> -			goto err;
> +			goto err_sbe;
>  
>  		tplg->pos += (sizeof(struct snd_soc_tplg_bytes_control)
> +
>  			      le32_to_cpu(be->priv.size));
> @@ -1529,10 +1525,8 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_dbytes_create(
>  			be->hdr.name, be->hdr.access);
>  
>  		kc[i].name = kstrdup(be->hdr.name, GFP_KERNEL);
> -		if (kc[i].name == NULL) {
> -			kfree(sbe);
> -			goto err;
> -		}
> +		if (kc[i].name == NULL)
> +			goto err_sbe;
>  		kc[i].private_value = (long)sbe;
>  		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
>  		kc[i].access = be->hdr.access;
> @@ -1544,8 +1538,7 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_dbytes_create(
>  		err = soc_tplg_kcontrol_bind_io(&be->hdr, &kc[i],
> tplg);
>  		if (err) {
>  			soc_control_err(tplg, &be->hdr, be->hdr.name);
> -			kfree(sbe);
> -			continue;
> +			goto err_sbe;
>  		}
>  
>  		/* pass control to driver for optional further init */
> @@ -1554,20 +1547,20 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_dbytes_create(
>  		if (err < 0) {
>  			dev_err(tplg->dev, "ASoC: failed to init %s\n",
>  				be->hdr.name);
> -			kfree(sbe);
> -			continue;
> +			goto err_sbe;
>  		}
>  	}
>  
>  	return kc;
>  
> -err:
> -	for (--i; i >= 0; i--) {
> -		kfree((void *)kc[i].private_value);
> +err_sbe:
> +	for (; i >= 0; i--) {
> +		sbe = (struct soc_bytes_ext *)kc[i].private_value;
> +		kfree(sbe);
>  		kfree(kc[i].name);
>  	}
> -
>  	kfree(kc);
> +
>  	return NULL;
>  }
>  

Re: [PATCH v2 03/11] ASoC: compress: Fix memory leak from snd_soc_new_compress

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

On Mon, Jun 17, 2019 at 01:36:36PM +0200, Amadeusz Sławiński wrote:
> Change kzalloc to devm_kzalloc, so compr gets automatically freed when
> it's no longer needed.
> 
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
> ---
>  sound/soc/soc-compress.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)

This will mean it'll get freed at some point if the driver for the
device it's allocated against gets removed but it will still get
allocated every time the card gets instntiated (eg, due to deferred
probe or due to some other driver getting probed and removed).

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

Re: [PATCH v2 00/11] Fix driver reload issues

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

On Mon, Jun 17, 2019 at 01:36:33PM +0200, Amadeusz Sławiński wrote:
> Hi,
> 
> This series of patches introduces fixes to various issues found while
> trying to unload all snd* modules and then loading them again. This
> allows for modules to be really _modules_ and be unloaded and loaded on
> demand, making it easier to develop and test them without constant
> system reboots.

Pierre?  You did comment on the general concept in one of the patches
but not on any of the patches directly.

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

Re: [alsa-devel] [PATCH v2 00/11] Fix driver reload issues

[Pierre-Louis Bossart]

On 6/25/19 7:04 AM, Mark Brown wrote:
> On Mon, Jun 17, 2019 at 01:36:33PM +0200, Amadeusz Sławiński wrote:
>> Hi,
>>
>> This series of patches introduces fixes to various issues found while
>> trying to unload all snd* modules and then loading them again. This
>> allows for modules to be really _modules_ and be unloaded and loaded on
>> demand, making it easier to develop and test them without constant
>> system reboots.
> 
> Pierre?  You did comment on the general concept in one of the patches
> but not on any of the patches directly.

I did review the patches internally and the v1. For the v2 I could only 
do an airport lounge review and didn't see any blatant issues, so feel 
free to take the following tag for the series.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Re: [PATCH v2 05/11] ASoC: Intel: Skylake: Remove static table index when parsing topology

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

On Mon, Jun 17, 2019 at 01:36:38PM +0200, Amadeusz Sławiński wrote:
> Currently when we remove and reload driver we use previous ref_count
> value to start iterating over skl->modules which leads to out of table
> access. To fix this just inline the function and calculate indexes
> everytime we parse UUID token.

This doesn't apply against current code, please check and resend.

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