On Wednesday, June 24, 2015 11:02:54 AM Lv Zheng wrote: > ACPICA commit 368eb60778b27b6ae94d3658ddc902ca1342a963 > ACPICA commit 70f62a80d65515e1285fdeeb50d94ee6f07df4bd > > The following commit is reported to have broken s2ram on some platforms: > Commit: 0249ed2444d65d65fc3f3f64f398f1ad0b7e54cd > ACPICA: Add option to favor 32-bit FADT addresses. > The platform reports 2 FACS tables (which is not allowed by ACPI > specification) and the new 32-bit address favor rule forces OSPMs to use > the FACS table reported via FADT's X_FIRMWARE_CTRL field. > > The root cause of the reported bug might be one of the followings: > 1. BIOS may favor the 64-bit firmware waking vector address when the > version of the FACS is greater than 0 and Linux currently only supports > resuming from the real mode, so the 64-bit firmware waking vector has > never been set and might be invalid to BIOS while the commit enables > higher version FACS. > 2. BIOS may favor the FACS reported via the "FIRMWARE_CTRL" field in the > FADT while the commit doesn't set the firmware waking vector address of > the FACS reported by "FIRMWARE_CTRL", it only sets the firware waking > vector address of the FACS reported by "X_FIRMWARE_CTRL". > > This patch excludes the cases that can trigger the bugs caused by the root > cause 2. > > There is no handshaking mechanism can be used by OSPM to tell BIOS which > FACS is currently used. Thus the FACS reported by "FIRMWARE_CTRL" may still > be used by BIOS and the 0 value of the 32-bit firmware waking vector might > trigger such failure. > > This patch enables the firmware waking vectors for both 32bit/64bit FACS > tables in order to ensure we can exclude the cases that trigger the bugs > caused by the root cause 2. The exclusion is split into 2 commits so that > if it turns out not to be necessary, this single commit can be reverted > without affecting the useful one. Lv Zheng, Bob Moore. > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=74021 > Link: https://github.com/acpica/acpica/commit/368eb607 > Link: https://github.com/acpica/acpica/commit/70f62a80 > Reported-and-tested-by: Oswald Buddenhagen <ossi@xxxxxxx> > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx> > Signed-off-by: Bob Moore <robert.moore@xxxxxxxxx> > --- > drivers/acpi/acpica/acglobal.h | 2 ++ > drivers/acpi/acpica/hwxfsleep.c | 74 ++++++++++++++++++++++++++++++++------- > drivers/acpi/acpica/tbutils.c | 14 ++++---- > 3 files changed, 71 insertions(+), 19 deletions(-) > > diff --git a/drivers/acpi/acpica/acglobal.h b/drivers/acpi/acpica/acglobal.h > index a0c4787..53f96a3 100644 > --- a/drivers/acpi/acpica/acglobal.h > +++ b/drivers/acpi/acpica/acglobal.h > @@ -61,6 +61,8 @@ ACPI_GLOBAL(struct acpi_table_header, acpi_gbl_original_dsdt_header); > > #if (!ACPI_REDUCED_HARDWARE) > ACPI_GLOBAL(struct acpi_table_facs *, acpi_gbl_FACS); > +ACPI_GLOBAL(struct acpi_table_facs *, acpi_gbl_facs32); > +ACPI_GLOBAL(struct acpi_table_facs *, acpi_gbl_facs64); > > #endif /* !ACPI_REDUCED_HARDWARE */ > > diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c > index c67cd32..e273b2e 100644 > --- a/drivers/acpi/acpica/hwxfsleep.c > +++ b/drivers/acpi/acpica/hwxfsleep.c > @@ -50,6 +50,13 @@ > ACPI_MODULE_NAME("hwxfsleep") > > /* Local prototypes */ > +#if (!ACPI_REDUCED_HARDWARE) > +static acpi_status > +acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs, > + acpi_physical_address physical_address, > + acpi_physical_address physical_address64); > +#endif > + > static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id); > > /* > @@ -79,9 +86,10 @@ static struct acpi_sleep_functions acpi_sleep_dispatch[] = { > #if (!ACPI_REDUCED_HARDWARE) > /******************************************************************************* > * > - * FUNCTION: acpi_set_firmware_waking_vector > + * FUNCTION: acpi_hw_set_firmware_waking_vector > * > - * PARAMETERS: physical_address - 32-bit physical address of ACPI real mode > + * PARAMETERS: facs - Pointer to FACS table > + * physical_address - 32-bit physical address of ACPI real mode > * entry point > * physical_address64 - 64-bit physical address of ACPI protected > * entry point > @@ -92,11 +100,12 @@ static struct acpi_sleep_functions acpi_sleep_dispatch[] = { > * > ******************************************************************************/ > > -acpi_status > -acpi_set_firmware_waking_vector(acpi_physical_address physical_address, > - acpi_physical_address physical_address64) > +static acpi_status > +acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs, > + acpi_physical_address physical_address, > + acpi_physical_address physical_address64) > { > - ACPI_FUNCTION_TRACE(acpi_set_firmware_waking_vector); > + ACPI_FUNCTION_TRACE(acpi_hw_set_firmware_waking_vector); > > > /* > @@ -109,25 +118,66 @@ acpi_set_firmware_waking_vector(acpi_physical_address physical_address, > > /* Set the 32-bit vector */ > > - acpi_gbl_FACS->firmware_waking_vector = (u32)physical_address; > + facs->firmware_waking_vector = (u32)physical_address; > > - if (acpi_gbl_FACS->length > 32) { > - if (acpi_gbl_FACS->version >= 1) { > + if (facs->length > 32) { > + if (facs->version >= 1) { > > /* Set the 64-bit vector */ > > - acpi_gbl_FACS->xfirmware_waking_vector = > - physical_address64; > + facs->xfirmware_waking_vector = physical_address64; > } else { > /* Clear the 64-bit vector if it exists */ > > - acpi_gbl_FACS->xfirmware_waking_vector = 0; > + facs->xfirmware_waking_vector = 0; > } > } > > return_ACPI_STATUS(AE_OK); > } > > +/******************************************************************************* > + * > + * FUNCTION: acpi_set_firmware_waking_vector > + * > + * PARAMETERS: physical_address - 32-bit physical address of ACPI real mode > + * entry point > + * physical_address64 - 64-bit physical address of ACPI protected > + * entry point > + * > + * RETURN: Status > + * > + * DESCRIPTION: Sets the firmware_waking_vector fields of the FACS > + * > + ******************************************************************************/ > + > +acpi_status > +acpi_set_firmware_waking_vector(acpi_physical_address physical_address, > + acpi_physical_address physical_address64) The question here is: Why does the host OS need to care about the second argument of this function that will always be 0? Why didn't you keep the old header of acpi_set_firmware_waking_vector() as a one-argument function taking a u32 and why didn't you add something like acpi_status acpi_set_firmware_waking_vector_full(u32 real_mode_address, acpi_physical_address high_address) and why didn't you redefine acpi_set_firmware_waking_vector() as acpi_status acpi_set_firmware_waking_vector(u32 real_mode_address) { return acpi_set_firmware_waking_vector_full(real_mode_address, 0); } ? If you did that, there wouldn't be any need to touch the code in drivers/acpi/sleep.c and the arch headers, so can you please explain to me why *exactly* you didn't do that? Rafael -- 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
