Messages in this thread Patch in this message | | | Date | Wed, 17 Oct 2007 23:18:32 -0400 | From | Mark Lord <> | Subject | [PATCH] Fix /proc/acpi/alarm BCD alarm encodings |
| |
>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 <mlord@pobox.com>
--- 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |