Re: [PATCH v2 1/2] x86/time: split CMOS time fetching into wrapper

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/08/2024 10:52 am, Roger Pau Monne wrote:
> Split the logic that deals with probing and fetching the CMOS time into a
> separate helper.  While moving the code also take the opportunity to reduce 
> the
> scope of some local variables.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

This does look like a straight rearrangement, so

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, although...

> +static unsigned long get_cmos_time(void)
> +{
> +    struct rtc_time rtc;
> +    static bool __read_mostly cmos_rtc_probe;
> +    boolean_param("cmos-rtc-probe", cmos_rtc_probe);
> +
> +    if ( efi_enabled(EFI_RS) )
> +    {
> +        unsigned long res = efi_get_time();
> +
> +        if ( res )
> +            return res;
> +    }
> +
> +    if ( likely(!(acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_CMOS_RTC)) )
> +        cmos_rtc_probe = false;
> +    else if ( system_state < SYS_STATE_smp_boot && !cmos_rtc_probe )
> +        panic("System with no CMOS RTC advertised must be booted from EFI"
> +              " (or with command line option \"cmos-rtc-probe\")\n");
> +
> +    if ( unlikely(!read_cmos_time(&rtc, cmos_rtc_probe)) )
>          panic("No CMOS RTC found - system must be booted from EFI\n");

... this really does show some (preexisting) tangled logic.

All of this should live in an init function, and not be re-evaluated
each time we call get_wallclock_time(), not that we call it very often.

cmos_rtc_probe should be __initdata or at least __ro_after_init, but it
gets written on every pass through the function on most systems.

Lets focus on not crashing.  Cleanup can come later.

~Andrew