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

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:

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 */

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;

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);