FAILED: patch "[PATCH] rtc: mc146818-lib: fix RTC presence check" failed to apply to 5.16-stable tree

FAILED: patch "[PATCH] rtc: mc146818-lib: fix RTC presence check" failed to apply to 5.16-stable tree

[gregkh]

The patch below does not apply to the 5.16-stable tree.
If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit
id to <stable@vger.kernel.org>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From ea6fa4961aab8f90a8aa03575a98b4bda368d4b6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mateusz=20Jo=C5=84czyk?= <mat.jonczyk@o2.pl>
Date: Fri, 10 Dec 2021 21:01:26 +0100
Subject: [PATCH] rtc: mc146818-lib: fix RTC presence check
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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]. This problem was also reported in Fedora [4].

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.

The functions mc146818_get_time() and mc146818_does_rtc_work() will be
refactored later in this patch series, in order to fix a separate
problem with reading / setting the RTC alarm time. This is done so to
avoid a confusion about what is being fixed when.

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.

[4] https://bugzilla.redhat.com/show_bug.cgi?id=1936688

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>
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
Link: https://lore.kernel.org/r/20211210200131.153887-5-mat.jonczyk@o2.pl

diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
index d0f58cca5c20..b90a603d6b12 100644
--- a/drivers/rtc/rtc-cmos.c
+++ b/drivers/rtc/rtc-cmos.c
@@ -800,16 +800,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 ccd974b8a75a..d8e67a01220e 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,13 @@ 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) {
 		memset(time, 0, sizeof(*time));
 		return -EIO;
 	}
+	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);
 

Re: FAILED: patch "[PATCH] rtc: mc146818-lib: fix RTC presence check" failed to apply to 5.16-stable tree

[Mateusz Jończyk]

W dniu 24.01.2022 o 14:08, gregkh@linuxfoundation.org pisze:
> The patch below does not apply to the 5.16-stable tree.
> If someone wants it applied there, or to any other stable or longterm
> tree, then please email the backport, including the original git commit
> id to <stable@vger.kernel.org>.
>
> thanks,
>
> greg k-h
Wait, this patch was not intended for submission into stable yet, at least not in this form.
Why did it end up in the stable queue?

The return values from mc146818_get_time() changed in between,
so it is natural that it does not apply.

See
commit d35786b3a28dee20 ("rtc: mc146818-lib: change return values of mc146818_get_time()")

Greetings,
Mateusz Jończyk
> ------------------ original commit in Linus's tree ------------------
>
> >From ea6fa4961aab8f90a8aa03575a98b4bda368d4b6 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Mateusz=20Jo=C5=84czyk?= <mat.jonczyk@o2.pl>
> Date: Fri, 10 Dec 2021 21:01:26 +0100
> Subject: [PATCH] rtc: mc146818-lib: fix RTC presence check
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> 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]. This problem was also reported in Fedora [4].
>
> 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.
>
> The functions mc146818_get_time() and mc146818_does_rtc_work() will be
> refactored later in this patch series, in order to fix a separate
> problem with reading / setting the RTC alarm time. This is done so to
> avoid a confusion about what is being fixed when.
>
> 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.
>
> [4] https://bugzilla.redhat.com/show_bug.cgi?id=1936688
>
> 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>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Link: https://lore.kernel.org/r/20211210200131.153887-5-mat.jonczyk@o2.pl
>
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index d0f58cca5c20..b90a603d6b12 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -800,16 +800,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 ccd974b8a75a..d8e67a01220e 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,13 @@ 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) {
>  		memset(time, 0, sizeof(*time));
>  		return -EIO;
>  	}
> +	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);
>  
>

Re: FAILED: patch "[PATCH] rtc: mc146818-lib: fix RTC presence check" failed to apply to 5.16-stable tree

[Greg KH]

