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
