[PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[Zhao Yakui]

Subject: ACPI : Set 32bit and 64bit waking vector in FCAS table
From: Zhao Yakui <yakui.zhao@intel.com>

    On some laptops only the 64bit waking vecotr is set for ACPI 2.0 FACS.
But when the system is resumed, BIOS will transfer control directly to 
32bit waking vector. In such case the system can't be resumed correctly.
    Maybe it will be more appropriate that both the 32bit and 64bit waking 
vector will be set for the ACPI 2.0 FACS when the system enters the S3 
sleeping state.

 http://bugzilla.kernel.org/show_bug.cgi?id=11368

Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/hardware/hwsleep.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6/drivers/acpi/hardware/hwsleep.c
@@ -80,14 +80,14 @@ acpi_set_firmware_waking_vector(acpi_phy
 
 	/* Set the vector */
 
-	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
-		/*
-		 * ACPI 1.0 FACS or short table or optional X_ field is zero
-		 */
-		facs->firmware_waking_vector = (u32) physical_address;
-	} else {
+	/* 32 Bit wakeing vector is always set */
+	facs->firmware_waking_vector = (u32) physical_address;
+
+	if (facs->length >= 32) {
 		/*
-		 * ACPI 2.0 FACS with valid X_ field
+		 * ACPI 2.0 FACS with the valid X_filed. Its length
+		 * will be more than 32 bytes. In such case the 64 bit
+		 * waking vector is also set.
 		 */
 		facs->xfirmware_waking_vector = physical_address;
 	}

Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[Rafael J. Wysocki]

On Thursday, 4 of September 2008, Zhao Yakui wrote:
> Subject: ACPI : Set 32bit and 64bit waking vector in FCAS table
> From: Zhao Yakui <yakui.zhao@intel.com>
> 
>     On some laptops only the 64bit waking vecotr is set for ACPI 2.0 FACS.
> But when the system is resumed, BIOS will transfer control directly to 
> 32bit waking vector. In such case the system can't be resumed correctly.
>     Maybe it will be more appropriate that both the 32bit and 64bit waking 
> vector will be set for the ACPI 2.0 FACS when the system enters the S3 
> sleeping state.
> 
>  http://bugzilla.kernel.org/show_bug.cgi?id=11368

Well, the spec (2.0c) says we should use facs->firmware_waking_vector only
if facs->xfirmware_waking_vector is zero, so I'm afraid this change may cause
regressions to happen.

Moreover, both 1.0b and 2.0c say that facs->length should be at least 64 bytes,
so the (facs->length >= 32) check is actually redundant (unless there is a
quirk setting this value below 32 for some broken systems I'm not aware of).

So, the question is what happens if we ignore the facs->length check on systems
this patch is supposed to fix.  If they _still_ require
facs->firmware_waking_vector to be set even if
facs->xfirmware_waking_vector is non-zero, I'd prefer to add quirks for them
instead of affecting everybody else.

> Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/hardware/hwsleep.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/hardware/hwsleep.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/hardware/hwsleep.c
> +++ linux-2.6/drivers/acpi/hardware/hwsleep.c
> @@ -80,14 +80,14 @@ acpi_set_firmware_waking_vector(acpi_phy
>  
>  	/* Set the vector */
>  
> -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> -		/*
> -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> -		 */
> -		facs->firmware_waking_vector = (u32) physical_address;
> -	} else {
> +	/* 32 Bit wakeing vector is always set */
> +	facs->firmware_waking_vector = (u32) physical_address;
> +
> +	if (facs->length >= 32) {
>  		/*
> -		 * ACPI 2.0 FACS with valid X_ field
> +		 * ACPI 2.0 FACS with the valid X_filed. Its length
> +		 * will be more than 32 bytes. In such case the 64 bit
> +		 * waking vector is also set.
>  		 */
>  		facs->xfirmware_waking_vector = physical_address;
>  	}
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[Zhang Rui]

On Thu, 2008-09-04 at 10:10 +0200, Rafael J. Wysocki wrote:
> On Thursday, 4 of September 2008, Zhao Yakui wrote:
> > Subject: ACPI : Set 32bit and 64bit waking vector in FCAS table
> > From: Zhao Yakui <yakui.zhao@intel.com>
> > 
> >     On some laptops only the 64bit waking vecotr is set for ACPI 2.0 FACS.
> > But when the system is resumed, BIOS will transfer control directly to 
> > 32bit waking vector. In such case the system can't be resumed correctly.
> >     Maybe it will be more appropriate that both the 32bit and 64bit waking 
> > vector will be set for the ACPI 2.0 FACS when the system enters the S3 
> > sleeping state.
> > 
> >  http://bugzilla.kernel.org/show_bug.cgi?id=11368
> 
> Well, the spec (2.0c) says we should use facs->firmware_waking_vector only
> if facs->xfirmware_waking_vector is zero, so I'm afraid this change may cause
> regressions to happen.
> 
> Moreover, both 1.0b and 2.0c say that facs->length should be at least 64 bytes,
> so the (facs->length >= 32) check is actually redundant (unless there is a
> quirk setting this value below 32 for some broken systems I'm not aware of).
> 
> So, the question is what happens if we ignore the facs->length check on systems
> this patch is supposed to fix.
this should have no impact.
facs->length is also 64 bytes.
without the patch, xfirmware_waking_vector is set.
with this patch applied, both xfirmware and firmware_waking_vector are
set.
so it seems that the BIOS sets facs->xfirmware_waking_vector during
POST, but uses facs->firmware_waking_vector to get back during resume.

thanks,
rui

>   If they _still_ require
> facs->firmware_waking_vector to be set even if
> facs->xfirmware_waking_vector is non-zero, I'd prefer to add quirks for them
> instead of affecting everybody else.
> 
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/hardware/hwsleep.c |   14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > Index: linux-2.6/drivers/acpi/hardware/hwsleep.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/hardware/hwsleep.c
> > +++ linux-2.6/drivers/acpi/hardware/hwsleep.c
> > @@ -80,14 +80,14 @@ acpi_set_firmware_waking_vector(acpi_phy
> >  
> >  	/* Set the vector */
> >  
> > -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> > -		/*
> > -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> > -		 */
> > -		facs->firmware_waking_vector = (u32) physical_address;
> > -	} else {
> > +	/* 32 Bit wakeing vector is always set */
> > +	facs->firmware_waking_vector = (u32) physical_address;
> > +
> > +	if (facs->length >= 32) {
> >  		/*
> > -		 * ACPI 2.0 FACS with valid X_ field
> > +		 * ACPI 2.0 FACS with the valid X_filed. Its length
> > +		 * will be more than 32 bytes. In such case the 64 bit
> > +		 * waking vector is also set.
> >  		 */
> >  		facs->xfirmware_waking_vector = physical_address;
> >  	}
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[Rafael J. Wysocki]

On Thursday, 4 of September 2008, Zhang Rui wrote:
> On Thu, 2008-09-04 at 10:10 +0200, Rafael J. Wysocki wrote:
> > On Thursday, 4 of September 2008, Zhao Yakui wrote:
> > > Subject: ACPI : Set 32bit and 64bit waking vector in FCAS table
> > > From: Zhao Yakui <yakui.zhao@intel.com>
> > > 
> > >     On some laptops only the 64bit waking vecotr is set for ACPI 2.0 FACS.
> > > But when the system is resumed, BIOS will transfer control directly to 
> > > 32bit waking vector. In such case the system can't be resumed correctly.
> > >     Maybe it will be more appropriate that both the 32bit and 64bit waking 
> > > vector will be set for the ACPI 2.0 FACS when the system enters the S3 
> > > sleeping state.
> > > 
> > >  http://bugzilla.kernel.org/show_bug.cgi?id=11368
> > 
> > Well, the spec (2.0c) says we should use facs->firmware_waking_vector only
> > if facs->xfirmware_waking_vector is zero, so I'm afraid this change may cause
> > regressions to happen.
> > 
> > Moreover, both 1.0b and 2.0c say that facs->length should be at least 64 bytes,
> > so the (facs->length >= 32) check is actually redundant (unless there is a
> > quirk setting this value below 32 for some broken systems I'm not aware of).
> > 
> > So, the question is what happens if we ignore the facs->length check on systems
> > this patch is supposed to fix.
> this should have no impact.
> facs->length is also 64 bytes.
> without the patch, xfirmware_waking_vector is set.
> with this patch applied, both xfirmware and firmware_waking_vector are
> set.

OK, sorry.  In the original code the 'facs->length < 32' check doesn't matter.

> so it seems that the BIOS sets facs->xfirmware_waking_vector during
> POST, but uses facs->firmware_waking_vector to get back during resume.

So the BIOS is buggy, so let's add a quirk for it.

IMO we should follow the spec in the first place and add quirks for systems
where that doesn't work.  Otherwise we are likely to break systems that do
follow the spec and that's not acceptable.

Thanks,
Rafael


> >   If they _still_ require
> > facs->firmware_waking_vector to be set even if
> > facs->xfirmware_waking_vector is non-zero, I'd prefer to add quirks for them
> > instead of affecting everybody else.
> > 
> > > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > >  drivers/acpi/hardware/hwsleep.c |   14 +++++++-------
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > > 
> > > Index: linux-2.6/drivers/acpi/hardware/hwsleep.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/acpi/hardware/hwsleep.c
> > > +++ linux-2.6/drivers/acpi/hardware/hwsleep.c
> > > @@ -80,14 +80,14 @@ acpi_set_firmware_waking_vector(acpi_phy
> > >  
> > >  	/* Set the vector */
> > >  
> > > -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> > > -		/*
> > > -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> > > -		 */
> > > -		facs->firmware_waking_vector = (u32) physical_address;
> > > -	} else {
> > > +	/* 32 Bit wakeing vector is always set */
> > > +	facs->firmware_waking_vector = (u32) physical_address;
> > > +
> > > +	if (facs->length >= 32) {
> > >  		/*
> > > -		 * ACPI 2.0 FACS with valid X_ field
> > > +		 * ACPI 2.0 FACS with the valid X_filed. Its length
> > > +		 * will be more than 32 bytes. In such case the 64 bit
> > > +		 * waking vector is also set.
> > >  		 */
> > >  		facs->xfirmware_waking_vector = physical_address;
> > >  	}
> > > 
> > > 
> > > --
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[Matthew Garrett]

On Thu, Sep 04, 2008 at 11:37:51AM +0200, Rafael J. Wysocki wrote:
> > so it seems that the BIOS sets facs->xfirmware_waking_vector during
> > POST, but uses facs->firmware_waking_vector to get back during resume.
> 
> So the BIOS is buggy, so let's add a quirk for it.

Does the machine resume in Windows? If so, do we have any evidence that 
Windows has a quirks list to handle this case? If not, then I suspect 
that Windows sets both and this is what everyone has tested against.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[Zhao Yakui]

On Thu, 2008-09-04 at 13:07 +0100, Matthew Garrett wrote:
> On Thu, Sep 04, 2008 at 11:37:51AM +0200, Rafael J. Wysocki wrote:
> > > so it seems that the BIOS sets facs->xfirmware_waking_vector during
> > > POST, but uses facs->firmware_waking_vector to get back during resume.
> > 
> > So the BIOS is buggy, so let's add a quirk for it.
> 
> Does the machine resume in Windows? If so, do we have any evidence that 
> Windows has a quirks list to handle this case? If not, then I suspect 
> that Windows sets both and this is what everyone has tested against.
The laptop can be resumed on windows.(XP & Vista). And we don't know
whether there exists the quirk list to handler this case on windows. 
Maybe what you said is right. 
In fact it is harmless when both 32bit and 64bit waking vector in FACS
table are set. When the system is resumed, BIOS will transfer control to
the predefined waking vector. As we set the same waking vector, either
of them is OK. 

There exists the difference between 32bit and 64bit waking vector unless
the waking address is above 4GB. But in fact the waking address is below
1MB on most machines as the waking address needs to be accessed by BIOS.

So in most cases the 32bit and 64bit waking vector are the same value.
BIOS can transfer control to either of them.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[Li, Shaohua]

>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
>owner@vger.kernel.org] On Behalf Of Zhao Yakui
>Sent: Friday, September 05, 2008 9:17 AM
>To: Matthew Garrett
>Cc: Rafael J. Wysocki; Zhang, Rui; lenb@kernel.org; linux-
>acpi@vger.kernel.org; andi@firstfloor.org
>Subject: Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS
>table
>
>On Thu, 2008-09-04 at 13:07 +0100, Matthew Garrett wrote:
>> On Thu, Sep 04, 2008 at 11:37:51AM +0200, Rafael J. Wysocki wrote:
>> > > so it seems that the BIOS sets facs->xfirmware_waking_vector during
>> > > POST, but uses facs->firmware_waking_vector to get back during resume.
>> >
>> > So the BIOS is buggy, so let's add a quirk for it.
>>
>> Does the machine resume in Windows? If so, do we have any evidence that
>> Windows has a quirks list to handle this case? If not, then I suspect
>> that Windows sets both and this is what everyone has tested against.
>The laptop can be resumed on windows.(XP & Vista). And we don't know
>whether there exists the quirk list to handler this case on windows.
>Maybe what you said is right.
>In fact it is harmless when both 32bit and 64bit waking vector in FACS
>table are set. When the system is resumed, BIOS will transfer control to
>the predefined waking vector. As we set the same waking vector, either
>of them is OK.
>
>There exists the difference between 32bit and 64bit waking vector unless
>the waking address is above 4GB. But in fact the waking address is below
>1MB on most machines as the waking address needs to be accessed by BIOS.
>
>So in most cases the 32bit and 64bit waking vector are the same value.
>BIOS can transfer control to either of them.
There was discussion about this issue several months ago (intel's ml), looks people forgot to take action after the discussion. The spec owner said 64bit vector is used in protected mode. That is if OS sets it, wakeup code is called in protected mode by BIOS. So the 64-bit vector shouldn't be used.

Thanks,
Shaohua

Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[Rafael J. Wysocki]

On Friday, 5 of September 2008, Li, Shaohua wrote:
> 
> >-----Original Message-----
> >From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> >owner@vger.kernel.org] On Behalf Of Zhao Yakui
> >Sent: Friday, September 05, 2008 9:17 AM
> >To: Matthew Garrett
> >Cc: Rafael J. Wysocki; Zhang, Rui; lenb@kernel.org; linux-
> >acpi@vger.kernel.org; andi@firstfloor.org
> >Subject: Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS
> >table
> >
> >On Thu, 2008-09-04 at 13:07 +0100, Matthew Garrett wrote:
> >> On Thu, Sep 04, 2008 at 11:37:51AM +0200, Rafael J. Wysocki wrote:
> >> > > so it seems that the BIOS sets facs->xfirmware_waking_vector during
> >> > > POST, but uses facs->firmware_waking_vector to get back during resume.
> >> >
> >> > So the BIOS is buggy, so let's add a quirk for it.
> >>
> >> Does the machine resume in Windows? If so, do we have any evidence that
> >> Windows has a quirks list to handle this case? If not, then I suspect
> >> that Windows sets both and this is what everyone has tested against.
> >The laptop can be resumed on windows.(XP & Vista). And we don't know
> >whether there exists the quirk list to handler this case on windows.
> >Maybe what you said is right.
> >In fact it is harmless when both 32bit and 64bit waking vector in FACS
> >table are set. When the system is resumed, BIOS will transfer control to
> >the predefined waking vector. As we set the same waking vector, either
> >of them is OK.
> >
> >There exists the difference between 32bit and 64bit waking vector unless
> >the waking address is above 4GB. But in fact the waking address is below
> >1MB on most machines as the waking address needs to be accessed by BIOS.
> >
> >So in most cases the 32bit and 64bit waking vector are the same value.
> >BIOS can transfer control to either of them.
> There was discussion about this issue several months ago (intel's ml), looks
> people forgot to take action after the discussion. The spec owner said 64bit
> vector is used in protected mode. That is if OS sets it, wakeup code is
> called in protected mode by BIOS.

Hm, that's interesting.  Do we have any examples of _working_ systems on which
we set the 64-bit vector?

> So the 64-bit vector shouldn't be used.    

OK

So perhaps let's remove the setting of the 64-bit vector altogether (with an
appropriate comment in the source code) and see if that causes any regressions
to happen.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ACPI suspend: Always use the 32-bit waking vector

[Rafael J. Wysocki]

On Friday, 5 of September 2008, Li, Shaohua wrote:
> 
> >-----Original Message-----
> >From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-
> >owner@vger.kernel.org] On Behalf Of Zhao Yakui
> >Sent: Friday, September 05, 2008 9:17 AM
> >To: Matthew Garrett
> >Cc: Rafael J. Wysocki; Zhang, Rui; lenb@kernel.org; linux-
> >acpi@vger.kernel.org; andi@firstfloor.org
> >Subject: Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS
> >table
> >
> >On Thu, 2008-09-04 at 13:07 +0100, Matthew Garrett wrote:
> >> On Thu, Sep 04, 2008 at 11:37:51AM +0200, Rafael J. Wysocki wrote:
> >> > > so it seems that the BIOS sets facs->xfirmware_waking_vector during
> >> > > POST, but uses facs->firmware_waking_vector to get back during resume.
> >> >
> >> > So the BIOS is buggy, so let's add a quirk for it.
> >>
> >> Does the machine resume in Windows? If so, do we have any evidence that
> >> Windows has a quirks list to handle this case? If not, then I suspect
> >> that Windows sets both and this is what everyone has tested against.
> >The laptop can be resumed on windows.(XP & Vista). And we don't know
> >whether there exists the quirk list to handler this case on windows.
> >Maybe what you said is right.
> >In fact it is harmless when both 32bit and 64bit waking vector in FACS
> >table are set. When the system is resumed, BIOS will transfer control to
> >the predefined waking vector. As we set the same waking vector, either
> >of them is OK.
> >
> >There exists the difference between 32bit and 64bit waking vector unless
> >the waking address is above 4GB. But in fact the waking address is below
> >1MB on most machines as the waking address needs to be accessed by BIOS.
> >
> >So in most cases the 32bit and 64bit waking vector are the same value.
> >BIOS can transfer control to either of them.
> There was discussion about this issue several months ago (intel's ml), looks
> people forgot to take action after the discussion. The spec owner said 64bit
> vector is used in protected mode. That is if OS sets it, wakeup code is
> called in protected mode by BIOS. So the 64-bit vector shouldn't be used.   

Well, I read this part of the spec (2.0c, 3.0b) more carefully and it matches
what you're saying.  Moreover, my understanding of it is that we should
actually _clear_ the 64-bit vector on systems that support it, because
otherwise the BIOS is supposed to use it and call the wake-up code in protected
mode.

The appended patch is based on this observation.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

ACPI suspend: Always use the 32-bit waking vector

According to the ACPI specification 2.0c and later, the 64-bit waking vector
should be cleared and the 32-bit waking vector should be used, unless we want
the wake-up code to be called by the BIOS in Protected Mode.  Moreover, some
systems (for example HP dv5-1004nr) are known to fail to resume if the 64-bit
waking vector is used.  Therefore, modify the code to clear the 64-bit waking
vector, for FACS version 1 or greater, and set the 32-bit one before suspend.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/acpi/hardware/hwsleep.c |   37 +++++++++++--------------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/acpi/hardware/hwsleep.c
===================================================================
--- linux-2.6.orig/drivers/acpi/hardware/hwsleep.c
+++ linux-2.6/drivers/acpi/hardware/hwsleep.c
@@ -78,19 +78,17 @@ acpi_set_firmware_waking_vector(acpi_phy
 		return_ACPI_STATUS(status);
 	}
 
-	/* Set the vector */
+	/*
+	 * According to the ACPI specification 2.0c and later, the 64-bit
+	 * waking vector should be cleared and the 32-bit waking vector should
+	 * be used, unless we want the wake-up code to be called by the BIOS in
+	 * Protected Mode.  Some systems (for example HP dv5-1004nr) are known
+	 * to fail to resume if the 64-bit vector is used.
+	 */
+	if (facs->version >= 1)
+		facs->xfirmware_waking_vector = 0;
 
-	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
-		/*
-		 * ACPI 1.0 FACS or short table or optional X_ field is zero
-		 */
-		facs->firmware_waking_vector = (u32) physical_address;
-	} else {
-		/*
-		 * ACPI 2.0 FACS with valid X_ field
-		 */
-		facs->xfirmware_waking_vector = physical_address;
-	}
+	facs->firmware_waking_vector = (u32)physical_address;
 
 	return_ACPI_STATUS(AE_OK);
 }
@@ -134,20 +132,7 @@ acpi_get_firmware_waking_vector(acpi_phy
 	}
 
 	/* Get the vector */
-
-	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
-		/*
-		 * ACPI 1.0 FACS or short table or optional X_ field is zero
-		 */
-		*physical_address =
-		    (acpi_physical_address) facs->firmware_waking_vector;
-	} else {
-		/*
-		 * ACPI 2.0 FACS with valid X_ field
-		 */
-		*physical_address =
-		    (acpi_physical_address) facs->xfirmware_waking_vector;
-	}
+	*physical_address = (acpi_physical_address)facs->firmware_waking_vector;
 
 	return_ACPI_STATUS(AE_OK);
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI suspend: Always use the 32-bit waking vector

[Pavel Machek]

Hi!

> > There was discussion about this issue several months ago (intel's ml), looks
> > people forgot to take action after the discussion. The spec owner said 64bit
> > vector is used in protected mode. That is if OS sets it, wakeup code is
> > called in protected mode by BIOS. So the 64-bit vector shouldn't be used.   
> 
> Well, I read this part of the spec (2.0c, 3.0b) more carefully and it matches
> what you're saying.  Moreover, my understanding of it is that we should
> actually _clear_ the 64-bit vector on systems that support it, because
> otherwise the BIOS is supposed to use it and call the wake-up code in protected
> mode.
> 
> The appended patch is based on this observation.

Hmm, nice.

64-bit waking is the one that is 'recommended' to use, right? Could we
get some strange machines (kohjisha?) to jump to the waking vector by
using 64-bit one?

Do windows use 32-bit or 64-bit vector?

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> ACPI suspend: Always use the 32-bit waking vector
> 
> According to the ACPI specification 2.0c and later, the 64-bit waking vector
> should be cleared and the 32-bit waking vector should be used, unless we want
> the wake-up code to be called by the BIOS in Protected Mode.  Moreover, some
> systems (for example HP dv5-1004nr) are known to fail to resume if the 64-bit
> waking vector is used.  Therefore, modify the code to clear the 64-bit waking
> vector, for FACS version 1 or greater, and set the 32-bit one before suspend.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK.

> ---
>  drivers/acpi/hardware/hwsleep.c |   37 +++++++++++--------------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/hardware/hwsleep.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/hardware/hwsleep.c
> +++ linux-2.6/drivers/acpi/hardware/hwsleep.c
> @@ -78,19 +78,17 @@ acpi_set_firmware_waking_vector(acpi_phy
>  		return_ACPI_STATUS(status);
>  	}
>  
> -	/* Set the vector */
> +	/*
> +	 * According to the ACPI specification 2.0c and later, the 64-bit
> +	 * waking vector should be cleared and the 32-bit waking vector should
> +	 * be used, unless we want the wake-up code to be called by the BIOS in
> +	 * Protected Mode.  Some systems (for example HP dv5-1004nr) are known
> +	 * to fail to resume if the 64-bit vector is used.
> +	 */
> +	if (facs->version >= 1)
> +		facs->xfirmware_waking_vector = 0;
>  
> -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> -		/*
> -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> -		 */
> -		facs->firmware_waking_vector = (u32) physical_address;
> -	} else {
> -		/*
> -		 * ACPI 2.0 FACS with valid X_ field
> -		 */
> -		facs->xfirmware_waking_vector = physical_address;
> -	}
> +	facs->firmware_waking_vector = (u32)physical_address;
>  
>  	return_ACPI_STATUS(AE_OK);
>  }
> @@ -134,20 +132,7 @@ acpi_get_firmware_waking_vector(acpi_phy
>  	}
>  
>  	/* Get the vector */
> -
> -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> -		/*
> -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> -		 */
> -		*physical_address =
> -		    (acpi_physical_address) facs->firmware_waking_vector;
> -	} else {
> -		/*
> -		 * ACPI 2.0 FACS with valid X_ field
> -		 */
> -		*physical_address =
> -		    (acpi_physical_address) facs->xfirmware_waking_vector;
> -	}
> +	*physical_address = (acpi_physical_address)facs->firmware_waking_vector;
>  
>  	return_ACPI_STATUS(AE_OK);
>  }


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] ACPI suspend: Always use the 32-bit waking vector

