| Message ID | 20210404065929.52501-1-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: control_led: fix stack frame usage over 1024 bytes in snd_ctl_led_get() | expand |
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
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 >
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
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 --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;
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(-)