[PATCH] ALSA: control_led - fix the stack usage (control element ops)

[Jaroslav Kysela]

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 at perex.cz>
>>>> Cc: Nathan Chancellor <nathan at kernel.org>
>>>> Cc: Takashi Sakamoto <o-takashi at sakamocchi.jp>
>>>
>>> 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 at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.