[PATCH v2 resend 0/2] x86/rtc: refactoring

[PATCH v2 resend 0/2] x86/rtc: refactoring

[Mateusz Jończyk]

Hello,

The first and main patch of this series removes duplicated code in
mach_get_cmos_time() in arch/x86/kernel/rtc.c . This function performed
the same thing as mc146818_get_time() - reading the time from the CMOS
RTC - but used a different algorithm. So modify it to use common
mc146818_get_time().

The second patch renames a function in arch/x86/kernel/rtc.c.

Tested originally (on top of 5.18-rc3) on 3 computers, and in i386 and
amd64 VMs.

Greetings,
Mateusz

v2:
  - drop the previously first patch ("rtc-mc146818-lib: reduce RTC_UIP
    polling period"). Its intent was to make behaviour of
    mc146818_get_time() more similar to the existing version of
    mach_get_cmos_time() by decreasing the polling period in
    mc146818_get_time(). However, I determined that this does not matter
    for users of mach_get_cmos_time().

  - use pr_err instead of pr_err_ratelimited in the first remaining
    patch,

  - small patch description modifications.
        
Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: x86@kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>

Mateusz Jończyk (2):
  x86/rtc: rewrite mach_get_cmos_time to delete duplicated code
  x86/rtc: rename mach_set_rtc_mmss

 arch/x86/include/asm/mc146818rtc.h |  2 +-
 arch/x86/kernel/rtc.c              | 63 +++++-------------------------
 arch/x86/kernel/x86_init.c         |  2 +-
 3 files changed, 11 insertions(+), 56 deletions(-)


base-commit: 42226c989789d8da4af1de0c31070c96726d990c
-- 
2.25.1

[PATCH v2 resend 1/2] x86/rtc: rewrite mach_get_cmos_time to delete duplicated code

[Mateusz Jończyk]

There are functions in drivers/rtc/rtc-mc146818-lib.c that handle
reading from / writing to the CMOS RTC clock. mach_get_cmos_time() in
arch/x86/kernel/rtc.c did not use them and was mostly a duplicate of
mc146818_get_time(). Modify mach_get_cmos_time() to use
mc146818_get_time() and remove the duplicated code.

mach_get_cmos_time() used a different algorithm than
mc146818_get_time(), but these functions are equivalent. The major
differences were:

- mc146818_get_time() is better refined: it was updated in
  commit 05a0302c3548 ("rtc: mc146818: Prevent reading garbage")
  to take account of various edge conditions,

- when the UIP ("Update in progress") bit of the RTC is set,
  mach_get_cmos_time() was busy waiting with cpu_relax() while
  mc146818_get_time() is now using mdelay(1) in every loop iteration,

- mach_get_cmos_time() assumed that the RTC year must be >= 2000, which
  may not be true on some old boxes with a dead battery,

- mach_get_cmos_time() was holding the rtc_lock for a long time.

As a bonus, mach_get_cmos_time() after the modification does not hang
indefinitely if the CMOS RTC is not present.

The RTC writing counterpart, mach_set_rtc_mmss() is already using
mc146818_get_time() from drivers/rtc. This was done in
        commit 3195ef59cb42 ("x86: Do full rtc synchronization with ntp")
It appears that mach_get_cmos_time() was simply forgotten.

mach_get_cmos_time() is really used only in read_persistent_clock64(),
which is called only in a few places in kernel/time/timekeeping.c .

Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: x86@kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Prarit Bhargava <prarit@redhat.com>

---

v2:
  - use pr_err() in place of pr_err_ratelimited(). mach_get_cmos_time()
    is not called frequently, so ratelimiting is not necessary.
  - tweak commit description.
---
 arch/x86/kernel/rtc.c | 59 +++++--------------------------------------
 1 file changed, 7 insertions(+), 52 deletions(-)

diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 586f718b8e95..1cadc8a15267 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -4,11 +4,8 @@
  */
 #include <linux/platform_device.h>
 #include <linux/mc146818rtc.h>
-#include <linux/acpi.h>
-#include <linux/bcd.h>
 #include <linux/export.h>
 #include <linux/pnp.h>
-#include <linux/of.h>
 
 #include <asm/vsyscall.h>
 #include <asm/x86_init.h>
@@ -20,15 +17,12 @@
 /*
  * This is a special lock that is owned by the CPU and holds the index
  * register we are working with.  It is required for NMI access to the
- * CMOS/RTC registers.  See include/asm-i386/mc146818rtc.h for details.
+ * CMOS/RTC registers.  See arch/x86/include/asm/mc146818rtc.h for details.
  */
 volatile unsigned long cmos_lock;
 EXPORT_SYMBOL(cmos_lock);
 #endif /* CONFIG_X86_32 */
 
-/* For two digit years assume time is always after that */
-#define CMOS_YEARS_OFFS 2000
-
 DEFINE_SPINLOCK(rtc_lock);
 EXPORT_SYMBOL(rtc_lock);
 
@@ -62,8 +56,7 @@ int mach_set_rtc_mmss(const struct timespec64 *now)
 
 void mach_get_cmos_time(struct timespec64 *now)
 {
-	unsigned int status, year, mon, day, hour, min, sec, century = 0;
-	unsigned long flags;
+	struct rtc_time tm;
 
 	/*
 	 * If pm_trace abused the RTC as storage, set the timespec to 0,
@@ -74,51 +67,13 @@ void mach_get_cmos_time(struct timespec64 *now)
 		return;
 	}
 
-	spin_lock_irqsave(&rtc_lock, flags);
-
-	/*
-	 * If UIP is clear, then we have >= 244 microseconds before
-	 * RTC registers will be updated.  Spec sheet says that this
-	 * is the reliable way to read RTC - registers. If UIP is set
-	 * then the register access might be invalid.
-	 */
-	while ((CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP))
-		cpu_relax();
-
-	sec = CMOS_READ(RTC_SECONDS);
-	min = CMOS_READ(RTC_MINUTES);
-	hour = CMOS_READ(RTC_HOURS);
-	day = CMOS_READ(RTC_DAY_OF_MONTH);
-	mon = CMOS_READ(RTC_MONTH);
-	year = CMOS_READ(RTC_YEAR);
-
-#ifdef CONFIG_ACPI
-	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
-	    acpi_gbl_FADT.century)
-		century = CMOS_READ(acpi_gbl_FADT.century);
-#endif
-
-	status = CMOS_READ(RTC_CONTROL);
-	WARN_ON_ONCE(RTC_ALWAYS_BCD && (status & RTC_DM_BINARY));
-
-	spin_unlock_irqrestore(&rtc_lock, flags);
-
-	if (RTC_ALWAYS_BCD || !(status & RTC_DM_BINARY)) {
-		sec = bcd2bin(sec);
-		min = bcd2bin(min);
-		hour = bcd2bin(hour);
-		day = bcd2bin(day);
-		mon = bcd2bin(mon);
-		year = bcd2bin(year);
+	if (mc146818_get_time(&tm)) {
+		pr_err("Unable to read current time from RTC\n");
+		now->tv_sec = now->tv_nsec = 0;
+		return;
 	}
 
-	if (century) {
-		century = bcd2bin(century);
-		year += century * 100;
-	} else
-		year += CMOS_YEARS_OFFS;
-
-	now->tv_sec = mktime64(year, mon, day, hour, min, sec);
+	now->tv_sec = rtc_tm_to_time64(&tm);
 	now->tv_nsec = 0;
 }
 
-- 
2.25.1

Re: [PATCH v2 resend 1/2] x86/rtc: rewrite mach_get_cmos_time to delete duplicated code

[Alexandre Belloni]

On 17/05/2022 22:21:30+0200, Mateusz Jończyk wrote:
> There are functions in drivers/rtc/rtc-mc146818-lib.c that handle
> reading from / writing to the CMOS RTC clock. mach_get_cmos_time() in
> arch/x86/kernel/rtc.c did not use them and was mostly a duplicate of
> mc146818_get_time(). Modify mach_get_cmos_time() to use
> mc146818_get_time() and remove the duplicated code.
> 
> mach_get_cmos_time() used a different algorithm than
> mc146818_get_time(), but these functions are equivalent. The major
> differences were:
> 
> - mc146818_get_time() is better refined: it was updated in
>   commit 05a0302c3548 ("rtc: mc146818: Prevent reading garbage")
>   to take account of various edge conditions,
> 
> - when the UIP ("Update in progress") bit of the RTC is set,
>   mach_get_cmos_time() was busy waiting with cpu_relax() while
>   mc146818_get_time() is now using mdelay(1) in every loop iteration,
> 
> - mach_get_cmos_time() assumed that the RTC year must be >= 2000, which
>   may not be true on some old boxes with a dead battery,
> 
> - mach_get_cmos_time() was holding the rtc_lock for a long time.
> 
> As a bonus, mach_get_cmos_time() after the modification does not hang
> indefinitely if the CMOS RTC is not present.
> 
> The RTC writing counterpart, mach_set_rtc_mmss() is already using
> mc146818_get_time() from drivers/rtc. This was done in
>         commit 3195ef59cb42 ("x86: Do full rtc synchronization with ntp")
> It appears that mach_get_cmos_time() was simply forgotten.
> 
> mach_get_cmos_time() is really used only in read_persistent_clock64(),
> which is called only in a few places in kernel/time/timekeeping.c .
> 
> Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: x86@kernel.org
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Cc: Prarit Bhargava <prarit@redhat.com>
> 
> ---
> 
> v2:
>   - use pr_err() in place of pr_err_ratelimited(). mach_get_cmos_time()
>     is not called frequently, so ratelimiting is not necessary.
>   - tweak commit description.
> ---
>  arch/x86/kernel/rtc.c | 59 +++++--------------------------------------
>  1 file changed, 7 insertions(+), 52 deletions(-)
> 
> diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
> index 586f718b8e95..1cadc8a15267 100644
> --- a/arch/x86/kernel/rtc.c
> +++ b/arch/x86/kernel/rtc.c
> @@ -4,11 +4,8 @@
>   */
>  #include <linux/platform_device.h>
>  #include <linux/mc146818rtc.h>
> -#include <linux/acpi.h>
> -#include <linux/bcd.h>
>  #include <linux/export.h>
>  #include <linux/pnp.h>
> -#include <linux/of.h>
>  
>  #include <asm/vsyscall.h>
>  #include <asm/x86_init.h>
> @@ -20,15 +17,12 @@
>  /*
>   * This is a special lock that is owned by the CPU and holds the index
>   * register we are working with.  It is required for NMI access to the
> - * CMOS/RTC registers.  See include/asm-i386/mc146818rtc.h for details.
> + * CMOS/RTC registers.  See arch/x86/include/asm/mc146818rtc.h for details.
>   */
>  volatile unsigned long cmos_lock;
>  EXPORT_SYMBOL(cmos_lock);
>  #endif /* CONFIG_X86_32 */
>  
> -/* For two digit years assume time is always after that */
> -#define CMOS_YEARS_OFFS 2000
> -
>  DEFINE_SPINLOCK(rtc_lock);
>  EXPORT_SYMBOL(rtc_lock);
>  
> @@ -62,8 +56,7 @@ int mach_set_rtc_mmss(const struct timespec64 *now)
>  
>  void mach_get_cmos_time(struct timespec64 *now)
>  {
> -	unsigned int status, year, mon, day, hour, min, sec, century = 0;
> -	unsigned long flags;
> +	struct rtc_time tm;
>  
>  	/*
>  	 * If pm_trace abused the RTC as storage, set the timespec to 0,
> @@ -74,51 +67,13 @@ void mach_get_cmos_time(struct timespec64 *now)
>  		return;
>  	}
>  
> -	spin_lock_irqsave(&rtc_lock, flags);
> -
> -	/*
> -	 * If UIP is clear, then we have >= 244 microseconds before
> -	 * RTC registers will be updated.  Spec sheet says that this
> -	 * is the reliable way to read RTC - registers. If UIP is set
> -	 * then the register access might be invalid.
> -	 */
> -	while ((CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP))
> -		cpu_relax();
> -
> -	sec = CMOS_READ(RTC_SECONDS);
> -	min = CMOS_READ(RTC_MINUTES);
> -	hour = CMOS_READ(RTC_HOURS);
> -	day = CMOS_READ(RTC_DAY_OF_MONTH);
> -	mon = CMOS_READ(RTC_MONTH);
> -	year = CMOS_READ(RTC_YEAR);
> -
> -#ifdef CONFIG_ACPI
> -	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
> -	    acpi_gbl_FADT.century)
> -		century = CMOS_READ(acpi_gbl_FADT.century);
> -#endif
> -
> -	status = CMOS_READ(RTC_CONTROL);
> -	WARN_ON_ONCE(RTC_ALWAYS_BCD && (status & RTC_DM_BINARY));
> -
> -	spin_unlock_irqrestore(&rtc_lock, flags);
> -
> -	if (RTC_ALWAYS_BCD || !(status & RTC_DM_BINARY)) {
> -		sec = bcd2bin(sec);
> -		min = bcd2bin(min);
> -		hour = bcd2bin(hour);
> -		day = bcd2bin(day);
> -		mon = bcd2bin(mon);
> -		year = bcd2bin(year);
> +	if (mc146818_get_time(&tm)) {
> +		pr_err("Unable to read current time from RTC\n");
> +		now->tv_sec = now->tv_nsec = 0;
> +		return;
>  	}
>  
> -	if (century) {
> -		century = bcd2bin(century);
> -		year += century * 100;
> -	} else
> -		year += CMOS_YEARS_OFFS;
> -
> -	now->tv_sec = mktime64(year, mon, day, hour, min, sec);
> +	now->tv_sec = rtc_tm_to_time64(&tm);
>  	now->tv_nsec = 0;
>  }
>  
> -- 
> 2.25.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v2 resend 2/2] x86/rtc: rename mach_set_rtc_mmss

