On Wed, Jun 26, 2013 at 10:41 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote: >>>> On 26.06.13 at 16:06, Ben Guthro <benjamin.guthro@xxxxxxxxxx> wrote: >> In version 3.4 acpi_os_prepare_sleep() got introduced in parallel with >> reduced hardware sleep support, and the two changes didn't get >> synchronized: The new code doesn't call the hook function (if so >> requested). Fix this, requiring a parameter to be added to the >> hook function to distinguish "extended" from "legacy" sleep. >> >> Signed-off-by: Ben Guthro <benjamin.guthro@xxxxxxxxxx> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> > > I think these are intended to reflect the flow of things, so > should be reversed (also in the other patches). > >> --- a/drivers/acpi/acpica/hwesleep.c >> +++ b/drivers/acpi/acpica/hwesleep.c >> @@ -43,6 +43,7 @@ >> */ >> >> #include <acpi/acpi.h> >> +#include <linux/acpi.h> > > This also got complaints, so I'd be very surprised if they took it now. I did see these complaints in the last version. However, the file drivers/acpi/acpica/hwsleep.c contains this include, and has since commit 09f98a825a821f7a3f1b162f9ed023f37213a63b Author: Tang Liang <liang.tang@xxxxxxxxxx> Date: Fri Dec 9 10:05:54 2011 +0800 So since this is the extended sleep file, vs the standard one - I don't see why such a restriction would be placed on the former, but not the latter. I would look for some guidance here from the ACPI guys, for how to handle this. > >> #include "accommon.h" >> >> #define _COMPONENT ACPI_HARDWARE >> @@ -128,6 +129,13 @@ acpi_status acpi_hw_extended_sleep(u8 sleep_state) >> >> ACPI_FLUSH_CPU_CACHE(); >> >> + status = acpi_os_prepare_sleep(sleep_state, acpi_gbl_sleep_type_a, >> + acpi_gbl_sleep_type_b, true); > > Without using "bool", using "true" and "false" is wrong (should > be TRUE and FALSE afaict). Thanks, I overlooked that. I'll fix it for the next version. > >> --- a/drivers/acpi/acpica/hwsleep.c >> +++ b/drivers/acpi/acpica/hwsleep.c >> @@ -153,7 +153,7 @@ acpi_status acpi_hw_legacy_sleep(u8 sleep_state) >> ACPI_FLUSH_CPU_CACHE(); >> >> status = acpi_os_prepare_sleep(sleep_state, pm1a_control, >> - pm1b_control); >> + pm1b_control, false); > > Same here. ack. > >> if (ACPI_SKIP(status)) >> return_ACPI_STATUS(AE_OK); >> if (ACPI_FAILURE(status)) > > And the split point ought to be here - everything below doesn't > modify ACPI CA code. Which in particular means that ... OK. > >> --- a/include/linux/acpi.h >> +++ b/include/linux/acpi.h >> @@ -477,11 +477,11 @@ static inline bool acpi_driver_match_device(struct device *dev, >> #endif /* !CONFIG_ACPI */ >> >> #ifdef CONFIG_ACPI >> -void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, >> - u32 pm1a_ctrl, u32 pm1b_ctrl)); >> +void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state, u32 val_a, >> + u32 val_b, u8 extended)); >> >> -acpi_status acpi_os_prepare_sleep(u8 sleep_state, >> - u32 pm1a_control, u32 pm1b_control); >> +acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 val_a, u32 val_b, >> + u8 extended); > > ... this needs to be moved elsewhere (under include/acpi/), but the > two incarnations of acpi_os_set_prepare_sleep() should presumably > remain here. If my comment above about hwsleep.c holds, would this be necessary? Thanks for the review. Ben > > Jan > >> #ifdef CONFIG_X86 >> void arch_reserve_mem_area(acpi_physical_address addr, size_t size); >> #else >> @@ -491,7 +491,7 @@ static inline void arch_reserve_mem_area(acpi_physical_address addr, >> } >> #endif /* CONFIG_X86 */ >> #else >> -#define acpi_os_set_prepare_sleep(func, pm1a_ctrl, pm1b_ctrl) do { } while (0) >> +#define acpi_os_set_prepare_sleep(func, val_a, val_b, ext) do { } while (0) >> #endif >> >> #if defined(CONFIG_ACPI) && defined(CONFIG_PM_RUNTIME) > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- 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