Re: [PATCH] ASoC: topology: Fix not to keep a reference to tplg fw

[Takashi Iwai <tiwai@suse.de>]

On Thu, 26 Nov 2015 17:13:43 +0100,
Vinod Koul wrote:
> 
> On Thu, Nov 26, 2015 at 12:46:24PM +0100, Takashi Iwai wrote:
> 
> > > Sorry it a oops, paging request failure and not a panic
> > > 
> > > > > Your second point is applicable here as card instantiation is delayed often
> > > > > for us as all components may not be present and delayed probe finally
> > > > > creates the card.
> > > > > 
> > > > > > > Issue is caught with id#87b5ed8ecb9fe05a696e1c0b53c7a49ea66432c1
> > > > > > 
> > > > > > You should put the commit subject, too.
> > > > > 
> > > > > Yes we will add that
> > > > > 
> > > > > > > So create a copy of the memory and assign to names instead.
> > > > > > 
> > > > > > And who releases these duplicated memory?  It looks like another
> > > > > > memory leak to me.
> > > > > 
> > > > > That is a good point and I think we should do devm_kstrdup() here so that
> > > > > this is freed when we cleanup the device, or do you have any better
> > > > > suggestion ?
> > > > 
> > > > devm_kstrdup() is bad in this case.  You can reload the topology
> > > > unlimitedly, and the memory won't be freed until the device unbind,
> > > > thus it keeps hogging.
> > > > 
> > > > You really need to identify which path hits the issue exactly how.  In
> > > > general, the string passed to template is only for creating the kctl.
> > > > Once when kctl is created, the whole snd_kcontrol_new template and the
> > > > allocated string is no use, so they can be freed.
> > > 
> > > but then question of where should these be freed. For current drivers they
> > > declare controls statically, so memory is always there.. How do free up in
> > > the cases where we allocate dynamically?
> > 
> > Well, for judging this, we have to follow the code more closely.  And
> > it's why I asked which path does it happen exactly.
> > 
> > There are two different paths where the snd_kcontrol_new is used: the
> > standard controls and dapm.  The former is immediately instantiated
> > via snd_soc_cnew(), so it's fine as is, no need to change.  But the
> > latter is different.
> > 
> > The latter, dapm case, always allocates the snd_kcontrol_new array in
> > kcontrol_news field.  So, we need to change in each function
> > allocating this to do kstrdump() for each kcontrol_new element, and
> > each place calling kfree() of kcontrol_news should free the string of
> > each item in return.
> 
> It is the latter dapm case with added complexity of topology core creating
> these kcontrols. I will reproduce this and send the oops tomorrow

Not too complex in this case because there are only a few users.
A totally untested patch is below.


Takashi

---
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 8d7ec80af51b..1f684975b541 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -427,6 +427,16 @@ static void remove_enum(struct snd_soc_component *comp,
 	kfree(se);
 }
 
+static void free_kcontrol_news(const struct snd_kcontrol_new *_wc, int nums)
+{
+	struct snd_kcontrol_new *wc = (struct snd_kcontrol_new *)_wc;
+	int i;
+
+	for (i = 0; i < nums && wc[i].name; i++)
+		kfree(wc[i].name);
+	kfree(wc);
+}
+
 /* remove a byte kcontrol */
 static void remove_bytes(struct snd_soc_component *comp,
 	struct snd_soc_dobj *dobj, int pass)
@@ -477,7 +487,7 @@ static void remove_widget(struct snd_soc_component *comp,
 			kfree(se->dobj.control.dtexts[i]);
 
 		kfree(se);
-		kfree(w->kcontrol_news);
+		free_kcontrol_news(w->kcontrol_news, 1);
 	} else {
 		/* non enumerated widget mixer */
 		for (i = 0; i < w->num_kcontrols; i++) {
@@ -490,7 +500,7 @@ static void remove_widget(struct snd_soc_component *comp,
 			snd_ctl_remove(card, w->kcontrols[i]);
 			kfree(sm);
 		}
-		kfree(w->kcontrol_news);
+		free_kcontrol_news(w->kcontrol_news, w->num_kcontrols);
 	}
 	/* widget w is freed by soc-dapm.c */
 }
@@ -1149,7 +1159,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create(
 		dev_dbg(tplg->dev, " adding DAPM widget mixer control %s at %d\n",
 			mc->hdr.name, i);
 
-		kc[i].name = mc->hdr.name;
+		kc[i].name = kstrdup(mc->hdr.name, GFP_KERNEL);
+		if (!kc[i].name)
+			goto err_str;
 		kc[i].private_value = (long)sm;
 		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc[i].access = mc->hdr.access;
@@ -1196,7 +1208,7 @@ err_str:
 err:
 	for (--i; i >= 0; i--)
 		kfree((void *)kc[i].private_value);
-	kfree(kc);
+	free_kcontrol_news(kc, num_kcontrols);
 	return NULL;
 }
 
@@ -1228,7 +1240,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_denum_create(
 	dev_dbg(tplg->dev, " adding DAPM widget enum control %s\n",
 		ec->hdr.name);
 
-	kc->name = ec->hdr.name;
+	kc->name = kstrdup(ec->hdr.name, GFP_KERNEL);
+	if (!kc->name)
+		goto err;
 	kc->private_value = (long)se;
 	kc->iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 	kc->access = ec->hdr.access;
@@ -1294,7 +1308,7 @@ err_se:
 
 	kfree(se);
 err:
-	kfree(kc);
+	free_kcontrol_news(kc, 1);
 
 	return NULL;
 }
@@ -1330,7 +1344,9 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
 			"ASoC: adding bytes kcontrol %s with access 0x%x\n",
 			be->hdr.name, be->hdr.access);
 
-		kc[i].name = be->hdr.name;
+		kc[i].name = kstrdup(be->hdr.name, GFP_KERNEL);
+		if (!kc[i].name)
+			goto err;
 		kc[i].private_value = (long)sbe;
 		kc[i].iface = SNDRV_CTL_ELEM_IFACE_MIXER;
 		kc[i].access = be->hdr.access;
@@ -1363,7 +1379,7 @@ err:
 	for (--i; i >= 0; i--)
 		kfree((void *)kc[i].private_value);
 
-	kfree(kc);
+	free_kcontrol_news(kc, count);
 	return NULL;
 }