Thread (33 messages) 33 messages, 7 authors, 2021-01-31

Re: [patch 1/8] rtc: mc146818: Prevent reading garbage - bug

From: Mickaël Salaün <mic@digikod.net>
Date: 2021-01-26 14:20:43
Also in: lkml

Thanks for the fix! It boots now with a new message:
rtc_cmos rtc_cmos: not accessible

I tested with rtc=on and rtc=off (which didn't make a difference before
this fix) on a microvm:
https://github.com/qemu/qemu/blob/master/docs/system/i386/microvm.rst

There is one issue with the memset though:

On 26/01/2021 14:26, Thomas Gleixner wrote:
quoted hunk ↗ jump to hunk
On Mon, Jan 25 2021 at 19:40, Mickaël Salaün wrote:
quoted
After some bisecting, I found that commit 05a0302c3548 ("rtc: mc146818:
Prevent reading garbage", this patch, introduced since v5.11-rc1) makes
my VM hang at boot. Before this commit, I got this (and didn't notice)
at every boot:
rtc_cmos rtc_cmos: registered as rtc0
rtc_cmos rtc_cmos: hctosys: unable to read the hardware clock
rtc_cmos rtc_cmos: alarms up to one day, 114 bytes nvram

I notice that this patch creates infinite loops, which my VM falls into
(cf. below).
quoted
+	time->tm_sec = CMOS_READ(RTC_SECONDS);
+
+	if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) {
+		spin_unlock_irqrestore(&rtc_lock, flags);
+		mdelay(1);
My VM loops here.
time->tm_sec is always 255.
That means there is no RTC and therefore the CMOS_READ($REG) returns
0xFF which makes the loop stuck because RTC_UIP is always set.

Yet another proof that VIRT creates more problems than it solves.

Fix below.

Thanks,

        tglx
---
Subject: rtc: mc146818: Detect and handle broken RTCs
From: Thomas Gleixner <redacted>
Date: Tue, 26 Jan 2021 11:38:40 +0100

The recent fix for handling the UIP bit unearthed another issue in the RTC
code. If the RTC is advertised but the readout is straight 0xFF because
it's not available, the old code just proceeded with crappy values, but the
new code hangs because it waits for the UIP bit to become low.

Add a sanity check in the RTC CMOS probe function which reads the RTC_VALID
register (Register D) which should have bit 0-6 cleared. If that's not the
case then fail to register the CMOS.

Add the same check to mc146818_get_time(), warn once when the condition
is true and invalidate the rtc_time data.

Reported-by: Mickaël Salaün <mic@digikod.net>
Signed-off-by: Thomas Gleixner <redacted>
---
 drivers/rtc/rtc-cmos.c         |    8 ++++++++
 drivers/rtc/rtc-mc146818-lib.c |    7 +++++++
 2 files changed, 15 insertions(+)
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -805,6 +805,14 @@ cmos_do_probe(struct device *dev, struct
 
 	spin_lock_irq(&rtc_lock);
 
+	/* Ensure that the RTC is accessible. Bit 0-6 must be 0! */
+	if ((CMOS_READ(RTC_VALID) & 0x7f) != 0) {
+		spin_unlock_irq(&rtc_lock);
+		dev_warn(dev, "not accessible\n");
+		retval = -ENXIO;
+		goto cleanup1;
+	}
+
 	if (!(flags & CMOS_RTC_FLAGS_NOFREQ)) {
 		/* force periodic irq to CMOS reset default of 1024Hz;
 		 *
--- a/drivers/rtc/rtc-mc146818-lib.c
+++ b/drivers/rtc/rtc-mc146818-lib.c
@@ -21,6 +21,13 @@ unsigned int mc146818_get_time(struct rt
 
 again:
 	spin_lock_irqsave(&rtc_lock, flags);
+	/* Ensure that the RTC is accessible. Bit 0-6 must be 0! */
+	if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x7f) != 0)) {
+		spin_unlock_irqrestore(&rtc_lock, flags);
+		memset(time, 0xff, sizeof(time));
This should be: sizeof(*time)
+		return 0;
+	}
+
 	/*
 	 * Check whether there is an update in progress during which the
 	 * readout is unspecified. The maximum update time is ~2ms. Poll
Tested-by: Mickaël Salaün <redacted>
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help