On Thu, 26 Nov 2015 12:24:58 +0100, Vinod Koul wrote: > > On Thu, Nov 26, 2015 at 10:19:51AM +0100, Takashi Iwai wrote: > > On Thu, 26 Nov 2015 10:10:16 +0100, > > Vinod Koul wrote: > > > > > > On Thu, Nov 26, 2015 at 09:48:47AM +0100, Takashi Iwai wrote: > > > > On Thu, 26 Nov 2015 15:11:00 +0100, > > > > Subhransu S. Prusty wrote: > > > > > > > > > > During element creation, the name of some of the elements point > > > > > to memory referenced in tplg fw. If the tplg fw is released after > > > > > tplg is parsed by framework, kernel panic happens during creation > > > > > of elements while card initialization. > > > > > > > > In which code path? When the kctl is already instantiated from > > > > snd_kcontrol_new template, we don't have to duplicate the string. > > > > The only case where the strdup() is required is to delay the > > > > instantiation, i.e. storing the kcontrol_new object itself instead of > > > > referring temporarily. > > > > > > So in SKL, we do request firmware of topology binary and topology core uses > > > that for strings here, so the patch 87b5ed8ec freed the topology binary > > > which causes panic while accessing kcontrols. > > > > This is strange. If it's about the kctl name string, the panic > > shouldn't happen at accessing the kctl but at instantiating the kctl > > from snd_kcontrol_new that contains the invalid string pointer. > > The kctl object contains the string in itself, and there copies the > > string from the template. > > > > Also I wonder why it kernel panics, not the normal Oops. > > 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. Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
