Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Unintended clobbering of the global `cpu_khz` variable in `native_calibrate_tsc()` due to a missing local variable declaration. --
commit 36e45379d4c9f40ae24c5c323b4f0471c94c6bef Author: Sean Christopherson <[email protected]> x86/tsc: Add standalone helper for getting CPU frequency from CPUID This commit extracts the logic for getting the CPU frequency via CPUID into a new standalone helper function. It also updates the TSC calibration code to use this new helper. > diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c > --- a/arch/x86/kernel/tsc.c > +++ b/arch/x86/kernel/tsc.c [ ... ] > @@ -754,13 +772,8 @@ unsigned long native_calibrate_tsc(void) > * clock, but we can easily calculate it to a high degree of accuracy > * by considering the crystal ratio and the CPU speed. > */ > - if (!info.crystal_khz && boot_cpu_data.cpuid_level >= CPUID_LEAF_FREQ) { > - unsigned int eax_base_mhz, ebx, ecx, edx; > - > - cpuid(CPUID_LEAF_FREQ, &eax_base_mhz, &ebx, &ecx, &edx); > - info.crystal_khz = eax_base_mhz * 1000 * > - info.denominator / info.numerator; > - } > + if (!info.crystal_khz && !cpuid_get_cpu_freq(&cpu_khz)) > + info.crystal_khz = cpu_khz * info.denominator / info.numerator; Does this unintentionally clobber the global cpu_khz variable? Because cpu_khz is not declared locally in native_calibrate_tsc(), this appears to pass a pointer to the global cpu_khz variable to cpuid_get_cpu_freq(). Looking at cpuid_get_cpu_freq(), it unconditionally zeroes the target pointer before checking if CPUID_LEAF_FREQ is supported: arch/x86/kernel/tsc.c:cpuid_get_cpu_freq() { ... *cpu_khz = 0; if (boot_cpu_data.cpuid_level < CPUID_LEAF_FREQ) return -ENOENT; ... } If the system exposes CPUID.0x15 with a 0 crystal clock but does not support CPUID.0x16, cpuid_get_cpu_freq() returns -ENOENT and leaves the global cpu_khz as 0. Could this overwrite the previously calibrated CPU frequency and force the kernel to fall back to legacy PIT/HPET calibration? If so, this might cause boot regressions on systems where these legacy timers are disabled or unreliable. Even if CPUID.0x16 is supported, it seems this would silently overwrite the dynamically calibrated global value with the CPUID base frequency. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=40
