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

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

On Tue, Sep 03, 2024 at 09:53:47AM +0200, Jan Beulich wrote:
> On 03.09.2024 09:35, Roger Pau Monné wrote:
> > On Tue, Sep 03, 2024 at 08:24:18AM +0200, Jan Beulich wrote:
> >> 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.
> > 
> > Note the code in get_cmos_time() is modified, so the variable is no
> > longer used past the call to read_cmos_time().  Instead the signaling
> > of whether the CMOS is functional or not is done using the return
> > value of the newly introduced read_cmos_time() function.
> 
> I wasn't concerned of the further processing on the 1st invocation, but
> of the behavior of the 2nd invocation. But yes, there the flag will end
> up being cleared because of the FADT flag also having been cleared. Not
> easily visible, though. Could minimally do with a remark in the
> description.

Indeed, the variable gets clearer on further invocations, as the
adjustment to ACPI_FADT_NO_CMOS_RTC gets propagated.

Given Andrew comments, I've reworked all of this and I think it ended
up being clearer.

Thanks, Roger.