Re: [PATCH V2] rtc: mc146818: Detect and handle broken RTCs

Hi!  "Me too":

... triggers here on a different box (Xiaomi mi notebook air 12.5):

[    3.524002] ------------[ cut here ]------------
[    3.528317] WARNING: CPU: 3 PID: 273 at drivers/rtc/rtc-mc146818-lib.c:25 mc146818_get_time+0x1b6/0x210
[    3.532558] CPU: 3 PID: 273 Comm: udevadm Not tainted 5.11.0-rc6 #760
[    3.536748] Hardware name: Timi TM1612/TM1612, BIOS A04 08/06/2016
[    3.540947] RIP: 0010:mc146818_get_time+0x1b6/0x210
[    3.545103] Code: 76 0b 0f b6 d0 83 ea 13 6b d2 64 01 d5 83 fd 45 89 6b 14 7f 06 83 c5 64 89 6b 14 41 83 ed 01 b8 02 00 00 00 44 89 6b 10 eb 39 <0f> 0b 48 c7 c7 b4 e0 9e 82 48 89 ee e8 29 6b 34 00 48 c7 03 ff ff
[    3.549883] RSP: 0000:ffffc900012efe30 EFLAGS: 00010002
[    3.554387] RAX: 0000000000000081 RBX: ffffc900012efe64 RCX: 000000000005b8d7
[    3.558867] RDX: 0000000000000001 RSI: ffff8881000aa000 RDI: 000000000000000d
[    3.563333] RBP: 0000000000000046 R08: 0000000000000004 R09: fffffffe5e075ac6
[    3.567748] iwlwifi 0000:02:00.0: Applying debug destination EXTERNAL_DRAM
[    3.567822] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[    3.567827] R13: ffffc900012efedc R14: 0000000000000008 R15: ffff888100051200
[    3.577223] FS:  0000000000000000(0000) GS:ffff88816ad80000(0000) knlGS:0000000000000000
[    3.579870] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    3.581947] CR2: 00007fface455e28 CR3: 0000000103244005 CR4: 00000000003706a0
[    3.583836] Call Trace:
[    3.585699]  hpet_rtc_interrupt+0x1af/0x220
[    3.587585]  __handle_irq_event_percpu+0x5a/0xc0
[    3.589230]  handle_irq_event_percpu+0x1b/0x50
[    3.590673]  handle_irq_event+0x22/0x40
[    3.592107]  handle_edge_irq+0x6b/0x190
[    3.593545]  common_interrupt+0x67/0x130
[    3.594983]  ? asm_common_interrupt+0x8/0x40
[    3.596432]  asm_common_interrupt+0x1e/0x40
[    3.597618] RIP: 0033:0x7ffaceac9b31
[    3.598794] Code: 48 83 fe 0a 0f 87 f5 fe ff ff be 41 ff ff 6f 48 29 d6 48 89 04 f1 e9 e4 fe ff ff 48 85 ff 74 79 49 8b 44 24 60 48 85 c0 74 04 <48> 01 78 08 49 8b 44 24 58 48 85 c0 74 04 48 01 78 08 49 8b 44 24
[    3.600048] RSP: 002b:00007ffc12303b00 EFLAGS: 00010202
[    3.601343] RAX: 00007fface455e20 RBX: 000000006ffffdff RCX: 00007fface80c040
[    3.602587] RDX: 0000000000000000 RSI: 0000000000000029 RDI: 00007fface451000
[    3.603809] RBP: 00007ffc12303c50 R08: 000000006fffffff R09: 00000000effffef5
[    3.605015] R10: 0000000070000022 R11: 0000000000000032 R12: 00007fface80c000
[    3.606223] R13: 000000006ffffeff R14: 000000006ffffe35 R15: 00007ffc12303ce0
[    3.607421] ---[ end trace 5922ddf43b0f7b83 ]---
[    3.608692] hpet: Lost 3 RTC interrupts

The recent change to validate the RTC turned out to be overly tight.

