Skip to contentSkip to conversation

Fix /proc/acpi/alarm BCD alarm encodings

PATCH In review Mark Lord 3 participants 4 messages Oct 18, 2007 – Oct 25, 2007 ACPI

lore
Share & tools
lore

Mark Lord

author
#0 lore raw

PATCH 1/1 Fix /proc/acpi/alarm BCD alarm encodings

Quoted 25 lines

Linus Torvalds wrote:

Quoted 9 lines · Mark Lord

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:

Quoted 7 lines
       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(-)
drivers/acpi/sleep/proc.c +0 -0
index 3839efd..1538355 100644
@@ -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);
#1 lore raw
Quoted 10 lines · On Wed, 17 Oct 2007 23:18:32 -0400 Mark Lord <[email protected]> wrote:
Quoted 4 lines

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.

This patch is presently (stuck) in -mm. I'll move it to my must-be-in-2.6.24-even-if-the-maintainer-fluffs-it queue.

#2 lore raw
Quoted 8 lines · Andrew Morton
Quoted 5 lines · On Wed, 17 Oct 2007 23:18:32 -0400 Mark Lord <[email protected]> wrote:
Quoted 4 lines

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

Quoted 8 lines
Quoted 5 lines

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.

This patch is presently (stuck) in -mm. I'll move it to my must-be-in-2.6.24-even-if-the-maintainer-fluffs-it queue.

I noticed that apparently this never happened, and it didn't go into -rc1.

I'll put it there myself, since I'm the author and Mark tested it, and the old code was definitely totally buggy. The earlier it gets in, the better, in case there are cleanups and/or other issues.

Linus

#3 lore raw
Quoted 18 lines · On Thu, 25 Oct 2007 15:04:18 -0700 (PDT) Linus Torvalds…
Quoted 14 lines · Andrew Morton
Quoted 10 lines · On Wed, 17 Oct 2007 23:18:32 -0400 Mark Lord <[email protected]> wrote:
Quoted 4 lines

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.

Quoted 9 lines
Quoted 2 lines

This patch is presently (stuck) in -mm. I'll move it to my must-be-in-2.6.24-even-if-the-maintainer-fluffs-it queue.

I noticed that apparently this never happened, and it didn't go into -rc1.

I'll put it there myself, since I'm the author and Mark tested it, and the old code was definitely totally buggy. The earlier it gets in, the better, in case there are cleanups and/or other issues.

I'll send over my copy. I don't think it changed.