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;
}
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
