public inbox for linux-rtc@vger.kernel.org
 help / color / mirror / Atom feed
* rtc,x86: CMOS RTC fixes
@ 2021-09-12 12:42 Mateusz Jończyk
  2021-09-12 12:42 ` [PATCH RESEND 1/7] rtc-cmos: take rtc_lock while reading from CMOS Mateusz Jończyk
                   ` (6 more replies)
  0 siblings, 7 replies; 12+ messages in thread
From: Mateusz Jończyk @ 2021-09-12 12:42 UTC (permalink / raw)
  To: linux-rtc
  Cc: Mateusz Jończyk, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, x86, H. Peter Anvin, Alessandro Zummo,
	Alexandre Belloni

Hello,

This patch series fixes some issues in the RTC CMOS handling code.

First three patches fix bugs in drivers/rtc:
1. Fix rtc_lock usage in cmos_set_alarm() in rtc-cmos.c.
2. Do not touch RTC alarm registers when the RTC update is in progress. 
   (On some Intel chipsets, this causes bogus values being read or writes to
   fail silently.)
3. Fix presence check of the RTC CMOS: the clock was misdetected as broken on
   one of my systems.

Patches 1 and 2 are Cced stable. I'm going to  submit patch 3 manually
to stable after some regression testing in master.

Next three patches contain fixes for arch/x86/rtc.c: duplicate code removal
and a renaming of a function.

The final patch reverts a buggy commit - to prevent a possible deadlock at
resume from suspend.

I'm also considering whether function x86_wallclock_init() in
arch/x86/kernel/x86_init.c should call mc146818_does_rtc_work() to make sure
that the CMOS RTC is present and behaves itself. This might be useful.

Tested on top of v5.14.2, on two systems.

This is my first patch series, so please review carefully.

Greetings,
Mateusz

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: x86@kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>