While it cures the problem on the reporters machine it breaks machines
with Intel chipsets which use bit 0-5 of the D register. So check only
for bit 6 being 0 which is the case on these Intel machines as well.

Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs")
Reported-by: Serge Belyshev <redacted>
Reported-by: Dirk Gouders <redacted>
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 6 must be 0! */
+	if ((CMOS_READ(RTC_VALID) & 0x40) != 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 6 must be 0! */
+	if (WARN_ON_ONCE((CMOS_READ(RTC_VALID) & 0x40) != 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

The recent change to validate the RTC turned out to be overly tight.

While it cures the problem on the reporters machine it breaks machines
with Intel chipsets which use bit 0-5 of the D register. So check only
for bit 6 being 0 which is the case on these Intel machines as well.

Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs")
Reported-by: Serge Belyshev <redacted>
Reported-by: Dirk Gouders <redacted>
Signed-off-by: Thomas Gleixner <redacted>
---
V2: Provide the actual delta patch. Should have stayed away from
    computers today....
---
 drivers/rtc/rtc-cmos.c         |    4 ++--
 drivers/rtc/rtc-mc146818-lib.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c

@@ -805,8 +805,8 @@ 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) {
+	/* 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");
 		retval = -ENXIO;
--- a/drivers/rtc/rtc-mc146818-lib.c
+++ b/drivers/rtc/rtc-mc146818-lib.c

@@ -21,8 +21,8 @@ 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)) {
+	/* 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);
 		memset(time, 0xff, sizeof(*time));
 		return 0;

On Mon, Feb 1, 2021 at 11:24 AM Thomas Gleixner [off-list ref] wrote:

While it cures the problem on the reporters machine it breaks machines
with Intel chipsets which use bit 0-5 of the D register. So check only
for bit 6 being 0 which is the case on these Intel machines as well.

This looks fine, but it might also be worth it simply just checking
for the only really special value: 0xff, and going "ok, that looks
like missing hardware".

That's what a few other drivers historically do in their probing
routines, so it's not unheard of (ie you can find drivers doing that
kind of

        /* If we read 0xff from the LSR, there is no UART here. */
        if (inb(.. port ..) == 0xff)

in their init routines.

Not a big deal either way, I just think it would be more in like with
what other places do in similar situations

      Linus

On 01/02/2021 20:24:17+0100, Thomas Gleixner wrote:

The recent change to validate the RTC turned out to be overly tight.

While it cures the problem on the reporters machine it breaks machines
with Intel chipsets which use bit 0-5 of the D register. So check only
for bit 6 being 0 which is the case on these Intel machines as well.

Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs")
Reported-by: Serge Belyshev <redacted>
Reported-by: Dirk Gouders <redacted>
Signed-off-by: Thomas Gleixner <redacted>

Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

I'm still fine with that going through your tree.

Thanks for this work I do hope this will be the last issue...

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

On Mon, Feb 01 2021 at 11:32, Linus Torvalds wrote:

On Mon, Feb 1, 2021 at 11:24 AM Thomas Gleixner [off-list ref] wrote:

quoted

While it cures the problem on the reporters machine it breaks machines
with Intel chipsets which use bit 0-5 of the D register. So check only
for bit 6 being 0 which is the case on these Intel machines as well.

This looks fine, but it might also be worth it simply just checking
for the only really special value: 0xff, and going "ok, that looks
like missing hardware".

That's what a few other drivers historically do in their probing
routines, so it's not unheard of (ie you can find drivers doing that
kind of

        /* If we read 0xff from the LSR, there is no UART here. */
        if (inb(.. port ..) == 0xff)

in their init routines.

Not a big deal either way, I just think it would be more in like with
what other places do in similar situations

Yeah, we can do that as well. Either way is fine.

Thanks,

        tglx

On Mon, Feb 01, 2021 at 08:24:17PM +0100, Thomas Gleixner wrote:

The recent change to validate the RTC turned out to be overly tight.

While it cures the problem on the reporters machine it breaks machines
with Intel chipsets which use bit 0-5 of the D register. So check only
for bit 6 being 0 which is the case on these Intel machines as well.

Fixes: 211e5db19d15 ("rtc: mc146818: Detect and handle broken RTCs")
Reported-by: Serge Belyshev <redacted>
Reported-by: Dirk Gouders <redacted>
Signed-off-by: Thomas Gleixner <redacted>
---
V2: Provide the actual delta patch. Should have stayed away from
    computers today....
---
 drivers/rtc/rtc-cmos.c         |    4 ++--
 drivers/rtc/rtc-mc146818-lib.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

FWIW:

Reported-by: Borislav Petkov <redacted>
Tested-by: Borislav Petkov <redacted>

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Thanks for the update, Thomas.

V1 prevented rc6 automated suspend/resume testing on all 13 of my
local machines.
V2 applied, and they are back in business.

tested-by: Len Brown <redacted>

On Mon, Feb 1, 2021 at 2:25 PM Thomas Gleixner [off-list ref] wrote:

-- 
Len Brown, Intel Open Source Technology Center

FWIW, it's still OK for me.

Tested-by: Mickaël Salaün <redacted>

On 01/02/2021 20:24, Thomas Gleixner wrote:

On Mon, 1 Feb 2021, Thomas Gleixner wrote:

quoted

quoted

While it cures the problem on the reporters machine it breaks machines
with Intel chipsets which use bit 0-5 of the D register. So check only
for bit 6 being 0 which is the case on these Intel machines as well.

This looks fine, but it might also be worth it simply just checking
for the only really special value: 0xff, and going "ok, that looks
like missing hardware".

That's what a few other drivers historically do in their probing
routines, so it's not unheard of (ie you can find drivers doing that
kind of

        /* If we read 0xff from the LSR, there is no UART here. */
        if (inb(.. port ..) == 0xff)

in their init routines.

Not a big deal either way, I just think it would be more in like with
what other places do in similar situations

Yeah, we can do that as well. Either way is fine.

 Given that evidently vendors appear to start playing with 146818 clones 
it may be worth it to peek at the D and the C register and checking they 
are not 0xff both at a time for robustness before concluding no RTC is 
present.  The C register is supposed to hold zeros in bits 3:0.  A read of 
the C register will drop interrupt bits, but I guess it does not matter at 
the probe time.

 FWIW,

  Maciej