[PATCH 0/7] ASoC: topology: fixes and improvements

[PATCH 0/7] ASoC: topology: fixes and improvements

[Pierre-Louis Bossart]

Module load/unload tests exposed a set of weaknesses in both the SOF
and Skylake drivers, some of which are related to the topology
framework. This patchset provides a combined set of fixes identified
by the two teams.

DAPM routes were also not properly handled, leading to confusing
issues when the topology resources were released.

All these patches were tested on top of the SOF tree, which tracks
Mark's branch on a weekly basis.

Amadeusz Sławiński (3):
  ASoC: topology: Reduce number of dereferences when accessing dobj
  ASoC: topology: Remove widgets from dobj list
  ASoC: topology: Fix memory leak from soc_tplg_denum_create_texts

Bard liao (2):
  ASoC: topology: fix memory leak in soc_tplg_dapm_widget_create
  ASoC: topology: unload physical dai link in remove

Ranjani Sridharan (2):
  ASoC: topology: add SND_SOC_DOBJ_GRAPH type for dapm routes
  ASoC: topology: modify dapm route loading routine and add dapm route
    unloading

 include/sound/soc-dapm.h     |   2 +
 include/sound/soc-topology.h |   8 +-
 sound/soc/soc-topology.c     | 165 +++++++++++++++++++++++++++++------
 3 files changed, 144 insertions(+), 31 deletions(-)

-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 1/7] ASoC: topology: Reduce number of dereferences when accessing dobj

[Pierre-Louis Bossart]

From: Amadeusz Sławiński <amadeuszx.slawinski@intel.com>

We already have passed dobj, there is no reason to access it through
containing structs.

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

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 045ef136903d..b02c41614f96 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -382,10 +382,10 @@ static void remove_mixer(struct snd_soc_component *comp,
 	if (dobj->ops && dobj->ops->control_unload)
 		dobj->ops->control_unload(comp, dobj);
 
-	if (sm->dobj.control.kcontrol->tlv.p)
-		p = sm->dobj.control.kcontrol->tlv.p;
-	snd_ctl_remove(card, sm->dobj.control.kcontrol);
-	list_del(&sm->dobj.list);
+	if (dobj->control.kcontrol->tlv.p)
+		p = dobj->control.kcontrol->tlv.p;
+	snd_ctl_remove(card, dobj->control.kcontrol);
+	list_del(&dobj->list);
 	kfree(sm);
 	kfree(p);
 }
@@ -404,12 +404,12 @@ static void remove_enum(struct snd_soc_component *comp,
 	if (dobj->ops && dobj->ops->control_unload)
 		dobj->ops->control_unload(comp, dobj);
 
-	snd_ctl_remove(card, se->dobj.control.kcontrol);
-	list_del(&se->dobj.list);
+	snd_ctl_remove(card, dobj->control.kcontrol);
+	list_del(&dobj->list);
 
-	kfree(se->dobj.control.dvalues);
+	kfree(dobj->control.dvalues);
 	for (i = 0; i < se->items; i++)
-		kfree(se->dobj.control.dtexts[i]);
+		kfree(dobj->control.dtexts[i]);
 	kfree(se);
 }
 
@@ -427,8 +427,8 @@ static void remove_bytes(struct snd_soc_component *comp,
 	if (dobj->ops && dobj->ops->control_unload)
 		dobj->ops->control_unload(comp, dobj);
 
-	snd_ctl_remove(card, sb->dobj.control.kcontrol);
-	list_del(&sb->dobj.list);
+	snd_ctl_remove(card, dobj->control.kcontrol);
+	list_del(&dobj->list);
 	kfree(sb);
 }
 
@@ -464,9 +464,9 @@ static void remove_widget(struct snd_soc_component *comp,
 
 			snd_ctl_remove(card, kcontrol);
 
-			kfree(se->dobj.control.dvalues);
+			kfree(dobj->control.dvalues);
 			for (j = 0; j < se->items; j++)
-				kfree(se->dobj.control.dtexts[j]);
+				kfree(dobj->control.dtexts[j]);
 
 			kfree(se);
 			kfree(w->kcontrol_news[i].name);
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 2/7] ASoC: topology: Remove widgets from dobj list

[Pierre-Louis Bossart]

From: Amadeusz Sławiński <amadeuszx.slawinski@intel.com>

