David Brownell wrote:
On Saturday 03 November 2007, Mark Lord wrote:
David Brownell wrote:
..
While you're fixing goofs in this general area, you might make that
/proc/acpi/alarm file not write the century when it changes the
alarm. That field isn't relevant to the alarm triggering.
You mean like this?
Yes, but you found another little nest of bugs. I'm
surprised this little bit of ACPI code is so buggy!!
I Cc'd Len Brown; ACPI patches should be submitted
through the ACPI team, as a rule. Even for code they
are slowly getting rid of. ;)
This patch (against 2.6.24-rc1-git12) fixes /proc/acpi/alarm to not
overwrite the RTC century field when setting a wake alarm.
The alarm hardware doesn't have a century field, and we really
should not be fiddling with the RTC century field here.
Signed-off-by: Mark Lord <mlord@xxxxxxxxx>
---
--- a/drivers/acpi/sleep/proc.c 2007-11-03 16:24:15.000000000 -0400
+++ b/drivers/acpi/sleep/proc.c 2007-11-03 22:10:14.000000000 -0400
@@ -268,7 +268,6 @@
day -= 31;
}
if (mo > 12) {
- yr += 1;
This is an unrelated nest of bugs; I'd not change that with
this patch. I think that whole block of code trying to fix
..
I'm not trying to fix anything other than removing the write
to the century register (near the bottom). However, once that
write has been removed, there is no other use for the "yr",
so the rest of the patch is simply getting rid of it everywhere
where it looked safe to do so.
..
broken alarm times is dubious, and I can't see why broken
values shouldn't just cause an "-EINVAL" return. Notice it
accepts illegal dates like November 31 (or February 30) too..
mo -= 12;
}
@@ -277,7 +276,6 @@
rtc_control = CMOS_READ(RTC_CONTROL);
if (adjust) {
- 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);
@@ -304,7 +302,6 @@
day -= 31;
}
if (mo > 12) {
- yr++;
Not the same. This block of code would make more sense if
it were in that "if (adjust) ..." block. If the preceding
block returns an error, this one will become a NOP unless
the alarm was adjusted; it's more just ugly code than a
set of bugs.
However ... this code is missing range validation checks.
If I try to set an alarm two days in the future, and this
chip only supports 24 hour alarms (FADT.day_alarm is zero),
that would seem to be a case where this legacy code should
report an error. Likewise with two months in the future
(with FADT.month_alarm or FADT.day_alarm zero) ... or over
one year from now, in every case.
mo -= 12;
}
@@ -331,8 +328,6 @@
cmos_bcd_write(day, acpi_gbl_FADT.day_alarm, rtc_control);
if (acpi_gbl_FADT.month_alarm)
cmos_bcd_write(mo, acpi_gbl_FADT.month_alarm, rtc_control);
- if (acpi_gbl_FADT.century)
- cmos_bcd_write(yr / 100, acpi_gbl_FADT.century, rtc_control);
This is what I was referring to, and that change is correct.
You're right to strip out use of the "year" field entirely,
unless range validation gets added.
/* 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-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
