Re: [PATCH v3 2/7] x86/time: move CMOS edge detection into read helper

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.09.2024 15:02, Roger Pau Monne wrote:
> Move the logic that ensures the CMOS RTC data is read just after it's been
> updated into the __get_cmos_time() function that does the register reads.  
> This
> requires returning a boolean from __get_cmos_time() to signal whether the read
> has been successfully performed after an update.

Considering the limited use of both function, that's probably fine a change
to make, despite me otherwise thinking that this is the slightly wrong move.
I'd generally expect __get_cmos_time() to be usable without that checking,
so long as the results are interpreted appropriately.

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1247,8 +1247,26 @@ struct rtc_time {
>      unsigned int year, mon, day, hour, min, sec;
>  };
>  
> -static void __get_cmos_time(struct rtc_time *rtc)
> +static bool __get_cmos_time(struct rtc_time *rtc)
>  {
> +    s_time_t start, t1, t2;
> +    unsigned long flags;
> +
> +    spin_lock_irqsave(&rtc_lock, flags);
> +
> +    /* read RTC exactly on falling edge of update flag */
> +    start = NOW();
> +    do { /* may take up to 1 second... */
> +        t1 = NOW() - start;
> +    } while ( !(CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
> +              t1 <= SECONDS(1) );
> +
> +    start = NOW();
> +    do { /* must try at least 2.228 ms */
> +        t2 = NOW() - start;
> +    } while ( (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) &&
> +              t2 < MILLISECS(3) );
> +
>      rtc->sec  = CMOS_READ(RTC_SECONDS);
>      rtc->min  = CMOS_READ(RTC_MINUTES);
>      rtc->hour = CMOS_READ(RTC_HOURS);
> @@ -1268,11 +1286,15 @@ static void __get_cmos_time(struct rtc_time *rtc)
>  
>      if ( (rtc->year += 1900) < 1970 )
>          rtc->year += 100;
> +
> +    spin_unlock_irqrestore(&rtc_lock, flags);

Imo this unlock wants placing at least ahead of the if() in context. The
lock needs to protect only the port accesses, not any of the calculations.

Jan