[alsa-devel] [PATCH] ASoC: topology: Fix not to keep a reference to tplg fw

[Subhransu S. Prusty]

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

--