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