^ permalink raw reply	[flat|nested] 12+ messages in thread
* [PATCH RESEND 2/6] rtc-cmos: dont touch alarm registers during update
@ 2021-10-17 19:39 Mateusz Jończyk
  2021-10-17 19:41 ` [TEST PATCH] rtc-cmos: cmos_read_alarm bug demonstration Mateusz Jończyk
  0 siblings, 1 reply; 12+ messages in thread
From: Mateusz Jończyk @ 2021-10-17 19:39 UTC (permalink / raw)
  To: linux-kernel, linux-rtc
  Cc: Mateusz Jończyk, Alessandro Zummo, Alexandre Belloni, stable

Some Intel chipsets disconnect the time and date RTC registers when the
clock update is in progress: during this time reads may return bogus
values and writes fail silently. This includes the RTC alarm registers.
[1]

cmos_read_alarm() and cmos_set_alarm() did not take account for that,
which caused alarm time reads to sometimes return bogus values. This can
be shown with a test patch that I am attaching to this patch series.
Setting the alarm clock also probably did fail sometimes.

To make this patch suitable for inclusion in stable kernels, I'm using a
simple method for avoiding the RTC update cycle. This method is used in
mach_set_rtc_mmss() in arch/x86/kernel/rtc.c. A more elaborate algorithm
- as in mc146818_get_time() in drivers/rtc/rtc-mc146818-lib.c - would be
too complcated for stable. [2]

cmos_wait_for_uip_clear() has the rtc_lock taken while waiting for the
UIP bit to become clear. This should be harmless as during the UIP the RTC
cannot be read from anyway. mach_get_cmos_time() in arch/x86/kernel/rtc.c
does things the same way.

[1] 7th Generation Intel ® Processor Family I/O for U/Y Platforms [...]
Datasheet, Volume 1 of 2 (Intel's Document Number: 334658-006)
Page 208
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/7th-and-8th-gen-core-family-mobile-u-y-processor-lines-i-o-datasheet-vol-1.pdf
        "If a RAM read from the ten time and date bytes is attempted
        during an update cycle, the value read do not necessarily
        represent the true contents of those locations. Any RAM writes
        under the same conditions are ignored.'

[2] I'm going to submit a unification patch for a later kernel release -
prefer to see this in stable.

Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: stable@vger.kernel.org
---
 drivers/rtc/rtc-cmos.c | 38 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 2cd0fe728ab2..643433d984ab 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -248,6 +248,31 @@ static int cmos_set_time(struct device *dev, struct rtc_time *t)
 	return mc146818_set_time(t);
 }
 
+/* Some Intel chipsets disconnect the alarm registers when the clock update is
+ * in progress - during this time reads return bogus values and writes may fail
+ * silently. See for example "7th Generation Intel® Processor Family I/O for
+ * U/Y Platforms [...] Datasheet", section 27.7.1
+ *
+ * Check the UIP bit to prevent this, waiting for max 10ms for it to become
+ * clear.
+ *
+ * This function has to be called with rtc_lock taken.
+ */
+static int cmos_wait_for_uip_clear(struct device *dev)
+{
+	int i;
+
+	for (i = 0; i < 100; i++) {
+		if ((CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) == 0)
+			return 0;
+		udelay(100);
+	}
+
+	dev_warn_ratelimited(dev, "UIP bit is stuck, cannot access RTC registers\n");
+
+	return 1;
+}
+
 static int cmos_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 {
 	struct cmos_rtc	*cmos = dev_get_drvdata(dev);
@@ -257,12 +282,17 @@ static int cmos_read_alarm(struct device *dev, struct rtc_wkalrm *t)
 	if (!is_valid_irq(cmos->irq))
 		return -EIO;
 
+	spin_lock_irq(&rtc_lock);
+
+	if (cmos_wait_for_uip_clear(dev)) {
+		spin_unlock_irq(&rtc_lock);
+		return -EIO;
+	}
+
 	/* Basic alarms only support hour, minute, and seconds fields.
 	 * Some also support day and month, for alarms up to a year in
 	 * the future.
 	 */
-
-	spin_lock_irq(&rtc_lock);
 	t->time.tm_sec = CMOS_READ(RTC_SECONDS_ALARM);
 	t->time.tm_min = CMOS_READ(RTC_MINUTES_ALARM);
 	t->time.tm_hour = CMOS_READ(RTC_HOURS_ALARM);
@@ -477,6 +507,10 @@ static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
 	}
 
 	spin_lock_irq(&rtc_lock);
+	if (cmos_wait_for_uip_clear(dev)) {
+		spin_unlock_irq(&rtc_lock);
+		return -EIO;
+	}
 
 	/* next rtc irq must not be from previous alarm setting */
 	cmos_irq_disable(cmos, RTC_AIE);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-10-17 19:42 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-12 12:42 rtc,x86: CMOS RTC fixes Mateusz Jończyk
2021-09-12 12:42 ` [PATCH RESEND 1/7] rtc-cmos: take rtc_lock while reading from CMOS Mateusz Jończyk
2021-09-12 12:42 ` [PATCH 2/7] rtc-cmos: dont touch alarm registers during update Mateusz Jończyk
2021-09-12 12:44   ` [TEST PATCH] rtc-cmos: cmos_read_alarm bug demonstration Mateusz Jończyk
2021-09-12 12:42 ` [PATCH v2 3/7] rtc-mc146818-lib: fix RTC presence check Mateusz Jończyk
2021-09-12 12:42 ` [PATCH 4/7] rtc-mc146818-lib: reduce RTC_UIP polling period Mateusz Jończyk
2021-09-12 12:42 ` [PATCH 5/7] x86/rtc: mach_get_cmos_time - rm duplicated code Mateusz Jończyk
2021-09-12 12:42 ` [PATCH 6/7] x86/rtc: rename mach_set_rtc_mmss Mateusz Jończyk
2021-09-12 12:42 ` [PATCH 7/7] Revert "rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard IRQ" Mateusz Jończyk
2021-09-14  8:02   ` Ville Syrjälä
2021-09-14  8:22     ` Alexandre Belloni
  -- strict thread matches above, loose matches on Subject: below --
2021-10-17 19:39 [PATCH RESEND 2/6] rtc-cmos: dont touch alarm registers during update Mateusz Jończyk
2021-10-17 19:41 ` [TEST PATCH] rtc-cmos: cmos_read_alarm bug demonstration Mateusz Jończyk

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox