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.

The goal, albeit not accomplished by this patch, is to be able to split the
probing and the reading of the CMOS RTC data into two separate functions.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v2:
 - New in this version.
---
 xen/arch/x86/time.c | 50 +++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index d022db4bd4a0..2a64687bf45b 100644
--- 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);
+
+    return t1 <= SECONDS(1) && t2 < MILLISECS(3);
 }
 
 static unsigned long get_cmos_time(void)
 {
-    unsigned long res, flags;
+    unsigned long res;
     struct rtc_time rtc;
     unsigned int seconds = 60;
     static bool __read_mostly cmos_rtc_probe;
@@ -1293,29 +1315,9 @@ static unsigned long get_cmos_time(void)
 
     for ( ; ; )
     {
-        s_time_t start, t1, t2;
-
-        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) );
-
-        __get_cmos_time(&rtc);
-
-        spin_unlock_irqrestore(&rtc_lock, flags);
+        bool success = __get_cmos_time(&rtc);
 
-        if ( likely(!cmos_rtc_probe) ||
-             t1 > SECONDS(1) || t2 >= MILLISECS(3) ||
+        if ( likely(!cmos_rtc_probe) || !success ||
              rtc.sec >= 60 || rtc.min >= 60 || rtc.hour >= 24 ||
              !rtc.day || rtc.day > 31 ||
              !rtc.mon || rtc.mon > 12 )
-- 
2.46.0

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

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.