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

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