RE: [PATCH v5 1/7] cpufreq: amd-pstate: cleanup the unused and duplicated headers declaration

[Yuan, Perry]

[AMD Official Use Only - General]

Hi Punit, 

> -----Original Message-----
> From: Punit Agrawal <punit.agrawal@xxxxxxxxxxxxx>
> Sent: Monday, August 15, 2022 11:05 PM
> To: Yuan, Perry <Perry.Yuan@xxxxxxx>
> Cc: rafael.j.wysocki@xxxxxxxxx; Huang, Ray <Ray.Huang@xxxxxxx>;
> viresh.kumar@xxxxxxxxxx; Sharma, Deepak <Deepak.Sharma@xxxxxxx>;
> Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Fontenot, Nathan
> <Nathan.Fontenot@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Su, Jinzhou (Joe) <Jinzhou.Su@xxxxxxx>;
> Huang, Shimmer <Shimmer.Huang@xxxxxxx>; Du, Xiaojian
> <Xiaojian.Du@xxxxxxx>; Meng, Li (Jassmine) <Li.Meng@xxxxxxx>; linux-
> pm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 1/7] cpufreq: amd-pstate: cleanup the unused and
> duplicated headers declaration
> 
> [CAUTION: External Email]
> 
> Hi Perry,
> 
> Perry Yuan <Perry.Yuan@xxxxxxx> writes:
> 
> > Cleanup the headers declaration which are not used actually and some
> > duplicated declaration which is declarated in some other headers
> > already, it will help to simplify the header part.
> 
> We usually don't get rid of indirectly included headers as long as definitions from
> header are used in the code. This avoids problems if for some reason the
> included header gets dropped - it'll leave the code in an uncompilable state.
> 
> More below.
> 
> >
> > Reviewed-by: Huang Rui <ray.huang@xxxxxxx>
> > Acked-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > Signed-off-by: Perry Yuan <Perry.Yuan@xxxxxxx>
> > ---
> >  drivers/cpufreq/amd-pstate.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/cpufreq/amd-pstate.c
> > b/drivers/cpufreq/amd-pstate.c index 9ac75c1cde9c..19a078e232dd 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -31,19 +31,14 @@
> >  #include <linux/compiler.h>
> >  #include <linux/dmi.h>
> >  #include <linux/slab.h>
> > -#include <linux/acpi.h>
> >  #include <linux/io.h>
> >  #include <linux/delay.h>
> >  #include <linux/uaccess.h>
> >  #include <linux/static_call.h>
> >
> > -#include <acpi/processor.h>
> >  #include <acpi/cppc_acpi.h>
> >
> >  #include <asm/msr.h>
> > -#include <asm/processor.h>
> 
> On a quick scan, I noticed that "boot_cpu_data" and "boot_cpu_has()" in the
> module init function are defined in "asm/processor.h" that is being removed
> here. It may compile for now but makes the code more fragile as explained
> above.
> 
> Please ensure that only the header files that have no definitions used in this file
> (amd-pstate.c) are dropped.

Here is the part of the headers we are working on.

==============================================
#include <linux/static_call.h>

#include <acpi/cppc_acpi.h>

#include <asm/msr.h>
#include "amd-pstate-trace.h"

============================================

I have tested the latest Linux-pm/bleeding-edge branch to verify the changes working.
As I know  the "asm/processor.h" is included  by the  "acpi/cppc_acpi.h" subpath

#include <acpi/cppc_acpi.h> 
   #include <linux/acpi.h>
         #include <acpi/acpi.h> 
              #include <asm/acpi.h>  
                       #include <asm/processor.h>
 
To make it comfortable for us, I will send V6 for this patch (1/7)

Perry.

> 
> > -#include <asm/cpufeature.h>
> > -#include <asm/cpu_device_id.h>
> >  #include "amd-pstate-trace.h"
> >
> >  #define AMD_PSTATE_TRANSITION_LATENCY        0x20000