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

[Takashi Sakamoto]

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 at sakamocchi.jp>
>> 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