Currently when we unload and reload machine driver few times we end with
corrupted list and try to cleanup no longer existing objects. Fix this
by removing dobj from the list.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/soc-topology.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index b02c41614f96..abc2d804d5bf 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -493,6 +493,8 @@ static void remove_widget(struct snd_soc_component *comp,
 free_news:
 	kfree(w->kcontrol_news);
 
+	list_del(&dobj->list);
+
 	/* widget w is freed by soc-dapm.c */
 }
 
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 3/7] ASoC: topology: Fix memory leak from soc_tplg_denum_create_texts

[Pierre-Louis Bossart]

From: Amadeusz Sławiński <amadeuszx.slawinski@intel.com>

dtexts is two dimensional array, so we also need to free it after
freeing its fields.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/soc-topology.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index abc2d804d5bf..71bc5b8a9bd3 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -410,6 +410,7 @@ static void remove_enum(struct snd_soc_component *comp,
 	kfree(dobj->control.dvalues);
 	for (i = 0; i < se->items; i++)
 		kfree(dobj->control.dtexts[i]);
+	kfree(dobj->control.dtexts);
 	kfree(se);
 }
 
@@ -467,6 +468,7 @@ static void remove_widget(struct snd_soc_component *comp,
 			kfree(dobj->control.dvalues);
 			for (j = 0; j < se->items; j++)
 				kfree(dobj->control.dtexts[j]);
+			kfree(dobj->control.dtexts);
 
 			kfree(se);
 			kfree(w->kcontrol_news[i].name);
@@ -1361,6 +1363,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
 		kfree(se->dobj.control.dvalues);
 		for (j = 0; j < ec->items; j++)
 			kfree(se->dobj.control.dtexts[j]);
+		kfree(se->dobj.control.dtexts);
 
 		kfree(se);
 		kfree(kc[i].name);
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 4/7] ASoC: topology: fix memory leak in soc_tplg_dapm_widget_create

[Pierre-Louis Bossart]

From: Bard liao <yung-chuan.liao@linux.intel.com>

template.sname and template.name are only freed when an error occur.
They should be freed in the success return case, too.

Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/soc-topology.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 71bc5b8a9bd3..2cb0a05e2368 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1583,6 +1583,9 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg,
 	if (ret < 0)
 		goto ready_err;
 
+	kfree(template.sname);
+	kfree(template.name);
+
 	return 0;
 
 ready_err:
-- 
2.17.1

[PATCH 5/7] ASoC: topology: add SND_SOC_DOBJ_GRAPH type for dapm routes

[Pierre-Louis Bossart]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

Add a new dobj type SND_SOC_DOBJ_GRAPH for dapm routes
and add snd_soc_dobj member to struct snd_soc_dapm_route.
This enables device drivers to save driver specific
data pertaining to dapm routes and also be able
to clean up the data when the driver module is unloaded.

Also, reorder the snd_soc_dobj_type types to align with
matching topology header types.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/soc-dapm.h     | 2 ++
 include/sound/soc-topology.h | 7 ++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index bd8163f151cb..46f2ba3ffcb7 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -540,6 +540,8 @@ struct snd_soc_dapm_route {
 	/* Note: currently only supported for links where source is a supply */
 	int (*connected)(struct snd_soc_dapm_widget *source,
 			 struct snd_soc_dapm_widget *sink);
+
+	struct snd_soc_dobj dobj;
 };
 
 /* dapm audio path between two widgets */
diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h
index fa4b8413d2e2..8c43cfc240fa 100644
--- a/include/sound/soc-topology.h
+++ b/include/sound/soc-topology.h
@@ -38,12 +38,13 @@ struct snd_soc_dapm_route;
 enum snd_soc_dobj_type {
 	SND_SOC_DOBJ_NONE		= 0,	/* object is not dynamic */
 	SND_SOC_DOBJ_MIXER,
-	SND_SOC_DOBJ_ENUM,
 	SND_SOC_DOBJ_BYTES,
-	SND_SOC_DOBJ_PCM,
+	SND_SOC_DOBJ_ENUM,
+	SND_SOC_DOBJ_GRAPH,
+	SND_SOC_DOBJ_WIDGET,
 	SND_SOC_DOBJ_DAI_LINK,
+	SND_SOC_DOBJ_PCM,
 	SND_SOC_DOBJ_CODEC_LINK,
-	SND_SOC_DOBJ_WIDGET,
 };
 
 /* dynamic control object */
-- 
2.17.1

[PATCH 6/7] ASoC: topology: modify dapm route loading routine and add dapm route unloading

[Pierre-Louis Bossart]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