[Rafael J. Wysocki]

On Sunday, 14 of September 2008, Pavel Machek wrote:
> Hi!
> 
> > > There was discussion about this issue several months ago (intel's ml), looks
> > > people forgot to take action after the discussion. The spec owner said 64bit
> > > vector is used in protected mode. That is if OS sets it, wakeup code is
> > > called in protected mode by BIOS. So the 64-bit vector shouldn't be used.   
> > 
> > Well, I read this part of the spec (2.0c, 3.0b) more carefully and it matches
> > what you're saying.  Moreover, my understanding of it is that we should
> > actually _clear_ the 64-bit vector on systems that support it, because
> > otherwise the BIOS is supposed to use it and call the wake-up code in protected
> > mode.
> > 
> > The appended patch is based on this observation.
> 
> Hmm, nice.
> 
> 64-bit waking is the one that is 'recommended' to use, right? Could we
> get some strange machines (kohjisha?) to jump to the waking vector by
> using 64-bit one?

If the 32-bit one is set and the 64-bit one is zero, the BIOS is _required_ to
use the 32-bit one.

There may be BIOSes that don't follow the spec in that respect (ie. are only
able to use the 64-bit vector and can only enter the wake-up code in Protected
Mode), but I'm not aware of any.

> Do windows use 32-bit or 64-bit vector?

I think XP uses the 32-bit one.  I don't know about Vista, though.

Thanks,
Rafael

Re: [PATCH] ACPI suspend: Always use the 32-bit waking vector

[Pavel Machek]

Hi!

> Well, I read this part of the spec (2.0c, 3.0b) more carefully and it matches
> what you're saying.  Moreover, my understanding of it is that we should
> actually _clear_ the 64-bit vector on systems that support it, because
> otherwise the BIOS is supposed to use it and call the wake-up code in protected
> mode.
> 
> The appended patch is based on this observation.
> 
> Thanks,
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> ACPI suspend: Always use the 32-bit waking vector
> 
> According to the ACPI specification 2.0c and later, the 64-bit waking vector
> should be cleared and the 32-bit waking vector should be used, unless we want
> the wake-up code to be called by the BIOS in Protected Mode.  Moreover, some
> systems (for example HP dv5-1004nr) are known to fail to resume if the 64-bit
> waking vector is used.  Therefore, modify the code to clear the 64-bit waking
> vector, for FACS version 1 or greater, and set the 32-bit one before suspend.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/hardware/hwsleep.c |   37 +++++++++++--------------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
> 
> @@ -134,20 +132,7 @@ acpi_get_firmware_waking_vector(acpi_phy
>  	}
>  
>  	/* Get the vector */
> -
> -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> -		/*
> -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> -		 */
> -		*physical_address =
> -		    (acpi_physical_address) facs->firmware_waking_vector;
> -	} else {
> -		/*
> -		 * ACPI 2.0 FACS with valid X_ field
> -		 */
> -		*physical_address =
> -		    (acpi_physical_address) facs->xfirmware_waking_vector;
> -	}
> +	*physical_address = (acpi_physical_address)facs->firmware_waking_vector;
>  
>  	return_ACPI_STATUS(AE_OK);
>  }

