On Tue, Sep 03, 2024 at 05:02:21PM +0200, Jan Beulich wrote:
> 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.
I've expanded the commit message a bit to reflect what you mention
here.
> > --- 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.
I could even cache the value of CMOS_READ(RTC_CONTROL) ahead of the
check, so that the drop could be dropped earlier, but I'm not going to
do it here.
Thanks, Roger.
