Re: [PATCH v3 01/14] x86/msr: Move rdtsc{,_ordered}() to

Xin Li
2025-04-26T07:28:13Z

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