Actually, I guess we should kill acpi_get_firmware_waking_vector: It
is completely useless, and it is indeed never used in whole Linux...
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] ACPI suspend: Always use the 32-bit waking vector

[Rafael J. Wysocki]

On Monday, 15 of September 2008, Pavel Machek wrote:
> Hi!
> 
> > Well, I read this part of the spec (2.0c, 3.0b) more carefully and it matches
> > what you're saying.  Moreover, my understanding of it is that we should
> > actually _clear_ the 64-bit vector on systems that support it, because
> > otherwise the BIOS is supposed to use it and call the wake-up code in protected
> > mode.
> > 
> > The appended patch is based on this observation.
> > 
> > Thanks,
> > Rafael
> > 
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > ACPI suspend: Always use the 32-bit waking vector
> > 
> > According to the ACPI specification 2.0c and later, the 64-bit waking vector
> > should be cleared and the 32-bit waking vector should be used, unless we want
> > the wake-up code to be called by the BIOS in Protected Mode.  Moreover, some
> > systems (for example HP dv5-1004nr) are known to fail to resume if the 64-bit
> > waking vector is used.  Therefore, modify the code to clear the 64-bit waking
> > vector, for FACS version 1 or greater, and set the 32-bit one before suspend.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/acpi/hardware/hwsleep.c |   37 +++++++++++--------------------------
> >  1 file changed, 11 insertions(+), 26 deletions(-)
> > 
> > @@ -134,20 +132,7 @@ acpi_get_firmware_waking_vector(acpi_phy
> >  	}
> >  
> >  	/* Get the vector */
> > -
> > -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> > -		/*
> > -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> > -		 */
> > -		*physical_address =
> > -		    (acpi_physical_address) facs->firmware_waking_vector;
> > -	} else {
> > -		/*
> > -		 * ACPI 2.0 FACS with valid X_ field
> > -		 */
> > -		*physical_address =
> > -		    (acpi_physical_address) facs->xfirmware_waking_vector;
> > -	}
> > +	*physical_address = (acpi_physical_address)facs->firmware_waking_vector;
> >  
> >  	return_ACPI_STATUS(AE_OK);
> >  }
> 
> Actually, I guess we should kill acpi_get_firmware_waking_vector: It
> is completely useless, and it is indeed never used in whole Linux...

Well, it's under '#ifdef 0' anyway, but I wanted it to be consistent with the
code we actually use. :-)

Thanks,
Rafael

Re: [PATCH] ACPI suspend: Always use the 32-bit waking vector

[Pavel Machek]

On Wed 2008-09-17 07:46:42, Rafael J. Wysocki wrote:
> On Monday, 15 of September 2008, Pavel Machek wrote:
> > Hi!
> > 
> > > Well, I read this part of the spec (2.0c, 3.0b) more carefully and it matches
> > > what you're saying.  Moreover, my understanding of it is that we should
> > > actually _clear_ the 64-bit vector on systems that support it, because
> > > otherwise the BIOS is supposed to use it and call the wake-up code in protected
> > > mode.
> > > 
> > > The appended patch is based on this observation.
> > > 
> > > Thanks,
> > > Rafael
> > > 
> > > ---
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > ACPI suspend: Always use the 32-bit waking vector
> > > 
> > > According to the ACPI specification 2.0c and later, the 64-bit waking vector
> > > should be cleared and the 32-bit waking vector should be used, unless we want
> > > the wake-up code to be called by the BIOS in Protected Mode.  Moreover, some
> > > systems (for example HP dv5-1004nr) are known to fail to resume if the 64-bit
> > > waking vector is used.  Therefore, modify the code to clear the 64-bit waking
> > > vector, for FACS version 1 or greater, and set the 32-bit one before suspend.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > >  drivers/acpi/hardware/hwsleep.c |   37 +++++++++++--------------------------
> > >  1 file changed, 11 insertions(+), 26 deletions(-)
> > > 
> > > @@ -134,20 +132,7 @@ acpi_get_firmware_waking_vector(acpi_phy
> > >  	}
> > >  
> > >  	/* Get the vector */
> > > -
> > > -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> > > -		/*
> > > -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> > > -		 */
> > > -		*physical_address =
> > > -		    (acpi_physical_address) facs->firmware_waking_vector;
> > > -	} else {
> > > -		/*
> > > -		 * ACPI 2.0 FACS with valid X_ field
> > > -		 */
> > > -		*physical_address =
> > > -		    (acpi_physical_address) facs->xfirmware_waking_vector;
> > > -	}
> > > +	*physical_address = (acpi_physical_address)facs->firmware_waking_vector;
> > >  
> > >  	return_ACPI_STATUS(AE_OK);
> > >  }
> > 
> > Actually, I guess we should kill acpi_get_firmware_waking_vector: It
> > is completely useless, and it is indeed never used in whole Linux...
> 
> Well, it's under '#ifdef 0' anyway, but I wanted it to be consistent with the
> code we actually use. :-)

Yes, removing dead code is separate battle, I see.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

ACPI suspend: test 64-bit waking vector (was Re: [PATCH] ACPI suspend: Always use the 32-bit waking vector)

[Pavel Machek]

Hi!

ACPI specificiation tells us that x_firmware_waking_vector is
preffered, and maybe it works better than firmware_waking_vector on
some machines.

Unfortunately, it does not seem to work on thinkpad x60... but I am
not sure if I'm not doing something wrong.

Testing/ideas would be welcome.
								Pavel

diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 426e5d9..8ce0899 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -164,3 +164,24 @@ #endif
 }
 
 __setup("acpi_sleep=", acpi_sleep_setup);
+
+void acpi_pmode_wakeup(void)
+{
+	asm volatile ("\
+	cli; \
+	inb     $97, %al; \
+        outb    %al, $0x80;     \
+        movb    $3, %al;        \
+        outb    %al, $97;       \
+        outb    %al, $0x80;     \
+        movb    $-74, %al;      \
+        outb    %al, $67;       \
+        outb    %al, $0x80;     \
+        movb    $-119, %al;     \
+        outb    %al, $66;       \
+        outb    %al, $0x80;     \
+        movb    $15, %al;       \
+        outb    %al, $66;  \
+	1: jmp 1b; ");
+}
+
diff --git a/drivers/acpi/hardware/hwsleep.c b/drivers/acpi/hardware/hwsleep.c
index dba3cfb..33f157a 100644
--- a/drivers/acpi/hardware/hwsleep.c
+++ b/drivers/acpi/hardware/hwsleep.c
@@ -44,6 +44,7 @@
 
 #include <acpi/acpi.h>
 #include <acpi/actables.h>
+#include <asm/io.h>
 
 #define _COMPONENT          ACPI_HARDWARE
 ACPI_MODULE_NAME("hwsleep")
@@ -60,6 +61,9 @@ ACPI_MODULE_NAME("hwsleep")
  * DESCRIPTION: Access function for the firmware_waking_vector field in FACS
  *
  ******************************************************************************/
+
+extern void acpi_pmode_wakeup(void);
+
 acpi_status
 acpi_set_firmware_waking_vector(acpi_physical_address physical_address)
 {
@@ -80,6 +84,12 @@ acpi_set_firmware_waking_vector(acpi_phy
 
 	/* Set the vector */
 
+	if (facs->length < 32)
+		panic("only acpi1 supported?!");
+
+	facs->firmware_waking_vector = 0;
+	facs->xfirmware_waking_vector = virt_to_phys(acpi_pmode_wakeup);
+#if 0
 	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
 		/*
 		 * ACPI 1.0 FACS or short table or optional X_ field is zero
@@ -91,6 +101,7 @@ acpi_set_firmware_waking_vector(acpi_phy
 		 */
 		facs->xfirmware_waking_vector = physical_address;
 	}
+#endif
 
 	return_ACPI_STATUS(AE_OK);
 }


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: ACPI suspend: test 64-bit waking vector (was Re: [PATCH] ACPI suspend: Always use the 32-bit waking vector)

[Rafael J. Wysocki]

