Re: [PATCH v5 02/51] x86/apic: Add CONFIG_X86_LOCAL_APIC=n stubs for apic_set_timer_period_{,k}hz()
From: sashiko-bot@kernel.org
Date: 2026-07-01 19:48:26
Also in:
kvm
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `source` argument in `apic_set_timer_period_hz()` and `apic_set_timer_period_khz()` is completely unused, leading to dead code and lost debug logging.
--
commit a7b2ed4060ee2d44136d35fad056804351b581ca
Author: Sean Christopherson [off-list ref]
x86/apic: Add CONFIG_X86_LOCAL_APIC=n stubs for apic_set_timer_period_{,k}hz()
This commit adds stubs for apic_set_timer_period_hz() and
apic_set_timer_period_khz() when the kernel is built without local APIC
support. It also removes the corresponding #ifdef guards from the calling
code in various x86 subsystems.
quoted hunk ↗ jump to hunk
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h index cd84a94688a2f..035998555e992 100644 --- a/arch/x86/include/asm/apic.h +++ b/arch/x86/include/asm/apic.h@@ -189,6 +189,8 @@ static inline void disable_local_APIC(void) { } # define setup_boot_APIC_clock x86_init_noop # define setup_secondary_APIC_clock x86_init_noop static inline void lapic_update_tsc_freq(void) { } +static inline void apic_set_timer_period_hz(u64 period_hz, const char *source) { } +static inline void apic_set_timer_period_khz(u64 period_khz, const char *source) { }
[Severity: Medium]
Is it intentional that the source argument is completely unused here in the
stubs?
Looking at the main implementation in arch/x86/kernel/apic/apic.c:
void apic_set_timer_period_khz(u64 period_khz, const char *source)
{
lapic_timer_period = mul_u64_u32_div(period_khz, 1000, HZ);
}
The source parameter is discarded without being used for debug logging. Since
any code calling apic_set_timer_period_hz() or apic_set_timer_period_khz()
passes a string literal describing the source (such as "VMware hypervisor"
or "CPUID 0x15/0x16"), this results in dropped debug information.
Could the source parameter be removed from the API entirely to clean up the
dead code, or should the boot-time logging be restored?
static inline void init_bsp_APIC(void) { }
static inline void apic_intr_mode_select(void) { }
static inline void apic_intr_mode_init(void) { }[ ... ] -- Sashiko AI review · https://sashiko.dev/#/patchset/20260701193212.749551-1-seanjc@google.com?part=2