Re: drivers/acpi/processor_thermal.c:66:1: warning: the frame size of 2160 bytes is larger than 2048 bytes

[Geert Uytterhoeven]

Hi Rafael,

On Tue, Jan 21, 2020 at 1:40 AM Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote:
> On Monday, January 20, 2020 11:16:12 AM CET Rafael J. Wysocki wrote:
> > On Fri, Jan 17, 2020 at 4:51 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> > > On Fri, Nov 15, 2019 at 6:23 AM kbuild test robot <lkp@xxxxxxxxx> wrote:
> > > > tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > > > head:   96b95eff4a591dbac582c2590d067e356a18aacb
> > > > commit: 3000ce3c52f8b8db093e4dc649cd172390f71137 cpufreq: Use per-policy frequency QoS
> > > > date:   4 weeks ago
> > > > config: ia64-randconfig-a001-20191115 (attached as .config)
> > > > compiler: ia64-linux-gcc (GCC) 7.4.0
> > > > reproduce:
> > > >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > > >         chmod +x ~/bin/make.cross
> > > >         git checkout 3000ce3c52f8b8db093e4dc649cd172390f71137
> > > >         # save the attached .config to linux build tree
> > > >         GCC_VERSION=7.4.0 make.cross ARCH=ia64
> > > >
> > > > If you fix the issue, kindly add following tag
> > > > Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> > >
> > > Seeing similar warnings on arm64, so this triggered my attention.
> > >
> > > > --
> > > >    drivers/cpufreq/cpufreq.c: In function 'refresh_frequency_limits.part.33':
> > > > >> drivers/cpufreq/cpufreq.c:1116:1: warning: the frame size of 2160 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> > >
> > > |       struct cpufreq_policy new_policy;
> > >
> > > That's a large struct on the stack...
> > >
> > > |       if (!policy_is_inactive(policy)) {
> > > |               new_policy = *policy;
> > >
> > > Let's make a copy?
> > > How well does this work, given struct cpufreq_policy contains a
> > > work_struct, list_head, kobject, completion, semaphore, spinlock_t,
> > > wait_queue_head_t, and two notifier_blocks, which are all objects you
> > > cannot just copy and reuse?
> > >
> > > |               pr_debug("updating policy for CPU %u\n", policy->cpu);
> > > |
> > > |               cpufreq_set_policy(policy, &new_policy);
> > >
> > > If cpufreq_set_policy() uses only a few fields from new_policy,
> >
> > That's really the case.
> >
> > > it might be a good idea to extract those into its own structure.
> >
> > Or organize the code differently.
> >
> > This is old code that hasn't been change, but I'll look at it since it
> > is problematic.
>
> So what about the patch below (untested)?
>
> It should be mostly self-explanatory, and the point is basically that
> cpufreq_set_policy() gets the limits values from freq QoS, so it only
> needs to get the new governor/policy value from the caller and more
> data need to be passed to the driver's ->verify() callback.

Thanks, LGTM!
No Rb, as I'm not that familiar with the code, and only browsed through
the core parts.

I can confirm it gets rids of the -Wframe-larger-than warnings on arm64,
though.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds