On Fri, 27 Nov 2015 12:42:04 +0100, Subhransu S. Prusty wrote:
On Fri, Nov 27, 2015 at 06:54:15AM +0100, Takashi Iwai wrote:
On Fri, 27 Nov 2015 10:15:19 +0100, Subhransu S. Prusty wrote:
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:
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);@@ -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.
It is already freed in the original code. The only addition is to free the newly allocated strings in kcontrol_news. So kfree() is replaced with free_kcontrol_news().
Otherwise shouldn't the devm version of kstrdup work good as it just frees the memory when the device is removed?
No, as already mentioned, devm won't release the data until unbind and the topology data might be reloaded repeatedly, thus user can hog the kernel memory unlimitedly.
Then which of the APIs snd_soc_tplg_widget_remove or snd_soc_tplg_component_remove should free the memory in this scenairo? I guess it should be snd_soc_tplg_widget_remove, but I don't see a caller of this API.
Then it's a driver's failure. It needs to call it appropriately before reloading. Or it was supposed to be invoked from snd_soc_tplg_component_remove()? I don't know.
Takashi