[alsa-devel] [PATCH 00/14] Fix driver reload issues
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.
Small note: Patch 2 in this series was already send to this list along with SOF counterpart, however it seems that there is some problem: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-May/149638.html and related patch on SOF side (with discussion): https://mailman.alsa-project.org/pipermail/alsa-devel/2019-May/149640.html It is included in this patchset for completeness.
Amadeusz Sławiński (14): ASoC: Intel: Skylake: Initialize lists before access so they are safe to use ALSA: hdac: fix memory release for SST and SOF drivers 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 SoC: rt274: Fix internal jack assignment in set_jack callback ASoC: core: Tell codec that jack is being removed 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 | 12 ++- sound/soc/codecs/hdac_hdmi.c | 6 ++ sound/soc/codecs/rt274.c | 3 +- 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-core.c | 1 + sound/soc/soc-topology.c | 114 ++++++++++++------------- 13 files changed, 143 insertions(+), 115 deletions(-)
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 9735e2412251..2a0ba40d8098 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -1486,9 +1486,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 f864f7b3df3a..6d6401410250 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -438,7 +438,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); @@ -867,6 +866,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 @@ -1116,6 +1118,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);
During the integration of HDaudio support, we changed the way in which we get hdev in snd_hdac_ext_bus_device_init() to use one preallocated with devm_kzalloc(), however it still left kfree(hdev) in snd_hdac_ext_bus_device_exit(). It leads to oopses when trying to rmmod and modprobe. Fix it, by just removing kfree call.
SOF also uses some of the snd_hdac_ functions for HDAudio support but allocated the memory with kzalloc. A matching fix is provided separately to align all users of the snd_hdac_ library.
Fixes: 6298542fa33b ("ALSA: hdac: remove memory allocation from snd_hdac_ext_bus_device_init") Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/hda/ext/hdac_ext_bus.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c index c203af71a099..f33ba58b753c 100644 --- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c @@ -170,7 +170,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_init); void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev) { snd_hdac_device_exit(hdev); - kfree(hdev); } EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_exit);
On 6/5/19 8:45 AM, Amadeusz Sławiński wrote:
During the integration of HDaudio support, we changed the way in which we get hdev in snd_hdac_ext_bus_device_init() to use one preallocated with devm_kzalloc(), however it still left kfree(hdev) in snd_hdac_ext_bus_device_exit(). It leads to oopses when trying to rmmod and modprobe. Fix it, by just removing kfree call.
SOF also uses some of the snd_hdac_ functions for HDAudio support but allocated the memory with kzalloc. A matching fix is provided separately to align all users of the snd_hdac_ library.
There are stability issues with this change (already shared in a separate series) and additional findings reported by Libin so this should not be applied for now.
Fixes: 6298542fa33b ("ALSA: hdac: remove memory allocation from snd_hdac_ext_bus_device_init") Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
sound/hda/ext/hdac_ext_bus.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c index c203af71a099..f33ba58b753c 100644 --- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c @@ -170,7 +170,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_init); void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev) { snd_hdac_device_exit(hdev);
- kfree(hdev); } EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_exit);
On Wed, 5 Jun 2019 10:06:47 -0500 Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:
On 6/5/19 8:45 AM, Amadeusz Sławiński wrote:
During the integration of HDaudio support, we changed the way in which we get hdev in snd_hdac_ext_bus_device_init() to use one preallocated with devm_kzalloc(), however it still left kfree(hdev) in snd_hdac_ext_bus_device_exit(). It leads to oopses when trying to rmmod and modprobe. Fix it, by just removing kfree call.
SOF also uses some of the snd_hdac_ functions for HDAudio support but allocated the memory with kzalloc. A matching fix is provided separately to align all users of the snd_hdac_ library.
There are stability issues with this change (already shared in a separate series) and additional findings reported by Libin so this should not be applied for now.
Yes, as mentioned in cover letter there is pending discussion, I provided it for completeness.
Fixes: 6298542fa33b ("ALSA: hdac: remove memory allocation from snd_hdac_ext_bus_device_init") Reviewed-by: Takashi Iwai tiwai@suse.de Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- sound/hda/ext/hdac_ext_bus.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c index c203af71a099..f33ba58b753c 100644 --- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c @@ -170,7 +170,6 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_init); void snd_hdac_ext_bus_device_exit(struct hdac_device *hdev) { snd_hdac_device_exit(hdev);
- kfree(hdev); } EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_device_exit);
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
From: Amadeusz Sławiński amadeuszx.slawinski@intel.com
This resets internal index used for enumarating codecs. This will only work on assumption that platform has one codec. Anyway if there is more, it won't work with current machine drivers, because we can't guarantee order in which they are enumerated. This workarounds the fact that most intel machine drivers have the following defined: .codec_name = "ehdaudio0D2", However when we unload and reload machine driver idx gets incremented, so .codec_name would've needed to be set to ehdaudio1D2 on first reload and so on.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@intel.com --- sound/hda/ext/hdac_ext_bus.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c index f33ba58b753c..c84d69c2eba4 100644 --- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c @@ -77,6 +77,8 @@ static const struct hdac_io_ops hdac_ext_default_io = { .dma_free_pages = hdac_ext_dma_free_pages, };
+static int idx; + /** * snd_hdac_ext_bus_init - initialize a HD-audio extended bus * @ebus: the pointer to extended bus object @@ -93,7 +95,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) @@ -118,6 +119,14 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_init); void snd_hdac_ext_bus_exit(struct hdac_bus *bus) { snd_hdac_bus_exit(bus); + /* FIXME: this is workaround + * reset index used for bus->idx, because machine drivers expect + * the codec name to be ehdaudio0D2, where 0 is bus->idx + * we only perform reset if there is one used device, if there is more + * all bets are off + */ + if (idx == 1) + idx = 0; WARN_ON(!list_empty(&bus->hlink_list)); } EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_exit);
On 6/5/19 8:45 AM, Amadeusz Sławiński wrote:
From: Amadeusz Sławiński amadeuszx.slawinski@intel.com
This resets internal index used for enumarating codecs. This will only work on assumption that platform has one codec. Anyway if there is more, it won't work with current machine drivers, because we can't guarantee order in which they are enumerated. This workarounds the fact that most intel machine drivers have the following defined: .codec_name = "ehdaudio0D2", However when we unload and reload machine driver idx gets incremented, so .codec_name would've needed to be set to ehdaudio1D2 on first reload and so on.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@intel.com
sound/hda/ext/hdac_ext_bus.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/sound/hda/ext/hdac_ext_bus.c b/sound/hda/ext/hdac_ext_bus.c index f33ba58b753c..c84d69c2eba4 100644 --- a/sound/hda/ext/hdac_ext_bus.c +++ b/sound/hda/ext/hdac_ext_bus.c @@ -77,6 +77,8 @@ static const struct hdac_io_ops hdac_ext_default_io = { .dma_free_pages = hdac_ext_dma_free_pages, };
+static int idx;
- /**
- snd_hdac_ext_bus_init - initialize a HD-audio extended bus
- @ebus: the pointer to extended bus object
@@ -93,7 +95,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)
@@ -118,6 +119,14 @@ EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_init); void snd_hdac_ext_bus_exit(struct hdac_bus *bus) { snd_hdac_bus_exit(bus);
- /* FIXME: this is workaround
* reset index used for bus->idx, because machine drivers expect* the codec name to be ehdaudio0D2, where 0 is bus->idx* we only perform reset if there is one used device, if there is more* all bets are off*/- if (idx == 1)
idx = 0;
The real fix would be to stop incrementing idx in snd_hdac_ext_bus_init, which would make sense only if we had multiple controllers. SOF pegged bus->idx to zero.
WARN_ON(!list_empty(&bus->hlink_list)); } EXPORT_SYMBOL_GPL(snd_hdac_ext_bus_exit);
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);
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 2a0ba40d8098..44062806fbed 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -1427,11 +1427,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
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;
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 5d7ac2ee7a3c..e81c3dafc0d0 100644 --- a/sound/soc/intel/skylake/skl-debug.c +++ b/sound/soc/intel/skylake/skl-debug.c @@ -259,3 +259,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 85f8bb6687dc..d2e269867a44 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -164,6 +164,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); @@ -172,6 +173,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)
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 44062806fbed..7e8110c15258 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -1459,8 +1459,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 82282cac9751..7d32c61c73e7 100644 --- a/sound/soc/intel/skylake/skl-topology.h +++ b/sound/soc/intel/skylake/skl-topology.h @@ -471,6 +471,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 6d6401410250..e4881ff427ea 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -1119,14 +1119,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);
On 2019-06-05 15:45, Amadeusz Sławiński wrote:
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 44062806fbed..7e8110c15258 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -1459,8 +1459,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);
+}
In debugfs cleanup patch: [PATCH 07/14] ASoC: Intel: Skylake: Add function to cleanup debugfs interface
you define skl_debugfs_exit separately - its usage is split and present in this very patch instead. However, for tplg counterpart - skl_tplg_exit - you've decided to combine both together. Why not separate tplg cleanup too?
diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h index 82282cac9751..7d32c61c73e7 100644 --- a/sound/soc/intel/skylake/skl-topology.h +++ b/sound/soc/intel/skylake/skl-topology.h @@ -471,6 +471,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);diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 6d6401410250..e4881ff427ea 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -1119,14 +1119,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);
On Mon, 10 Jun 2019 09:17:21 +0200 Cezary Rojewski cezary.rojewski@intel.com wrote:
On 2019-06-05 15:45, Amadeusz Sławiński wrote:
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 44062806fbed..7e8110c15258 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -1459,8 +1459,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);
+}
In debugfs cleanup patch: [PATCH 07/14] ASoC: Intel: Skylake: Add function to cleanup debugfs interface
you define skl_debugfs_exit separately - its usage is split and present in this very patch instead. However, for tplg counterpart - skl_tplg_exit - you've decided to combine both together. Why not separate tplg cleanup too?
This is done because skl_debugfs_exit() can be introduced standalone. However skl_tplg_exit() has code that is being moved from other places. If I introduced it in separate commit, other one would be adding call to this function while removing things that happen inside, losing part of information, about the fact that code is actually just being moved.
diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h index 82282cac9751..7d32c61c73e7 100644 --- a/sound/soc/intel/skylake/skl-topology.h +++ b/sound/soc/intel/skylake/skl-topology.h @@ -471,6 +471,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);diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 6d6401410250..e4881ff427ea 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -1119,14 +1119,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);
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
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);
When we call snd_soc_component_set_jack(component, NULL, NULL) we should set rt274->jack to passed jack, so when interrupt is triggered it calls snd_soc_jack_report(rt274->jack, ...) with proper value.
This fixes problem in machine where in register, we call snd_soc_register(component, &headset, NULL), which just calls rt274_mic_detect via callback. Now when machine driver is removed "headset" will be gone, so we need to tell codec driver that it's gone with: snd_soc_register(component, NULL, NULL), but we also need to be able to handle NULL jack argument here gracefully. If we don't set it to NULL, next time the rt274_irq runs it will call snd_soc_jack_report with first argument being invalid pointer and there will be Oops.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/codecs/rt274.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/rt274.c b/sound/soc/codecs/rt274.c index adf59039a3b6..cdd312db3e78 100644 --- a/sound/soc/codecs/rt274.c +++ b/sound/soc/codecs/rt274.c @@ -405,6 +405,8 @@ static int rt274_mic_detect(struct snd_soc_component *component, { struct rt274_priv *rt274 = snd_soc_component_get_drvdata(component);
+ rt274->jack = jack; + if (jack == NULL) { /* Disable jack detection */ regmap_update_bits(rt274->regmap, RT274_EAPD_GPIO_IRQ_CTRL, @@ -412,7 +414,6 @@ static int rt274_mic_detect(struct snd_soc_component *component,
return 0; } - rt274->jack = jack;
regmap_update_bits(rt274->regmap, RT274_EAPD_GPIO_IRQ_CTRL, RT274_IRQ_EN, RT274_IRQ_EN);
When component is being removed we should disable jack, otherwise some codecs will try to trigger interrupt using freed structures.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/soc-core.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 7abb017a83f3..ace5fb01d9a0 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -951,6 +951,7 @@ static int soc_bind_dai_link(struct snd_soc_card *card,
static void soc_cleanup_component(struct snd_soc_component *component) { + snd_soc_component_set_jack(component, NULL, NULL); list_del(&component->card_list); snd_soc_dapm_free(snd_soc_component_get_dapm(component)); soc_cleanup_component_debugfs(component);
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 660e0587f399..da3835b703f5 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -1867,6 +1867,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); }
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.ht...
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 3299ebb48c1a..c09853467d35 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);
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 c09853467d35..03dc4f101d58 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; }
participants (3)
-
Amadeusz Sławiński -
Cezary Rojewski -
Pierre-Louis Bossart