On Wed, 03 Oct 2018 17:02:59 +0200,
Takashi Sakamoto wrote:
>
> Hi,
>
> On Oct 3 2018 22:14, Takashi Sakamoto wrote:
> > On Oct 3 2018 19:30, Takashi Iwai wrote:
> >> On Wed, 03 Oct 2018 10:12:34 +0200,
> >>> 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.
> >
> > I have this concern against the usage of devres for a long time.
> >
> > A device can be referred by several handlers; e.g. character device
> > driver and kernel driver. An example is USB devices. Userspace
> > applications can transfer message to the device via character device.
> > At the same time, iface driver in kernel land can do the same work.
> >
> > The lifetime of device is apparently different from each of the handler.
> > Even if kernel driver returns negative value in its .probe callback, the
> > other handlers can refer to and use it.
> >
> > If the kernel driver leave the devm-allocated memory in error path, it
> > remains till all of the handler release its reference to the device.
> > Even it the memory objects are maintained by devres, several small parts
> > of kernel logical space is unavailable. This is similar to memory leak,
> > in my view.
>
> I wrote a patch to e06afb87065c (HEAD of 'topic/devres' branch) for this
> purpose. I've not tested thoroughly but expect it solves the above issue.
>
> ======== 8< --------
>
> >From 2ef56174ea3c07eb5d6c2adb60573321c04a02af Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
> Date: Wed, 3 Oct 2018 23:44:43 +0900
> Subject: [PATCH] ALSA: core: release memory object for sound card immediately
> in error path for a case of devm usage
>
> Drivers can snd_card_free() in error path between calls of
> snd_devm_card_new() and snd_card_register(), In this case,
> memory object for sound card structure is not deallocated
> immediately because it's associated to target device by
> devres framework. The memory object remains till the target
> device is released. This looks like memory leak.
Not really, this is *intentional* and designed behavior.
Imagine the case if you have a device-managed irq handler and a
device-managed IO memory. They are not freed at snd_card_free() call
time, but at a later stage of devres release. Hence the *all*
devres-managed resources have to be released there.
thanks,
Takashi
>
> This commit postpones association of the memory object to
> target device till a call of snd_card_register(). In error
> path, a call of snd_card_free() releases the memory object
> in the last part of snd_card_do_free().
> ---
> sound/core/init.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/sound/core/init.c b/sound/core/init.c
> index 2eade57db4b4..49cb22a735c1 100644
> --- a/sound/core/init.c
> +++ b/sound/core/init.c
> @@ -272,7 +272,6 @@ int snd_devm_card_new(struct device *parent, int idx, const char *xid,
> return err;
> }
>
> - devres_add(parent, card);
> *card_ret = card;
> return 0;
> }
> @@ -569,6 +568,8 @@ static int snd_card_do_free(struct snd_card *card)
> complete(card->release_completion);
> if (!card->managed)
> kfree(card);
> + else if (!devres_find(card->dev, __snd_card_release, NULL, NULL))
> + devres_free(card);
> return 0;
> }
>
> @@ -849,15 +850,6 @@ int snd_card_register(struct snd_card *card)
> if (err < 0)
> return err;
> card->registered = true;
> - } else {
> - if (card->managed)
> - devm_remove_action(card->dev, trigger_card_free, card);
> - }
> -
> - if (card->managed) {
> - err = devm_add_action(card->dev, trigger_card_free, card);
> - if (err < 0)
> - return err;
> }
>
> if ((err = snd_device_register_all(card)) < 0)
> @@ -887,6 +879,14 @@ int snd_card_register(struct snd_card *card)
> if (snd_mixer_oss_notify_callback)
> snd_mixer_oss_notify_callback(card, SND_MIXER_OSS_NOTIFY_REGISTER);
> #endif
> + if (card->managed &&
> + !devres_find(card->dev, __snd_card_release, NULL, NULL)) {
> + err = devm_add_action(card->dev, trigger_card_free, card);
> + if (err < 0)
> + return err;
> + devres_add(card->dev, card);
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL(snd_card_register);
> --
> 2.17.1
>
>
> Thanks
>
> Takashi Sakamoto
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
