On Thu, 2008-10-16 at 03:41 +0800, Moore, Robert wrote: > I'll go ahead and implement the following: > > http://www.acpica.org/bugzilla/show_bug.cgi?id=731 > > > It looks like we have an issue on some machines where both the FIRMWARE_CTRL and X_FIRMWARE_CTRL fields in the FADT are valid, but contain different addresses. In this case, we don't really know which one is the real FACS. > > Currently, if the X_FIRMWARE_CTRL field is valid, we use it exclusively (as per the ACPI spec.) This doesn’t work when the “real” FACS is actually the one pointed to by the FIRMWARE_CTRL field. > > The proposed change for the case where both FACS pointers are valid is to write the firmware waking vector value to both FACS tables. > yes, that's the problem I want to fix in this patch. another problem is the global lock. we may get an invalid global lock if the wrong FACS is used, right? but currently we don't know which FACS is actually used by BIOS/Windows, this patch could only fix the waking vector issue, > We would have something like this: > > Set wake vector using FACS pointed to by X_FIRMWARE_CTRL > If (FIRMWARE_CTRL is non-zero) AND (FIRMWARE_CTRL != X_FIRMWARE_CTRL) > Set wake vector using FACS pointed to by FIRMWARE_CTRL > Yes. this could fix the problem too, but much more complex. As we need to add both of the FACS tables to the acpi_gbl_root_table_list, while the index of FACS table is hard coded there, i.e. ACPI_TABLE_INDEX_FACS. Bob, do you think we should fix the multiple FACS issue in some other way, or just follow the proposal in this patch? thanks, rui > > > >-----Original Message----- > >From: Zhang, Rui > >Sent: Monday, October 13, 2008 6:09 PM > >To: Moore, Robert > >Cc: Len Brown; linux-acpi; Rafael J. Wysocki > >Subject: RE: [PATCH 5/6] ACPI: set waking vector in inactive facs table > > > >On Mon, 2008-10-13 at 11:46 -0600, Moore, Robert wrote: > >> So, is this the bottom line of the hwsleep patch: > >> > >> If there is a second FACS in the system (pointed to by the unused RSDT), > >then set the firmware waking vectors in BOTH FACS tables? > >yes. > > > >there is another way to fix this issue. > >as FADT1 exports two different FACS tables, setting the waking vectors > >in both of them (pointed to by fadt->facs and fadt->Xfadt) also works. > > > >I don't know which way is better. > > > >thanks, > >rui > >> > >> > >> > >> >-----Original Message----- > >> >From: Zhang, Rui > >> >Sent: Monday, October 13, 2008 1:11 AM > >> >To: Len Brown > >> >Cc: linux-acpi; Moore, Robert; Rafael J. Wysocki; Zhang, Rui > >> >Subject: [PATCH 5/6] ACPI: set waking vector in inactive facs table > >> > > >> > > >> >RSDT and XSDT may export different FACS tables. > >> >Some of the BIOSes only checks one of them when resuming. > >> >And Linux reboots instead of resuming because BIOS can't find > >> >the waking vector. > >> >For example, > >> >I have a platform which suspends well, but always reboots instead of > >> >resuming when pressing the power button. > >> >I found that there are two FACS tables on this platform, > >> >XSDT-->FADT1-->Xfacs-->FACS1 > >> > |----->facs-| > >> > |->FACS2 > >> >RSDT-->FADT2-->facs-| > >> >Linux uses XSDT on this platform and sets the waking vector in FACS1 > >> >when suspending. > >> >But it seems that the BIOS only cares for the waking vector in FACS2, > >> >thus it reboots when resuming because the waking vector is not set at > >all. > >> > > >> >Set the waking vector in inactive FACS table in this patch. > >> > > >> >Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx> > >> >--- > >> > drivers/acpi/hardware/hwsleep.c | 20 ++++++++++++++++++++ > >> > drivers/acpi/tables/tbfadt.c | 6 ++++-- > >> > 2 files changed, 24 insertions(+), 2 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 > >> >@@ -65,6 +65,7 @@ acpi_set_firmware_waking_vector(acpi_phy > >> > { > >> > struct acpi_table_facs *facs; > >> > acpi_status status; > >> >+ u32 table_index; > >> > > >> > ACPI_FUNCTION_TRACE(acpi_set_firmware_waking_vector); > >> > > >> >@@ -92,6 +93,25 @@ acpi_set_firmware_waking_vector(acpi_phy > >> > facs->xfirmware_waking_vector = physical_address; > >> > } > >> > > >> >+ /* set the firmware vector in the inactive facs table */ > >> >+ for (table_index = 0, facs = NULL; ; table_index++) { > >> >+ status = acpi_get_inactive_table_by_index(table_index, > >> >+ ACPI_CAST_INDIRECT_PTR(struct > >> >acpi_table_header, > >> >+ &facs)); > >> >+ if (status == AE_TABLE_NOT_SUPPORTED) > >> >+ continue; > >> >+ if (ACPI_FAILURE(status)) > >> >+ break; > >> >+ > >> >+ if (!ACPI_COMPARE_NAME(&facs->signature, ACPI_SIG_FACS)) > >> >+ continue; > >> >+ > >> >+ if (facs->version >= 1) > >> >+ facs->xfirmware_waking_vector = 0; > >> >+ facs->firmware_waking_vector = (u32)physical_address; > >> >+ break; > >> >+ } > >> >+ > >> > return_ACPI_STATUS(AE_OK); > >> > } > >> > > >> >Index: linux-2.6/drivers/acpi/tables/tbfadt.c > >> >=================================================================== > >> >--- linux-2.6.orig/drivers/acpi/tables/tbfadt.c > >> >+++ linux-2.6/drivers/acpi/tables/tbfadt.c > >> >@@ -290,11 +290,13 @@ static void acpi_tb_convert_fadt(void) > >> > > >> > if (!acpi_gbl_FADT.Xfacs) { > >> > acpi_gbl_FADT.Xfacs = (u64) acpi_gbl_FADT.facs; > >> >- } > >> >+ } else if (acpi_gbl_FADT.Xfacs != acpi_gbl_FADT.facs) > >> >+ ACPI_WARNING((AE_INFO, "32 bit FACS and 64 bit FACS not > >> >match\n")); > >> > > >> > if (!acpi_gbl_FADT.Xdsdt) { > >> > acpi_gbl_FADT.Xdsdt = (u64) acpi_gbl_FADT.dsdt; > >> >- } > >> >+ } else if (acpi_gbl_FADT.Xdsdt != acpi_gbl_FADT.dsdt) > >> >+ ACPI_WARNING((AE_INFO, "32 bit DSDT and 64 bit DSDT not > >> >match\n")); > >> > > >> > /* > >> > * For ACPI 1.0 FADTs (revision 1 or 2), ensure that reserved > >> >fields which > >> > > >> > -- 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
