Re: [PATCH] cpufreq: Avoid creating excessively large stack frames

[Viresh Kumar]

On 23-01-20, 00:16, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> 
> In the process of modifying a cpufreq policy, the cpufreq core makes
> a copy of it including all of the internals which is stored on the
> CPU stack.  Because struct cpufreq_policy is relatively large, this
> may cause the size of the stack frame to exceed the 2 KB limit and
> so the GCC complains when -Wframe-larger-than= is used.
> 
> In fact, it is not necessary to copy the entire policy structure
> in order to modify it, however.
> 
> First, because cpufreq_set_policy() obtains the min and max policy
> limits from frequency QoS now, it is not necessary to pass the limits
> to it from the callers.  The only things that need to be passed to it
> from there are the new governor pointer or (if there is a built-in
> governor in the driver) the "policy" value representing the governor
> choice.  They both can be passed as individual arguments, though, so
> make cpufreq_set_policy() take them this way and rework its callers
> accordingly.  This avoids making copies of cpufreq policies in the
> callers of cpufreq_set_policy().
> 
> Second, cpufreq_set_policy() still needs to pass the new policy
> data to the ->verify() callback of the cpufreq driver whose task
> is to sanitize the min and max policy limits.  It still does not
> need to make a full copy of struct cpufreq_policy for this purpose,
> but it needs to pass a few items from it to the driver in case they
> are needed (different drivers have different needs in that respect
> and all of them have to be covered).  For this reason, introduce
> struct cpufreq_policy_data to hold copies of the members of
> struct cpufreq_policy used by the existing ->verify() driver
> callbacks and pass a pointer to a temporary structure of that
> type to ->verify() (instead of passing a pointer to full struct
> cpufreq_policy to it).
> 
> While at it, notice that intel_pstate doesn't really need to verify
> the "policy" value in struct cpufreq_policy, so drop that check from
> it to avoid copying "policy" into struct cpufreq_policy_data (which
> allows it to be slightly smaller).
> 
> Also while at it fix up white space in a couple of places and make
> cpufreq_set_policy() static (as it can be so).
> 
> Fixes: 3000ce3c52f8 ("cpufreq: Use per-policy frequency QoS")
> Link: https://lore.kernel.org/linux-pm/CAMuHMdX6-jb1W8uC2_237m8ctCpsnGp=JCxqt8pCWVqNXHmkVg@xxxxxxxxxxxxxx
> Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Cc: 5.4+ <stable@xxxxxxxxxxxxxxx> # 5.4+
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
>  drivers/cpufreq/cppc_cpufreq.c     |    2 
>  drivers/cpufreq/cpufreq-nforce2.c  |    2 
>  drivers/cpufreq/cpufreq.c          |  147 +++++++++++++++++--------------------
>  drivers/cpufreq/freq_table.c       |    4 -
>  drivers/cpufreq/gx-suspmod.c       |    2 
>  drivers/cpufreq/intel_pstate.c     |   38 ++++-----
>  drivers/cpufreq/longrun.c          |    2 
>  drivers/cpufreq/pcc-cpufreq.c      |    2 
>  drivers/cpufreq/sh-cpufreq.c       |    2 
>  drivers/cpufreq/unicore2-cpufreq.c |    2 
>  include/linux/cpufreq.h            |   32 +++++---
>  11 files changed, 119 insertions(+), 116 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>

-- 
viresh