On Mon, Jan 24, 2022 at 03:45:30PM +0100, Mateusz Jończyk wrote:
> W dniu 24.01.2022 o 14:08, gregkh@linuxfoundation.org pisze:
> > The patch below does not apply to the 5.16-stable tree.
> > If someone wants it applied there, or to any other stable or longterm
> > tree, then please email the backport, including the original git commit
> > id to <stable@vger.kernel.org>.
> >
> > thanks,
> >
> > greg k-h
> Wait, this patch was not intended for submission into stable yet, at least not in this form.
> Why did it end up in the stable queue?

Because it was marked with a "Fixes:" tag and I reviewed it and it
looked like it actually fixed an issue.

Is this not the case?

> The return values from mc146818_get_time() changed in between,
> so it is natural that it does not apply.
> 
> See
> commit d35786b3a28dee20 ("rtc: mc146818-lib: change return values of mc146818_get_time()")

Ok, so will a working backport be sent sometime in the future?

thanks,

greg k-h

Re: FAILED: patch "[PATCH] rtc: mc146818-lib: fix RTC presence check" failed to apply to 5.16-stable tree

[Mateusz Jończyk]

W dniu 24.01.2022 o 15:52, Greg KH pisze:
> On Mon, Jan 24, 2022 at 03:45:30PM +0100, Mateusz Jończyk wrote:
>> W dniu 24.01.2022 o 14:08, gregkh@linuxfoundation.org pisze:
>>> The patch below does not apply to the 5.16-stable tree.
>>> If someone wants it applied there, or to any other stable or longterm
>>> tree, then please email the backport, including the original git commit
>>> id to <stable@vger.kernel.org>.
>>>
>>> thanks,
>>>
>>> greg k-h
>> Wait, this patch was not intended for submission into stable yet, at least not in this form.
>> Why did it end up in the stable queue?
> Because it was marked with a "Fixes:" tag and I reviewed it and it
> looked like it actually fixed an issue.
>
> Is this not the case?

Yes, that's correct. But I'm afraid that this patch will cause regressions on some systems.
(Mr Alexandre Belloni said something similar of my series). So I'd like to wait till at least Linux 5.17 (final) is
released to see if it causes any trouble.

>> The return values from mc146818_get_time() changed in between,
>> so it is natural that it does not apply.
>>
>> See
>> commit d35786b3a28dee20 ("rtc: mc146818-lib: change return values of mc146818_get_time()")
> Ok, so will a working backport be sent sometime in the future?
> thanks,
>
> greg k-h

Greetings,

Mateusz

Re: FAILED: patch "[PATCH] rtc: mc146818-lib: fix RTC presence check" failed to apply to 5.16-stable tree

[Alexandre Belloni]

On 24/01/2022 17:27:16+0100, Mateusz Jończyk wrote:
> W dniu 24.01.2022 o 15:52, Greg KH pisze:
> > On Mon, Jan 24, 2022 at 03:45:30PM +0100, Mateusz Jończyk wrote:
> >> W dniu 24.01.2022 o 14:08, gregkh@linuxfoundation.org pisze:
> >>> The patch below does not apply to the 5.16-stable tree.
> >>> If someone wants it applied there, or to any other stable or longterm
> >>> tree, then please email the backport, including the original git commit
> >>> id to <stable@vger.kernel.org>.
> >>>
> >>> thanks,
> >>>
> >>> greg k-h
> >> Wait, this patch was not intended for submission into stable yet, at least not in this form.
> >> Why did it end up in the stable queue?
> > Because it was marked with a "Fixes:" tag and I reviewed it and it
> > looked like it actually fixed an issue.
> >
> > Is this not the case?
> 
> Yes, that's correct. But I'm afraid that this patch will cause regressions on some systems.
> (Mr Alexandre Belloni said something similar of my series). So I'd like to wait till at least Linux 5.17 (final) is
> released to see if it causes any trouble.
> 

Well, it will be in 5.17, any regression it causes will have to be
fixed and we can then backport the fix.


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