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@perex.cz Cc: Nathan Chancellor nathan@kernel.org Cc: Takashi Sakamoto o-takashi@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.
Jaroslav