> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx] > 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: > > On Tue, Jan 10, 2012 at 08:10:53PM +0000, Cihula, Joseph wrote: > > > ACK, but tboot_sleep() calls tboot_shutdown() and there are error conditions in that which you > are not reflecting upward--i.e. you should make tboot_shutdown() return an 'int' and propagate its > errors through tboot_sleep(). > > > > 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 > > 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 @@ static > int 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; > } > -- 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
