Re: [PATCH] ASoC: topology: Fix not to keep a reference to tplg fw

[Vinod Koul <vinod.koul@intel.com>]

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?

-- 
~Vinod