struct snd_soc_dapm_route has been modified to be a dynamic
object so that it can be used to save driver specific
data while parsing topology and clean up
driver-specific data during driver unloading.

This patch makes the following changes to accomplish the above:
1. Set the dobj member of snd_soc_dapm_route during the
SOC_TPLG_PASS_GRAPH pass of topology parsing.
2. Add the remove_route() routine that will be called while
removing all dynamic objects from the component driver.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/soc-topology.c | 102 +++++++++++++++++++++++++++++++++------
 1 file changed, 86 insertions(+), 16 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 2cb0a05e2368..23d421370e6c 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -433,6 +433,23 @@ static void remove_bytes(struct snd_soc_component *comp,
 	kfree(sb);
 }
 
+/* remove a route */
+static void remove_route(struct snd_soc_component *comp,
+			 struct snd_soc_dobj *dobj, int pass)
+{
+	struct snd_soc_dapm_route *route =
+		container_of(dobj, struct snd_soc_dapm_route, dobj);
+
+	if (pass != SOC_TPLG_PASS_GRAPH)
+		return;
+
+	if (dobj->ops && dobj->ops->dapm_route_unload)
+		dobj->ops->dapm_route_unload(comp, dobj);
+
+	list_del(&dobj->list);
+	kfree(route);
+}
+
 /* remove a widget and it's kcontrols - routes must be removed first */
 static void remove_widget(struct snd_soc_component *comp,
 	struct snd_soc_dobj *dobj, int pass)
