Re: [PATCH v5 02/51] x86/apic: Add CONFIG_X86_LOCAL_APIC=n stubs for apic_set_timer_period_{,k}hz()

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 <[email protected]>

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.

> 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/[email protected]?part=2