Skip to contentSkip to conversation

RTC seems broken on 2.6.38-rc1 for RTCs lacking alarms.

DISCUSSION David Daney 4 participants 6 messages Jan 20, 2011 – Jan 21, 2011 Real time clock (RTC)

lore
Share & tools
lore

David Daney

author
#0 lore raw

The hwclock program on my MIPS Debian GNU/Linux 5.0 system quit working with 2.6.38-rc1.

This particular ds1307 has no interrupt connection, so the alarm feature cannot be used. Because of this the rtc_class_ops.set_alarm() function always will return -EINVAL

This problem appears to be related to commits:
042620a RTC: Remove UIE emulation
6610e08 RTC: Rework RTC code to use timerqueue for events

My system uses a ds1307 RTC which still works as evidenced by the kernel boot messages.

.
.
.
rtc-ds1307 0-0068: setting system clock to 2011-01-20 19:58:48 UTC (1295553528) .
.
.

But hwclock now fails here is some nice strace output:
.
.
.
open("/dev/rtc0", O_RDONLY|O_LARGEFILE) = 3
ioctl(3, PRESTO_GETMOUNT or RTC_UIE_ON, 0) = 0
_newselect(4, [3], NULL, NULL, {5, 0}) = 0 (Timeout)
write(2, "select() to /dev/rtc0 to wait for"..., 55select() to /dev/rtc0 to wait for clock tick timed out ) = 55
ioctl(3, PRESTO_SETPID or RTC_UIE_OFF, 0) = 0
close(3) = 0
exit_group(1) = ?

The hwclock program is asking to put the clock in UIE mode and then does a select() on it. Since the alarm doesn't work, the select times out.

Previously the ioctl(RTC_UIE_ON) would return EINVAL:
.
.
.
open("/dev/rtc0", O_RDONLY|O_LARGEFILE) = 3
ioctl(3, PRESTO_GETMOUNT or RTC_UIE_ON, 0) = -1 EINVAL (Invalid argument)
ioctl(3, PRESTO_SETOPT or RTC_RD_TIME, {tm_sec=48, tm_min=41, tm_hour=20, tm_mday=20, tm_mon=0, tm_year=111, ...}) = 0 ioctl(3, PRESTO_SETOPT or RTC_RD_TIME, {tm_sec=48, tm_min=41, tm_hour=20, tm_mday=20, tm_mon=0, tm_year=111, ...}) = 0 .
.
.

The problem seems to be that rtc_update_irq_enable() no longer returns -EINVAL when it cannot set the alarm.

David Daney

#1 lore raw
Quoted 11 lines · David Daney

open("/dev/rtc0", O_RDONLY|O_LARGEFILE) = 3
ioctl(3, PRESTO_GETMOUNT or RTC_UIE_ON, 0) = 0
_newselect(4, [3], NULL, NULL, {5, 0}) = 0 (Timeout) write(2, "select() to /dev/rtc0 to wait for"..., 55select() to /dev/rtc0 to wait for clock tick timed out
) = 55
ioctl(3, PRESTO_SETPID or RTC_UIE_OFF, 0) = 0
close(3) = 0
exit_group(1) = ?

Quoted 4 lines

The hwclock program is asking to put the clock in UIE mode and then does a select() on it. Since the alarm doesn't work, the select times out.

Previously the ioctl(RTC_UIE_ON) would return EINVAL:

Ah. Good diagnosis! Let me try to get a patch for you and Andreas to test.

thanks
-john

#2 lore raw
Quoted 12 lines · John Stultz
Quoted 11 lines · David Daney

open("/dev/rtc0", O_RDONLY|O_LARGEFILE) = 3
ioctl(3, PRESTO_GETMOUNT or RTC_UIE_ON, 0) = 0
_newselect(4, [3], NULL, NULL, {5, 0})  = 0 (Timeout) write(2, "select() to /dev/rtc0 to wait for"..., 55select() to /dev/rtc0 to wait for clock tick timed out
) = 55
ioctl(3, PRESTO_SETPID or RTC_UIE_OFF, 0) = 0
close(3)                                = 0
exit_group(1)                           = ?

Quoted 7 lines
Quoted 4 lines

The hwclock program is asking to put the clock in UIE mode and then does a select() on it.  Since the alarm doesn't work, the select times out.

Previously the ioctl(RTC_UIE_ON) would return EINVAL:

Ah. Good diagnosis! Let me try to get a patch for you and Andreas to test.

I'm also seeing this on m68k (ARAnyM, rtc-generic).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds

#3 lore raw
Quoted 16 lines · Geert Uytterhoeven
Quoted 15 lines · John Stultz
Quoted 14 lines · David Daney

open("/dev/rtc0", O_RDONLY|O_LARGEFILE) = 3
ioctl(3, PRESTO_GETMOUNT or RTC_UIE_ON, 0) = 0
_newselect(4, [3], NULL, NULL, {5, 0}) = 0 (Timeout) write(2, "select() to /dev/rtc0 to wait for"..., 55select() to /dev/rtc0 to wait for clock tick timed out
) = 55
ioctl(3, PRESTO_SETPID or RTC_UIE_OFF, 0) = 0
close(3) = 0
exit_group(1) = ?

The hwclock program is asking to put the clock in UIE mode and then does a select() on it. Since the alarm doesn't work, the select times out.

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

Previously the ioctl(RTC_UIE_ON) would return EINVAL:

Ah. Good diagnosis! Let me try to get a patch for you and Andreas to test.

I'm also seeing this on m68k (ARAnyM, rtc-generic).

Geert, David, Andreas,
Could you try the following? Its a bit messy of a patch doing a couple
of things:

1) Simplify the timer->enabled management by pushing it into rtc_timer_enqueue/remove (needed cleanup for #2).

2) Properly propagating errors from __rtc_set_alarm back through rtc_timer_enqueue and users.

3) Trivial clenaup making rtc_timer_enqueue/remove static.

4) Fixup virtualized rtc_read_alarm to check hardware capabilities and return errors (also restores zeroing of the rtc_wkalrm stucture).

I'll be cleaning these up and breaking them into commits I can send upward, but I wanted to make sure it resolves the issue for you.

Let me know if it fixes things.

thanks
-john

NOT FOR INCLUSION! NOT FOR INCLUSION! NOT FOR INCLUSION!

Signed-off-by John Stultz <[email protected]>
drivers/rtc/interface.c +40 -25
index 90384b9..db816cd 100644
@@ -16,6 +16,9 @@
 #include <linux/log2.h>
 #include <linux/workqueue.h>
 
+static int rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer);
+static void rtc_timer_remove(struct rtc_device *rtc, struct rtc_timer *timer);
+
 static int __rtc_read_time(struct rtc_device *rtc, struct rtc_time *tm)
 {
 	int err;

@@ -120,12 +123,20 @@ int rtc_read_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
 	err = mutex_lock_interruptible(&rtc->ops_lock);
 	if (err)
 		return err;
-	alarm->enabled = rtc->aie_timer.enabled;
-	if (alarm->enabled)
-		alarm->time = rtc_ktime_to_tm(rtc->aie_timer.node.expires);
+	if (rtc->ops == NULL)
+		err = -ENODEV;
+	else if (!rtc->ops->read_alarm)
+		err = -EINVAL;
+	else {
+		memset(alarm, 0, sizeof(struct rtc_wkalrm));
+		alarm->enabled = rtc->aie_timer.enabled;
+		if (alarm->enabled)
+			alarm->time =
+				rtc_ktime_to_tm(rtc->aie_timer.node.expires);
+	}
 	mutex_unlock(&rtc->ops_lock);
 
-	return 0;
+	return err;
 }
 EXPORT_SYMBOL_GPL(rtc_read_alarm);
 

