Hi, Rafael > From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx] > Sent: Thursday, June 25, 2015 7:57 AM > > 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? Host OS can set non 0 address for both real_mode_address and high_address to indicate that it can support both 32-bit and 64-bit resume environments. So if a BIOS favors 32-bit resume environment, it can resume from here; if another BIOS favors 64-bit resume environment, it can resume from there. And host OSes can be implemented using only 1 binary to work with both BIOSes. Thanks and best regards -Lv ��.n��������+%������w��{.n�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
