Re: [PATCH 2/7] tboot: Add return values for tboot_sleep
From: Cihula, Joseph <hidden>
Date: 2012-01-26 00:16:44
Also in:
linux-acpi, lkml
From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] Sent: Thursday, January 12, 2012 12:23 PM
eturn values for tboot_sleep
On Thu, Jan 12, 2012 at 09:49:58AM -0800, Konrad Rzeszutek Wilk wrote:quoted
On Tue, Jan 10, 2012 at 08:10:53PM +0000, Cihula, Joseph wrote:quoted
ACK, but tboot_sleep() calls tboot_shutdown() and there are error conditions in that which youare not reflecting upward--i.e. you should make tboot_shutdown() return an 'int' and propagate its errors through tboot_sleep().quoted
Hey Joe, Thanks for looking at the patches. Right now [with this patch applied] the code looks as so: 297 298 tboot_shutdown(acpi_shutdown_map[sleep_state]); 299 return 0; 300 } If we do make tboot_shutdown() return an int, there will be a need to modify other callers: native_machine_emergency_restart, native_machine_halt, native_machine_power_off, and native_play_dead as well. Perhaps that could be done in another patch and leave this one as is where the 'tboot_sleep' would just return 0 instead of 'return tboot_shutdown(...)' ? Perhaps another way to do this is to extract the common code of tboot_sleep and tboot_shutdown?
I'd be happier to see the callers above changed to handle a tboot_shutdown() that returned an error than splitting this up and only checking the error in one path. Joe
quoted hunk ↗ jump to hunk
Like this (not yet tested):diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c index 6410744..2f93a50 100644 --- a/arch/x86/kernel/tboot.c +++ b/arch/x86/kernel/tboot.c@@ -212,17 +212,16 @@ static int tboot_setup_sleep(void) { /* S3 shutdown requested, but S3 not supported by the kernel... */ BUG(); - return -1; + return -ENOSYS; } #endif -void tboot_shutdown(u32 shutdown_type) -{ - void (*shutdown)(void); +static int tboot_check(u32 shutdown_type) { + if (!tboot_enabled()) - return; + return -ENODEV; /* * if we're being called before the 1:1 mapping is set up then just @@ -230,12 +229,21 @@void tboot_shutdown(u32 shutdown_type) * due to very early panic() */ if (!tboot_pg_dir) - return; + return -EBUSY; /* if this is S3 then set regions to MAC */ - if (shutdown_type == TB_SHUTDOWN_S3) - if (tboot_setup_sleep()) - return; + if (shutdown_type == TB_SHUTDOWN_S3) { + int err = tboot_setup_sleep(); + if (err) + return err; + } + + return 0; +} + +static void __tboot_shutdown(u32 shutdown_type) { + void (*shutdown)(void); tboot->shutdown_type = shutdown_type;@@ -248,6 +256,13 @@ void tboot_shutdown(u32 shutdown_type) while (1) halt(); } +void tboot_shutdown(u32 shutdown_type) +{ + if (!tboot_check(shutdown_type)) + return; + + __tboot_shutdown(shutdown_type); +} static void tboot_copy_fadt(const struct acpi_table_fadt *fadt) { @@ -279,9 +294,17 @@ staticint tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control) /* S3: */ TB_SHUTDOWN_S3, /* S4: */ TB_SHUTDOWN_S4, /* S5: */ TB_SHUTDOWN_S5 }; + int err; - if (!tboot_enabled()) - return 0; + if (sleep_state >= ACPI_S_STATE_COUNT || + acpi_shutdown_map[sleep_state] == -1) { + pr_warning("unsupported sleep state 0x%x\n", sleep_state); + return -ENOSYS; + } + + err = tboot_check(acpi_shutdown_map[sleep_state]); + if (!err && err != EBUSY) + return err; tboot_copy_fadt(&acpi_gbl_FADT); tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control; @@ -289,13 +312,7 @@ static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control) /* we always use the 32b wakeup vector */ tboot->acpi_sinfo.vector_width = 32; - if (sleep_state >= ACPI_S_STATE_COUNT || - acpi_shutdown_map[sleep_state] == -1) { - pr_warning("unsupported sleep state 0x%x\n", sleep_state); - return -1; - } - - tboot_shutdown(acpi_shutdown_map[sleep_state]); + __tboot_shutdown((acpi_shutdown_map[sleep_state])); return 0; }