@@ -175,16 +186,14 @@ int rtc_set_alarm(struct rtc_device *rtc, struct rtc_wkalrm *alarm)
 		return err;
 	if (rtc->aie_timer.enabled) {
 		rtc_timer_remove(rtc, &rtc->aie_timer);
-		rtc->aie_timer.enabled = 0;
 	}
 	rtc->aie_timer.node.expires = rtc_tm_to_ktime(alarm->time);
 	rtc->aie_timer.period = ktime_set(0, 0);
 	if (alarm->enabled) {
-		rtc->aie_timer.enabled = 1;
-		rtc_timer_enqueue(rtc, &rtc->aie_timer);
+		err = rtc_timer_enqueue(rtc, &rtc->aie_timer);
 	}
 	mutex_unlock(&rtc->ops_lock);
-	return 0;
+	return err;
 }
 EXPORT_SYMBOL_GPL(rtc_set_alarm);
 

@@ -195,15 +204,15 @@ int rtc_alarm_irq_enable(struct rtc_device *rtc, unsigned int enabled)
 		return err;
 
 	if (rtc->aie_timer.enabled != enabled) {
-		if (enabled) {
-			rtc->aie_timer.enabled = 1;
-			rtc_timer_enqueue(rtc, &rtc->aie_timer);
-		} else {
+		if (enabled)
+			err = rtc_timer_enqueue(rtc, &rtc->aie_timer);
+		else
 			rtc_timer_remove(rtc, &rtc->aie_timer);
-			rtc->aie_timer.enabled = 0;
-		}
 	}
 
+	if (err)
+		return err;
+
 	if (!rtc->ops)
 		err = -ENODEV;
 	else if (!rtc->ops->alarm_irq_enable)

@@ -235,12 +244,9 @@ int rtc_update_irq_enable(struct rtc_device *rtc, unsigned int enabled)
 		now = rtc_tm_to_ktime(tm);
 		rtc->uie_rtctimer.node.expires = ktime_add(now, onesec);
 		rtc->uie_rtctimer.period = ktime_set(1, 0);
-		rtc->uie_rtctimer.enabled = 1;
-		rtc_timer_enqueue(rtc, &rtc->uie_rtctimer);
-	} else {
+		err = rtc_timer_enqueue(rtc, &rtc->uie_rtctimer);
+	} else
 		rtc_timer_remove(rtc, &rtc->uie_rtctimer);
-		rtc->uie_rtctimer.enabled = 0;
-	}
 
 out:
 	mutex_unlock(&rtc->ops_lock);

@@ -488,10 +494,13 @@ EXPORT_SYMBOL_GPL(rtc_irq_set_freq);
  * Enqueues a timer onto the rtc devices timerqueue and sets
  * the next alarm event appropriately.
  *
+ * Sets the enabled bit on the added timer.
+ *
  * Must hold ops_lock for proper serialization of timerqueue
  */
-void rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer)
+static int rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer)
 {
+	timer->enabled = 1;
 	timerqueue_add(&rtc->timerqueue, &timer->node);
 	if (&timer->node == timerqueue_getnext(&rtc->timerqueue)) {
 		struct rtc_wkalrm alarm;

@@ -501,7 +510,13 @@ void rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer)
 		err = __rtc_set_alarm(rtc, &alarm);
 		if (err == -ETIME)
 			schedule_work(&rtc->irqwork);
+		else if (err) {
+			timerqueue_del(&rtc->timerqueue, &timer->node);
+			timer->enabled = 0;
+			return err;
+		}
 	}
+	return 0;
 }
 
 /**

@@ -512,13 +527,15 @@ void rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer)
  * Removes a timer onto the rtc devices timerqueue and sets
  * the next alarm event appropriately.
  *
+ * Clears the enabled bit on the removed timer.
+ *
  * Must hold ops_lock for proper serialization of timerqueue
  */
