Re: [PATCH v3 01/14] x86/msr: Move rdtsc{,_ordered}() to <asm/tsc.h>

[Xin Li <xin@zytor.com>]

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