[PATCH] ALSA: control_led: fix stack frame usage over 1024 bytes in snd_ctl_led_get()

[Nathan Chancellor]

On Sun, Apr 04, 2021 at 03:59:29PM +0900, Takashi Sakamoto wrote:
> GCC warns that snd_ctl_led_get() uses stack frame over 1024 bytes.
> 
> control_led.c: In function ‘snd_ctl_led_get’:
> control_led.c:128:1: warning: the frame size of 1504 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> When taking care of convension in Linux kernel development. it's preferable
> to suppress too large usage of kernel stack, when the function call is not
> in critical part.
> 
> This commit fixes the bug, including some minor code refactoring relevant
> to variable names.
> 
> Fixes: 22d8de62f11b ("ALSA: control - add generic LED trigger module as the new control layer")
> Signed-off-by: Takashi Sakamoto <o-takashi at sakamocchi.jp>
> ---
>  sound/core/control_led.c | 68 ++++++++++++++++++++++++++--------------
>  1 file changed, 44 insertions(+), 24 deletions(-)
> 
> diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> index b97f118cd54e..1be854a861f0 100644
> --- a/sound/core/control_led.c
> +++ b/sound/core/control_led.c
> @@ -94,34 +94,35 @@ 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_ctl *lctl, struct snd_ctl_elem_info *elem_info,
> +			   struct snd_ctl_elem_value *elem_value)
>  {
>  	struct snd_kcontrol *kctl = lctl->kctl;
> -	struct snd_ctl_elem_info info;
> -	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 (result < 0)
> +	int err;
> +
> +	memset(elem_info, 0, sizeof(*elem_info));
> +	elem_info->id = kctl->id;
> +	elem_info->id.index += lctl->index_offset;
> +	elem_info->id.numid += lctl->index_offset;
> +	err = kctl->info(kctl, elem_info);
> +	if (err < 0)
>  		return -1;
> -	memset(&value, 0, sizeof(value));
> -	value.id = info.id;
> -	result = kctl->get(kctl, &value);
> -	if (result < 0)
> +
> +	memset(elem_value, 0, sizeof(*elem_value));
> +	elem_value->id = elem_info->id;
> +	err = kctl->get(kctl, elem_value);
> +	if (err < 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 (elem_info->type == SNDRV_CTL_ELEM_TYPE_BOOLEAN ||
> +	    elem_info->type == SNDRV_CTL_ELEM_TYPE_INTEGER) {
> +		for (i = 0; i < elem_info->count; i++)
> +			if (elem_value->value.integer.value[i] != elem_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 (elem_info->type == SNDRV_CTL_ELEM_TYPE_INTEGER64) {
> +		for (i = 0; i < elem_info->count; i++)
> +			if (elem_value->value.integer64.value[i] != elem_info->value.integer64.min)
>  				return 1;
>  	}
>  	return 0;
> @@ -132,6 +133,8 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
>  {
>  	struct snd_ctl_led *led;
>  	struct snd_ctl_led_ctl *lctl;
> +	struct snd_ctl_elem_info *elem_info;
> +	struct snd_ctl_elem_value *elem_value;
>  	int route;
>  	bool found;
>  
> @@ -146,10 +149,24 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
>  		mutex_unlock(&snd_ctl_led_mutex);
>  		return;
>  	}
> +
> +	elem_info = kmalloc(sizeof(*elem_info), GFP_KERNEL);
> +	if (!elem_info) {
> +		mutex_unlock(&snd_ctl_led_mutex);
> +		return;
> +	}
> +
> +	elem_value = kmalloc(sizeof(*elem_value), GFP_KERNEL);
> +	if (!elem_value) {
> +		kfree(elem_info);
> +		mutex_unlock(&snd_ctl_led_mutex);
> +		return;
> +	}
> +
>  	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(lctl, elem_info, elem_value));
>  	}
>  	if (!found && kctl && card) {
>  		lctl = kzalloc(sizeof(*lctl), GFP_KERNEL);
> @@ -159,10 +176,13 @@ 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(lctl, elem_info, elem_value));
>  		}
>  		kfree(lctl);

This patch is still relevant on latest -next and this hunk prevents the
patch from applying cleanly because that patch was NAK'd:

https://lore.kernel.org/alsa-devel/20210404064031.48711-1-o-takashi@sakamocchi.jp/

Cheers,
Nathan

>  	}
> +
> +	kfree(elem_value);
> +	kfree(elem_info);
>  	mutex_unlock(&snd_ctl_led_mutex);
>  	switch (led->mode) {
>  	case MODE_OFF:		route = 1; break;
> -- 
> 2.27.0
>