[Mateusz Jończyk]

Once upon a time, before
commit 3195ef59cb42 ("x86: Do full rtc synchronization with ntp")
in 2013, the function set only the minute and seconds registers
of the CMOS RTC. This is no longer true, so rename the function to
mach_set_cmos_time.

Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: x86@kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 arch/x86/include/asm/mc146818rtc.h | 2 +-
 arch/x86/kernel/rtc.c              | 4 ++--
 arch/x86/kernel/x86_init.c         | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/mc146818rtc.h b/arch/x86/include/asm/mc146818rtc.h
index 97198001e567..6115bb3d5795 100644
--- a/arch/x86/include/asm/mc146818rtc.h
+++ b/arch/x86/include/asm/mc146818rtc.h
@@ -95,7 +95,7 @@ static inline unsigned char current_lock_cmos_reg(void)
 unsigned char rtc_cmos_read(unsigned char addr);
 void rtc_cmos_write(unsigned char val, unsigned char addr);
 
-extern int mach_set_rtc_mmss(const struct timespec64 *now);
+extern int mach_set_cmos_time(const struct timespec64 *now);
 extern void mach_get_cmos_time(struct timespec64 *now);
 
 #define RTC_IRQ 8
diff --git a/arch/x86/kernel/rtc.c b/arch/x86/kernel/rtc.c
index 1cadc8a15267..349046434513 100644
--- a/arch/x86/kernel/rtc.c
+++ b/arch/x86/kernel/rtc.c
@@ -27,13 +27,13 @@ DEFINE_SPINLOCK(rtc_lock);
 EXPORT_SYMBOL(rtc_lock);
 
 /*
- * In order to set the CMOS clock precisely, set_rtc_mmss has to be
+ * In order to set the CMOS clock precisely, mach_set_cmos_time has to be
  * called 500 ms after the second nowtime has started, because when
  * nowtime is written into the registers of the CMOS clock, it will
  * jump to the next second precisely 500 ms later. Check the Motorola
  * MC146818A or Dallas DS12887 data sheet for details.
  */
-int mach_set_rtc_mmss(const struct timespec64 *now)
+int mach_set_cmos_time(const struct timespec64 *now)
 {
 	unsigned long long nowtime = now->tv_sec;
 	struct rtc_time tm;
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index e84ee5cdbd8c..57353519bc11 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -138,7 +138,7 @@ struct x86_platform_ops x86_platform __ro_after_init = {
 	.calibrate_cpu			= native_calibrate_cpu_early,
 	.calibrate_tsc			= native_calibrate_tsc,
 	.get_wallclock			= mach_get_cmos_time,
-	.set_wallclock			= mach_set_rtc_mmss,
+	.set_wallclock			= mach_set_cmos_time,
 	.iommu_shutdown			= iommu_shutdown_noop,
 	.is_untracked_pat_range		= is_ISA_range,
 	.nmi_init			= default_nmi_init,
-- 
2.25.1