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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan