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(-)
--
2.17.1
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 <[email protected]>
Signed-off-by: Amadeusz Sławiński <[email protected]>
Signed-off-by: Pierre-Louis Bossart <[email protected]>
---
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);
--
2.17.1
Change kzalloc to devm_kzalloc, so compr gets automatically freed when
it's no longer needed.
Signed-off-by: Amadeusz Sławiński <[email protected]>
---
sound/soc/soc-compress.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/sound/soc/soc-compress.c b/sound/soc/soc-compress.c
index 03d5b9ccd3fc..ddef4ff677ce 100644
--- a/sound/soc/soc-compress.c
+++ b/sound/soc/soc-compress.c
@@ -896,16 +896,14 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
else
direction = SND_COMPRESS_CAPTURE;
- compr = kzalloc(sizeof(*compr), GFP_KERNEL);
+ compr = devm_kzalloc(rtd->card->dev, sizeof(*compr), GFP_KERNEL);
if (!compr)
return -ENOMEM;
compr->ops = devm_kzalloc(rtd->card->dev, sizeof(soc_compr_ops),
GFP_KERNEL);
- if (!compr->ops) {
- ret = -ENOMEM;
- goto compr_err;
- }
+ if (!compr->ops)
+ return -ENOMEM;
if (rtd->dai_link->dynamic) {
snprintf(new_name, sizeof(new_name), "(%s)",
@@ -918,7 +916,7 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
dev_err(rtd->card->dev,
"Compress ASoC: can't create compressed for %s: %d\n",
rtd->dai_link->name, ret);
- goto compr_err;
+ return ret;
}
rtd->pcm = be_pcm;
@@ -954,7 +952,7 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
dev_err(component->dev,
"Compress ASoC: can't create compress for codec %s: %d\n",
component->name, ret);
- goto compr_err;
+ return ret;
}
/* DAPM dai link stream work */
@@ -965,10 +963,7 @@ int snd_soc_new_compress(struct snd_soc_pcm_runtime *rtd, int num)
dev_info(rtd->card->dev, "Compress ASoC: %s <-> %s mapping ok\n",
codec_dai->name, cpu_dai->name);
- return ret;
-compr_err:
- kfree(compr);
- return ret;
+ return 0;
}
EXPORT_SYMBOL_GPL(snd_soc_new_compress);
--
2.17.1
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 <[email protected]>
---
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)
--
2.17.1
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 <[email protected]>
---
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);
--
2.17.1
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 <[email protected]>
---
sound/soc/intel/skylake/skl-ssp-clk.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c b/sound/soc/intel/skylake/skl-ssp-clk.c
index cda1b5fa7436..5bb6e40d4d3e 100644
--- a/sound/soc/intel/skylake/skl-ssp-clk.c
+++ b/sound/soc/intel/skylake/skl-ssp-clk.c
@@ -276,10 +276,8 @@ static void unregister_parent_src_clk(struct skl_clk_parent *pclk,
static void unregister_src_clk(struct skl_clk_data *dclk)
{
- u8 cnt = dclk->avail_clk_cnt;
-
- while (cnt--)
- clkdev_drop(dclk->clk[cnt]->lookup);
+ while (dclk->avail_clk_cnt--)
+ clkdev_drop(dclk->clk[dclk->avail_clk_cnt]->lookup);
}
static int skl_register_parent_clks(struct device *dev,
@@ -381,13 +379,13 @@ static int skl_clk_dev_probe(struct platform_device *pdev)
if (clks[i].rate_cfg[0].rate == 0)
continue;
- data->clk[i] = register_skl_clk(dev, &clks[i], clk_pdata, i);
- if (IS_ERR(data->clk[i])) {
- ret = PTR_ERR(data->clk[i]);
+ data->clk[data->avail_clk_cnt] = register_skl_clk(dev,
+ &clks[i], clk_pdata, i);
+
+ if (IS_ERR(data->clk[data->avail_clk_cnt])) {
+ ret = PTR_ERR(data->clk[data->avail_clk_cnt++]);
goto err_unreg_skl_clk;
}
-
- data->avail_clk_cnt++;
}
platform_set_drvdata(pdev, data);
--
2.17.1
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 <[email protected]>
---
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;
}
--
2.17.1
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 <[email protected]>
---
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);
--
2.17.1
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 <[email protected]>
---
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
--
2.17.1
From: Amadeusz Sławiński <[email protected]>
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 <[email protected]>
---
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);
--
2.17.1
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 <[email protected]>
---
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);
--
2.17.1
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 <[email protected]>
---
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);
--
2.17.1
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 <[email protected]>
---
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);
}
--
2.17.1
Provide helper functions and use them to free dtexts and dvalues in
topology. This is followup cleanup after related changes in this area as
suggested in:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-January/144761.html
Signed-off-by: Amadeusz Sławiński <[email protected]>
---
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);
--
2.17.1
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 <[email protected]>
---
sound/soc/intel/skylake/skl-topology.c | 35 ++++++++++----------------
1 file changed, 13 insertions(+), 22 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index c69d999d7bf1..44f3b29a7210 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -3477,25 +3477,6 @@ static int skl_tplg_get_int_tkn(struct device *dev,
return tkn_count;
}
-static int skl_tplg_get_manifest_uuid(struct device *dev,
- struct skl *skl,
- struct snd_soc_tplg_vendor_uuid_elem *uuid_tkn)
-{
- static int ref_count;
- struct skl_module *mod;
-
- if (uuid_tkn->token == SKL_TKN_UUID) {
- mod = skl->modules[ref_count];
- memcpy(&mod->uuid, &uuid_tkn->uuid, sizeof(uuid_tkn->uuid));
- ref_count++;
- } else {
- dev_err(dev, "Not an UUID token tkn %d\n", uuid_tkn->token);
- return -EINVAL;
- }
-
- return 0;
-}
-
/*
* Fill the manifest structure by parsing the tokens based on the
* type.
@@ -3506,6 +3487,7 @@ static int skl_tplg_get_manifest_tkn(struct device *dev,
{
int tkn_count = 0, ret;
int off = 0, tuple_size = 0;
+ u8 uuid_index = 0;
struct snd_soc_tplg_vendor_array *array;
struct snd_soc_tplg_vendor_value_elem *tkn_elem;
@@ -3528,9 +3510,18 @@ static int skl_tplg_get_manifest_tkn(struct device *dev,
continue;
case SND_SOC_TPLG_TUPLE_TYPE_UUID:
- ret = skl_tplg_get_manifest_uuid(dev, skl, array->uuid);
- if (ret < 0)
- return ret;
+ if (array->uuid->token != SKL_TKN_UUID) {
+ dev_err(dev, "Not an UUID token: %d\n",
+ array->uuid->token);
+ return -EINVAL;
+ }
+ if (uuid_index >= skl->nr_modules) {
+ dev_err(dev, "Too many UUID tokens\n");
+ return -EINVAL;
+ }
+ memcpy(&skl->modules[uuid_index++]->uuid,
+ &array->uuid->uuid,
+ sizeof(array->uuid->uuid));
tuple_size += sizeof(*array->uuid);
continue;
--
2.17.1
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 <[email protected]>
> Signed-off-by: Amadeusz Sławiński <[email protected]>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
> ---
> 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:
> From: Amadeusz Sławiński <[email protected]>
>
> 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 <[email protected]>
> ---
> 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);
>
On Wed, 5 Jun 2019 10:06:47 -0500
Pierre-Louis Bossart <[email protected]> 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
> > <[email protected]> Signed-off-by: Amadeusz Sławiński
> > <[email protected]> Signed-off-by: Pierre-Louis
> > Bossart <[email protected]> ---
> > 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
> [email protected]
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
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 <[email protected]>
> ---
> 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);
> 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 Mon, 10 Jun 2019 09:17:21 +0200
Cezary Rojewski <[email protected]> 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
> > <[email protected]> ---
> > 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);
> > 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);
> >
> _______________________________________________
> Alsa-devel mailing list
> [email protected]
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel