* [PATCH v3 1/7] rtc-cmos: take rtc_lock while reading from CMOS
2021-10-30 21:46 [PATCH v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes Mateusz Jończyk
@ 2021-10-30 21:46 ` Mateusz Jończyk
2021-10-30 21:46 ` [PATCH v3 2/7] rtc-mc146818-lib: fix RTC presence check Mateusz Jończyk
` (6 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Mateusz Jończyk @ 2021-10-30 21:46 UTC (permalink / raw)
To: linux-rtc, linux-kernel
Cc: Mateusz Jończyk, Nobuhiro Iwamatsu, Alessandro Zummo,
Alexandre Belloni, stable
Reading from the CMOS involves writing to the index register and then
reading from the data register. Therefore access to the CMOS has to be
serialized with rtc_lock. This invocation of CMOS_READ was not
serialized, which could cause trouble when other code is accessing CMOS
at the same time.
Use spin_lock_irq() like the rest of the function.
Nothing in kernel modifies the RTC_DM_BINARY bit, so there could be a
separate pair of spin_lock_irq() / spin_unlock_irq() before doing the
math.
Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Reviewed-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: stable@vger.kernel.org
---
drivers/rtc/rtc-cmos.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 4eb53412b808..dc3f8b0dde98 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -457,7 +457,10 @@ static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
min = t->time.tm_min;
sec = t->time.tm_sec;
+ spin_lock_irq(&rtc_lock);
rtc_control = CMOS_READ(RTC_CONTROL);
+ spin_unlock_irq(&rtc_lock);
+
if (!(rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
/* Writing 0xff means "don't care" or "match all". */
mon = (mon <= 12) ? bin2bcd(mon) : 0xff;
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 2/7] rtc-mc146818-lib: fix RTC presence check
2021-10-30 21:46 [PATCH v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes Mateusz Jończyk
2021-10-30 21:46 ` [PATCH v3 1/7] rtc-cmos: take rtc_lock while reading from CMOS Mateusz Jończyk
@ 2021-10-30 21:46 ` Mateusz Jończyk
2021-10-30 21:46 ` [PATCH v3 3/7] rtc-mc146818-lib: extract mc146818_do_avoiding_UIP Mateusz Jończyk
` (5 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Mateusz Jończyk @ 2021-10-30 21:46 UTC (permalink / raw)
To: linux-rtc, linux-kernel
Cc: Mateusz Jończyk, Thomas Gleixner, Alessandro Zummo,
Alexandre Belloni
To prevent an infinite loop in mc146818_get_time(),
commit 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs")
added a check for RTC availability. Together with a later fix, it
checked if bit 6 in register 0x0d is cleared. This, however, caused a
false negative on a motherboard with an AMD SB710 southbridge; according
to the specification [1], bit 6 of register 0x0d of this chipset is a
scratchbit.
This caused a regression in Linux 5.11 - the RTC was determined broken
by the kernel and not used by rtc-cmos.c [3].
As a better alternative, check whether the UIP ("Update-in-progress")
bit is set for longer then 10ms. If that is the case, then apparently
the RTC is either absent (and all register reads return 0xff) or broken.
Also limit the number of loop iterations in mc146818_get_time() to 10 to
prevent an infinite loop there.
In a previous approach to this problem, I implemented a check whether
the RTC_HOURS register contains a value <= 24. This, however, sometimes
did not work correctly on my Intel Kaby Lake laptop. According to
Intel's documentation [2], "the time and date RAM locations (0-9) are
disconnected from the external bus" during the update cycle so reading
this register without checking the UIP bit is incorrect.
[1] AMD SB700/710/750 Register Reference Guide, page 308,
https://developer.amd.com/wordpress/media/2012/10/43009_sb7xx_rrg_pub_1.00.pdf
[2] 7th Generation Intel ® Processor Family I/O for U/Y Platforms [...] Datasheet
Volume 1 of 2, page 209
Intel's Document Number: 334658-006,
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
[3] Functions in arch/x86/kernel/rtc.c apparently were using it.
Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs")
Fixes: ebb22a059436 ("rtc: mc146818: Dont test for bit 0-5 in Register D")
Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
v2: Tweak commit description, remove "Cc: stable" (I'll send it manually
after more regression testing).
v3: - add "EXPORT_SYMBOL_GPL(mc146818_does_rtc_work)",
- change return type from mc146818_does_rtc_work to bool
---
drivers/rtc/rtc-cmos.c | 10 ++++------
drivers/rtc/rtc-mc146818-lib.c | 35 ++++++++++++++++++++++++++++++----
include/linux/mc146818rtc.h | 1 +
3 files changed, 36 insertions(+), 10 deletions(-)
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index dc3f8b0dde98..9404f58ee01d 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -793,16 +793,14 @@ cmos_do_probe(struct device *dev, struct resource *ports, int rtc_irq)
rename_region(ports, dev_name(&cmos_rtc.rtc->dev));
- spin_lock_irq(&rtc_lock);
-
- /* Ensure that the RTC is accessible. Bit 6 must be 0! */
- if ((CMOS_READ(RTC_VALID) & 0x40) != 0) {
- spin_unlock_irq(&rtc_lock);
- dev_warn(dev, "not accessible\n");
+ if (!mc146818_does_rtc_work()) {
+ dev_warn(dev, "broken or not accessible\n");
retval = -ENXIO;
goto cleanup1;
}
+ spin_lock_irq(&rtc_lock);
+
if (!(flags & CMOS_RTC_FLAGS_NOFREQ)) {
/* force periodic irq to CMOS reset default of 1024Hz;
*
diff --git a/drivers/rtc/rtc-mc146818-lib.c b/drivers/rtc/rtc-mc146818-lib.c
index dcfaf09946ee..b50612ce1a6d 100644
--- a/drivers/rtc/rtc-mc146818-lib.c
+++ b/drivers/rtc/rtc-mc146818-lib.c
@@ -8,10 +8,36 @@
#include <linux/acpi.h>
#endif
+/*
+ * If the UIP (Update-in-progress) bit of the RTC is set for more then
+ * 10ms, the RTC is apparently broken or not present.
+ */
+bool mc146818_does_rtc_work(void)
+{
+ int i;
+ unsigned char val;
+ unsigned long flags;
+
+ for (i = 0; i < 10; i++) {
+ spin_lock_irqsave(&rtc_lock, flags);
+ val = CMOS_READ(RTC_FREQ_SELECT);
+ spin_unlock_irqrestore(&rtc_lock, flags);
+
+ if ((val & RTC_UIP) == 0)
+ return true;
+
+ mdelay(1);
+ }
+
+ return false;
+}
+EXPORT_SYMBOL_GPL(mc146818_does_rtc_work);
+
unsigned int mc146818_get_time(struct rtc_time *time)
{
unsigned char ctrl;
unsigned long flags;
+ unsigned int iter_count = 0;
unsigned char century = 0;
bool retry;
@@ -20,13 +46,14 @@ unsigned int mc146818_get_time(struct rtc_time *time)
#endif
again:
- spin_lock_irqsave(&rtc_lock, flags);
- /* Ensure that the RTC is accessible. Bit 6 must be 0! */
- if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x40) != 0)) {
- spin_unlock_irqrestore(&rtc_lock, flags);
+ if (iter_count > 10) {
+ pr_err_ratelimited("Unable to read current time from RTC\n");
memset(time, 0xff, sizeof(*time));
return 0;
}
+ iter_count++;
+
+ spin_lock_irqsave(&rtc_lock, flags);
/*
* Check whether there is an update in progress during which the
diff --git a/include/linux/mc146818rtc.h b/include/linux/mc146818rtc.h
index 0661af17a758..69c80c4325bf 100644
--- a/include/linux/mc146818rtc.h
+++ b/include/linux/mc146818rtc.h
@@ -123,6 +123,7 @@ struct cmos_rtc_board_info {
#define RTC_IO_EXTENT_USED RTC_IO_EXTENT
#endif /* ARCH_RTC_LOCATION */
+bool mc146818_does_rtc_work(void);
unsigned int mc146818_get_time(struct rtc_time *time);
int mc146818_set_time(struct rtc_time *time);
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 3/7] rtc-mc146818-lib: extract mc146818_do_avoiding_UIP
2021-10-30 21:46 [PATCH v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes Mateusz Jończyk
2021-10-30 21:46 ` [PATCH v3 1/7] rtc-cmos: take rtc_lock while reading from CMOS Mateusz Jończyk
2021-10-30 21:46 ` [PATCH v3 2/7] rtc-mc146818-lib: fix RTC presence check Mateusz Jończyk
@ 2021-10-30 21:46 ` Mateusz Jończyk
2021-10-30 21:46 ` [PATCH v3 4/7] rtc-mc146818-lib: refactor mc146818_get_time Mateusz Jończyk
` (4 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Mateusz Jończyk @ 2021-10-30 21:46 UTC (permalink / raw)
To: linux-rtc, linux-kernel
Cc: Mateusz Jończyk, Alessandro Zummo, Alexandre Belloni
Function mc146818_get_time() contains an elaborate mechanism of reading
the RTC time while no RTC update is in progress. It turns out that
reading the RTC alarm clock also requires avoiding the RTC update (see
following patches). Therefore, the mechanism in mc146818_get_time()
should be reused - so extract it into a separate function.
The logic in mc146818_do_avoiding_UIP() is same as in
mc146818_get_time() except that after every
if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) {
there is now "mdelay(1)".
To avoid producing an unreadable diff, mc146818_get_time() will be
refactored to use mc146818_do_avoiding_UIP() in the next patch.
Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
drivers/rtc/rtc-mc146818-lib.c | 69 ++++++++++++++++++++++++++++++++++
include/linux/mc146818rtc.h | 3 ++
2 files changed, 72 insertions(+)
diff --git a/drivers/rtc/rtc-mc146818-lib.c b/drivers/rtc/rtc-mc146818-lib.c
index b50612ce1a6d..946ad43a512c 100644
--- a/drivers/rtc/rtc-mc146818-lib.c
+++ b/drivers/rtc/rtc-mc146818-lib.c
@@ -8,6 +8,75 @@
#include <linux/acpi.h>
#endif
+/*
+ * Execute a function while the UIP (Update-in-progress) bit of the RTC is
+ * unset.
+ *
+ * Warning: callback may be executed more then once.
+ */
+bool mc146818_do_avoiding_UIP(mc146818_callback_t callback, void *param)
+{
+ int i;
+ unsigned long flags;
+ unsigned char seconds;
+
+ for (i = 0; i < 10; i++) {
+ spin_lock_irqsave(&rtc_lock, flags);
+
+ /*
+ * Check whether there is an update in progress during which the
+ * readout is unspecified. The maximum update time is ~2ms. Poll
+ * every msec for completion.
+ *
+ * Store the second value before checking UIP so a long lasting
+ * NMI which happens to hit after the UIP check cannot make
+ * an update cycle invisible.
+ */
+ seconds = CMOS_READ(RTC_SECONDS);
+
+ if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) {
+ spin_unlock_irqrestore(&rtc_lock, flags);
+ mdelay(1);
+ continue;
+ }
+
+ /* Revalidate the above readout */
+ if (seconds != CMOS_READ(RTC_SECONDS)) {
+ spin_unlock_irqrestore(&rtc_lock, flags);
+ continue;
+ }
+
+ if (callback)
+ callback(seconds, param);
+
+ /*
+ * Check for the UIP bit again. If it is set now then
+ * the above values may contain garbage.
+ */
+ if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) {
+ spin_unlock_irqrestore(&rtc_lock, flags);
+ mdelay(1);
+ continue;
+ }
+
+ /*
+ * A NMI might have interrupted the above sequence so check
+ * whether the seconds value has changed which indicates that
+ * the NMI took longer than the UIP bit was set. Unlikely, but
+ * possible and there is also virt...
+ */
+ if (seconds != CMOS_READ(RTC_SECONDS)) {
+ spin_unlock_irqrestore(&rtc_lock, flags);
+ continue;
+ }
+ spin_unlock_irqrestore(&rtc_lock, flags);
+
+ return true;
+ }
+ return false;
+}
+EXPORT_SYMBOL_GPL(mc146818_do_avoiding_UIP);
+
/*
* If the UIP (Update-in-progress) bit of the RTC is set for more then
* 10ms, the RTC is apparently broken or not present.
diff --git a/include/linux/mc146818rtc.h b/include/linux/mc146818rtc.h
index 69c80c4325bf..c0cea97461a0 100644
--- a/include/linux/mc146818rtc.h
+++ b/include/linux/mc146818rtc.h
@@ -127,4 +127,7 @@ bool mc146818_does_rtc_work(void);
unsigned int mc146818_get_time(struct rtc_time *time);
int mc146818_set_time(struct rtc_time *time);
+typedef void (*mc146818_callback_t)(unsigned char seconds, void *param);
+bool mc146818_do_avoiding_UIP(mc146818_callback_t callback, void *param);
+
#endif /* _MC146818RTC_H */
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 4/7] rtc-mc146818-lib: refactor mc146818_get_time
2021-10-30 21:46 [PATCH v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes Mateusz Jończyk
` (2 preceding siblings ...)
2021-10-30 21:46 ` [PATCH v3 3/7] rtc-mc146818-lib: extract mc146818_do_avoiding_UIP Mateusz Jończyk
@ 2021-10-30 21:46 ` Mateusz Jończyk
2021-10-30 21:46 ` [PATCH v3 5/7] rtc-mc146818-lib: refactor mc146818_does_rtc_work Mateusz Jończyk
` (3 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Mateusz Jończyk @ 2021-10-30 21:46 UTC (permalink / raw)
To: linux-rtc, linux-kernel
Cc: Mateusz Jończyk, Alessandro Zummo, Alexandre Belloni
Refactor mc146818_get_time() to make use of mc146818_do_avoiding_UIP().
Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
drivers/rtc/rtc-mc146818-lib.c | 111 +++++++++++++--------------------
1 file changed, 43 insertions(+), 68 deletions(-)
I'm sorry that the diff is quite difficult to read, but I was unable to
fix this easily.
diff --git a/drivers/rtc/rtc-mc146818-lib.c b/drivers/rtc/rtc-mc146818-lib.c
index 946ad43a512c..f3178244db37 100644
--- a/drivers/rtc/rtc-mc146818-lib.c
+++ b/drivers/rtc/rtc-mc146818-lib.c
@@ -102,50 +102,20 @@ bool mc146818_does_rtc_work(void)
}
EXPORT_SYMBOL_GPL(mc146818_does_rtc_work);
-unsigned int mc146818_get_time(struct rtc_time *time)
-{
+struct mc146818_get_time_callback_param {
+ struct rtc_time *time;
unsigned char ctrl;
- unsigned long flags;
- unsigned int iter_count = 0;
- unsigned char century = 0;
- bool retry;
-
+#ifdef CONFIG_ACPI
+ unsigned char century;
+#endif
#ifdef CONFIG_MACH_DECSTATION
unsigned int real_year;
#endif
+};
-again:
- if (iter_count > 10) {
- pr_err_ratelimited("Unable to read current time from RTC\n");
- memset(time, 0xff, sizeof(*time));
- return 0;
- }
- iter_count++;
-
- spin_lock_irqsave(&rtc_lock, flags);
-
- /*
- * Check whether there is an update in progress during which the
- * readout is unspecified. The maximum update time is ~2ms. Poll
- * every msec for completion.
- *
- * Store the second value before checking UIP so a long lasting NMI
- * which happens to hit after the UIP check cannot make an update
- * cycle invisible.
- */
- time->tm_sec = CMOS_READ(RTC_SECONDS);
-
- if (CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP) {
- spin_unlock_irqrestore(&rtc_lock, flags);
- mdelay(1);
- goto again;
- }
-
- /* Revalidate the above readout */
- if (time->tm_sec != CMOS_READ(RTC_SECONDS)) {
- spin_unlock_irqrestore(&rtc_lock, flags);
- goto again;
- }
+static void mc146818_get_time_callback(unsigned char seconds, void *param_in)
+{
+ struct mc146818_get_time_callback_param *p = param_in;
/*
* Only the values that we read from the RTC are set. We leave
@@ -153,39 +123,40 @@ unsigned int mc146818_get_time(struct rtc_time *time)
* RTC has RTC_DAY_OF_WEEK, we ignore it, as it is only updated
* by the RTC when initially set to a non-zero value.
*/
- time->tm_min = CMOS_READ(RTC_MINUTES);
- time->tm_hour = CMOS_READ(RTC_HOURS);
- time->tm_mday = CMOS_READ(RTC_DAY_OF_MONTH);
- time->tm_mon = CMOS_READ(RTC_MONTH);
- time->tm_year = CMOS_READ(RTC_YEAR);
+ p->time->tm_sec = seconds;
+ p->time->tm_min = CMOS_READ(RTC_MINUTES);
+ p->time->tm_hour = CMOS_READ(RTC_HOURS);
+ p->time->tm_mday = CMOS_READ(RTC_DAY_OF_MONTH);
+ p->time->tm_mon = CMOS_READ(RTC_MONTH);
+ p->time->tm_year = CMOS_READ(RTC_YEAR);
#ifdef CONFIG_MACH_DECSTATION
- real_year = CMOS_READ(RTC_DEC_YEAR);
+ p->real_year = CMOS_READ(RTC_DEC_YEAR);
#endif
#ifdef CONFIG_ACPI
if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
- acpi_gbl_FADT.century)
- century = CMOS_READ(acpi_gbl_FADT.century);
+ acpi_gbl_FADT.century) {
+ p->century = CMOS_READ(acpi_gbl_FADT.century);
+ } else {
+ p->century = 0;
+ }
#endif
- ctrl = CMOS_READ(RTC_CONTROL);
- /*
- * Check for the UIP bit again. If it is set now then
- * the above values may contain garbage.
- */
- retry = CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP;
- /*
- * A NMI might have interrupted the above sequence so check whether
- * the seconds value has changed which indicates that the NMI took
- * longer than the UIP bit was set. Unlikely, but possible and
- * there is also virt...
- */
- retry |= time->tm_sec != CMOS_READ(RTC_SECONDS);
- spin_unlock_irqrestore(&rtc_lock, flags);
+ p->ctrl = CMOS_READ(RTC_CONTROL);
+}
- if (retry)
- goto again;
+unsigned int mc146818_get_time(struct rtc_time *time)
+{
+ struct mc146818_get_time_callback_param p = {
+ .time = time
+ };
- if (!(ctrl & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
+ if (!mc146818_do_avoiding_UIP(mc146818_get_time_callback, &p)) {
+ pr_err_ratelimited("Unable to read current time from RTC\n");
+ memset(time, 0xff, sizeof(*time));
+ return 0;
+ }
+
+ if (!(p.ctrl & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
{
time->tm_sec = bcd2bin(time->tm_sec);
time->tm_min = bcd2bin(time->tm_min);
@@ -193,15 +164,19 @@ unsigned int mc146818_get_time(struct rtc_time *time)
time->tm_mday = bcd2bin(time->tm_mday);
time->tm_mon = bcd2bin(time->tm_mon);
time->tm_year = bcd2bin(time->tm_year);
- century = bcd2bin(century);
+#ifdef CONFIG_ACPI
+ p.century = bcd2bin(p.century);
+#endif
}
#ifdef CONFIG_MACH_DECSTATION
- time->tm_year += real_year - 72;
+ time->tm_year += p.real_year - 72;
#endif
- if (century > 20)
- time->tm_year += (century - 19) * 100;
+#ifdef CONFIG_ACPI
+ if (p.century > 20)
+ time->tm_year += (p.century - 19) * 100;
+#endif
/*
* Account for differences between how the RTC uses the values
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 5/7] rtc-mc146818-lib: refactor mc146818_does_rtc_work
2021-10-30 21:46 [PATCH v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes Mateusz Jończyk
` (3 preceding siblings ...)
2021-10-30 21:46 ` [PATCH v3 4/7] rtc-mc146818-lib: refactor mc146818_get_time Mateusz Jończyk
@ 2021-10-30 21:46 ` Mateusz Jończyk
2021-10-30 21:46 ` [PATCH v3 6/7] rtc-cmos: avoid UIP when reading alarm time Mateusz Jończyk
` (2 subsequent siblings)
7 siblings, 0 replies; 10+ messages in thread
From: Mateusz Jończyk @ 2021-10-30 21:46 UTC (permalink / raw)
To: linux-rtc, linux-kernel
Cc: Mateusz Jończyk, Alessandro Zummo, Alexandre Belloni
Refactor mc146818_get_time() to make use of mc146818_do_avoiding_UIP().
It is enough to call mc146818_do_avoiding_UIP() with no callback.
Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
drivers/rtc/rtc-mc146818-lib.c | 17 +----------------
1 file changed, 1 insertion(+), 16 deletions(-)
diff --git a/drivers/rtc/rtc-mc146818-lib.c b/drivers/rtc/rtc-mc146818-lib.c
index f3178244db37..09d6c20726c1 100644
--- a/drivers/rtc/rtc-mc146818-lib.c
+++ b/drivers/rtc/rtc-mc146818-lib.c
@@ -83,22 +83,7 @@ EXPORT_SYMBOL_GPL(mc146818_do_avoiding_UIP);
*/
bool mc146818_does_rtc_work(void)
{
- int i;
- unsigned char val;
- unsigned long flags;
-
- for (i = 0; i < 10; i++) {
- spin_lock_irqsave(&rtc_lock, flags);
- val = CMOS_READ(RTC_FREQ_SELECT);
- spin_unlock_irqrestore(&rtc_lock, flags);
-
- if ((val & RTC_UIP) == 0)
- return true;
-
- mdelay(1);
- }
-
- return false;
+ return mc146818_do_avoiding_UIP(NULL, NULL);
}
EXPORT_SYMBOL_GPL(mc146818_does_rtc_work);
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 6/7] rtc-cmos: avoid UIP when reading alarm time
2021-10-30 21:46 [PATCH v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes Mateusz Jończyk
` (4 preceding siblings ...)
2021-10-30 21:46 ` [PATCH v3 5/7] rtc-mc146818-lib: refactor mc146818_does_rtc_work Mateusz Jończyk
@ 2021-10-30 21:46 ` Mateusz Jończyk
2021-10-30 21:46 ` [PATCH v3 7/7] rtc-cmos: avoid UIP when writing " Mateusz Jończyk
2021-10-30 21:46 ` [DEBUG PATCH v3] rtc-cmos: cmos_read_alarm bug demonstration Mateusz Jończyk
7 siblings, 0 replies; 10+ messages in thread
From: Mateusz Jończyk @ 2021-10-30 21:46 UTC (permalink / raw)
To: linux-rtc, linux-kernel
Cc: Mateusz Jończyk, Alessandro Zummo, Alexandre Belloni
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() 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.
[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."
Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
drivers/rtc/rtc-cmos.c | 72 ++++++++++++++++++++++++++++--------------
1 file changed, 49 insertions(+), 23 deletions(-)
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 9404f58ee01d..2def331e88b6 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -242,10 +242,46 @@ static int cmos_set_time(struct device *dev, struct rtc_time *t)
return mc146818_set_time(t);
}
+struct cmos_read_alarm_callback_param {
+ struct cmos_rtc *cmos;
+ struct rtc_time *time;
+ unsigned char rtc_control;
+};
+
+static void cmos_read_alarm_callback(unsigned char __always_unused seconds,
+ void *param_in)
+{
+ struct cmos_read_alarm_callback_param *p =
+ (struct cmos_read_alarm_callback_param *) param_in;
+ struct rtc_time *time = p->time;
+
+ time->tm_sec = CMOS_READ(RTC_SECONDS_ALARM);
+ time->tm_min = CMOS_READ(RTC_MINUTES_ALARM);
+ time->tm_hour = CMOS_READ(RTC_HOURS_ALARM);
+
+ if (p->cmos->day_alrm) {
+ /* ignore upper bits on readback per ACPI spec */
+ time->tm_mday = CMOS_READ(p->cmos->day_alrm) & 0x3f;
+ if (!time->tm_mday)
+ time->tm_mday = -1;
+
+ if (p->cmos->mon_alrm) {
+ time->tm_mon = CMOS_READ(p->cmos->mon_alrm);
+ if (!time->tm_mon)
+ time->tm_mon = -1;
+ }
+ }
+
+ p->rtc_control = CMOS_READ(RTC_CONTROL);
+}
+
static int cmos_read_alarm(struct device *dev, struct rtc_wkalrm *t)
{
struct cmos_rtc *cmos = dev_get_drvdata(dev);
- unsigned char rtc_control;
+ struct cmos_read_alarm_callback_param p = {
+ .cmos = cmos,
+ .time = &(t->time)
+ };
/* This not only a rtc_op, but also called directly */
if (!is_valid_irq(cmos->irq))
@@ -256,28 +292,18 @@ static int cmos_read_alarm(struct device *dev, struct rtc_wkalrm *t)
* 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);
-
- if (cmos->day_alrm) {
- /* ignore upper bits on readback per ACPI spec */
- t->time.tm_mday = CMOS_READ(cmos->day_alrm) & 0x3f;
- if (!t->time.tm_mday)
- t->time.tm_mday = -1;
-
- if (cmos->mon_alrm) {
- t->time.tm_mon = CMOS_READ(cmos->mon_alrm);
- if (!t->time.tm_mon)
- t->time.tm_mon = -1;
- }
- }
-
- rtc_control = CMOS_READ(RTC_CONTROL);
- spin_unlock_irq(&rtc_lock);
+ /* 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
+ *
+ * Use the mc146818_do_avoiding_UIP() function to avoid this.
+ */
+ if (!mc146818_do_avoiding_UIP(cmos_read_alarm_callback, &p))
+ return -EIO;
- if (!(rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
+ if (!(p.rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
if (((unsigned)t->time.tm_sec) < 0x60)
t->time.tm_sec = bcd2bin(t->time.tm_sec);
else
@@ -306,7 +332,7 @@ static int cmos_read_alarm(struct device *dev, struct rtc_wkalrm *t)
}
}
- t->enabled = !!(rtc_control & RTC_AIE);
+ t->enabled = !!(p.rtc_control & RTC_AIE);
t->pending = 0;
return 0;
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 7/7] rtc-cmos: avoid UIP when writing alarm time
2021-10-30 21:46 [PATCH v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes Mateusz Jończyk
` (5 preceding siblings ...)
2021-10-30 21:46 ` [PATCH v3 6/7] rtc-cmos: avoid UIP when reading alarm time Mateusz Jończyk
@ 2021-10-30 21:46 ` Mateusz Jończyk
2021-10-30 21:46 ` [DEBUG PATCH v3] rtc-cmos: cmos_read_alarm bug demonstration Mateusz Jończyk
7 siblings, 0 replies; 10+ messages in thread
From: Mateusz Jończyk @ 2021-10-30 21:46 UTC (permalink / raw)
To: linux-rtc, linux-kernel
Cc: Mateusz Jończyk, Alessandro Zummo, Alexandre Belloni
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_set_alarm() did not take account for that, fix it.
[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."
Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
drivers/rtc/rtc-cmos.c | 108 +++++++++++++++++++++++++----------------
1 file changed, 67 insertions(+), 41 deletions(-)
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index 2def331e88b6..b6d7dd3cf066 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -463,10 +463,58 @@ static int cmos_validate_alarm(struct device *dev, struct rtc_wkalrm *t)
return 0;
}
+struct cmos_set_alarm_callback_param {
+ struct cmos_rtc *cmos;
+ unsigned char mon, mday, hrs, min, sec;
+ struct rtc_wkalrm *t;
+};
+
+/* Note: this function may be executed by mc146818_do_avoiding_UIP() more then
+ * once
+ */
+static void cmos_set_alarm_callback(unsigned char __always_unused seconds,
+ void *param_in)
+{
+ struct cmos_set_alarm_callback_param *p =
+ (struct cmos_set_alarm_callback_param *) param_in;
+
+ /* next rtc irq must not be from previous alarm setting */
+ cmos_irq_disable(p->cmos, RTC_AIE);
+
+ /* update alarm */
+ CMOS_WRITE(p->hrs, RTC_HOURS_ALARM);
+ CMOS_WRITE(p->min, RTC_MINUTES_ALARM);
+ CMOS_WRITE(p->sec, RTC_SECONDS_ALARM);
+
+ /* the system may support an "enhanced" alarm */
+ if (p->cmos->day_alrm) {
+ CMOS_WRITE(p->mday, p->cmos->day_alrm);
+ if (p->cmos->mon_alrm)
+ CMOS_WRITE(p->mon, p->cmos->mon_alrm);
+ }
+
+ if (use_hpet_alarm()) {
+ /*
+ * FIXME the HPET alarm glue currently ignores day_alrm
+ * and mon_alrm ...
+ */
+ hpet_set_alarm_time(p->t->time.tm_hour, p->t->time.tm_min,
+ p->t->time.tm_sec);
+ }
+
+ if (p->t->enabled)
+ cmos_irq_enable(p->cmos, RTC_AIE);
+
+}
+
static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
{
struct cmos_rtc *cmos = dev_get_drvdata(dev);
- unsigned char mon, mday, hrs, min, sec, rtc_control;
+ struct cmos_set_alarm_callback_param p = {
+ .cmos = cmos,
+ .t = t
+ };
+ unsigned char rtc_control;
int ret;
/* This not only a rtc_op, but also called directly */
@@ -477,11 +525,11 @@ static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
if (ret < 0)
return ret;
- mon = t->time.tm_mon + 1;
- mday = t->time.tm_mday;
- hrs = t->time.tm_hour;
- min = t->time.tm_min;
- sec = t->time.tm_sec;
+ p.mon = t->time.tm_mon + 1;
+ p.mday = t->time.tm_mday;
+ p.hrs = t->time.tm_hour;
+ p.min = t->time.tm_min;
+ p.sec = t->time.tm_sec;
spin_lock_irq(&rtc_lock);
rtc_control = CMOS_READ(RTC_CONTROL);
@@ -489,43 +537,21 @@ static int cmos_set_alarm(struct device *dev, struct rtc_wkalrm *t)
if (!(rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
/* Writing 0xff means "don't care" or "match all". */
- mon = (mon <= 12) ? bin2bcd(mon) : 0xff;
- mday = (mday >= 1 && mday <= 31) ? bin2bcd(mday) : 0xff;
- hrs = (hrs < 24) ? bin2bcd(hrs) : 0xff;
- min = (min < 60) ? bin2bcd(min) : 0xff;
- sec = (sec < 60) ? bin2bcd(sec) : 0xff;
+ p.mon = (p.mon <= 12) ? bin2bcd(p.mon) : 0xff;
+ p.mday = (p.mday >= 1 && p.mday <= 31) ? bin2bcd(p.mday) : 0xff;
+ p.hrs = (p.hrs < 24) ? bin2bcd(p.hrs) : 0xff;
+ p.min = (p.min < 60) ? bin2bcd(p.min) : 0xff;
+ p.sec = (p.sec < 60) ? bin2bcd(p.sec) : 0xff;
}
- spin_lock_irq(&rtc_lock);
-
- /* next rtc irq must not be from previous alarm setting */
- cmos_irq_disable(cmos, RTC_AIE);
-
- /* update alarm */
- CMOS_WRITE(hrs, RTC_HOURS_ALARM);
- CMOS_WRITE(min, RTC_MINUTES_ALARM);
- CMOS_WRITE(sec, RTC_SECONDS_ALARM);
-
- /* the system may support an "enhanced" alarm */
- if (cmos->day_alrm) {
- CMOS_WRITE(mday, cmos->day_alrm);
- if (cmos->mon_alrm)
- CMOS_WRITE(mon, cmos->mon_alrm);
- }
-
- if (use_hpet_alarm()) {
- /*
- * FIXME the HPET alarm glue currently ignores day_alrm
- * and mon_alrm ...
- */
- hpet_set_alarm_time(t->time.tm_hour, t->time.tm_min,
- t->time.tm_sec);
- }
-
- if (t->enabled)
- cmos_irq_enable(cmos, RTC_AIE);
-
- spin_unlock_irq(&rtc_lock);
+ /*
+ * Some Intel chipsets disconnect the alarm registers when the clock
+ * update is in progress - during this time writes fail silently.
+ *
+ * Use mc146818_do_avoiding_UIP() to avoid this.
+ */
+ if (!mc146818_do_avoiding_UIP(cmos_set_alarm_callback, &p))
+ return -EIO;
cmos->alarm_expires = rtc_tm_to_time64(&t->time);
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [DEBUG PATCH v3] rtc-cmos: cmos_read_alarm bug demonstration
2021-10-30 21:46 [PATCH v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes Mateusz Jończyk
` (6 preceding siblings ...)
2021-10-30 21:46 ` [PATCH v3 7/7] rtc-cmos: avoid UIP when writing " Mateusz Jończyk
@ 2021-10-30 21:46 ` Mateusz Jończyk
7 siblings, 0 replies; 10+ messages in thread
From: Mateusz Jończyk @ 2021-10-30 21:46 UTC (permalink / raw)
To: linux-rtc, linux-kernel
Cc: Mateusz Jończyk, Alessandro Zummo, Alexandre Belloni
Before my commit "rtc-cmos: avoid UIP when reading alarm time",
applying this patch and reading the CMOS time like so:
while true; do cat /sys/class/rtc/rtc0/time ; sleep 0.5; done
produces errors in dmesg on my Intel Kaby Lake laptop.
Signed-off-by: Mateusz Jończyk <mat.jonczyk@o2.pl>
Cc: Alessandro Zummo <a.zummo@towertech.it>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
drivers/rtc/rtc-cmos.c | 61 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index b6d7dd3cf066..3de049930745 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -43,6 +43,9 @@
#include <linux/dmi.h>
#endif
+#include <linux/ktime.h>
+#include <linux/timekeeping.h>
+
/* this is for "generic access to PC-style RTC" using CMOS_READ/CMOS_WRITE */
#include <linux/mc146818rtc.h>
@@ -220,6 +223,8 @@ static inline void cmos_write_bank2(unsigned char val, unsigned char addr)
/*----------------------------------------------------------------*/
+static void cmos_read_alarm_uip_debugging(struct device *dev);
+
static int cmos_read_time(struct device *dev, struct rtc_time *t)
{
/*
@@ -230,6 +235,8 @@ static int cmos_read_time(struct device *dev, struct rtc_time *t)
return -EIO;
mc146818_get_time(t);
+
+ cmos_read_alarm_uip_debugging(dev);
return 0;
}
@@ -338,6 +345,60 @@ static int cmos_read_alarm(struct device *dev, struct rtc_wkalrm *t)
return 0;
}
+static void cmos_read_alarm_uip_debugging(struct device *dev)
+{
+ unsigned long flags;
+ unsigned char rtc_uip_baseline, rtc_uip;
+ struct rtc_wkalrm t_baseline, t;
+ ktime_t time_start, time_end;
+ int i;
+
+ spin_lock_irqsave(&rtc_lock, flags);
+ rtc_uip_baseline = CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP;
+ spin_unlock_irqrestore(&rtc_lock, flags);
+
+ cmos_read_alarm(dev, &t_baseline);
+
+ time_start = ktime_get();
+
+ for (i = 0; i < 2000; i++) {
+ spin_lock_irqsave(&rtc_lock, flags);
+ rtc_uip = CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP;
+ spin_unlock_irqrestore(&rtc_lock, flags);
+
+ cmos_read_alarm(dev, &t);
+
+ if (t_baseline.time.tm_sec != t.time.tm_sec) {
+ dev_err(dev,
+ "Inconsistent alarm tm_sec value: %d != %d (RTC_UIP = %d; %d)\n",
+ t_baseline.time.tm_sec,
+ t.time.tm_sec,
+ rtc_uip_baseline, rtc_uip);
+ }
+ if (t_baseline.time.tm_min != t.time.tm_min) {
+ dev_err(dev,
+ "Inconsistent alarm tm_min value: %d != %d (RTC_UIP = %d; %d)\n",
+ t_baseline.time.tm_min,
+ t.time.tm_min,
+ rtc_uip_baseline, rtc_uip);
+ }
+ if (t_baseline.time.tm_hour != t.time.tm_hour) {
+ dev_err(dev,
+ "Inconsistent alarm tm_hour value: %d != %d (RTC_UIP = %d; %d)\n",
+ t_baseline.time.tm_hour,
+ t.time.tm_hour,
+ rtc_uip_baseline, rtc_uip);
+ }
+
+ }
+
+ time_end = ktime_get();
+
+ pr_info_ratelimited("%s: loop executed in %lld ns\n",
+ __func__, ktime_to_ns(ktime_sub(time_end, time_start)));
+}
+
+
static void cmos_checkintr(struct cmos_rtc *cmos, unsigned char rtc_control)
{
unsigned char rtc_intr;
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread