[PATCH] [suspend/resume] wrong wakeup address

[PATCH] [suspend/resume] wrong wakeup address

[Li, Shaohua]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

Hi,
acpi_wakeup_address is returned by 'alloc_bootmem_low', so it's a
virtual address, but 'acpi_set_firmware_wakeing_vector' requires a
physical address. But what makes me confused is my Toshiba works (S3)
with/without the patch. Hope this patch (against 2.6.9-rc4) fixes some
systems which can't resume.

Thanks,
Shaohua

Signed-off-by Li Shaohua <Shaohua.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

===== drivers/acpi/sleep/main.c 1.40 vs edited =====
--- 1.40/drivers/acpi/sleep/main.c	2004-08-17 23:15:34 +08:00
+++ edited/drivers/acpi/sleep/main.c	2004-10-15 16:58:33 +08:00
@@ -12,6 +12,7 @@
 #include <linux/irq.h>
 #include <linux/dmi.h>
 #include <linux/device.h>
+#include <asm/io.h>
 #include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -56,7 +57,8 @@ static int acpi_pm_prepare(u32 pm_state)
 		if (!acpi_wakeup_address)
 			return -EFAULT;
 		acpi_set_firmware_waking_vector(
-			(acpi_physical_address) acpi_wakeup_address);
+			(acpi_physical_address) virt_to_phys(
+			(void *)acpi_wakeup_address));
 	}
 	ACPI_FLUSH_CPU_CACHE();
 	acpi_enable_wakeup_device_prep(acpi_state);

[-- Attachment #2: wakeup_addr.patch --]
[-- Type: application/octet-stream, Size: 728 bytes --]

===== drivers/acpi/sleep/main.c 1.40 vs edited =====
--- 1.40/drivers/acpi/sleep/main.c	2004-08-17 23:15:34 +08:00
+++ edited/drivers/acpi/sleep/main.c	2004-10-15 16:58:33 +08:00
@@ -12,6 +12,7 @@
 #include <linux/irq.h>
 #include <linux/dmi.h>
 #include <linux/device.h>
+#include <asm/io.h>
 #include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -56,7 +57,8 @@ static int acpi_pm_prepare(u32 pm_state)
 		if (!acpi_wakeup_address)
 			return -EFAULT;
 		acpi_set_firmware_waking_vector(
-			(acpi_physical_address) acpi_wakeup_address);
+			(acpi_physical_address) virt_to_phys(
+			(void *)acpi_wakeup_address));
 	}
 	ACPI_FLUSH_CPU_CACHE();
 	acpi_enable_wakeup_device_prep(acpi_state);

Re: [PATCH] [suspend/resume] wrong wakeup address

[Pavel Machek]

Hi!

> acpi_wakeup_address is returned by 'alloc_bootmem_low', so it's a
> virtual address, but 'acpi_set_firmware_wakeing_vector' requires a
> physical address. But what makes me confused is my Toshiba works (S3)
> with/without the patch. Hope this patch (against 2.6.9-rc4) fixes some
> systems which can't resume.

Could you verify that address passed to firmware_waking_vector() is
actually different? If the toshiba works with wrong return
address... perhaps only high bits differ and toshiba just ignores
them?

Anyway it could explain quite a lot of failures.

								Pavel 

> Thanks,
> Shaohua
> 
> Signed-off-by Li Shaohua <Shaohua.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> ===== drivers/acpi/sleep/main.c 1.40 vs edited =====
> --- 1.40/drivers/acpi/sleep/main.c	2004-08-17 23:15:34 +08:00
> +++ edited/drivers/acpi/sleep/main.c	2004-10-15 16:58:33 +08:00
> @@ -12,6 +12,7 @@
>  #include <linux/irq.h>
>  #include <linux/dmi.h>
>  #include <linux/device.h>
> +#include <asm/io.h>
>  #include <linux/suspend.h>
>  #include <acpi/acpi_bus.h>
>  #include <acpi/acpi_drivers.h>
> @@ -56,7 +57,8 @@ static int acpi_pm_prepare(u32 pm_state)
>  		if (!acpi_wakeup_address)
>  			return -EFAULT;
>  		acpi_set_firmware_waking_vector(
> -			(acpi_physical_address) acpi_wakeup_address);
> +			(acpi_physical_address) virt_to_phys(
> +			(void *)acpi_wakeup_address));
>  	}
>  	ACPI_FLUSH_CPU_CACHE();
>  	acpi_enable_wakeup_device_prep(acpi_state);



-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

Re: [PATCH] [suspend/resume] wrong wakeup address

[ole.rohne-vJEk5272eHo]

Pavel> Could you verify that address passed to firmware_waking_vector() is
Pavel> actually different? 

I haven't checked lately, but I think without any fancy BIGMEM
options, the difference is 0xc0000000, only affecting the upper 4
bits.

Pavel> If the toshiba works with wrong return address... perhaps only
Pavel> high bits differ and toshiba just ignores them?

IUC, a (32bit) ACPI implementation should use bits 4-19 for CS
and bits 0-3 for IP for the real-mode wake-up address.

Ole



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

Re: [PATCH] [suspend/resume] wrong wakeup address

[Hiroshi 2 Itoh]

Hi, Shaohua-san,

I had a glance at your patch and zapped also the codes around ACPI sleep
area.

acpi_wakeup_address is the entry address of wakeup_start() routine jumped
from firmware, which is switching from real mode to protected mode. So its
virtual and physical address must be identical for switching.  See "9.9
Mode Swtching" in IA-32 Intel Architecture Software Developer's Manual.

Can acpi_reserve_bootmem() routine reserve such identical memory to switch
the processor mode?

If so, the virt_to_phys implementation is not required. While if not,
init_low_mapping() treating page directory in acpi_save_state_mem() and
your virt_to_phys() patch are working in pair.

After adding your patch, both are no longer independent each other. I
wonder why waking vector setting is in acpi_pm_prepare(), not in
acpi_pm_enter() with acpi_save_state_mem().

Thanks for your patch.
Hiroshi Itoh
hiroit-JE5g2YyFxFHQT0dZR+AlfA@public.gmane.org



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

Re: [PATCH] [suspend/resume] wrong wakeup address

[Hiroshi 2 Itoh]

[-- Attachment #1: Type: text/plain, Size: 3241 bytes --]





Hi, Shaohua-san,

I checked acpi_wakeup_address and
*acpi_gbl_common.fACS.firmware_waking_vector after virt_to_phys().
Both values are 0xC0002000 (width=32bit) on my ThinkPad. This means BIOS
must call the wakeup entry in big-real mode?

If so, the following comment in wakeup.S seems to be strange.
# If physical address of wakeup_code is 0x12345, BIOS should call us with
# cs = 0x1234, eip = 0x05

Hiroshi Itoh
hiroit-JE5g2YyFxFHQT0dZR+AlfA@public.gmane.org


                                                                           
             "Li, Shaohua"                                                 
             <shaohua.li@intel                                             
             .com>                                                      To 
             Sent by:                  <acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>  
             acpi-devel-admin@                                          cc 
             lists.sourceforge         "Brown, Len" <len.brown-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, 
             .net                      "Pavel Machek" <pavel-+ZI9xUNit7I@public.gmane.org>       
                                                                   Subject 
                                       [ACPI] [PATCH] [suspend/resume]     
             2004/10/15 18:21          wrong wakeup address                
                                                                           
                                                                           
                                                                           
                                                                           
                                                                           
                                                                           




Hi,
acpi_wakeup_address is returned by 'alloc_bootmem_low', so it's a
virtual address, but 'acpi_set_firmware_wakeing_vector' requires a
physical address. But what makes me confused is my Toshiba works (S3)
with/without the patch. Hope this patch (against 2.6.9-rc4) fixes some
systems which can't resume.

Thanks,
Shaohua

Signed-off-by Li Shaohua <Shaohua.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

===== drivers/acpi/sleep/main.c 1.40 vs edited =====
--- 1.40/drivers/acpi/sleep/main.c         2004-08-17 23:15:34 +08:00
+++ edited/drivers/acpi/sleep/main.c             2004-10-15 16:58:33 +08:00
@@ -12,6 +12,7 @@
 #include <linux/irq.h>
 #include <linux/dmi.h>
 #include <linux/device.h>
+#include <asm/io.h>
 #include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -56,7 +57,8 @@ static int acpi_pm_prepare(u32 pm_state)
                         if (!acpi_wakeup_address)
                                     return -EFAULT;
                         acpi_set_firmware_waking_vector(
-                                    (acpi_physical_address)
acpi_wakeup_address);
+                                    (acpi_physical_address) virt_to_phys(
+                                    (void *)acpi_wakeup_address));
             }
             ACPI_FLUSH_CPU_CACHE();
             acpi_enable_wakeup_device_prep(acpi_state);
(See attached file: wakeup_addr.patch)

[-- Attachment #2: wakeup_addr.patch --]
[-- Type: application/octet-stream, Size: 728 bytes --]

===== drivers/acpi/sleep/main.c 1.40 vs edited =====
--- 1.40/drivers/acpi/sleep/main.c	2004-08-17 23:15:34 +08:00
+++ edited/drivers/acpi/sleep/main.c	2004-10-15 16:58:33 +08:00
@@ -12,6 +12,7 @@
 #include <linux/irq.h>
 #include <linux/dmi.h>
 #include <linux/device.h>
+#include <asm/io.h>
 #include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
@@ -56,7 +57,8 @@ static int acpi_pm_prepare(u32 pm_state)
 		if (!acpi_wakeup_address)
 			return -EFAULT;
 		acpi_set_firmware_waking_vector(
-			(acpi_physical_address) acpi_wakeup_address);
+			(acpi_physical_address) virt_to_phys(
+			(void *)acpi_wakeup_address));
 	}
 	ACPI_FLUSH_CPU_CACHE();
 	acpi_enable_wakeup_device_prep(acpi_state);

