[PATCH] rtc: rtc-at91rm9200: fix lost ACKUPD interrupt

[PATCH] rtc: rtc-at91rm9200: fix lost ACKUPD interrupt

[Boris BREZILLON]

If an alarm interrupt raises just after the CR register is written and
before the ACKUPD is unmasked the ACKUPD bit might be cleaned up without
waking up the waiting thread.

Unmask the ACKUPD interrupt before writing the CR register so that the
ACKUPD cannot be lost.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Reported-by: Bryan Evenson <bevenson@melinkcorp.com>
---
Hello Bryan,

Could you try to apply this patch and tell me if it fixes your bug ?

Best Regards,

Boris

 drivers/rtc/rtc-at91rm9200.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 3281c90..c16c423 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -163,9 +163,9 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm)
 
 	/* Stop Time/Calendar from counting */
 	cr = at91_rtc_read(AT91_RTC_CR);
+	at91_rtc_write_ier(AT91_RTC_ACKUPD);
 	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL | AT91_RTC_UPDTIM);
 
-	at91_rtc_write_ier(AT91_RTC_ACKUPD);
 	wait_for_completion(&at91_rtc_updated);	/* wait for ACKUPD interrupt */
 	at91_rtc_write_idr(AT91_RTC_ACKUPD);
 
-- 
1.8.3.2

RE: [PATCH] rtc: rtc-at91rm9200: fix lost ACKUPD interrupt

[Bryan Evenson]

Boris,

> -----Original Message-----
> From: Boris BREZILLON [mailto:boris.brezillon@free-electrons.com]
> Sent: Monday, May 05, 2014 4:23 PM
> To: Bryan Evenson
> Cc: Andrew Victor; Nicolas Ferre; Jean-Christophe Plagniol-Villard; linux-arm-
> kernel@lists.infradead.org; Alessandro Zummo; rtc-
> linux@googlegroups.com; linux-kernel@vger.kernel.org; Boris BREZILLON
> Subject: [PATCH] rtc: rtc-at91rm9200: fix lost ACKUPD interrupt
> 
> If an alarm interrupt raises just after the CR register is written and before the
> ACKUPD is unmasked the ACKUPD bit might be cleaned up without waking
> up the waiting thread.
> 
> Unmask the ACKUPD interrupt before writing the CR register so that the
> ACKUPD cannot be lost.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> Reported-by: Bryan Evenson <bevenson@melinkcorp.com>
> ---
> Hello Bryan,
> 
> Could you try to apply this patch and tell me if it fixes your bug ?
> 

I'll incorporate the patch and see what happens.  I'll be able to let you know the results tomorrow morning.

Regards,
Bryan

> Best Regards,
> 
> Boris
> 
>  drivers/rtc/rtc-at91rm9200.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c index
> 3281c90..c16c423 100644
> --- a/drivers/rtc/rtc-at91rm9200.c
> +++ b/drivers/rtc/rtc-at91rm9200.c
> @@ -163,9 +163,9 @@ static int at91_rtc_settime(struct device *dev, struct
> rtc_time *tm)
> 
>  	/* Stop Time/Calendar from counting */
>  	cr = at91_rtc_read(AT91_RTC_CR);
> +	at91_rtc_write_ier(AT91_RTC_ACKUPD);
>  	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL |
> AT91_RTC_UPDTIM);
> 
> -	at91_rtc_write_ier(AT91_RTC_ACKUPD);
>  	wait_for_completion(&at91_rtc_updated);	/* wait for ACKUPD
> interrupt */
>  	at91_rtc_write_idr(AT91_RTC_ACKUPD);
> 
> --
> 1.8.3.2

RE: [PATCH] rtc: rtc-at91rm9200: fix lost ACKUPD interrupt

[Bryan Evenson]

Boris,

> -----Original Message-----
> From: Boris BREZILLON [mailto:boris.brezillon@free-electrons.com]
> Sent: Monday, May 05, 2014 4:23 PM
> To: Bryan Evenson
> Cc: Andrew Victor; Nicolas Ferre; Jean-Christophe Plagniol-Villard; linux-arm-
> kernel@lists.infradead.org; Alessandro Zummo; rtc-
> linux@googlegroups.com; linux-kernel@vger.kernel.org; Boris BREZILLON
> Subject: [PATCH] rtc: rtc-at91rm9200: fix lost ACKUPD interrupt
> 
> If an alarm interrupt raises just after the CR register is written and before the
> ACKUPD is unmasked the ACKUPD bit might be cleaned up without waking
> up the waiting thread.
> 
> Unmask the ACKUPD interrupt before writing the CR register so that the
> ACKUPD cannot be lost.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> Reported-by: Bryan Evenson <bevenson@melinkcorp.com>
> ---
> Hello Bryan,
> 
> Could you try to apply this patch and tell me if it fixes your bug ?
> 

I tried the patch and it didn't help.  I still have the same issue.  Do you see this problem on your end?  Is there debug output that would be helpful to see?

Regards,
Bryan

> Best Regards,
> 
> Boris
> 
>  drivers/rtc/rtc-at91rm9200.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c index
> 3281c90..c16c423 100644
> --- a/drivers/rtc/rtc-at91rm9200.c
> +++ b/drivers/rtc/rtc-at91rm9200.c
> @@ -163,9 +163,9 @@ static int at91_rtc_settime(struct device *dev, struct
> rtc_time *tm)
> 
>  	/* Stop Time/Calendar from counting */
>  	cr = at91_rtc_read(AT91_RTC_CR);
> +	at91_rtc_write_ier(AT91_RTC_ACKUPD);
>  	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL |
> AT91_RTC_UPDTIM);
> 
> -	at91_rtc_write_ier(AT91_RTC_ACKUPD);
>  	wait_for_completion(&at91_rtc_updated);	/* wait for ACKUPD
> interrupt */
>  	at91_rtc_write_idr(AT91_RTC_ACKUPD);
> 
> --
> 1.8.3.2

RE: [PATCH] rtc: rtc-at91rm9200: fix lost ACKUPD interrupt

[Bryan Evenson]

Boris,

> -----Original Message-----
> From: Boris BREZILLON [mailto:boris.brezillon@free-electrons.com]
> Sent: Monday, May 05, 2014 4:23 PM
> To: Bryan Evenson
> Cc: Andrew Victor; Nicolas Ferre; Jean-Christophe Plagniol-Villard; linux-arm-
> kernel@lists.infradead.org; Alessandro Zummo; rtc-
> linux@googlegroups.com; linux-kernel@vger.kernel.org; Boris BREZILLON
> Subject: [PATCH] rtc: rtc-at91rm9200: fix lost ACKUPD interrupt
> 
> If an alarm interrupt raises just after the CR register is written and before the
> ACKUPD is unmasked the ACKUPD bit might be cleaned up without waking
> up the waiting thread.
> 
> Unmask the ACKUPD interrupt before writing the CR register so that the
> ACKUPD cannot be lost.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> Reported-by: Bryan Evenson <bevenson@melinkcorp.com>
> ---
> Hello Bryan,
> 
> Could you try to apply this patch and tell me if it fixes your bug ?
> 

I have a little more information for you.  On my system hwclock is part of Busybox (version 1.20.2 of Busybox).  On my build I do not have CONFIG_ENABLE_FEATURE_CLEAN_UP enabled for Busybox, so it does not close the file descriptor to the RTC after it is done with it.  Here is the relevant section in the Busybox code that is setting the RTC time: http://git.busybox.net/busybox/tree/util-linux/hwclock.c?h=1_20_stable#n224.  Per a suggestion from someone on the Busybox forum, I applied a patch which added a flock() with LOCK_EX on the file descriptor and I still had the same issue.  Running two instances of the previously mentioned test scripts in the background, here are the two strace outputs from the point when one access hung:

