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

[Thomas Gleixner]

On Mon, Jan 25 2021 at 19:40, Mickaël Salaün wrote:
> 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).
>> +	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 <tglx@xxxxxxxxxxxxx>
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@xxxxxxxxxxx>
Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
---
 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));
+		return 0;
+	}
+
 	/*
 	 * Check whether there is an update in progress during which the
 	 * readout is unspecified. The maximum update time is ~2ms. Poll