RE: [PATCH] [suspend/resume] wrong wakeup address

[Li, Shaohua]

Yes, I guess the BIOS ignored the high bits. The difference is
unsurprised PAGE_OFFSET. Somebody has verified the patch helps his
system.
http://bugme.osdl.org/show_bug.cgi?id=3390


Thanks,
Shaohua

>-----Original Message-----
>From: Pavel Machek [mailto:pavel-+ZI9xUNit7I@public.gmane.org]
>Sent: Friday, October 15, 2004 7:16 PM
>To: Li, Shaohua
>Cc: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Brown, Len
>Subject: Re: [PATCH] [suspend/resume] wrong wakeup address
>
>Hi!
>
>> acpi_wakeup_address is returned by 'alloc_bootmem_low', so it's a
>> virtual address, but 'acpi_set_firmware_wakeing_vector' requires a
>> physical address. But what makes me confused is my Toshiba works (S3)
>> with/without the patch. Hope this patch (against 2.6.9-rc4) fixes
some
>> systems which can't resume.
>
>Could you verify that address passed to firmware_waking_vector() is
>actually different? If the toshiba works with wrong return
>address... perhaps only high bits differ and toshiba just ignores
>them?
>
>Anyway it could explain quite a lot of failures.
>
>								Pavel
>
>> Thanks,
>> Shaohua
>>
>> Signed-off-by Li Shaohua <Shaohua.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>> ===== drivers/acpi/sleep/main.c 1.40 vs edited =====
>> --- 1.40/drivers/acpi/sleep/main.c	2004-08-17 23:15:34 +08:00
>> +++ edited/drivers/acpi/sleep/main.c	2004-10-15 16:58:33 +08:00
>> @@ -12,6 +12,7 @@
>>  #include <linux/irq.h>
>>  #include <linux/dmi.h>
>>  #include <linux/device.h>
>> +#include <asm/io.h>
>>  #include <linux/suspend.h>
>>  #include <acpi/acpi_bus.h>
>>  #include <acpi/acpi_drivers.h>
>> @@ -56,7 +57,8 @@ static int acpi_pm_prepare(u32 pm_state)
>>  		if (!acpi_wakeup_address)
>>  			return -EFAULT;
>>  		acpi_set_firmware_waking_vector(
>> -			(acpi_physical_address) acpi_wakeup_address);
>> +			(acpi_physical_address) virt_to_phys(
>> +			(void *)acpi_wakeup_address));
>>  	}
>>  	ACPI_FLUSH_CPU_CACHE();
>>  	acpi_enable_wakeup_device_prep(acpi_state);
>
>
>
>--
>People were complaining that M$ turns users into beta-testers...
>...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

RE: [PATCH] [suspend/resume] wrong wakeup address

[Li, Shaohua]

>I checked acpi_wakeup_address and
>*acpi_gbl_common.fACS.firmware_waking_vector after virt_to_phys().
>Both values are 0xC0002000 (width=32bit) on my ThinkPad. This means
BIOS
>must call the wakeup entry in big-real mode?
The address with the patch is still 0xC0002000? How do you test it? The
patch doesn't change the address of acpi_wakeup_address, but fACS..
should be changed. The patch just tells BIOS a correct physical address.

As for your another email, the purpose of the patch isn't to change the
position of wakeup code, again, it just tells BIOS the physical address.
In the first resume time, there is no virtual/physical memory mapping. 

Thanks,
Shaohua


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

RE: [PATCH] [suspend/resume] wrong wakeup address

[Hiroshi 2 Itoh]

Shaohua-san,

>>I checked acpi_wakeup_address and
>>*acpi_gbl_common.fACS.firmware_waking_vector after virt_to_phys().
>>Both values are 0xC0002000 (width=32bit) on my ThinkPad. This means BIOS
>>must call the wakeup entry in big-real mode?
>The address with the patch is still 0xC0002000? How do you test it? The
>patch doesn't change the address of acpi_wakeup_address, but fACS..
>should be changed. The patch just tells BIOS a correct physical address.

I verified acpi_wakeup_address and
*acpi_gbl_common.fACS.firmware_waking_vector in
acpi_set_firmware_waking_vector().
(This is at suspend time.) The both values were 0xC0002000.

