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

[Takashi Iwai <tiwai@suse.de>]

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.


thanks,

Takashi


> ---
>  sound/core/control_led.c | 52 ++++++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index 93b201063c7d..9d1612b1a6ff 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -37,6 +37,7 @@ struct snd_ctl_led {
>  	unsigned int group;
>  	enum led_audio trigger_type;
>  	enum snd_ctl_led_mode mode;
> +	void *info_and_value;
>  	struct snd_ctl_led_card *cards[SNDRV_CARDS];
>  };
>  
> @@ -94,34 +95,41 @@ static struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access)
>  	return &snd_ctl_leds[group];
>  }
>  
> -static int snd_ctl_led_get(struct snd_ctl_led_ctl *lctl)
> +static int snd_ctl_led_get(struct snd_ctl_led *led, struct snd_ctl_led_ctl *lctl)
>  {
>  	struct snd_kcontrol *kctl = lctl->kctl;
> -	struct snd_ctl_elem_info info;
> -	struct snd_ctl_elem_value value;
> +	struct snd_ctl_elem_info *info = led->info_and_value;
> +	struct snd_ctl_elem_value *value;
>  	unsigned int i;
>  	int result;
>  
> -	memset(&info, 0, sizeof(info));
> -	info.id = kctl->id;
> -	info.id.index += lctl->index_offset;
> -	info.id.numid += lctl->index_offset;
> -	result = kctl->info(kctl, &info);
> +	if (info == NULL) {
> +		info = kmalloc(sizeof(*info) + sizeof(*value), GFP_KERNEL);
> +		if (info == NULL)
> +			return -1;
> +		led->info_and_value = info;
> +	}
> +	value = (void *)(info + 1);
> +	memset(info, 0, sizeof(*info));
> +	info->id = kctl->id;
> +	info->id.index += lctl->index_offset;
> +	info->id.numid += lctl->index_offset;
> +	result = kctl->info(kctl, info);
>  	if (result < 0)
>  		return -1;
> -	memset(&value, 0, sizeof(value));
> -	value.id = info.id;
> -	result = kctl->get(kctl, &value);
> +	memset(value, 0, sizeof(*value));
> +	value->id = info->id;
> +	result = kctl->get(kctl, value);
>  	if (result < 0)
>  		return -1;
> -	if (info.type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
> -	    info.type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
> -		for (i = 0; i < info.count; i++)
> -			if (value.value.integer.value[i] != info.value.integer.min)
> +	if (info->type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
> +	    info->type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
> +		for (i = 0; i < info->count; i++)
> +			if (value->value.integer.value[i] != info->value.integer.min)
>  				return 1;
> -	} else if (info.type == SNDRV_CTL_ELEM_TYPE_INTEGER64) {
> -		for (i = 0; i < info.count; i++)
> -			if (value.value.integer64.value[i] != info.value.integer64.min)
> +	} else if (info->type == SNDRV_CTL_ELEM_TYPE_INTEGER64) {
> +		for (i = 0; i < info->count; i++)
> +			if (value->value.integer64.value[i] != info->value.integer64.min)
>  				return 1;
>  	}
>  	return 0;
> @@ -149,7 +157,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
>  	list_for_each_entry(lctl, &led->controls, list) {
>  		if (lctl->kctl == kctl && lctl->index_offset == ioff)
>  			found = true;
> -		UPDATE_ROUTE(route, snd_ctl_led_get(lctl));
> +		UPDATE_ROUTE(route, snd_ctl_led_get(led, lctl));
>  	}
>  	if (!found && kctl && card) {
>  		lctl = kzalloc(sizeof(*lctl), GFP_KERNEL);
> @@ -159,7 +167,7 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
>  			lctl->kctl = kctl;
>  			lctl->index_offset = ioff;
>  			list_add(&lctl->list, &led->controls);
> -			UPDATE_ROUTE(route, snd_ctl_led_get(lctl));
> +			UPDATE_ROUTE(route, snd_ctl_led_get(led, lctl));
>  		}
>  	}
>  	mutex_unlock(&snd_ctl_led_mutex);
> @@ -300,6 +308,10 @@ static void snd_ctl_led_clean(struct snd_card *card)
>  				snd_ctl_led_ctl_destroy(lctl);
>  				goto repeat;
>  			}
> +		if (!card) {
> +			kfree(led->info_and_value);
> +			led->info_and_value = NULL;
> +		}
>  	}
>  }
>  
> -- 
> 2.30.2
>