[alsa-devel] [PATCH 2/7] ALSA: core: Add managed card creation

[Takashi Iwai]

On Wed, 03 Oct 2018 10:12:34 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Sep 21 2018 00:54, Takashi Iwai wrote:
> > Per popular demands, this patch adds a new ALSA core API function,
> > snd_devm_card_new(), to create a snd_card object in a managed way via
> > devres.  When a card object is created by this new function, it's
> > released automatically at the device release.  It includes also the
> > call of snd_card_free().
> >
> > However, the story isn't that simple.  A caveat is that We have to
> > call snd_card_new(), more specifically, the disconnection part, at
> > very first of the whole resource release procedure.  This assures that
> > the exposed devices are deleted and sync with the all accessing
> > processes getting closed.
> >
> > For achieving it, snd_card_register() adds a new devres action to
> > trigger snd_card_free() automatically when the given card object is a
> > "managed" one.  Since usually snd_card_register() is the last step of
> > the initialization, this should work in most cases.
> >
> > With all these tricks, some drivers can get rid of the whole the
> > driver remove callback.
> >
> > About a bit of implementation details: the patch adds two new flags to
> > snd_card object, managed and releasing.  The former indicates that the
> > object was created via snd_devm_card_new(), and the latter is used for
> > avoiding the double-free of snd_card_free() calls.  Both flags are
> > fairly internal and likely uninteresting to normal users.
> >
> > Signed-off-by: Takashi Iwai <tiwai at suse.de>
> > ---
> >   include/sound/core.h |  5 +++
> >   sound/core/init.c    | 95 ++++++++++++++++++++++++++++++++++++++++++--
> >   2 files changed, 96 insertions(+), 4 deletions(-)
> 
> I've investigated to use the new function, 'snd_devm_card_new()', and
> have a concern about timing to release memory object for sound card
> instance (and tailing private data) in error path between calls of the
> function and 'snd_card_register()'.
> 
> In the error path, 'snd_card_free()' is called to release instances
> associated to the sound card instance as expected, however memory object
> for the sound card instance is not released yet because in usual cases
> associated device structure does not lost its reference count in this
> timing. It's released when the associated device is removed. This means
> that the memory object remains against its practicality during lifetime
> of the device.
> 
> This is not a bug itself, so I have no opposition to this patchset. But
> it's reasonable for us to have a bit time to consider about it, IMO.

Well, it's exactly same as the usual devm_kzalloc() & co, which have
been already deployed at various places (even you posted a new patch
series for using them :)  The card memory is tied with the device, and
it's natural to align with the lifetime of the device.

Of course, the memory release timing would be slightly differed by
this code change, but this shouldn't be a problem, as long as it's
about the memory release.  That is:

- In the old situation, kfree() is called from snd_card_free() via
  driver remove callback called from  __device_release_driver().

- In the devres mode, kfree() is called via devres_release_all() that
  is called in __device_release_driver() right after drv->remove()
  call above.

So the difference of the release timing is only between these call
paths.


thanks,

Takashi