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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx/
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
>
