Linus Torvalds wrote:
On Wed, 26 Sep 2007, Mark Lord wrote:
The existing code in linux/drivers/acpi/sleep/proc.c has a nasty bug
that prevents BCD mode from working: the code converts binary to BCD
three times in a row, each time taking the previous result.
This thoroughly mangles the alarm timestamp, and never worked.
The patch below fixes it, by removing the first two (bogus)
conversions, and leaving only the final conversion in place.
Tested & working on my system here.
Actually, it cannot possibly really fix it.
That code is doing all the arithmetic in non-BCD mode, but the "adjust"
case doesn't do a BCD_TO_BIN() on that arithmetic.
In other words, the logic you removed was certainly total crap, but
there's more crap remaining inside the "if (adjust)" logic. I just suspect
you never tested that part (ie using a string that starts with '+').
In fact, for the non-broken case, the old broken code *should* have been a
no-op, since it did BIN_TO_BCD() followed by BCD_TO_BIN(). It then totally
stupidly seemed to expect that the "adjust" case would do a BCD addition!
So I don't even see why your patch should have mattered or worked!
So how about this cleanup instead? Does this work for you?
NOTE! I wanted to switch "acpi_system_alarm_seq_show()" over to this too,
but that code has a strange rule:
if (acpi_gbl_FADT.day_alarm)
/* ACPI spec: only low 6 its should be cared */
day = CMOS_READ(acpi_gbl_FADT.day_alarm) & 0x3F;
else
day = CMOS_READ(RTC_DAY_OF_MONTH);
and note the "& 0x3f" which is done in BCD mode. Strange. But we always
write zero (which may or may not be correct). So I'd have to add a mask to
the interface, and I decided it wasn't worth it until somebody talks about
how that thing actually works..
Linus, this is your patch from a few weeks ago.
It (still) solves problems for me here.
This should go into 2.6.24.
Fix BIN_TO_BCD()/BCD_TO_BIN() handling when setting the real-time alarm clock.
Signed-off-by: Mark Lord <[email protected]>
---
drivers/acpi/sleep/proc.c | 66 +++++++++++++++++++-------------------------
1 files changed, 29 insertions(+), 37 deletions(-)
diff --git a/drivers/acpi/sleep/proc.c b/drivers/acpi/sleep/proc.c
index 3839efd..1538355 100644
--- a/drivers/acpi/sleep/proc.c
+++ b/drivers/acpi/sleep/proc.c
@@ -194,6 +194,23 @@ static int get_date_field(char **p, u32 * value)
return result;
}
+/* Read a possibly BCD register, always return binary */
+static u32 cmos_bcd_read(int offset, int rtc_control)
+{
+ u32 val = CMOS_READ(offset);
+ if (!(rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
+ BCD_TO_BIN(val);
+ return val;
+}
+
+/* Write binary value into possibly BCD register */
+static void cmos_bcd_write(u32 val, int offset, int rtc_control)
+{
+ if (!(rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD)
+ BIN_TO_BCD(val);
+ CMOS_WRITE(val, offset);
+}
+
static ssize_t
acpi_system_write_alarm(struct file *file,
const char __user * buffer, size_t count, loff_t * ppos)
@@ -258,35 +275,18 @@ acpi_system_write_alarm(struct file *file,
spin_lock_irq(&rtc_lock);
rtc_control = CMOS_READ(RTC_CONTROL);
- if (!(rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
- BIN_TO_BCD(yr);
- BIN_TO_BCD(mo);
- BIN_TO_BCD(day);
- BIN_TO_BCD(hr);
- BIN_TO_BCD(min);
- BIN_TO_BCD(sec);
- }
if (adjust) {
- yr += CMOS_READ(RTC_YEAR);
- mo += CMOS_READ(RTC_MONTH);
- day += CMOS_READ(RTC_DAY_OF_MONTH);
- hr += CMOS_READ(RTC_HOURS);
- min += CMOS_READ(RTC_MINUTES);
- sec += CMOS_READ(RTC_SECONDS);
+ yr += cmos_bcd_read(RTC_YEAR, rtc_control);
+ mo += cmos_bcd_read(RTC_MONTH, rtc_control);
+ day += cmos_bcd_read(RTC_DAY_OF_MONTH, rtc_control);
+ hr += cmos_bcd_read(RTC_HOURS, rtc_control);
+ min += cmos_bcd_read(RTC_MINUTES, rtc_control);
+ sec += cmos_bcd_read(RTC_SECONDS, rtc_control);
}
spin_unlock_irq(&rtc_lock);
- if (!(rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
- BCD_TO_BIN(yr);
- BCD_TO_BIN(mo);
- BCD_TO_BIN(day);
- BCD_TO_BIN(hr);
- BCD_TO_BIN(min);
- BCD_TO_BIN(sec);
- }
-
if (sec > 59) {
min++;
sec -= 60;
@@ -307,14 +307,6 @@ acpi_system_write_alarm(struct file *file,
yr++;
mo -= 12;
}
- if (!(rtc_control & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
- BIN_TO_BCD(yr);
- BIN_TO_BCD(mo);
- BIN_TO_BCD(day);
- BIN_TO_BCD(hr);
- BIN_TO_BCD(min);
- BIN_TO_BCD(sec);
- }
spin_lock_irq(&rtc_lock);
/*
@@ -326,9 +318,9 @@ acpi_system_write_alarm(struct file *file,
CMOS_READ(RTC_INTR_FLAGS);
/* write the fields the rtc knows about */
- CMOS_WRITE(hr, RTC_HOURS_ALARM);
- CMOS_WRITE(min, RTC_MINUTES_ALARM);
- CMOS_WRITE(sec, RTC_SECONDS_ALARM);
+ cmos_bcd_write(hr, RTC_HOURS_ALARM, rtc_control);
+ cmos_bcd_write(min, RTC_MINUTES_ALARM, rtc_control);
+ cmos_bcd_write(sec, RTC_SECONDS_ALARM, rtc_control);
/*
* If the system supports an enhanced alarm it will have non-zero
@@ -336,11 +328,11 @@ acpi_system_write_alarm(struct file *file,
* to the RTC area of memory.
*/
if (acpi_gbl_FADT.day_alarm)
- CMOS_WRITE(day, acpi_gbl_FADT.day_alarm);
+ cmos_bcd_write(day, acpi_gbl_FADT.day_alarm, rtc_control);
if (acpi_gbl_FADT.month_alarm)
- CMOS_WRITE(mo, acpi_gbl_FADT.month_alarm);
+ cmos_bcd_write(mo, acpi_gbl_FADT.month_alarm, rtc_control);
if (acpi_gbl_FADT.century)
- CMOS_WRITE(yr / 100, acpi_gbl_FADT.century);
+ cmos_bcd_write(yr / 100, acpi_gbl_FADT.century, rtc_control);
/* enable the rtc alarm interrupt */
rtc_control |= RTC_AIE;
CMOS_WRITE(rtc_control, RTC_CONTROL);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