Instance #1
----------
1256  execve("/sbin/hwclock", ["/sbin/hwclock", "-w", "-u"], [/* 13 vars */]) = 0
1256  brk(0)                            = 0x9c000
1256  uname({sys="Linux", node="at91sam9x5ek", ...}) = 0
1256  access("/etc/ld.so.preload", R_OK) = -1 ENOENT (No such file or directory)
1256  open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
1256  fstat64(3, {st_mode=S_IFREG|0644, st_size=7053, ...}) = 0
1256  mmap2(NULL, 7053, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb6f47000
1256  close(3)                          = 0
1256  open("/lib/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
1256  read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0(\0\1\0\0\0\354\225JJ4\0\0\0"..., 512) = 512
1256  fstat64(3, {st_mode=S_IFREG|0755, st_size=1245200, ...}) = 0
1256  mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb6f46000
1256  mmap2(0x4a490000, 1284496, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x4a490000
1256  mprotect(0x4a5bc000, 32768, PROT_NONE) = 0
1256  mmap2(0x4a5c4000, 16384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x12c) = 0x4a5c4000
1256  mmap2(0x4a5c8000, 6544, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x4a5c8000
1256  close(3)                          = 0
1256  mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb6f45000
1256  set_tls(0xb6f454c0, 0xb6f45b98, 0x4a487058, 0xb6f454c0, 0x4a487058) = 0
1256  mprotect(0x4a5c4000, 8192, PROT_READ) = 0
1256  mprotect(0x4a486000, 4096, PROT_READ) = 0
1256  munmap(0xb6f47000, 7053)          = 0
1256  getuid32()                        = 0
1256  open("/dev/rtc", O_WRONLY|O_LARGEFILE) = 3
1256  gettimeofday({1399376901, 241610}, NULL) = 0
1256  brk(0)                            = 0x9c000
1256  brk(0xbd000)                      = 0xbd000
1256  open("/etc/localtime", O_RDONLY|O_CLOEXEC) = 4
1256  fcntl64(4, F_GETFD)               = 0x1 (flags FD_CLOEXEC)
1256  fstat64(4, {st_mode=S_IFREG|0644, st_size=3519, ...}) = 0
1256  fstat64(4, {st_mode=S_IFREG|0644, st_size=3519, ...}) = 0
1256  mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb6f48000
1256  read(4, "TZif2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\4\0\0\0\4\0\0\0\0"..., 4096) = 3519
1256  _llseek(4, -24, [3495], SEEK_CUR) = 0
1256  read(4, "\nEST5EDT,M3.2.0,M11.1.0\n", 4096) = 24
1256  close(4)                          = 0
1256  munmap(0xb6f48000, 4096)          = 0
1256  flock(3, LOCK_EX)                 = 0
1256  ioctl(3, RTC_SET_TIME, {tm_sec=21, tm_min=48, tm_hour=11, tm_mday=6, tm_mon=4, tm_year=114, ...}) = 0
1256  flock(3, LOCK_UN)                 = 0
1256  exit_group(0)                     = ?
1256  +++ exited with 0 +++
----------

Instance #2
----------
1261  execve("/sbin/hwclock", ["/sbin/hwclock", "-w", "-u"], [/* 13 vars */]) = 0
1261  brk(0)                            = 0x9c000
1261  uname({sys="Linux", node="at91sam9x5ek", ...}) = 0
1261  access("/etc/ld.so.preload", R_OK) = -1 ENOENT (No such file or directory)
1261  open("/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
1261  fstat64(3, {st_mode=S_IFREG|0644, st_size=7053, ...}) = 0
1261  mmap2(NULL, 7053, PROT_READ, MAP_PRIVATE, 3, 0) = 0xb6f4b000
1261  close(3)                          = 0
1261  open("/lib/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
1261  read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0(\0\1\0\0\0\354\225JJ4\0\0\0"..., 512) = 512
1261  fstat64(3, {st_mode=S_IFREG|0755, st_size=1245200, ...}) = 0
1261  mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb6f4a000
1261  mmap2(0x4a490000, 1284496, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x4a490000
1261  mprotect(0x4a5bc000, 32768, PROT_NONE) = 0
1261  mmap2(0x4a5c4000, 16384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x12c) = 0x4a5c4000
1261  mmap2(0x4a5c8000, 6544, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x4a5c8000
1261  close(3)                          = 0
1261  mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb6f49000
1261  set_tls(0xb6f494c0, 0xb6f49b98, 0x4a487058, 0xb6f494c0, 0x4a487058) = 0
1261  mprotect(0x4a5c4000, 8192, PROT_READ) = 0
1261  mprotect(0x4a486000, 4096, PROT_READ) = 0
1261  munmap(0xb6f4b000, 7053)          = 0
1261  getuid32()                        = 0
1261  open("/dev/rtc", O_WRONLY|O_LARGEFILE) = 3
1261  gettimeofday({1399376901, 655559}, NULL) = 0
1261  brk(0)                            = 0x9c000
1261  brk(0xbd000)                      = 0xbd000
1261  open("/etc/localtime", O_RDONLY|O_CLOEXEC) = 4
1261  fcntl64(4, F_GETFD)               = 0x1 (flags FD_CLOEXEC)
1261  fstat64(4, {st_mode=S_IFREG|0644, st_size=3519, ...}) = 0
1261  fstat64(4, {st_mode=S_IFREG|0644, st_size=3519, ...}) = 0
1261  mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb6f4c000
1261  read(4, "TZif2\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\4\0\0\0\4\0\0\0\0"..., 4096) = 3519
1261  _llseek(4, -24, [3495], SEEK_CUR) = 0
1261  read(4, "\nEST5EDT,M3.2.0,M11.1.0\n", 4096) = 24
1261  close(4)                          = 0
1261  munmap(0xb6f4c000, 4096)          = 0
1261  flock(3, LOCK_EX)                 = 0
1261  ioctl(3, RTC_SET_TIME, {tm_sec=21, tm_min=48, tm_hour=11, tm_mday=6, tm_mon=4, tm_year=114, ...}
----------

For the second instance, I had to "cat" the file that is still open by the strace process that has not yet returned.  If I open the strace output file with vi, the ioctl() call is not present.  If I understand flock() correctly, I think we can assume one ioctl() completes before the other ioctl() begins.  Is it possible there is something else going on in which the RTC is not yet ready for time to be set again?  Or is it a problem with setting the RTC to the same time twice?

If there is a different kernel version you would like me to try, or if you would like my kernel binary for your testing, let me know.

Regards,
Bryan

> Best Regards,
> 
> Boris
> 
>  drivers/rtc/rtc-at91rm9200.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c index
> 3281c90..c16c423 100644
> --- a/drivers/rtc/rtc-at91rm9200.c
> +++ b/drivers/rtc/rtc-at91rm9200.c
> @@ -163,9 +163,9 @@ static int at91_rtc_settime(struct device *dev, struct
> rtc_time *tm)
> 
>  	/* Stop Time/Calendar from counting */
>  	cr = at91_rtc_read(AT91_RTC_CR);
> +	at91_rtc_write_ier(AT91_RTC_ACKUPD);
>  	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL |
> AT91_RTC_UPDTIM);
> 
> -	at91_rtc_write_ier(AT91_RTC_ACKUPD);
>  	wait_for_completion(&at91_rtc_updated);	/* wait for ACKUPD
> interrupt */
>  	at91_rtc_write_idr(AT91_RTC_ACKUPD);
> 
> --
> 1.8.3.2

[PATCH] rtc: rtc-at91rm9200: fix infinite wait for ACKUPD irq

[Boris BREZILLON]

The rtc user must wait at least 1 sec between each time/calandar update
(see atmel's datasheet chapter "Updating Time/Calendar").

Use the 1Hz interrupt to update the at91_rtc_upd_rdy flag and wait for
the at91_rtc_wait_upd_rdy event if the rtc is not ready.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
Reported-by: Bryan Evenson <bevenson@melinkcorp.com>
---
Hello Bryan,

I reproduced your bug (using your script) and this patch seems to fix the
problem.

Could you try it and let me know if it works for you ?

Best Regards,

Boris

 drivers/rtc/rtc-at91rm9200.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
index 3281c90..e3fe54c 100644
--- a/drivers/rtc/rtc-at91rm9200.c
+++ b/drivers/rtc/rtc-at91rm9200.c
@@ -48,11 +48,13 @@ struct at91_rtc_config {
 
 static const struct at91_rtc_config *at91_rtc_config;
 static DECLARE_COMPLETION(at91_rtc_updated);
+static DECLARE_COMPLETION(at91_rtc_wait_upd_rdy);
 static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
 static void __iomem *at91_rtc_regs;
 static int irq;
 static DEFINE_SPINLOCK(at91_rtc_lock);
 static u32 at91_rtc_shadow_imr;
+static bool at91_rtc_upd_rdy;
 
 static void at91_rtc_write_ier(u32 mask)
 {
@@ -161,6 +163,8 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm)
 		1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
 		tm->tm_hour, tm->tm_min, tm->tm_sec);
 
+	wait_for_completion(&at91_rtc_wait_upd_rdy);
+
 	/* Stop Time/Calendar from counting */
 	cr = at91_rtc_read(AT91_RTC_CR);
 	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL | AT91_RTC_UPDTIM);
@@ -183,6 +187,7 @@ static int at91_rtc_settime(struct device *dev, struct rtc_time *tm)
 
 	/* Restart Time/Calendar */
 	cr = at91_rtc_read(AT91_RTC_CR);
+	at91_rtc_upd_rdy = 0;
 	at91_rtc_write(AT91_RTC_CR, cr & ~(AT91_RTC_UPDCAL | AT91_RTC_UPDTIM));
 
 	return 0;
@@ -290,8 +295,13 @@ static irqreturn_t at91_rtc_interrupt(int irq, void *dev_id)
 	if (rtsr) {		/* this interrupt is shared!  Is it ours? */
 		if (rtsr & AT91_RTC_ALARM)
 			events |= (RTC_AF | RTC_IRQF);
-		if (rtsr & AT91_RTC_SECEV)
+		if (rtsr & AT91_RTC_SECEV) {
 			events |= (RTC_UF | RTC_IRQF);
+			if (!at91_rtc_upd_rdy) {
+				at91_rtc_upd_rdy = 1;
+				complete(&at91_rtc_wait_upd_rdy);
+			}
+		}
 		if (rtsr & AT91_RTC_ACKUPD)
 			complete(&at91_rtc_updated);
 
@@ -413,6 +423,8 @@ static int __init at91_rtc_probe(struct platform_device *pdev)
 		return PTR_ERR(rtc);
 	platform_set_drvdata(pdev, rtc);
 
+	/* Enable 1Hz events */
+	at91_rtc_write_ier(AT91_RTC_SECEV);
 	dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
 	return 0;
 }
-- 
1.8.3.2

RE: [PATCH] rtc: rtc-at91rm9200: fix infinite wait for ACKUPD irq

[Bryan Evenson]

Boris,

> -----Original Message-----
> From: Boris BREZILLON [mailto:boris.brezillon@free-electrons.com]
> Sent: Tuesday, May 06, 2014 10:28 AM
> To: Bryan Evenson
> Cc: Andrew Victor; Nicolas Ferre; Jean-Christophe Plagniol-Villard; linux-arm-
> kernel@lists.infradead.org; Alessandro Zummo; rtc-
> linux@googlegroups.com; linux-kernel@vger.kernel.org; Boris BREZILLON
> Subject: [PATCH] rtc: rtc-at91rm9200: fix infinite wait for ACKUPD irq
> 
> The rtc user must wait at least 1 sec between each time/calandar update
> (see atmel's datasheet chapter "Updating Time/Calendar").
> 
> Use the 1Hz interrupt to update the at91_rtc_upd_rdy flag and wait for
> the at91_rtc_wait_upd_rdy event if the rtc is not ready.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> Reported-by: Bryan Evenson <bevenson@melinkcorp.com>
> ---
> Hello Bryan,
> 
> I reproduced your bug (using your script) and this patch seems to fix the
> problem.
> 
> Could you try it and let me know if it works for you ?

Looks good to me.  I modified the test script as follows:

----------
#!/bin/sh
i=0
while [ 1 ]; do
  hwclock -w -u > /dev/null 2>&1
  echo $$ $i $?
  : $((i++))
done
----------

This version then attempts to write to the RTC as often as possible (script change was due to a suggestion on the Busybox mailing list).  I ran two instances of the script, which each looped through about 60,000 times over a 30 minute run.  At no point has access to the RTC been permanently locked out on my system.  I'd call this fixed.

I'd assume this patch would be backported to the longterm releases?

Regards,
Bryan

> 
> Best Regards,
> 
> Boris
> 
>  drivers/rtc/rtc-at91rm9200.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-at91rm9200.c b/drivers/rtc/rtc-at91rm9200.c
> index 3281c90..e3fe54c 100644
> --- a/drivers/rtc/rtc-at91rm9200.c
> +++ b/drivers/rtc/rtc-at91rm9200.c
> @@ -48,11 +48,13 @@ struct at91_rtc_config {
> 
>  static const struct at91_rtc_config *at91_rtc_config;
>  static DECLARE_COMPLETION(at91_rtc_updated);
> +static DECLARE_COMPLETION(at91_rtc_wait_upd_rdy);
>  static unsigned int at91_alarm_year = AT91_RTC_EPOCH;
>  static void __iomem *at91_rtc_regs;
>  static int irq;
>  static DEFINE_SPINLOCK(at91_rtc_lock);
>  static u32 at91_rtc_shadow_imr;
> +static bool at91_rtc_upd_rdy;
> 
>  static void at91_rtc_write_ier(u32 mask)
>  {
> @@ -161,6 +163,8 @@ static int at91_rtc_settime(struct device *dev, struct
> rtc_time *tm)
>  		1900 + tm->tm_year, tm->tm_mon, tm->tm_mday,
>  		tm->tm_hour, tm->tm_min, tm->tm_sec);
> 
> +	wait_for_completion(&at91_rtc_wait_upd_rdy);
> +
>  	/* Stop Time/Calendar from counting */
>  	cr = at91_rtc_read(AT91_RTC_CR);
>  	at91_rtc_write(AT91_RTC_CR, cr | AT91_RTC_UPDCAL |
> AT91_RTC_UPDTIM);
> @@ -183,6 +187,7 @@ static int at91_rtc_settime(struct device *dev, struct
> rtc_time *tm)
> 
>  	/* Restart Time/Calendar */
>  	cr = at91_rtc_read(AT91_RTC_CR);
> +	at91_rtc_upd_rdy = 0;
>  	at91_rtc_write(AT91_RTC_CR, cr & ~(AT91_RTC_UPDCAL |
> AT91_RTC_UPDTIM));
> 
>  	return 0;
> @@ -290,8 +295,13 @@ static irqreturn_t at91_rtc_interrupt(int irq, void
> *dev_id)
>  	if (rtsr) {		/* this interrupt is shared!  Is it ours? */
>  		if (rtsr & AT91_RTC_ALARM)
>  			events |= (RTC_AF | RTC_IRQF);
> -		if (rtsr & AT91_RTC_SECEV)
> +		if (rtsr & AT91_RTC_SECEV) {
>  			events |= (RTC_UF | RTC_IRQF);
> +			if (!at91_rtc_upd_rdy) {
> +				at91_rtc_upd_rdy = 1;
> +				complete(&at91_rtc_wait_upd_rdy);
> +			}
> +		}
>  		if (rtsr & AT91_RTC_ACKUPD)
>  			complete(&at91_rtc_updated);
> 
> @@ -413,6 +423,8 @@ static int __init at91_rtc_probe(struct
> platform_device *pdev)
>  		return PTR_ERR(rtc);
>  	platform_set_drvdata(pdev, rtc);
> 
> +	/* Enable 1Hz events */
> +	at91_rtc_write_ier(AT91_RTC_SECEV);
>  	dev_info(&pdev->dev, "AT91 Real Time Clock driver.\n");
>  	return 0;
>  }
> --
> 1.8.3.2

Re: [PATCH] rtc: rtc-at91rm9200: fix infinite wait for ACKUPD irq

[Boris BREZILLON]

Hi,

On 06/05/2014 21:06, Bryan Evenson wrote:
> Boris,
>
>> -----Original Message-----
>> From: Boris BREZILLON [mailto:boris.brezillon@free-electrons.com]
>> Sent: Tuesday, May 06, 2014 10:28 AM
>> To: Bryan Evenson
>> Cc: Andrew Victor; Nicolas Ferre; Jean-Christophe Plagniol-Villard; linux-arm-
>> kernel@lists.infradead.org; Alessandro Zummo; rtc-
>> linux@googlegroups.com; linux-kernel@vger.kernel.org; Boris BREZILLON
>> Subject: [PATCH] rtc: rtc-at91rm9200: fix infinite wait for ACKUPD irq
>>
>> The rtc user must wait at least 1 sec between each time/calandar update
>> (see atmel's datasheet chapter "Updating Time/Calendar").
>>
>> Use the 1Hz interrupt to update the at91_rtc_upd_rdy flag and wait for
>> the at91_rtc_wait_upd_rdy event if the rtc is not ready.
>>
>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
>> Reported-by: Bryan Evenson <bevenson@melinkcorp.com>
>> ---
>> Hello Bryan,
>>
>> I reproduced your bug (using your script) and this patch seems to fix the
>> problem.
>>
>> Could you try it and let me know if it works for you ?
> Looks good to me.  I modified the test script as follows:
>
> ----------
> #!/bin/sh
> i=0
> while [ 1 ]; do
>   hwclock -w -u > /dev/null 2>&1
>   echo $$ $i $?
>   : $((i++))
> done
> ----------
>
> This version then attempts to write to the RTC as often as possible (script change was due to a suggestion on the Busybox mailing list).  I ran two instances of the script, which each looped through about 60,000 times over a 30 minute run.  At no point has access to the RTC been permanently locked out on my system.  I'd call this fixed.

Great!

> I'd assume this patch would be backported to the longterm releases?

I'd like to wait for Nicolas' ack before asking for a backport to stable
releases.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH] rtc: rtc-at91rm9200: fix infinite wait for ACKUPD irq

[Alexandre Belloni]

Hi,

On 06/05/2014 at 19:06:56 +0000, Bryan Evenson wrote :
> 
> I'd assume this patch would be backported to the longterm releases?
> 

If by longterm, you mean the linux4sam tree, 3.10 branch, it is up to
Nicolas to take it. I believe it will be pretty easy to convince him ;)

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH] rtc: rtc-at91rm9200: fix infinite wait for ACKUPD irq

[Nicolas Ferre]

On 06/05/2014 23:10, Alexandre Belloni :
> Hi,
> 
> On 06/05/2014 at 19:06:56 +0000, Bryan Evenson wrote :
>>
>> I'd assume this patch would be backported to the longterm releases?
>>
> 
> If by longterm, you mean the linux4sam tree, 3.10 branch, it is up to
> Nicolas to take it. I believe it will be pretty easy to convince him ;)

Absolutely, it is always easy to convince me with:
- good commit message
- script to reproduce the issue
- follow-up of the patch plus testing
- dialogue between skilled kernel developers

So, yes, I will "ack" the patch and immediately integrate it in our
linux4sam 3.10 branch.

On the other hand, I let Boris add the "stable" tag to his original patch.

Thanks to all of you for this fix. Bye,
-- 
Nicolas Ferre