Hi,
On Oct 4 2018 00:37, Takashi Iwai wrote:
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.
Indeed. This patch includes no extra care to your patch 01/03/04/05
because I've already proposed a solution to use of reference
counting without associating sound card instance to target device.
I decide not to use 'snd_devm_card_new()' for all drivers in ALSA
firewire stack because of several issues I've addressed. Thanks for
your time to discuss about them.
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
Regards
Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
