On Thu, Nov 26, 2015 at 06:39:02PM +0100, Takashi Iwai wrote:
> 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);
Hi Takashi,
I have not tested this patch yet. But it should fix the oops. Just looking
the code I find remove_widget is either called from snd_soc_tplg_widget_remove
or from snd_soc_tplg_component_remove. The xxx_component_remove is called
during unregister of the component and there is no caller to
snd_soc_tplg_widget_remove.
I guess the intention here is to free the kcontrol_news immediately after the
card is registered. Please correct me if I am wrong.
Otherwise shouldn't the devm version of kstrdup work good as it just frees
the memory when the device is removed?
Regards,
Subhransu
> }
> /* 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
