Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The `apic_set_timer_period_khz()` and `apic_set_timer_period_hz()` helper functions accept a `const char *source` argument which is never used, resulting in dead code and a silent loss of boot diagnostic logging on platforms where logging was removed. --
commit 4fb576b0d909663c9b3e6628dbed45d329c08a5d Author: Sean Christopherson <[email protected]> x86/apic: Provide helpers to set local APIC timer period in hz and khz This commit adds APIs to set the local APIC timer period, abstracting the HZ math previously open-coded by external callers. It also changes the lapic_timer_period variable to be local to apic.c. > diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c > index aa1e19979aa8e..8d3d930576fd7 100644 > --- a/arch/x86/kernel/apic/apic.c > +++ b/arch/x86/kernel/apic/apic.c [ ... ] > @@ -796,6 +796,16 @@ bool __init apic_needs_pit(void) > return lapic_timer_period == 0; > } > > +void apic_set_timer_period_khz(u64 period_khz, const char *source) > +{ > + lapic_timer_period = mul_u64_u32_div(period_khz, 1000, HZ); > +} > + > +void apic_set_timer_period_hz(u64 period_hz, const char *source) > +{ > + lapic_timer_period = div_u64(period_hz, HZ); > +} [Severity: Medium] It looks like the source parameter is accepted by apic_set_timer_period_khz() and apic_set_timer_period_hz() in arch/x86/kernel/apic/apic.c but is never used. Was it intended to log the frequency using this source string? > diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c > index 185d4f677ec0f..87beecec76f04 100644 > --- a/arch/x86/kernel/cpu/mshyperv.c > +++ b/arch/x86/kernel/cpu/mshyperv.c > @@ -646,10 +646,7 @@ static void __init ms_hyperv_init_platform(void) > u64 hv_lapic_frequency; > > rdmsrq(HV_X64_MSR_APIC_FREQUENCY, hv_lapic_frequency); > - hv_lapic_frequency = div_u64(hv_lapic_frequency, HZ); > - lapic_timer_period = hv_lapic_frequency; > - pr_info("Hyper-V: LAPIC Timer Frequency: %#x\n", > - lapic_timer_period); > + apic_set_timer_period_hz(hv_lapic_frequency, "Hyper-V > hypervisor"); > } > [ ... ] > diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c > index 34b73573b108d..36f779dd311dc 100644 > --- a/arch/x86/kernel/cpu/vmware.c > +++ b/arch/x86/kernel/cpu/vmware.c > @@ -424,9 +424,7 @@ static void __init vmware_platform_setup(void) > > #ifdef CONFIG_X86_LOCAL_APIC > /* Skip lapic calibration since we know the bus frequency. */ > - lapic_timer_period = ecx / HZ; > - pr_info("Host bus clock speed read from hypervisor : %u Hz\n", > - ecx); > + apic_set_timer_period_hz(ecx, "VMware hypervisor"); > #endif [Severity: Medium] These changes remove the pr_info() logging of the APIC frequency during early boot APIC initialization in ms_hyperv_init_platform() (arch/x86/kernel/cpu/mshyperv.c) and vmware_platform_setup() (arch/x86/kernel/cpu/vmware.c). Since the new apic_set_timer_period_hz() helper does not log this information, does this result in an unintended loss of boot diagnostic logging on these platforms? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
