Dne 14. 04. 21 v 12:52 Takashi Iwai napsal(a):
> On Wed, 14 Apr 2021 12:09:07 +0200,
> Jaroslav Kysela wrote:
>>
>> Dne 14. 04. 21 v 11:44 Takashi Iwai napsal(a):
>>> On Wed, 14 Apr 2021 11:30:31 +0200,
>>> Jaroslav Kysela wrote:
>>>>
>>>> It's a bad idea to allocate big structures on the stack. Allocate
>>>> the required structures on demand and cache them in the led
>>>> structure.
>>>>
>>>> Fixes: 22d8de62f11b ("ALSA: control - add generic LED trigger module as the new control layer")
>>>> Signed-off-by: Jaroslav Kysela <perex@xxxxxxxx>
>>>> Cc: Nathan Chancellor <nathan@xxxxxxxxxx>
>>>> Cc: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
>>>
>>> Thanks for the patch.
>>>
>>> But, wouldn't it be simpler if we just add snd_ctl_elem_info and
>>> _value in snd_ctl_led object itself?
>>>
>>> -- 8< --
>>> --- a/sound/core/control_led.c
>>> +++ b/sound/core/control_led.c
>>> @@ -38,6 +38,8 @@ struct snd_ctl_led {
>>> enum led_audio trigger_type;
>>> enum snd_ctl_led_mode mode;
>>> struct snd_ctl_led_card *cards[SNDRV_CARDS];
>>> + struct snd_ctl_elem_info elem_info;
>>> + struct snd_ctl_elem_value elem_value;
>>> };
>>>
>>> struct snd_ctl_led_ctl {
>>>
>>> -- 8< --
>>>
>>> Then we need no extra kmalloc. I guess snd_ctl_led_get() shall be
>>> called almost always, so we won't save much even if we allocate
>>> dynamically.
>>
>> The idea was to allocate this structure purely on demand. We can have the case
>> where some LED group is inactive (no speaker LED for example) or the LED
>> functionality is not used at all even if the module is loaded. And it's true
>> that those structures requires some more bytes.
>>
>> Another option is just to make the structures in snd_ctl_led_get() static -
>> two line patch. The calls are protected with snd_ctl_led_mutex . But it may be
>> problematic if we do a finer mutex locking per LED group in the future.
>
> OK, I see your points. OTOH, I prefer simplicity at this moment over
> yet another kmalloc. If the static variables are enough, that sounds
> like the best option. Not only that it's way simpler (only two line
> changes), it's even more byte-saving as it allocates only once
> globally, not per snd_ctl_led object. Having a comment should be good
> enough for avoiding the pitfall in future changes (if any).
Ok, new patch is out.
Jaroslav
--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
