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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.