[PATCH RESEND v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes

[PATCH RESEND v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes

[Mateusz Jończyk]

Hello,

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

1. A missing spin_lock_irq() / spin_unlock_irq() pair in cmos_set_alarm().
2. A failing presence check of the RTC: the clock was misdetected as
   broken since Linux 5.11 on one of our home systems.
3. Do not touch the 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.)

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

v2: Drop the last patch:
        Revert "rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard IRQ"
which was made obsolete by mainlining of 
commit 13be2efc390a ("rtc: cmos: Disable irq around direct invocation of cmos_interrupt()")

v3: Rework solution to problem 3 (I'd like to thank Greg KH for comment),
drop x86 refactoring patches (I'll send them later).

Greetings,
Mateusz

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: Greg KH <gregkh@linuxfoundation.org>

[PATCH RESEND v3 1/7] rtc-cmos: take rtc_lock while reading from CMOS

[Mateusz Jończyk]

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

[PATCH RESEND v3 2/7] rtc-mc146818-lib: fix RTC presence check

[Mateusz Jończyk]

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

Re: [PATCH RESEND v3 2/7] rtc-mc146818-lib: fix RTC presence check

[Alexandre Belloni]

Hello,

By moving this patch later on in the series, you'd avoid the subsequent
refactor. I think this would be better for bisection later on.

On 19/11/2021 21:42:16+0100, Mateusz Jończyk wrote:
> 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

This changelog should come after the --- marker

> ---
>  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");

I'd prefer if we could avoid adding strings in the lib.

>  		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
> 

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

Re: [PATCH RESEND v3 2/7] rtc-mc146818-lib: fix RTC presence check

[Mateusz Jończyk]

W dniu 24.11.2021 o 23:31, Alexandre Belloni pisze:
> Hello,
>
> By moving this patch later on in the series, you'd avoid the subsequent
> refactor. I think this would be better for bisection later on.

Hi,

There are three issues I'm trying to fix in this series:

1. (less important) Insufficient locking in cmos_set_alarm()
2. misdetection of the RTC CMOS as broken on some motherboards,
3. reading / writing of the RTC alarm time during RTC update-in-progress.

Do you mean I should drop the patch
    nr 2. ("rtc-mc146818-lib: fix RTC presence check")
and instead straight away introduce mc146818_avoid_UIP() with the new approach (as in patch 3 in the series),
then modify mc146818_get_time() to use it (as in patch 4 - fixing issue nr 2),
then modify cmos_read_alarm / cmos_set_alarm to use mc146818_avoid_UIP() (patches 5-6, fixing issue no. 3)?

I was afraid this risks some confusion what is being fixed when.

On 19/11/2021 21:42:16+0100, Mateusz Jończyk wrote:

>> 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
> This changelog should come after the --- marker
OK
>
>> ---
>> 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");
> I'd prefer if we could avoid adding strings in the lib.

The problem is that mc146818_get_time() is used 4 times in the kernel and the callers do not check
its return value. Before this patch, I was getting something like this in dmesg:

[    0.352905] ------------[ cut here ]------------
[    0.352905] WARNING: CPU: 0 PID: 1 at drivers/rtc/rtc-mc146818-lib.c:25 mc146818_get_time+0x1d5/0x230
[    0.352905] Modules linked in:
[    0.352905] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.13.0stacj-mj4 #3
[    0.352905] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./A780GXH/128M, BIOS P1.70 05/14/2010
[    0.352905] RIP: 0010:mc146818_get_time+0x1d5/0x230
[    0.352905] Code: 10 5b 41 5c 41 5d 41 5e 41 5f 5d c3 4c 89 e6 48 c7 c7 d0 a7 ba a3 e8 ca a8 2f 00 bf 58 89 41 00 e8 30 78 d0 ff e9 43 fe ff
ff <0f> 0b 48 c7 c7 d0 a7 ba a3 4c 89 e6 e8 aa a8 2f 00 48 c7 03 ff ff
[    0.352905] RSP: 0018:ffffbf8d8001fda8 EFLAGS: 00010002
[...]
[    0.352905] PM: RTC time: 18446744073709551615:18446744073709551615:18446744073709551615, date: 1899-00-18446744073709551615

which is not very helpful.

So I think I'll modify (in a separate patch) the callers to check the result and print a warning message (alspo in the callers).

For the record, the callers are:

drivers/rtc/rtc-cmos.c:    mc146818_get_time(t);
drivers/base/power/trace.c:    mc146818_get_time(&time);
arch/alpha/kernel/rtc.c:    mc146818_get_time(tm);
arch/x86/kernel/hpet.c:        mc146818_get_time(&curr_time);

Greetings,

Mateusz

>> 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
>>

Re: [PATCH RESEND v3 2/7] rtc-mc146818-lib: fix RTC presence check

[Alexandre Belloni]

Hi,

On 25/11/2021 23:12:42+0100, Mateusz Jończyk wrote:
> W dniu 24.11.2021 o 23:31, Alexandre Belloni pisze:
> > Hello,
> >
> > By moving this patch later on in the series, you'd avoid the subsequent
> > refactor. I think this would be better for bisection later on.
> 
> Hi,
> 
> There are three issues I'm trying to fix in this series:
> 
> 1. (less important) Insufficient locking in cmos_set_alarm()
> 2. misdetection of the RTC CMOS as broken on some motherboards,
> 3. reading / writing of the RTC alarm time during RTC update-in-progress.
> 
> Do you mean I should drop the patch
>     nr 2. ("rtc-mc146818-lib: fix RTC presence check")
> and instead straight away introduce mc146818_avoid_UIP() with the new approach (as in patch 3 in the series),
> then modify mc146818_get_time() to use it (as in patch 4 - fixing issue nr 2),
> then modify cmos_read_alarm / cmos_set_alarm to use mc146818_avoid_UIP() (patches 5-6, fixing issue no. 3)?
> 
> I was afraid this risks some confusion what is being fixed when.

I realize I never replied to this. This is fine, I'm planning to apply
the whole series once the few comments are fixed.

We'll probably get some breakage later on because many RTCs using this
driver are not adhering to the spec.

> 
> On 19/11/2021 21:42:16+0100, Mateusz Jończyk wrote:
> 
> >> 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
> > This changelog should come after the --- marker
> OK
> >
> >> ---
> >> 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");
> > I'd prefer if we could avoid adding strings in the lib.
> 
> The problem is that mc146818_get_time() is used 4 times in the kernel and the callers do not check
> its return value. Before this patch, I was getting something like this in dmesg:
> 
> [    0.352905] ------------[ cut here ]------------
> [    0.352905] WARNING: CPU: 0 PID: 1 at drivers/rtc/rtc-mc146818-lib.c:25 mc146818_get_time+0x1d5/0x230
> [    0.352905] Modules linked in:
> [    0.352905] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.13.0stacj-mj4 #3
> [    0.352905] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./A780GXH/128M, BIOS P1.70 05/14/2010
> [    0.352905] RIP: 0010:mc146818_get_time+0x1d5/0x230
> [    0.352905] Code: 10 5b 41 5c 41 5d 41 5e 41 5f 5d c3 4c 89 e6 48 c7 c7 d0 a7 ba a3 e8 ca a8 2f 00 bf 58 89 41 00 e8 30 78 d0 ff e9 43 fe ff
> ff <0f> 0b 48 c7 c7 d0 a7 ba a3 4c 89 e6 e8 aa a8 2f 00 48 c7 03 ff ff
> [    0.352905] RSP: 0018:ffffbf8d8001fda8 EFLAGS: 00010002
> [...]
> [    0.352905] PM: RTC time: 18446744073709551615:18446744073709551615:18446744073709551615, date: 1899-00-18446744073709551615
> 
> which is not very helpful.
> 
> So I think I'll modify (in a separate patch) the callers to check the result and print a warning message (alspo in the callers).
> 
> For the record, the callers are:
> 
> drivers/rtc/rtc-cmos.c:    mc146818_get_time(t);
> drivers/base/power/trace.c:    mc146818_get_time(&time);
> arch/alpha/kernel/rtc.c:    mc146818_get_time(tm);
> arch/x86/kernel/hpet.c:        mc146818_get_time(&curr_time);
> 
> Greetings,
> 
> Mateusz
> 
> >> 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
> >>
> 
> 

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

Re: [PATCH RESEND v3 2/7] rtc-mc146818-lib: fix RTC presence check

[Mateusz Jończyk]

W dniu 10.12.2021 o 16:26, Alexandre Belloni pisze:
> Hi,
>
> On 25/11/2021 23:12:42+0100, Mateusz Jończyk wrote:
>> W dniu 24.11.2021 o 23:31, Alexandre Belloni pisze:
>>> Hello,
>>>
>>> By moving this patch later on in the series, you'd avoid the subsequent
>>> refactor. I think this would be better for bisection later on.
>> Hi,
>>
>> There are three issues I'm trying to fix in this series:
>>
>> 1. (less important) Insufficient locking in cmos_set_alarm()
>> 2. misdetection of the RTC CMOS as broken on some motherboards,
>> 3. reading / writing of the RTC alarm time during RTC update-in-progress.
>>
>> Do you mean I should drop the patch
>>     nr 2. ("rtc-mc146818-lib: fix RTC presence check")
>> and instead straight away introduce mc146818_avoid_UIP() with the new approach (as in patch 3 in the series),
>> then modify mc146818_get_time() to use it (as in patch 4 - fixing issue nr 2),
>> then modify cmos_read_alarm / cmos_set_alarm to use mc146818_avoid_UIP() (patches 5-6, fixing issue no. 3)?
>>
>> I was afraid this risks some confusion what is being fixed when.
> I realize I never replied to this. This is fine, I'm planning to apply
> the whole series once the few comments are fixed.

Great!

I've got other things mostly sorted out and tested, so I'll send a v4 shortly after some last-minute checks.

> We'll probably get some breakage later on because many RTCs using this
> driver are not adhering to the spec.
>
Thanks,

Mateusz

[PATCH RESEND v3 3/7] rtc-mc146818-lib: extract mc146818_do_avoiding_UIP

[Mateusz Jończyk]

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

Re: [PATCH RESEND v3 3/7] rtc-mc146818-lib: extract mc146818_do_avoiding_UIP

[Alexandre Belloni]

On 19/11/2021 21:42:17+0100, Mateusz Jończyk wrote:
> 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)

mc146818_avoid_UIP would be a simpler name. Also, I'm pretty sure we can
avoid the mc146818_callback_t typedef

> +{
> +	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
> 

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

Re: [PATCH RESEND v3 3/7] rtc-mc146818-lib: extract mc146818_do_avoiding_UIP

[Mateusz Jończyk]

W dniu 24.11.2021 o 23:39, Alexandre Belloni pisze:
> On 19/11/2021 21:42:17+0100, Mateusz Jończyk wrote:
>> 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)
> mc146818_avoid_UIP would be a simpler name. 
Right
> Also, I'm pretty sure we can
> avoid the mc146818_callback_t typedef

Do you mean doing something like:

bool mc146818_avoid_UIP(
	void (*callback)(unsigned char seconds, void *param), void *param);

Thanks for reviewing.

Greetings,
Mateusz

>> +{
>> +	int i;
>> +	unsigned long flags;
[snip]

Re: [PATCH RESEND v3 3/7] rtc-mc146818-lib: extract mc146818_do_avoiding_UIP

[Alexandre Belloni]

On 25/11/2021 06:28:45+0100, Mateusz Jończyk wrote:
> W dniu 24.11.2021 o 23:39, Alexandre Belloni pisze:
> > On 19/11/2021 21:42:17+0100, Mateusz Jończyk wrote:
> >> 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)
> > mc146818_avoid_UIP would be a simpler name. 
> Right
> > Also, I'm pretty sure we can
> > avoid the mc146818_callback_t typedef
> 
> Do you mean doing something like:
> 
> bool mc146818_avoid_UIP(
> 	void (*callback)(unsigned char seconds, void *param), void *param);
> 

yes!

> Thanks for reviewing.
> 
> Greetings,
> Mateusz
> 
> >> +{
> >> +	int i;
> >> +	unsigned long flags;
> [snip]

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

[PATCH RESEND v3 4/7] rtc-mc146818-lib: refactor mc146818_get_time

[Mateusz Jończyk]

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

Re: [PATCH RESEND v3 4/7] rtc-mc146818-lib: refactor mc146818_get_time

[Alexandre Belloni]

On 19/11/2021 21:42:18+0100, Mateusz Jończyk wrote:
> 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

This is an unrelated change

> +	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
> 

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

Re: [PATCH RESEND v3 4/7] rtc-mc146818-lib: refactor mc146818_get_time

[Mateusz Jończyk]

W dniu 24.11.2021 o 23:41, Alexandre Belloni pisze:
> On 19/11/2021 21:42:18+0100, Mateusz Jończyk wrote:
>> 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
> This is an unrelated change

Well, now, when CONFIG_ACPI is not defined, p.century does not exist, so this #ifdef
is required. I could unconditionally define "century" in mc146818_get_time_callback_param and set
it manually to 0 when CONFIG_ACPI is not defined. I thought that this approach is more difficult
to analyse, so did not use it. Should I do this?

Previously, when CONFIG_ACPI was not defined, century was always 0, so
this doesn't change behaviour.

Greetings,

Mateusz

>> +	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
>>

Re: [PATCH RESEND v3 4/7] rtc-mc146818-lib: refactor mc146818_get_time

[Alexandre Belloni]

On 25/11/2021 06:48:38+0100, Mateusz Jończyk wrote:
> W dniu 24.11.2021 o 23:41, Alexandre Belloni pisze:
> > On 19/11/2021 21:42:18+0100, Mateusz Jończyk wrote:
> >> 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
> > This is an unrelated change
> 
> Well, now, when CONFIG_ACPI is not defined, p.century does not exist, so this #ifdef
> is required. I could unconditionally define "century" in mc146818_get_time_callback_param and set
> it manually to 0 when CONFIG_ACPI is not defined. I thought that this approach is more difficult
> to analyse, so did not use it. Should I do this?
> 

No, that's fine, thanks!

> Previously, when CONFIG_ACPI was not defined, century was always 0, so
> this doesn't change behaviour.
> 
> Greetings,
> 
> Mateusz
> 
> >> +	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
> >>
> 

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

[PATCH RESEND v3 5/7] rtc-mc146818-lib: refactor mc146818_does_rtc_work

[Mateusz Jończyk]

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

[PATCH RESEND v3 6/7] rtc-cmos: avoid UIP when reading alarm time

[Mateusz Jończyk]

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

[DEBUG PATCH v3] rtc-cmos: cmos_read_alarm bug demonstration

[Mateusz Jończyk]

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

Re: [PATCH RESEND v3 6/7] rtc-cmos: avoid UIP when reading alarm time

[Alexandre Belloni]

On 19/11/2021 21:42:20+0100, Mateusz Jończyk wrote:
> 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(-)
> 

You have a few checkpatch --strict issues in this patch and the next one
that I would be grateful if you could correct them.

> 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
> 

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

[PATCH RESEND v3 7/7] rtc-cmos: avoid UIP when writing alarm time

[Mateusz Jończyk]

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

Re: [PATCH RESEND v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes

[Alexandre Belloni]

Hello Mateusz,

I'm planning to review and take them soon (early next week), I found it
was a bit late for the previous version. I'd like that to sit in
linux-next for a while a rtc-cmos is kind of sensitive ;)

On 19/11/2021 21:42:14+0100, Mateusz Jończyk wrote:
> Hello,
> 
> This patch series fixes some issues in the RTC CMOS handling code:
> 
> 1. A missing spin_lock_irq() / spin_unlock_irq() pair in cmos_set_alarm().
> 2. A failing presence check of the RTC: the clock was misdetected as
>    broken since Linux 5.11 on one of our home systems.
> 3. Do not touch the 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.)
> 
> This is my first patch series, so please review carefully.
> 
> v2: Drop the last patch:
>         Revert "rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard IRQ"
> which was made obsolete by mainlining of 
> commit 13be2efc390a ("rtc: cmos: Disable irq around direct invocation of cmos_interrupt()")
> 
> v3: Rework solution to problem 3 (I'd like to thank Greg KH for comment),
> drop x86 refactoring patches (I'll send them later).
> 
> Greetings,
> Mateusz
> 
> 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: Greg KH <gregkh@linuxfoundation.org>

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

[PATCH v3 0/7] rtc-cmos,rtc-mc146818-lib: fixes

[Mateusz Jończyk]

Hello,

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

1. A missing spin_lock_irq() / spin_unlock_irq() pair in cmos_set_alarm().
2. A failing presence check of the RTC: the clock was misdetected as
   broken since Linux 5.11 on one of our home systems.
3. Do not touch the 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.)

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

v2: Drop the last patch:
        Revert "rtc: cmos: Replace spin_lock_irqsave with spin_lock in hard IRQ"
which was made obsolete by mainlining of 
commit 13be2efc390a ("rtc: cmos: Disable irq around direct invocation of cmos_interrupt()")

v3: Rework solution to problem 3 (I'd like to thank Greg KH for comment),
drop x86 refactoring patches (I'll send them later).

Greetings,
Mateusz

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: Greg KH <gregkh@linuxfoundation.org>

[DEBUG PATCH v3] rtc-cmos: cmos_read_alarm bug demonstration

[Mateusz Jończyk]

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