On 4/25/2025 8:45 AM, Ilpo Järvinen wrote:
To me this looks really a random set of source files, maybe it helped some
build success but it's hard for me to review this because there are still
cases that depend on indirect include chains.
Could you just look into solving all missing msr.h includes instead
as clearly some are still missing after 3 pre-existing ones and you adding
it into 3 files:
$ git grep -e rdmsr -e wrmsr -l drivers/platform/x86/
drivers/platform/x86/intel/ifs/core.c
drivers/platform/x86/intel/ifs/load.c
drivers/platform/x86/intel/ifs/runtest.c
drivers/platform/x86/intel/pmc/cnp.c
drivers/platform/x86/intel/pmc/core.c
drivers/platform/x86/intel/speed_select_if/isst_if_common.c
drivers/platform/x86/intel/speed_select_if/isst_if_mbox_msr.c
drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c
drivers/platform/x86/intel/tpmi_power_domains.c
drivers/platform/x86/intel/turbo_max_3.c
drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
drivers/platform/x86/intel_ips.c
$ git grep -e 'msr.h' -l drivers/platform/x86/
drivers/platform/x86/intel/pmc/core.c
drivers/platform/x86/intel/tpmi_power_domains.c
drivers/platform/x86/intel_ips.c
I think you want me to add all necessary direct inclusions, right?
This is the right thing to do, and I did try it but gave up later.
I will do it in the next iteration as you asked. But I want to make my
points:
1) It's not just two patterns {rd,wr}msr, there are a lot of definitions
in <asm/msr.h> and we need to cover all of them:
struct msr_info
struct msr_regs_info
struct saved_msr
struct saved_msrs
{read,write}_msr
rdpmc
.*msr.*_on_cpu
2) Once all necessary direct inclusions are in place, it's easy to
overlook adding a header inclusion in practice, especially if the
build passes. Besides we often forget to remove a header when a
definition is removed. In other words, direct inclusion is hard to
maintain.
3) Some random kernel configuration combinations can cause the current
kernel build to fail. I hit one in x86 UML.
We all know Ingo is the best person to discuss this with :). While my
understanding of the header inclusion issue may be inaccurate or
outdated.
So for me, using "make allyesconfig" is a practical method for a quick
local build check, plus I always send my patches to Intel LKP.
There probably wants a script that identifies all files that reference a
definition in a header thus need to include it explicitly. And indirect
includes should be zapped.
I'd also prefer the patch(es) adding missing includes be in a different
patch.
Great suggestion! It clearly highlights the most significant changes.
Thanks!
Xin
