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

Yuan, Perry
2022-09-01T05:35:02Z

[AMD Official Use Only - General]

Hi Punit,

> -----Original Message-----
> From: Punit Agrawal <punit.agrawal@bytedance.com>
> Sent: Monday, August 15, 2022 11:05 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>
> Cc: rafael.j.wysocki@intel.com; Huang, Ray <Ray.Huang@amd.com>;
> viresh.kumar@linaro.org; Sharma, Deepak <Deepak.Sharma@amd.com>;
> Limonciello, Mario <Mario.Limonciello@amd.com>; Fontenot, Nathan
> <Nathan.Fontenot@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Su, Jinzhou (Joe) <Jinzhou.Su@amd.com>;
> Huang, Shimmer <Shimmer.Huang@amd.com>; Du, Xiaojian
> <Xiaojian.Du@amd.com>; Meng, Li (Jassmine) <Li.Meng@amd.com>; linux-
> pm@vger.kernel.org; linux-kernel@vger.kernel.org
> 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@amd.com> 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@amd.com>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Perry Yuan <Perry.Yuan@amd.com>
> > ---
> > 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