[alsa-devel] [PATCH v2 00/11] 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.
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(-)
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);
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;
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
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);
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).
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
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;
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.
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)
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);
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 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); }
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);
}
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);
}
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);
}
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);
}
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
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
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
- rmmod sof_pci_dev
- rmmod snd_soc_sst_bxt_rt298
- 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
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
- rmmod sof_pci_dev
- rmmod snd_soc_sst_bxt_rt298
- 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
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
- rmmod sof_pci_dev
- rmmod snd_soc_sst_bxt_rt298
- 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.
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
- rmmod sof_pci_dev
- rmmod snd_soc_sst_bxt_rt298
- 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.
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 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);
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; }
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;@@ -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;-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);- 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;
}@@ -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;@@ -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);
- return NULL;
}
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.
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
participants (5)
-
Amadeusz Sławiński -
Mark Brown -
Pierre-Louis Bossart -
Ranjani Sridharan -
Takashi Iwai