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

["Rafael J. Wysocki" <rjw@sisk.pl>]

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