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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.08.2024 11:52, Roger Pau Monne wrote:
> @@ -1285,33 +1270,56 @@ static unsigned long get_cmos_time(void)
>          } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
>                    t2 < MILLISECS(3) );
>  
> -        __get_cmos_time(&rtc);
> +        __get_cmos_time(rtc);
>  
>          spin_unlock_irqrestore(&rtc_lock, flags);
>  
> -        if ( likely(!cmos_rtc_probe) ||
> -             t1 > SECONDS(1) || t2 >= MILLISECS(3) ||
> -             rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 ||
> -             !rtc.day || rtc.day > 31 ||
> -             !rtc.mon || rtc.mon > 12 )
> -            break;
> +        if ( likely(!cmos_rtc_probe) )
> +            return true;
> +
> +        if ( t1 > SECONDS(1) || t2 >= MILLISECS(3) ||
> +             rtc->sec >= 60 || rtc->min >= 60 || rtc->hour >= 24 ||
> +             !rtc->day || rtc->day > 31 ||
> +             !rtc->mon || rtc->mon > 12 )
> +            return false;
>  
>          if ( seconds < 60 )
>          {
> -            if ( rtc.sec != seconds )
> -            {
> -                cmos_rtc_probe = false;

This clearing of the variable is lost, which looks wrong to me.

Jan

> +            if ( rtc->sec != seconds )
>                  acpi_gbl_FADT.boot_flags &= ~ACPI_FADT_NO_CMOS_RTC;
> -            }
> -            break;
> +            return true;
>          }
>  
>          process_pending_softirqs();
>  
> -        seconds = rtc.sec;
> +        seconds = rtc->sec;
>      }
>  
> -    if ( unlikely(cmos_rtc_probe) )
> +    ASSERT_UNREACHABLE();
> +    return false;
> +}
> +
> +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");
>  
>      return mktime(rtc.year, rtc.mon, rtc.day, rtc.hour, rtc.min, rtc.sec);