lkml.org 
[lkml]   [2012]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH 2/7] tboot: Add return values for tboot_sleep
    Date


    > -----Original Message-----
    > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
    > Sent: Friday, February 03, 2012 12:05 PM
    > To: Cihula, Joseph
    > Cc: linux-kernel@vger.kernel.org; rjw@sisk.pl; x86@kernel.org; Brown, Len; linux-pm@lists.linux-
    > foundation.org; tboot-devel@lists.sourceforge.net; linux-acpi@vger.kernel.org;
    > liang.tang@oracle.com; hpa@zytor.com
    > Subject: Re: [PATCH 2/7] tboot: Add return values for tboot_sleep
    >
    > On Thu, Jan 26, 2012 at 12:16:39AM +0000, Cihula, Joseph wrote:
    > > > 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:
    > > > > 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.
    >
    > OK, so something like this then:

    Looks good.

    Joe

    >
    > From da60a441fd62aee83763758455c08c281893ae93 Mon Sep 17 00:00:00 2001
    > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    > Date: Thu, 8 Dec 2011 17:14:08 +0800
    > Subject: [PATCH] tboot: Add return values for tboot_sleep and consequently tboot_shutdown
    >
    > .. as appropiately. As tboot_sleep now returns values.
    > remove tboot_sleep_wrapper.
    >
    > Suggested-by: "Rafael J. Wysocki" <rjw@sisk.pl>
    > Acked-by: Joseph Cihula <joseph.cihula@intel.com>
    > [v1: Return -1/0/+1 instead of ACPI_xx values]
    > [v2: Also make tboot_shutdown return error code]
    > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
    > ---
    > arch/x86/kernel/reboot.c | 6 +++---
    > arch/x86/kernel/smpboot.c | 2 +-
    > arch/x86/kernel/tboot.c | 33 +++++++++++++++------------------
    > include/linux/tboot.h | 4 ++--
    > 4 files changed, 21 insertions(+), 24 deletions(-)
    >
    > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c index 37a458b..b29326a 100644
    > --- a/arch/x86/kernel/reboot.c
    > +++ b/arch/x86/kernel/reboot.c
    > @@ -549,7 +549,7 @@ static void native_machine_emergency_restart(void)
    > if (reboot_emergency)
    > emergency_vmx_disable_all();
    >
    > - tboot_shutdown(TB_SHUTDOWN_REBOOT);
    > + WARN_ON(tboot_shutdown(TB_SHUTDOWN_REBOOT));
    >
    > /* Tell the BIOS if we want cold or warm reboot */
    > *((unsigned short *)__va(0x472)) = reboot_mode; @@ -684,7 +684,7 @@ static void
    > native_machine_halt(void)
    > /* stop other cpus and apics */
    > machine_shutdown();
    >
    > - tboot_shutdown(TB_SHUTDOWN_HALT);
    > + WARN_ON(tboot_shutdown(TB_SHUTDOWN_HALT));
    >
    > /* stop this cpu */
    > stop_this_cpu(NULL);
    > @@ -698,7 +698,7 @@ static void native_machine_power_off(void)
    > pm_power_off();
    > }
    > /* a fallback in case there is no PM info available */
    > - tboot_shutdown(TB_SHUTDOWN_HALT);
    > + WARN_ON(tboot_shutdown(TB_SHUTDOWN_HALT));
    > }
    >
    > struct machine_ops machine_ops = {
    > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 66d250c..9aa903a 100644
    > --- a/arch/x86/kernel/smpboot.c
    > +++ b/arch/x86/kernel/smpboot.c
    > @@ -1419,7 +1419,7 @@ static inline void hlt_play_dead(void) void native_play_dead(void) {
    > play_dead_common();
    > - tboot_shutdown(TB_SHUTDOWN_WFS);
    > + WARN_ON(tboot_shutdown(TB_SHUTDOWN_WFS));
    >
    > mwait_play_dead(); /* Only returns on failure */
    > hlt_play_dead();
    > diff --git a/arch/x86/kernel/tboot.c b/arch/x86/kernel/tboot.c index 1a4ab7d..9504a7a 100644
    > --- a/arch/x86/kernel/tboot.c
    > +++ b/arch/x86/kernel/tboot.c
    > @@ -217,12 +217,12 @@ static int tboot_setup_sleep(void)
    >
    > #endif
    >
    > -void tboot_shutdown(u32 shutdown_type)
    > +int tboot_shutdown(u32 shutdown_type)
    > {
    > void (*shutdown)(void);
    >
    > if (!tboot_enabled())
    > - return;
    > + return 0;
    >
    > /*
    > * if we're being called before the 1:1 mapping is set up then just @@ -230,13 +230,14 @@
    > void tboot_shutdown(u32 shutdown_type)
    > * due to very early panic()
    > */
    > if (!tboot_pg_dir)
    > - return;
    > + return 0;
    >
    > /* 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 rc = tboot_setup_sleep();
    > + if (rc)
    > + return rc;
    > + }
    > tboot->shutdown_type = shutdown_type;
    >
    > switch_to_tboot_pt();
    > @@ -247,6 +248,8 @@ void tboot_shutdown(u32 shutdown_type)
    > /* should not reach here */
    > while (1)
    > halt();
    > +
    > + return 0;
    > }
    >
    > static void tboot_copy_fadt(const struct acpi_table_fadt *fadt) @@ -272,7 +275,7 @@ static void
    > tboot_copy_fadt(const struct acpi_table_fadt *fadt)
    > offsetof(struct acpi_table_facs, firmware_waking_vector); }
    >
    > -void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
    > +static int tboot_sleep(u8 sleep_state, u32 pm1a_control, u32
    > +pm1b_control)
    > {
    > static u32 acpi_shutdown_map[ACPI_S_STATE_COUNT] = {
    > /* S0,1,2: */ -1, -1, -1,
    > @@ -281,7 +284,7 @@ void tboot_sleep(u8 sleep_state, u32 pm1a_control, u32 pm1b_control)
    > /* S5: */ TB_SHUTDOWN_S5 };
    >
    > if (!tboot_enabled())
    > - return;
    > + return 0;
    >
    > tboot_copy_fadt(&acpi_gbl_FADT);
    > tboot->acpi_sinfo.pm1a_cnt_val = pm1a_control; @@ -292,16 +295,10 @@ void tboot_sleep(u8
    > sleep_state, u32 pm1a_control, u32 pm1b_control)
    > if (sleep_state >= ACPI_S_STATE_COUNT ||
    > acpi_shutdown_map[sleep_state] == -1) {
    > pr_warning("unsupported sleep state 0x%x\n", sleep_state);
    > - return;
    > + return -1;
    > }
    >
    > - tboot_shutdown(acpi_shutdown_map[sleep_state]);
    > -}
    > -static int tboot_sleep_wrapper(u8 sleep_state, u32 pm1a_control,
    > - u32 pm1b_control)
    > -{
    > - tboot_sleep(sleep_state, pm1a_control, pm1b_control);
    > - return 0;
    > + return tboot_shutdown(acpi_shutdown_map[sleep_state]);
    > }
    >
    > static atomic_t ap_wfs_count;
    > @@ -352,7 +349,7 @@ static __init int tboot_late_init(void)
    > atomic_set(&ap_wfs_count, 0);
    > register_hotcpu_notifier(&tboot_cpu_notifier);
    >
    > - acpi_os_set_prepare_sleep(&tboot_sleep_wrapper);
    > + acpi_os_set_prepare_sleep(&tboot_sleep);
    > return 0;
    > }
    >
    > diff --git a/include/linux/tboot.h b/include/linux/tboot.h index c75128b..8f14fe0 100644
    > --- a/include/linux/tboot.h
    > +++ b/include/linux/tboot.h
    > @@ -142,7 +142,7 @@ static inline int tboot_enabled(void) }
    >
    > extern void tboot_probe(void);
    > -extern void tboot_shutdown(u32 shutdown_type);
    > +extern int tboot_shutdown(u32 shutdown_type);
    > extern struct acpi_table_header *tboot_get_dmar_table(
    > struct acpi_table_header *dmar_tbl); extern int
    > tboot_force_iommu(void); @@ -151,7 +151,7 @@ extern int tboot_force_iommu(void);
    >
    > #define tboot_enabled() 0
    > #define tboot_probe() do { } while (0)
    > -#define tboot_shutdown(shutdown_type) do { } while (0)
    > +#define tboot_shutdown(shutdown_type) 0
    > #define tboot_sleep(sleep_state, pm1a_control, pm1b_control) \
    > do { } while (0)
    > #define tboot_get_dmar_table(dmar_tbl) (dmar_tbl)
    > --
    > 1.7.7.5



    \
     
     \ /
      Last update: 2012-03-19 20:21    [W:3.480 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site