Re: [PATCH v4 01/15] x86/msr: Add missing includes of <asm/msr.h>

[Xin Li <xin@zytor.com>]

On 4/29/2025 2:45 AM, Ilpo Järvinen wrote:
> On Sun, 27 Apr 2025, Xin Li (Intel) wrote:
> 
>> For some reason, there are some TSC-related functions in the MSR
>> header even though there is a tsc.h header.
>>
>> To facilitate the relocation of rdtsc{,_ordered}() from <asm/msr.h>
>> to <asm/tsc.h> and to eventually eliminate the inclusion of
>> <asm/msr.h> in <asm/tsc.h>, add <asm/msr.h> to the source files that
>> reference definitions from <asm/msr.h>.
>>
>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
>> Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
>> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>> ---
>>
>> Change in v4:
>> *) Add missing includes in a different patch (Ilpo Järvinen).
>> *) Add all necessary direct inclusions for msr.h (Ilpo Järvinen).
>>
>> Change in v3:
>> * Add a problem statement to the changelog (Dave Hansen).
>> ---
>>   arch/x86/events/msr.c                                         | 3 +++
>>   arch/x86/events/perf_event.h                                  | 1 +
>>   arch/x86/events/probe.c                                       | 2 ++
> 
> Under arch/x86/events/ a few files seem to be missing the include?
> 
>>   arch/x86/hyperv/ivm.c                                         | 1 +
> 
> Also under hyperv/ not all files are covered but I'm a bit hesitant to
> suggest a change there since I'm not sure if they (hypervisors) do
> something special w.r.t. msr.
> 
>>   arch/x86/include/asm/fred.h                                   | 1 +
>>   arch/x86/include/asm/microcode.h                              | 2 ++
>>   arch/x86/include/asm/mshyperv.h                               | 1 +
>>   arch/x86/include/asm/msr.h                                    | 1 +
>>   arch/x86/include/asm/suspend_32.h                             | 1 +
>>   arch/x86/include/asm/suspend_64.h                             | 1 +
>>   arch/x86/include/asm/switch_to.h                              | 2 ++
> 
> arch/x86/kernel/acpi/ ?
> acrh/x86/kernel/cet.c ?
> ...
> 
> There seem to be quite many under arch/x86/ that still don't have it, I
> didn't list them all as there were so many after this point.
> 
> But that's up to x86 maintainers how throughout they want you to be.
> 
> This command may be helpful to exclude the files which already have the
> include so you can focus on the ones that may still be missing it:
> 
> git grep -l -e rdmsr -e wrmsr | grep -v -f <(git grep -l -e 'asm/msr\.h')
> 
>>   arch/x86/kernel/cpu/resctrl/pseudo_lock.c                     | 1 +
>>   arch/x86/kernel/fpu/xstate.h                                  | 1 +
>>   arch/x86/kernel/hpet.c                                        | 1 +
>>   arch/x86/kernel/process_64.c                                  | 1 +
>>   arch/x86/kernel/trace_clock.c                                 | 2 +-
>>   arch/x86/kernel/tsc_sync.c                                    | 1 +
>>   arch/x86/lib/kaslr.c                                          | 2 +-
>>   arch/x86/mm/mem_encrypt_identity.c                            | 1 +
>>   arch/x86/realmode/init.c                                      | 1 +
>>   drivers/acpi/acpi_extlog.c                                    | 1 +
>>   drivers/acpi/processor_perflib.c                              | 1 +
>>   drivers/acpi/processor_throttling.c                           | 3 ++-
>>   drivers/char/agp/nvidia-agp.c                                 | 1 +
>>   drivers/cpufreq/amd-pstate-ut.c                               | 2 ++
>>   drivers/crypto/ccp/sev-dev.c                                  | 1 +
>>   drivers/edac/amd64_edac.c                                     | 1 +
>>   drivers/edac/ie31200_edac.c                                   | 1 +
>>   drivers/edac/mce_amd.c                                        | 1 +
>>   drivers/hwmon/hwmon-vid.c                                     | 4 ++++
>>   drivers/idle/intel_idle.c                                     | 1 +
>>   drivers/misc/cs5535-mfgpt.c                                   | 1 +
>>   drivers/net/vmxnet3/vmxnet3_drv.c                             | 4 ++++
>>   drivers/platform/x86/intel/ifs/core.c                         | 1 +
>>   drivers/platform/x86/intel/ifs/load.c                         | 1 +
>>   drivers/platform/x86/intel/ifs/runtest.c                      | 1 +
>>   drivers/platform/x86/intel/pmc/cnp.c                          | 1 +
>>   drivers/platform/x86/intel/speed_select_if/isst_if_common.c   | 1 +
>>   drivers/platform/x86/intel/speed_select_if/isst_if_mbox_msr.c | 1 +
>>   drivers/platform/x86/intel/speed_select_if/isst_tpmi_core.c   | 1 +
>>   drivers/platform/x86/intel/turbo_max_3.c                      | 1 +
>>   .../platform/x86/intel/uncore-frequency/uncore-frequency.c    | 1 +
>>   drivers/powercap/intel_rapl_common.c                          | 1 +
>>   drivers/powercap/intel_rapl_msr.c                             | 1 +
>>   .../thermal/intel/int340x_thermal/processor_thermal_device.c  | 1 +
>>   drivers/thermal/intel/intel_tcc_cooling.c                     | 1 +
>>   drivers/thermal/intel/x86_pkg_temp_thermal.c                  | 1 +
>>   drivers/video/fbdev/geode/display_gx.c                        | 1 +
>>   drivers/video/fbdev/geode/gxfb_core.c                         | 1 +
>>   drivers/video/fbdev/geode/lxfb_ops.c                          | 1 +
> 
> Under drivers/ this looked pretty complete. Nice work.
> 
> Acked-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> # for pdx86

Thanks a lot!


> 
> I also noticed these files might not need to include msr.h:
> 
> drivers/cpufreq/elanfreq.c
> drivers/cpufreq/sc520_freq.c
> drivers/accel/habanalabs/common/habanalabs_ioctl.c
> 
> ...so if you want, you may consider optionally adding a cleanup patch to
> remove the include from them.
> 
>> --- a/drivers/video/fbdev/geode/gxfb_core.c
>> +++ b/drivers/video/fbdev/geode/gxfb_core.c
>> @@ -30,6 +30,7 @@
>>   #include <linux/cs5535.h>
>>   
>>   #include <asm/olpc.h>
>> +#include <asm/msr.h>
> 
> In wrong order.
>>   
>>   #include "gxfb.h"
> 

I am more than happy to send v4A to address all your comments.