diff mbox series

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

Message ID 20210404065929.52501-1-o-takashi@sakamocchi.jp (mailing list archive)
State New
Headers show
Series ALSA: control_led: fix stack frame usage over 1024 bytes in snd_ctl_led_get() | expand

Commit Message

Takashi Sakamoto April 4, 2021, 6:59 a.m. UTC
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@sakamocchi.jp>
---
 sound/core/control_led.c | 68 ++++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 24 deletions(-)

Comments

Jaroslav Kysela April 4, 2021, 8:29 a.m. UTC | #1
Dne 04. 04. 21 v 8:59 Takashi Sakamoto napsal(a):
> 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@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)

I think info, value should be enough as names for the arguments here. It
increases readability.

					Jaroslav
Nathan Chancellor April 13, 2021, 8:53 p.m. UTC | #2
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@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
>
Takashi Iwai April 14, 2021, 7:32 a.m. UTC | #3
On Tue, 13 Apr 2021 22:53:34 +0200,
Nathan Chancellor wrote:
> 
> 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@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/

Can anyone re-submit a version that just does fix this issue without
doing anything else?


Takashi
Jaroslav Kysela April 14, 2021, 7:48 a.m. UTC | #4
Dne 14. 04. 21 v 9:32 Takashi Iwai napsal(a):

>> 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/
> 
> Can anyone re-submit a version that just does fix this issue without
> doing anything else?

I'll create a proper fix with the cached allocations in struct snd_ctl_led.

				Thanks,
					Jaroslav
diff mbox series

Patch

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);
 	}
+
+	kfree(elem_value);
+	kfree(elem_info);
 	mutex_unlock(&snd_ctl_led_mutex);
 	switch (led->mode) {
 	case MODE_OFF:		route = 1; break;