@@ -1119,9 +1136,10 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 	struct snd_soc_tplg_hdr *hdr)
 {
 	struct snd_soc_dapm_context *dapm = &tplg->comp->dapm;
-	struct snd_soc_dapm_route route;
 	struct snd_soc_tplg_dapm_graph_elem *elem;
-	int count = hdr->count, i;
+	struct snd_soc_dapm_route **routes;
+	int count = hdr->count, i, j;
+	int ret = 0;
 
 	if (tplg->pass != SOC_TPLG_PASS_GRAPH) {
 		tplg->pos += hdr->size + hdr->payload_size;
@@ -1140,36 +1158,85 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 	dev_dbg(tplg->dev, "ASoC: adding %d DAPM routes for index %d\n", count,
 		hdr->index);
 
+	/* allocate memory for pointer to array of dapm routes */
+	routes = kcalloc(count, sizeof(struct snd_soc_dapm_route *),
+			 GFP_KERNEL);
+	if (!routes)
+		return -ENOMEM;
+
+	/*
+	 * allocate memory for each dapm route in the array.
+	 * This needs to be done individually so that
+	 * each route can be freed when it is removed in remove_route().
+	 */
+	for (i = 0; i < count; i++) {
+		routes[i] = kzalloc(sizeof(*routes[i]), GFP_KERNEL);
+		if (!routes[i]) {
+			/* free previously allocated memory */
+			for (j = 0; j < i; j++)
+				kfree(routes[j]);
+
+			kfree(routes);
+			return -ENOMEM;
+		}
+	}
+
 	for (i = 0; i < count; i++) {
 		elem = (struct snd_soc_tplg_dapm_graph_elem *)tplg->pos;
 		tplg->pos += sizeof(struct snd_soc_tplg_dapm_graph_elem);
 
 		/* validate routes */
 		if (strnlen(elem->source, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			return -EINVAL;
+			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
+			ret = -EINVAL;
+			break;
+		}
 		if (strnlen(elem->sink, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			return -EINVAL;
+			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
+			ret = -EINVAL;
+			break;
+		}
 		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			return -EINVAL;
+			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
+			ret = -EINVAL;
+			break;
+		}
+
+		routes[i]->source = elem->source;
+		routes[i]->sink = elem->sink;
 
-		route.source = elem->source;
-		route.sink = elem->sink;
-		route.connected = NULL; /* set to NULL atm for tplg users */
+		/* set to NULL atm for tplg users */
+		routes[i]->connected = NULL;
 		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
-			route.control = NULL;
+			routes[i]->control = NULL;
 		else
-			route.control = elem->control;
+			routes[i]->control = elem->control;
+
+		/* add route dobj to dobj_list */
+		routes[i]->dobj.type = SND_SOC_DOBJ_GRAPH;
+		routes[i]->dobj.ops = tplg->ops;
+		routes[i]->dobj.index = tplg->index;
+		list_add(&routes[i]->dobj.list, &tplg->comp->dobj_list);
 
-		soc_tplg_add_route(tplg, &route);
+		soc_tplg_add_route(tplg, routes[i]);
 
 		/* add route, but keep going if some fail */
-		snd_soc_dapm_add_routes(dapm, &route, 1);
+		snd_soc_dapm_add_routes(dapm, routes[i], 1);
 	}
 
-	return 0;
+	/* free memory allocated for all dapm routes in case of error */
+	if (ret < 0)
+		for (i = 0; i < count ; i++)
+			kfree(routes[i]);
+
+	/*
+	 * free pointer to array of dapm routes as this is no longer needed.
+	 * The memory allocated for each dapm route will be freed
+	 * when it is removed in remove_route().
+	 */
+	kfree(routes);
+
+	return ret;
 }
 
 static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
@@ -2570,6 +2637,9 @@ int snd_soc_tplg_component_remove(struct snd_soc_component *comp, u32 index)
 			case SND_SOC_DOBJ_BYTES:
 				remove_bytes(comp, dobj, pass);
 				break;
+			case SND_SOC_DOBJ_GRAPH:
+				remove_route(comp, dobj, pass);
+				break;
 			case SND_SOC_DOBJ_WIDGET:
 				remove_widget(comp, dobj, pass);
 				break;
-- 
2.17.1

[PATCH 7/7] ASoC: topology: unload physical dai link in remove

[Pierre-Louis Bossart]

From: Bard liao <yung-chuan.liao@linux.intel.com>

soc_tplg_link_config() will find the physical dai link and call
soc_tplg_dai_link_load() to load the BE dai link. Currently remove_link()
is only used to remove the FE dai link which is created by the topology.
The BE dai link cannot however be unloaded in snd_soc_tplg_component
_remove(), which is problematic if anything needs to be released or
reinitialized.

This patch aligns the definitions of dynamic types with the existing
UAPI and adds a new remove_backend_link() routine to unload the the BE
dai link when snd_soc_tplg_component_remove() is invoked.

Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/soc-topology.h |  1 +
 sound/soc/soc-topology.c     | 31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h
index 8c43cfc240fa..5223896de26f 100644
--- a/include/sound/soc-topology.h
+++ b/include/sound/soc-topology.h
@@ -45,6 +45,7 @@ enum snd_soc_dobj_type {
 	SND_SOC_DOBJ_DAI_LINK,
 	SND_SOC_DOBJ_PCM,
 	SND_SOC_DOBJ_CODEC_LINK,
+	SND_SOC_DOBJ_BACKEND_LINK,
 };
 
 /* dynamic control object */
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 23d421370e6c..49e6fd76d9bc 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -557,6 +557,24 @@ static void remove_link(struct snd_soc_component *comp,
 	kfree(link);
 }
 
+/* unload dai link */
+static void remove_backend_link(struct snd_soc_component *comp,
+	struct snd_soc_dobj *dobj, int pass)
+{
+	if (pass != SOC_TPLG_PASS_LINK)
+		return;
+
+	if (dobj->ops && dobj->ops->link_unload)
+		dobj->ops->link_unload(comp, dobj);
+
+	/*
+	 * We don't free the link here as what remove_link() do since BE
+	 * links are not allocated by topology.
+	 */
+
+	list_del(&dobj->list);
+}
+
 /* bind a kcontrol to it's IO handlers */
 static int soc_tplg_kcontrol_bind_io(struct snd_soc_tplg_ctl_hdr *hdr,
 	struct snd_kcontrol_new *k,
@@ -2163,6 +2181,12 @@ static int soc_tplg_link_config(struct soc_tplg *tplg,
 		return ret;
 	}
 
+	/* for unloading it in snd_soc_tplg_component_remove */
+	link->dobj.index = tplg->index;
+	link->dobj.ops = tplg->ops;
+	link->dobj.type = SND_SOC_DOBJ_BACKEND_LINK;
+	list_add(&link->dobj.list, &tplg->comp->dobj_list);
+
 	return 0;
 }
 
@@ -2649,6 +2673,13 @@ int snd_soc_tplg_component_remove(struct snd_soc_component *comp, u32 index)
 			case SND_SOC_DOBJ_DAI_LINK:
 				remove_link(comp, dobj, pass);
 				break;
+			case SND_SOC_DOBJ_BACKEND_LINK:
+				/*
+				 * call link_unload ops if extra
+				 * deinitialization is needed.
+				 */
+				remove_backend_link(comp, dobj, pass);
+				break;
 			default:
 				dev_err(comp->dev, "ASoC: invalid component type %d for removal\n",
 					dobj->type);
-- 
2.17.1

Re: [PATCH 3/7] ASoC: topology: Fix memory leak from soc_tplg_denum_create_texts

[Takashi Iwai]

On Fri, 25 Jan 2019 21:06:44 +0100,
Pierre-Louis Bossart wrote:
> 
> From: Amadeusz Sławiński <amadeuszx.slawinski@intel.com>
> 
> dtexts is two dimensional array, so we also need to free it after
> freeing its fields.
> 
> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/soc/soc-topology.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index abc2d804d5bf..71bc5b8a9bd3 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -410,6 +410,7 @@ static void remove_enum(struct snd_soc_component *comp,
>  	kfree(dobj->control.dvalues);
>  	for (i = 0; i < se->items; i++)
>  		kfree(dobj->control.dtexts[i]);
> +	kfree(dobj->control.dtexts);
>  	kfree(se);
>  }
>  
> @@ -467,6 +468,7 @@ static void remove_widget(struct snd_soc_component *comp,
>  			kfree(dobj->control.dvalues);
>  			for (j = 0; j < se->items; j++)
>  				kfree(dobj->control.dtexts[j]);
> +			kfree(dobj->control.dtexts);
>  
>  			kfree(se);
>  			kfree(w->kcontrol_news[i].name);
> @@ -1361,6 +1363,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
>  		kfree(se->dobj.control.dvalues);
>  		for (j = 0; j < ec->items; j++)
>  			kfree(se->dobj.control.dtexts[j]);
> +		kfree(se->dobj.control.dtexts);
>  
>  		kfree(se);
>  		kfree(kc[i].name);

It's interesting that this patch shows three places doing the very
same thing.  It's worth to make a common helper, I suppose.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/7] ASoC: topology: Fix memory leak from soc_tplg_denum_create_texts

[Pierre-Louis Bossart]

On 1/26/19 11:43 AM, Takashi Iwai wrote:
> On Fri, 25 Jan 2019 21:06:44 +0100,
> Pierre-Louis Bossart wrote:
>> From: Amadeusz Sławiński <amadeuszx.slawinski@intel.com>
>>
>> dtexts is two dimensional array, so we also need to free it after
>> freeing its fields.
>>
>> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@intel.com>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   sound/soc/soc-topology.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>> index abc2d804d5bf..71bc5b8a9bd3 100644
>> --- a/sound/soc/soc-topology.c
>> +++ b/sound/soc/soc-topology.c
>> @@ -410,6 +410,7 @@ static void remove_enum(struct snd_soc_component *comp,
>>   	kfree(dobj->control.dvalues);
>>   	for (i = 0; i < se->items; i++)
>>   		kfree(dobj->control.dtexts[i]);
>> +	kfree(dobj->control.dtexts);
>>   	kfree(se);
>>   }
>>   
>> @@ -467,6 +468,7 @@ static void remove_widget(struct snd_soc_component *comp,
>>   			kfree(dobj->control.dvalues);
>>   			for (j = 0; j < se->items; j++)
>>   				kfree(dobj->control.dtexts[j]);
>> +			kfree(dobj->control.dtexts);
>>   
>>   			kfree(se);
>>   			kfree(w->kcontrol_news[i].name);
>> @@ -1361,6 +1363,7 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
>>   		kfree(se->dobj.control.dvalues);
>>   		for (j = 0; j < ec->items; j++)
>>   			kfree(se->dobj.control.dtexts[j]);
>> +		kfree(se->dobj.control.dtexts);
>>   
>>   		kfree(se);
>>   		kfree(kc[i].name);
> It's interesting that this patch shows three places doing the very
> same thing.  It's worth to make a common helper, I suppose.

Good point indeed, thanks for the suggestion. We'd probably need to dig 
deeper here. There is one loop that uses ce->items and the others 
se->items, the two variables seem similar but we'd need to figure out if 
this is correct.

If that's alright with you I'd prefer to add this helper in a follow-up 
patch.

-Pierre

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Applied "ASoC: topology: modify dapm route loading routine and add dapm route unloading" to the asoc tree

[Mark Brown]

The patch

   ASoC: topology: modify dapm route loading routine and add dapm route unloading

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 7df04ea7a31eaa75bdad2905f07cc097b15558ee Mon Sep 17 00:00:00 2001
From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Date: Fri, 25 Jan 2019 14:06:47 -0600
Subject: [PATCH] ASoC: topology: modify dapm route loading routine and add
 dapm route unloading

struct snd_soc_dapm_route has been modified to be a dynamic
object so that it can be used to save driver specific
data while parsing topology and clean up
driver-specific data during driver unloading.

This patch makes the following changes to accomplish the above:
1. Set the dobj member of snd_soc_dapm_route during the
SOC_TPLG_PASS_GRAPH pass of topology parsing.
2. Add the remove_route() routine that will be called while
removing all dynamic objects from the component driver.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/soc-topology.c | 102 +++++++++++++++++++++++++++++++++------
 1 file changed, 86 insertions(+), 16 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 2cb0a05e2368..23d421370e6c 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -433,6 +433,23 @@ static void remove_bytes(struct snd_soc_component *comp,
 	kfree(sb);
 }
 
+/* remove a route */
+static void remove_route(struct snd_soc_component *comp,
+			 struct snd_soc_dobj *dobj, int pass)
+{
+	struct snd_soc_dapm_route *route =
+		container_of(dobj, struct snd_soc_dapm_route, dobj);
+
+	if (pass != SOC_TPLG_PASS_GRAPH)
+		return;
+
+	if (dobj->ops && dobj->ops->dapm_route_unload)
+		dobj->ops->dapm_route_unload(comp, dobj);
+
+	list_del(&dobj->list);
+	kfree(route);
+}
+
 /* remove a widget and it's kcontrols - routes must be removed first */
 static void remove_widget(struct snd_soc_component *comp,
 	struct snd_soc_dobj *dobj, int pass)
@@ -1119,9 +1136,10 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 	struct snd_soc_tplg_hdr *hdr)
 {
 	struct snd_soc_dapm_context *dapm = &tplg->comp->dapm;
-	struct snd_soc_dapm_route route;
 	struct snd_soc_tplg_dapm_graph_elem *elem;
-	int count = hdr->count, i;
+	struct snd_soc_dapm_route **routes;
+	int count = hdr->count, i, j;
+	int ret = 0;
 
 	if (tplg->pass != SOC_TPLG_PASS_GRAPH) {
 		tplg->pos += hdr->size + hdr->payload_size;
@@ -1140,36 +1158,85 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 	dev_dbg(tplg->dev, "ASoC: adding %d DAPM routes for index %d\n", count,
 		hdr->index);
 
+	/* allocate memory for pointer to array of dapm routes */
+	routes = kcalloc(count, sizeof(struct snd_soc_dapm_route *),
+			 GFP_KERNEL);
+	if (!routes)
+		return -ENOMEM;
+
+	/*
+	 * allocate memory for each dapm route in the array.
+	 * This needs to be done individually so that
+	 * each route can be freed when it is removed in remove_route().
+	 */
+	for (i = 0; i < count; i++) {
+		routes[i] = kzalloc(sizeof(*routes[i]), GFP_KERNEL);
+		if (!routes[i]) {
+			/* free previously allocated memory */
+			for (j = 0; j < i; j++)
+				kfree(routes[j]);
+
+			kfree(routes);
+			return -ENOMEM;
+		}
+	}
+
 	for (i = 0; i < count; i++) {
 		elem = (struct snd_soc_tplg_dapm_graph_elem *)tplg->pos;
 		tplg->pos += sizeof(struct snd_soc_tplg_dapm_graph_elem);
 
 		/* validate routes */
 		if (strnlen(elem->source, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			return -EINVAL;
+			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
+			ret = -EINVAL;
+			break;
+		}
 		if (strnlen(elem->sink, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			return -EINVAL;
+			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
+			ret = -EINVAL;
+			break;
+		}
 		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
-			SNDRV_CTL_ELEM_ID_NAME_MAXLEN)
-			return -EINVAL;
+			    SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
+			ret = -EINVAL;
+			break;
+		}
+
+		routes[i]->source = elem->source;
+		routes[i]->sink = elem->sink;
 
-		route.source = elem->source;
-		route.sink = elem->sink;
-		route.connected = NULL; /* set to NULL atm for tplg users */
+		/* set to NULL atm for tplg users */
+		routes[i]->connected = NULL;
 		if (strnlen(elem->control, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
-			route.control = NULL;
+			routes[i]->control = NULL;
 		else
-			route.control = elem->control;
+			routes[i]->control = elem->control;
+
+		/* add route dobj to dobj_list */
+		routes[i]->dobj.type = SND_SOC_DOBJ_GRAPH;
+		routes[i]->dobj.ops = tplg->ops;
+		routes[i]->dobj.index = tplg->index;
+		list_add(&routes[i]->dobj.list, &tplg->comp->dobj_list);
 
-		soc_tplg_add_route(tplg, &route);
+		soc_tplg_add_route(tplg, routes[i]);
 
 		/* add route, but keep going if some fail */
-		snd_soc_dapm_add_routes(dapm, &route, 1);
+		snd_soc_dapm_add_routes(dapm, routes[i], 1);
 	}
 
-	return 0;
+	/* free memory allocated for all dapm routes in case of error */
+	if (ret < 0)
+		for (i = 0; i < count ; i++)
+			kfree(routes[i]);
+
+	/*
+	 * free pointer to array of dapm routes as this is no longer needed.
+	 * The memory allocated for each dapm route will be freed
+	 * when it is removed in remove_route().
+	 */
+	kfree(routes);
+
+	return ret;
 }
 
 static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
@@ -2570,6 +2637,9 @@ int snd_soc_tplg_component_remove(struct snd_soc_component *comp, u32 index)
 			case SND_SOC_DOBJ_BYTES:
 				remove_bytes(comp, dobj, pass);
 				break;
+			case SND_SOC_DOBJ_GRAPH:
+				remove_route(comp, dobj, pass);
+				break;
 			case SND_SOC_DOBJ_WIDGET:
 				remove_widget(comp, dobj, pass);
 				break;
-- 
2.20.1

Applied "ASoC: topology: add SND_SOC_DOBJ_GRAPH type for dapm routes" to the asoc tree

[Mark Brown]

The patch

   ASoC: topology: add SND_SOC_DOBJ_GRAPH type for dapm routes

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 5c30f43f0625a792c30e465f21dbeb1bb4dfc40b Mon Sep 17 00:00:00 2001
From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Date: Fri, 25 Jan 2019 14:06:46 -0600
Subject: [PATCH] ASoC: topology: add SND_SOC_DOBJ_GRAPH type for dapm routes

Add a new dobj type SND_SOC_DOBJ_GRAPH for dapm routes
and add snd_soc_dobj member to struct snd_soc_dapm_route.
This enables device drivers to save driver specific
data pertaining to dapm routes and also be able
to clean up the data when the driver module is unloaded.

Also, reorder the snd_soc_dobj_type types to align with
matching topology header types.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/sound/soc-dapm.h     | 2 ++
 include/sound/soc-topology.h | 7 ++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index bd8163f151cb..46f2ba3ffcb7 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -540,6 +540,8 @@ struct snd_soc_dapm_route {
 	/* Note: currently only supported for links where source is a supply */
 	int (*connected)(struct snd_soc_dapm_widget *source,
 			 struct snd_soc_dapm_widget *sink);
+
+	struct snd_soc_dobj dobj;
 };
 
 /* dapm audio path between two widgets */
diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h
index fa4b8413d2e2..8c43cfc240fa 100644
--- a/include/sound/soc-topology.h
+++ b/include/sound/soc-topology.h
@@ -38,12 +38,13 @@ struct snd_soc_dapm_route;
 enum snd_soc_dobj_type {
 	SND_SOC_DOBJ_NONE		= 0,	/* object is not dynamic */
 	SND_SOC_DOBJ_MIXER,
-	SND_SOC_DOBJ_ENUM,
 	SND_SOC_DOBJ_BYTES,
-	SND_SOC_DOBJ_PCM,
+	SND_SOC_DOBJ_ENUM,
+	SND_SOC_DOBJ_GRAPH,
+	SND_SOC_DOBJ_WIDGET,
 	SND_SOC_DOBJ_DAI_LINK,
+	SND_SOC_DOBJ_PCM,
 	SND_SOC_DOBJ_CODEC_LINK,
-	SND_SOC_DOBJ_WIDGET,
 };
 
 /* dynamic control object */
-- 
2.20.1

Applied "ASoC: topology: fix memory leak in soc_tplg_dapm_widget_create" to the asoc tree

[Mark Brown]

The patch

   ASoC: topology: fix memory leak in soc_tplg_dapm_widget_create

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 7620fe9161cec2722db880affe03f5e9e2bb93d5 Mon Sep 17 00:00:00 2001
From: Bard liao <yung-chuan.liao@linux.intel.com>
Date: Fri, 25 Jan 2019 14:06:45 -0600
Subject: [PATCH] ASoC: topology: fix memory leak in
 soc_tplg_dapm_widget_create

template.sname and template.name are only freed when an error occur.
They should be freed in the success return case, too.

Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/soc-topology.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 71bc5b8a9bd3..2cb0a05e2368 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1583,6 +1583,9 @@ static int soc_tplg_dapm_widget_create(struct soc_tplg *tplg,
 	if (ret < 0)
 		goto ready_err;
 
+	kfree(template.sname);
+	kfree(template.name);
+
 	return 0;
 
 ready_err:
-- 
2.20.1

Re: [PATCH 7/7] ASoC: topology: unload physical dai link in remove

[Pierre-Louis Bossart]

Hi Mark,

please discard this version. The changes are good but additional tests 
with module unload/reload expose a missing reset of the dobj.type. Will 
send a v2 shortly (along with a couple of DAPM/topology fixes in the 
ASoC core)

Thanks

-Pierre

On 1/25/19 2:06 PM, Pierre-Louis Bossart wrote:
> From: Bard liao <yung-chuan.liao@linux.intel.com>
>
> soc_tplg_link_config() will find the physical dai link and call
> soc_tplg_dai_link_load() to load the BE dai link. Currently remove_link()
> is only used to remove the FE dai link which is created by the topology.
> The BE dai link cannot however be unloaded in snd_soc_tplg_component
> _remove(), which is problematic if anything needs to be released or
> reinitialized.
>
> This patch aligns the definitions of dynamic types with the existing
> UAPI and adds a new remove_backend_link() routine to unload the the BE
> dai link when snd_soc_tplg_component_remove() is invoked.
>
> Signed-off-by: Bard liao <yung-chuan.liao@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>   include/sound/soc-topology.h |  1 +
>   sound/soc/soc-topology.c     | 31 +++++++++++++++++++++++++++++++
>   2 files changed, 32 insertions(+)
>
> diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h
> index 8c43cfc240fa..5223896de26f 100644
> --- a/include/sound/soc-topology.h
> +++ b/include/sound/soc-topology.h
> @@ -45,6 +45,7 @@ enum snd_soc_dobj_type {
>   	SND_SOC_DOBJ_DAI_LINK,
>   	SND_SOC_DOBJ_PCM,
>   	SND_SOC_DOBJ_CODEC_LINK,
> +	SND_SOC_DOBJ_BACKEND_LINK,
>   };
>   
>   /* dynamic control object */
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 23d421370e6c..49e6fd76d9bc 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -557,6 +557,24 @@ static void remove_link(struct snd_soc_component *comp,
>   	kfree(link);
>   }
>   
> +/* unload dai link */
> +static void remove_backend_link(struct snd_soc_component *comp,
> +	struct snd_soc_dobj *dobj, int pass)
> +{
> +	if (pass != SOC_TPLG_PASS_LINK)
> +		return;
> +
> +	if (dobj->ops && dobj->ops->link_unload)
> +		dobj->ops->link_unload(comp, dobj);
> +
> +	/*
> +	 * We don't free the link here as what remove_link() do since BE
> +	 * links are not allocated by topology.
> +	 */
> +
> +	list_del(&dobj->list);
> +}
> +
>   /* bind a kcontrol to it's IO handlers */
>   static int soc_tplg_kcontrol_bind_io(struct snd_soc_tplg_ctl_hdr *hdr,
>   	struct snd_kcontrol_new *k,
> @@ -2163,6 +2181,12 @@ static int soc_tplg_link_config(struct soc_tplg *tplg,
>   		return ret;
>   	}
>   
> +	/* for unloading it in snd_soc_tplg_component_remove */
> +	link->dobj.index = tplg->index;
> +	link->dobj.ops = tplg->ops;
> +	link->dobj.type = SND_SOC_DOBJ_BACKEND_LINK;
> +	list_add(&link->dobj.list, &tplg->comp->dobj_list);
> +
>   	return 0;
>   }
>   
> @@ -2649,6 +2673,13 @@ int snd_soc_tplg_component_remove(struct snd_soc_component *comp, u32 index)
>   			case SND_SOC_DOBJ_DAI_LINK:
>   				remove_link(comp, dobj, pass);
>   				break;
> +			case SND_SOC_DOBJ_BACKEND_LINK:
> +				/*
> +				 * call link_unload ops if extra
> +				 * deinitialization is needed.
> +				 */
> +				remove_backend_link(comp, dobj, pass);
> +				break;
>   			default:
>   				dev_err(comp->dev, "ASoC: invalid component type %d for removal\n",
>   					dobj->type);