On Monday, 15 of September 2008, Pavel Machek wrote:
> Hi!
> 
> ACPI specificiation tells us that x_firmware_waking_vector is
> preffered, and maybe it works better than firmware_waking_vector on
> some machines.
> 
> Unfortunately, it does not seem to work on thinkpad x60... but I am
> not sure if I'm not doing something wrong.
> 
> Testing/ideas would be welcome.

Well, the spec says that if x_firmware_waking_vector is non-zero, the BIOS is
supposed to call your wake-up code in Protected Mode ...

Thanks,
Rafael

-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/

Re: ACPI suspend: test 64-bit waking vector (was Re: [PATCH] ACPI suspend: Always use the 32-bit waking vector)

[Pavel Machek]

On Wed 2008-09-17 07:45:18, Rafael J. Wysocki wrote:
> On Monday, 15 of September 2008, Pavel Machek wrote:
> > Hi!
> > 
> > ACPI specificiation tells us that x_firmware_waking_vector is
> > preffered, and maybe it works better than firmware_waking_vector on
> > some machines.
> > 
> > Unfortunately, it does not seem to work on thinkpad x60... but I am
> > not sure if I'm not doing something wrong.
> > 
> > Testing/ideas would be welcome.
> 
> Well, the spec says that if x_firmware_waking_vector is non-zero, the BIOS is
> supposed to call your wake-up code in Protected Mode ...

That's why I'm passing physical address of 32-bit code...

or am I supposed to pass 48-bit selector:offset pair? But what GDT
will BIOS use in that case?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: ACPI suspend: test 64-bit waking vector (was Re: [PATCH] ACPI suspend: Always use the 32-bit waking vector)

[Rafael J. Wysocki]

On Wednesday, 17 of September 2008, Pavel Machek wrote:
> On Wed 2008-09-17 07:45:18, Rafael J. Wysocki wrote:
> > On Monday, 15 of September 2008, Pavel Machek wrote:
> > > Hi!
> > > 
> > > ACPI specificiation tells us that x_firmware_waking_vector is
> > > preffered, and maybe it works better than firmware_waking_vector on
> > > some machines.
> > > 
> > > Unfortunately, it does not seem to work on thinkpad x60... but I am
> > > not sure if I'm not doing something wrong.
> > > 
> > > Testing/ideas would be welcome.
> > 
> > Well, the spec says that if x_firmware_waking_vector is non-zero, the BIOS is
> > supposed to call your wake-up code in Protected Mode ...
> 
> That's why I'm passing physical address of 32-bit code...

Well, that should work in theory.  The BIOS is supposed to set up 4 GB of flat
memory starting from zero before jumping to the wake-up code (at least as far
as I understand the spec).

However, is your FACS version equal to 1?

Rafael

Re: [PATCH] ACPI suspend: Always use the 32-bit waking vector

[Len Brown]

> Well, I read this part of the spec (2.0c, 3.0b) more carefully and it matches
> what you're saying.  Moreover, my understanding of it is that we should
> actually _clear_ the 64-bit vector on systems that support it, because
> otherwise the BIOS is supposed to use it and call the wake-up code in protected
> mode.
> 
> The appended patch is based on this observation.
> 
> Thanks,
> Rafael

applied to acpi-test.

thanks,
-Len

> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> ACPI suspend: Always use the 32-bit waking vector
> 
> According to the ACPI specification 2.0c and later, the 64-bit waking vector
> should be cleared and the 32-bit waking vector should be used, unless we want
> the wake-up code to be called by the BIOS in Protected Mode.  Moreover, some
> systems (for example HP dv5-1004nr) are known to fail to resume if the 64-bit
> waking vector is used.  Therefore, modify the code to clear the 64-bit waking
> vector, for FACS version 1 or greater, and set the 32-bit one before suspend.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/acpi/hardware/hwsleep.c |   37 +++++++++++--------------------------
>  1 file changed, 11 insertions(+), 26 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/hardware/hwsleep.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/hardware/hwsleep.c
> +++ linux-2.6/drivers/acpi/hardware/hwsleep.c
> @@ -78,19 +78,17 @@ acpi_set_firmware_waking_vector(acpi_phy
>  		return_ACPI_STATUS(status);
>  	}
>  
> -	/* Set the vector */
> +	/*
> +	 * According to the ACPI specification 2.0c and later, the 64-bit
> +	 * waking vector should be cleared and the 32-bit waking vector should
> +	 * be used, unless we want the wake-up code to be called by the BIOS in
> +	 * Protected Mode.  Some systems (for example HP dv5-1004nr) are known
> +	 * to fail to resume if the 64-bit vector is used.
> +	 */
> +	if (facs->version >= 1)
> +		facs->xfirmware_waking_vector = 0;
>  
> -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> -		/*
> -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> -		 */
> -		facs->firmware_waking_vector = (u32) physical_address;
> -	} else {
> -		/*
> -		 * ACPI 2.0 FACS with valid X_ field
> -		 */
> -		facs->xfirmware_waking_vector = physical_address;
> -	}
> +	facs->firmware_waking_vector = (u32)physical_address;
>  
>  	return_ACPI_STATUS(AE_OK);
>  }
> @@ -134,20 +132,7 @@ acpi_get_firmware_waking_vector(acpi_phy
>  	}
>  
>  	/* Get the vector */
> -
> -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> -		/*
> -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> -		 */
> -		*physical_address =
> -		    (acpi_physical_address) facs->firmware_waking_vector;
> -	} else {
> -		/*
> -		 * ACPI 2.0 FACS with valid X_ field
> -		 */
> -		*physical_address =
> -		    (acpi_physical_address) facs->xfirmware_waking_vector;
> -	}
> +	*physical_address = (acpi_physical_address)facs->firmware_waking_vector;
>  
>  	return_ACPI_STATUS(AE_OK);
>  }
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[Zhao Yakui]