While I verified virt_to_phys(acpi_wakeup_address) in
acpi_reserve_bootmem() after calling alloc_bootmem_low().
(This is at boot time.) The value was correctly 0x00002000.

It is quite strange!!! Page tables have been changed until suspend?

Another thing I wonder:

I found u32 cast is used in acpi_set_firmware_waking_vector() in debugging.
It is natural for me to use u64 cast or substitute physical_address even if
the vector width is 32bit because
*acpi_gbl_common.fACS.firmware_waking_vector has 64bit width.
Len-san, this is a bug?

Regards, Hiro



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

RE: [PATCH] [suspend/resume] wrong wakeup address

[Hiroshi 2 Itoh]

>Another thing I wonder:

>I found u32 cast is used in acpi_set_firmware_waking_vector() in
debugging.
>It is natural for me to use u64 cast or substitute physical_address even
if
>the vector width is 32bit because
>*acpi_gbl_common.fACS.firmware_waking_vector has 64bit width.
>Len-san, this is a bug?

Sorry for my misunderstanding. acpi_gbl_common.fACS.firmware_waking_vector
is a pointer to 32/64bit size vector in FACS.

Regards, Hiro.



-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

Re: [PATCH] [suspend/resume] wrong wakeup address

[Pavel Machek]

Hi!

> Yes, I guess the BIOS ignored the high bits. The difference is
> unsurprised PAGE_OFFSET. Somebody has verified the patch helps his
> system.
> http://bugme.osdl.org/show_bug.cgi?id=3390

Okay, patch looks like to me, except..

===== drivers/acpi/sleep/main.c 1.40 vs edited =====
--- 1.40/drivers/acpi/sleep/main.c	2004-08-17 23:15:34 +08:00
+++ edited/drivers/acpi/sleep/main.c	2004-10-15 16:58:33 +08:00
@@ -12,6 +12,7 @@
 #include <linux/irq.h>
 #include <linux/dmi.h>
 #include <linux/device.h>
+#include <asm/io.h>
 #include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>

Normally asm/ includes are placed after linux/ includes. (Cosmetic
problem only).

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!


-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl

RE: [PATCH] [suspend/resume] wrong wakeup address

[Li, Shaohua]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

>Hi!
>
>> Yes, I guess the BIOS ignored the high bits. The difference is
>> unsurprised PAGE_OFFSET. Somebody has verified the patch helps his
>> system.
>> http://bugme.osdl.org/show_bug.cgi?id=3390
>
>Okay, patch looks like to me, except..
>
>===== drivers/acpi/sleep/main.c 1.40 vs edited =====
>--- 1.40/drivers/acpi/sleep/main.c	2004-08-17 23:15:34 +08:00
>+++ edited/drivers/acpi/sleep/main.c	2004-10-15 16:58:33 +08:00
>@@ -12,6 +12,7 @@
> #include <linux/irq.h>
> #include <linux/dmi.h>
> #include <linux/device.h>
>+#include <asm/io.h>
> #include <linux/suspend.h>
> #include <acpi/acpi_bus.h>
> #include <acpi/acpi_drivers.h>
>
>Normally asm/ includes are placed after linux/ includes. (Cosmetic
>problem only).
Yes, indeed. Len, please apply the attached patch.

Thanks,
Shaohua

[-- Attachment #2: wakeup_addr.patch --]
[-- Type: application/octet-stream, Size: 724 bytes --]

===== drivers/acpi/sleep/main.c 1.40 vs edited =====
--- 1.40/drivers/acpi/sleep/main.c	2004-08-17 23:15:34 +08:00
+++ edited/drivers/acpi/sleep/main.c	2004-10-18 10:56:10 +08:00
@@ -13,6 +13,7 @@
 #include <linux/dmi.h>
 #include <linux/device.h>
 #include <linux/suspend.h>
+#include <asm/io.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
 #include "sleep.h"
@@ -56,7 +57,8 @@ static int acpi_pm_prepare(u32 pm_state)
 		if (!acpi_wakeup_address)
 			return -EFAULT;
 		acpi_set_firmware_waking_vector(
-			(acpi_physical_address) acpi_wakeup_address);
+			(acpi_physical_address) virt_to_phys(
+			(void *)acpi_wakeup_address));
 	}
 	ACPI_FLUSH_CPU_CACHE();
 	acpi_enable_wakeup_device_prep(acpi_state);

RE: [PATCH] [suspend/resume] wrong wakeup address

[Len Brown]

Applied.

Added one cosmetic line:
 * Copyright (c) 2004 David Shaohua Li <shaohua.li-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Since the GPL says we're supposed to clearly record who changed
each file.

thanks,
-Len




-------------------------------------------------------
This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
Use IT products in your business? Tell us what you think of them. Give us
Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
http://productguide.itmanagersjournal.com/guidepromo.tmpl