-void rtc_timer_remove(struct rtc_device *rtc, struct rtc_timer *timer)
+static void rtc_timer_remove(struct rtc_device *rtc, struct rtc_timer *timer)
 {
 	struct timerqueue_node *next = timerqueue_getnext(&rtc->timerqueue);
 	timerqueue_del(&rtc->timerqueue, &timer->node);
-
+	timer->enabled = 0;
 	if (next == &timer->node) {
 		struct rtc_wkalrm alarm;
 		int err;

@@ -626,8 +643,7 @@ int rtc_timer_start(struct rtc_device *rtc, struct rtc_timer* timer,
 	timer->node.expires = expires;
 	timer->period = period;
 
-	timer->enabled = 1;
-	rtc_timer_enqueue(rtc, timer);
+	ret = rtc_timer_enqueue(rtc, timer);
 
 	mutex_unlock(&rtc->ops_lock);
 	return ret;

@@ -645,7 +661,6 @@ int rtc_timer_cancel(struct rtc_device *rtc, struct rtc_timer* timer)
 	mutex_lock(&rtc->ops_lock);
 	if (timer->enabled)
 		rtc_timer_remove(rtc, timer);
-	timer->enabled = 0;
 	mutex_unlock(&rtc->ops_lock);
 	return ret;
 }
include/linux/rtc.h +0 -2
index 3c995b4..bd4fbc3 100644
@@ -246,8 +246,6 @@ int rtc_register(rtc_task_t *task);
 int rtc_unregister(rtc_task_t *task);
 int rtc_control(rtc_task_t *t, unsigned int cmd, unsigned long arg);
 
-void rtc_timer_enqueue(struct rtc_device *rtc, struct rtc_timer *timer);
-void rtc_timer_remove(struct rtc_device *rtc, struct rtc_timer *timer);
 void rtc_timer_init(struct rtc_timer *timer, void (*f)(void* p), void* data);
 int rtc_timer_start(struct rtc_device *rtc, struct rtc_timer* timer,
 			ktime_t expires, ktime_t period);
author Tested-by
#4 lore raw
Quoted 39 lines · John Stultz
Quoted 16 lines · Geert Uytterhoeven
Quoted 15 lines · John Stultz
Quoted 14 lines · David Daney

open("/dev/rtc0", O_RDONLY|O_LARGEFILE) = 3
ioctl(3, PRESTO_GETMOUNT or RTC_UIE_ON, 0) = 0
_newselect(4, [3], NULL, NULL, {5, 0}) = 0 (Timeout) write(2, "select() to /dev/rtc0 to wait for"..., 55select() to /dev/rtc0 to wait for clock tick timed out
) = 55
ioctl(3, PRESTO_SETPID or RTC_UIE_OFF, 0) = 0
close(3) = 0
exit_group(1) = ?

The hwclock program is asking to put the clock in UIE mode and then does a select() on it. Since the alarm doesn't work, the select times out.

Quoted 6 lines
Quoted 4 lines
Quoted 1 line

Previously the ioctl(RTC_UIE_ON) would return EINVAL:

Ah. Good diagnosis! Let me try to get a patch for you and Andreas to test.

I'm also seeing this on m68k (ARAnyM, rtc-generic).

Geert, David, Andreas,
Could you try the following? Its a bit messy of a patch doing a couple
of things:

1) Simplify the timer->enabled management by pushing it into rtc_timer_enqueue/remove (needed cleanup for #2).

2) Properly propagating errors from __rtc_set_alarm back through rtc_timer_enqueue and users.

3) Trivial clenaup making rtc_timer_enqueue/remove static.

4) Fixup virtualized rtc_read_alarm to check hardware capabilities and return errors (also restores zeroing of the rtc_wkalrm stucture).

Quoted 8 lines

I'll be cleaning these up and breaking them into commits I can send upward, but I wanted to make sure it resolves the issue for you.

Let me know if it fixes things.

thanks
-john

John,

With that patch, it works now. Thanks.

You can add:

Tested-by David Daney <[email protected]>
#5 lore raw
Quoted 3 lines · David Daney

John,

With that patch, it works now. Thanks.

Works here as well.

Thanks, Andreas.

Signature · 3 lines
Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."