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

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

[Jaroslav Kysela]

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>
---
 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

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

[Takashi Iwai]

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
> 

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

[Jaroslav Kysela]

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

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Takashi Iwai]

On Wed, 14 Apr 2021 12:09:07 +0200,
Jaroslav Kysela wrote:
> 
> 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.

OK, I see your points.  OTOH, I prefer simplicity at this moment over
yet another kmalloc.  If the static variables are enough, that sounds
like the best option.  Not only that it's way simpler (only two line
changes), it's even more byte-saving as it allocates only once
globally, not per snd_ctl_led object.  Having a comment should be good
enough for avoiding the pitfall in future changes (if any).


thanks,

Takashi

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

[Jaroslav Kysela]

Dne 14. 04. 21 v 12:52 Takashi Iwai napsal(a):
> On Wed, 14 Apr 2021 12:09:07 +0200,
> Jaroslav Kysela wrote:
>>
>> 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.
> 
> OK, I see your points.  OTOH, I prefer simplicity at this moment over
> yet another kmalloc.  If the static variables are enough, that sounds
> like the best option.  Not only that it's way simpler (only two line
> changes), it's even more byte-saving as it allocates only once
> globally, not per snd_ctl_led object.  Having a comment should be good
> enough for avoiding the pitfall in future changes (if any).

Ok, new patch is out.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Jaroslav Kysela]

Dne 14. 04. 21 v 14:23 Takashi Iwai napsal(a):
> On Wed, 14 Apr 2021 12:58:58 +0200,
> Jaroslav Kysela wrote:
>>
>> It's a bad idea to allocate big structures on the stack.
>> Mark the variables as static and add a note for the locking.
>>
>> 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>
>> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> 
> Applied now (with a removal of the duplicated sign-off).

Thank you.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Takashi Iwai]

On Wed, 14 Apr 2021 12:58:58 +0200,
Jaroslav Kysela wrote:
> 
> It's a bad idea to allocate big structures on the stack.
> Mark the variables as static and add a note for the locking.
> 
> 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>
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>

Applied now (with a removal of the duplicated sign-off).

Thanks!


Takashi

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

[Jaroslav Kysela]

It's a bad idea to allocate big structures on the stack.
Mark the variables as static and add a note for the locking.

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>
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 sound/core/control_led.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index 93b201063c7d..25f57c14f294 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -94,11 +94,15 @@ static struct snd_ctl_led *snd_ctl_led_get_by_access(unsigned int access)
 	return &snd_ctl_leds[group];
 }
 
+/*
+ * A note for callers:
+ *   The two static variables info and value are protected using snd_ctl_led_mutex.
+ */
 static int snd_ctl_led_get(struct snd_ctl_led_ctl *lctl)
 {
+	static struct snd_ctl_elem_info info;
+	static struct snd_ctl_elem_value value;
 	struct snd_kcontrol *kctl = lctl->kctl;
-	struct snd_ctl_elem_info info;
-	struct snd_ctl_elem_value value;
 	unsigned int i;
 	int result;
 
-- 
2.30.2