On Thu, 2008-09-04 at 10:10 +0200, Rafael J. Wysocki wrote:
> On Thursday, 4 of September 2008, Zhao Yakui wrote:
> > Subject: ACPI : Set 32bit and 64bit waking vector in FCAS table
> > From: Zhao Yakui <yakui.zhao@intel.com>
> > 
> >     On some laptops only the 64bit waking vecotr is set for ACPI 2.0 FACS.
> > But when the system is resumed, BIOS will transfer control directly to 
> > 32bit waking vector. In such case the system can't be resumed correctly.
> >     Maybe it will be more appropriate that both the 32bit and 64bit waking 
> > vector will be set for the ACPI 2.0 FACS when the system enters the S3 
> > sleeping state.
> > 
> >  http://bugzilla.kernel.org/show_bug.cgi?id=11368
> 
> Well, the spec (2.0c) says we should use facs->firmware_waking_vector only
> if facs->xfirmware_waking_vector is zero, so I'm afraid this change may cause
> regressions to happen.
The 32bit and 64bit waking vector are for BIOS. For the ACPI 2.0 FACS
after the system is resumed, firmware will check whether the 64bit
waking vector is zero. If not zero, it will transfer controls to 64 bit
waking vector address. If zero, it will then check whether the 32bit is
zero. If not zero, it will transfer controls to the 32bit waking vector
address. 
If bios can follow above check, it is ok to set only one. And another is
set to zero. But if BIOS can't follow above check and only 64bit waking
vector is set, maybe the system can't be resumed very well.
> 
> Moreover, both 1.0b and 2.0c say that facs->length should be at least 64 bytes,
> so the (facs->length >= 32) check is actually redundant (unless there is a
> quirk setting this value below 32 for some broken systems I'm not aware of).
What you said is right. In ACPI 1.0b FACS the length is also 64 Bytes. 
We should use the facs->version to determine whether 64bit waking vector
needs to be set.
> So, the question is what happens if we ignore the facs->length check on systems
> this patch is supposed to fix.  If they _still_ require
> facs->firmware_waking_vector to be set even if
> facs->xfirmware_waking_vector is non-zero, I'd prefer to add quirks for them
> instead of affecting everybody else.
> 
> > Signed-off-by: Zhao Yakui <yakui.zhao@intel.com>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> >  drivers/acpi/hardware/hwsleep.c |   14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > Index: linux-2.6/drivers/acpi/hardware/hwsleep.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/hardware/hwsleep.c
> > +++ linux-2.6/drivers/acpi/hardware/hwsleep.c
> > @@ -80,14 +80,14 @@ acpi_set_firmware_waking_vector(acpi_phy
> >  
> >  	/* Set the vector */
> >  
> > -	if ((facs->length < 32) || (!(facs->xfirmware_waking_vector))) {
> > -		/*
> > -		 * ACPI 1.0 FACS or short table or optional X_ field is zero
> > -		 */
> > -		facs->firmware_waking_vector = (u32) physical_address;
> > -	} else {
> > +	/* 32 Bit wakeing vector is always set */
> > +	facs->firmware_waking_vector = (u32) physical_address;
> > +
> > +	if (facs->length >= 32) {
> >  		/*
> > -		 * ACPI 2.0 FACS with valid X_ field
> > +		 * ACPI 2.0 FACS with the valid X_filed. Its length
> > +		 * will be more than 32 bytes. In such case the 64 bit
> > +		 * waking vector is also set.
> >  		 */
> >  		facs->xfirmware_waking_vector = physical_address;
> >  	}
> > 
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> 
> 

Re: [PATCH]: ACPI : Set 32bit and 64bit waking vector in FCAS table

[Rafael J. Wysocki]

On Thursday, 4 of September 2008, Zhao Yakui wrote:
> On Thu, 2008-09-04 at 10:10 +0200, Rafael J. Wysocki wrote:
> > On Thursday, 4 of September 2008, Zhao Yakui wrote:
> > > Subject: ACPI : Set 32bit and 64bit waking vector in FCAS table
> > > From: Zhao Yakui <yakui.zhao@intel.com>
> > > 
> > >     On some laptops only the 64bit waking vecotr is set for ACPI 2.0 FACS.
> > > But when the system is resumed, BIOS will transfer control directly to 
> > > 32bit waking vector. In such case the system can't be resumed correctly.
> > >     Maybe it will be more appropriate that both the 32bit and 64bit waking 
> > > vector will be set for the ACPI 2.0 FACS when the system enters the S3 
> > > sleeping state.
> > > 
> > >  http://bugzilla.kernel.org/show_bug.cgi?id=11368
> > 
> > Well, the spec (2.0c) says we should use facs->firmware_waking_vector only
> > if facs->xfirmware_waking_vector is zero, so I'm afraid this change may cause
> > regressions to happen.
> The 32bit and 64bit waking vector are for BIOS. For the ACPI 2.0 FACS
> after the system is resumed, firmware will check whether the 64bit
> waking vector is zero. If not zero, it will transfer controls to 64 bit
> waking vector address. If zero, it will then check whether the 32bit is
> zero. If not zero, it will transfer controls to the 32bit waking vector
> address. 
> If bios can follow above check, it is ok to set only one. And another is
> set to zero. But if BIOS can't follow above check and only 64bit waking
> vector is set, maybe the system can't be resumed very well.

I understand that, but I'm not sure what happens if we set
facs->firmware_waking_vector even though the BIOS expects us not to set it
(probably nothing, but still).  In fact we should test that on all machines
known to work with the current code and that's impossible.  Let's just
add quirks for BIOSes that don't follow the spec (we can extend the
acpi_sleep= option for that too for the user's convenience).

> > Moreover, both 1.0b and 2.0c say that facs->length should be at least 64 bytes,
> > so the (facs->length >= 32) check is actually redundant (unless there is a
> > quirk setting this value below 32 for some broken systems I'm not aware of).
> What you said is right. In ACPI 1.0b FACS the length is also 64 Bytes. 
> We should use the facs->version to determine whether 64bit waking vector
> needs to be set.

Well, yes.  It's possible that some buggy BIOSes provide non-zero
facs->xfirmware_waking_vector although they shouldn't.  Still, there may be
broken BIOSes that set both facs->version and facs->xfirmware_waking_vector
although they shouldn't. :-)

In any case, I think we should add quirks for buggy BIOSes rather than slightly
depart from the spec in order to handle them.

Thanks,
Rafael