Skip to contentSkip to conversation

check that acpi_wakeup_address is below 1MB

PATCH In review matthieu castet 3 participants 3 messages Feb 6, 2008

Subsystems (3)
x86 ACPI Suspend to RAM
lore
Share & tools
lore

matthieu castet

author
#0 lore raw

PATCH 1/1 check that acpi_wakeup_address is below 1MB

Hi,

this patch had a check that the memory allocated is in the first 1MB. The check is similar to the one in smp_alloc_memory.

Signed-off-by "Matthieu CASTET <[email protected]>"
arch/x86/kernel/acpi/sleep.c +4 -0
index 6bc815c..65ab23c 100644
@@ -65,6 +65,10 @@ void __init acpi_reserve_bootmem(void)
 	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE*2);
 	if (!acpi_wakeup_address)
 		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
+
+	/* check if we are in first 1MB of memory */
+	if (__pa(acpi_wakeup_address) >= 1024*1024-PAGE_SIZE*2)
+		BUG();
 }
#1 lore raw
Quoted 13 lines · matthieu castet

Hi,

this patch had a check that the memory allocated is in the first 1MB. The check is similar to the one in smp_alloc_memory.

Signed-off-by "Matthieu CASTET <[email protected]>"
arch/x86/kernel/acpi/sleep.c +0 -0
index 6bc815c..65ab23c 100644

arch/x86/kernel/acpi/sleep.c void __init acpi_reserve_bootmem(void) · line 71

Quoted 7 lines
arch/x86/kernel/acpi/sleep.c +4 -0
@@ -65,6 +65,10 @@ void __init acpi_reserve_bootmem(void)
 	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE*2);
 	if (!acpi_wakeup_address)
 		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
+
+	/* check if we are in first 1MB of memory */
+	if (__pa(acpi_wakeup_address) >= 1024*1024-PAGE_SIZE*2)
+		BUG();

please never BUG() out unless totally unavoidable - especially in such early bootup code - that might prevent people from being able to report anything beyond 'my bootup hung'.

print a WARN_ON() (that way kerneloops.org can pick it up), and perhaps disable ACPI sleep functionality. BUG()-ing out is way too drastic and way too user-unfriendly.

Ingo

#2 lore raw
In reply to matthieu castet
Quoted 1 line · matthieu castet

Hi,

Hi,

Quoted 5 lines

this patch had a check that the memory allocated is in the first 1MB. The check is similar to the one in smp_alloc_memory.

Signed-off-by "Matthieu CASTET <[email protected]>"

Please don't attach patches if possible. It's easier to comment them if they are in the message body.

arch/x86/kernel/acpi/sleep.c void __init acpi_reserve_bootmem(void) · line 71

Quoted 12 lines
arch/x86/kernel/acpi/sleep.c +4 -0
index 6bc815c..65ab23c 100644
@@ -65,6 +65,10 @@ void __init acpi_reserve_bootmem(void)
 	acpi_wakeup_address = (unsigned long)alloc_bootmem_low(PAGE_SIZE*2);
 	if (!acpi_wakeup_address)
 		printk(KERN_ERR "ACPI: Cannot allocate lowmem, S3 disabled.\n");
+
+	/* check if we are in first 1MB of memory */
+	if (__pa(acpi_wakeup_address) >= 1024*1024-PAGE_SIZE*2)
+		BUG();

I don't like this.

First, you should use BUG_ON(something) rather that "if (something) BUG();".

Second, and more importantly, it's enough to put "acpi_wakeup_address = 0" to disable the feature without crashing the kernel.
Of course, I'd add an error message describing what's wrong to that.

arch/x86/kernel/acpi/sleep.c void __init acpi_reserve_bootmem(void) · line 72

Quoted 1 line
arch/x86/kernel/acpi/sleep.c +0 -0
 }

Thanks,
Rafael

Signature · 1 line
"Premature optimization is the root of all evil." - Donald Knuth