Ben Guthro
2013-Jun-27 15:02 UTC
[PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new extended parameter
Change the function definitions of acpi_os_prepare_sleep() and
acpi_os_set_prepare_sleep() to pass along the new extended sleep
parameter.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com>
Cc: Bob Moore <robert.moore@intel.com>
Cc: Rafaell J. Wysocki <rjw@sisk.pl>
Cc: linux-acpi@vger.kernel.org
---
drivers/acpi/osl.c | 16 ++++++++--------
include/linux/acpi.h | 6 +++---
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index e721863..0251c9b 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger);
extern char line_buf[80];
#endif /*ENABLE_DEBUGGER */
-static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl,
- u32 pm1b_ctrl);
+static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a, u32 val_b,
+ bool extended);
static acpi_osd_handler acpi_irq_handler;
static void *acpi_irq_context;
@@ -1757,13 +1757,13 @@ acpi_status acpi_os_terminate(void)
return AE_OK;
}
-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)
{
int rc = 0;
if (__acpi_os_prepare_sleep)
- rc = __acpi_os_prepare_sleep(sleep_state,
- pm1a_control, pm1b_control);
+ rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b,
+ extended);
if (rc < 0)
return AE_ERROR;
else if (rc > 0)
@@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32
pm1a_control,
return AE_OK;
}
-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, bool extended))
{
__acpi_os_prepare_sleep = func;
}
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 709a2f2..26f9996 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -477,8 +477,8 @@ 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, bool extended));
#ifdef CONFIG_X86
void arch_reserve_mem_area(acpi_physical_address addr, size_t size);
#else
@@ -488,7 +488,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)
--
1.7.9.5
Jan Beulich
2013-Jun-27 15:11 UTC
Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new extended parameter
>>> On 27.06.13 at 17:02, Ben Guthro <benjamin.guthro@citrix.com> wrote: > Change the function definitions of acpi_os_prepare_sleep() and > acpi_os_set_prepare_sleep() to pass along the new extended sleep > parameter. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com> > Cc: Bob Moore <robert.moore@intel.com> > Cc: Rafaell J. Wysocki <rjw@sisk.pl> > Cc: linux-acpi@vger.kernel.org > --- > drivers/acpi/osl.c | 16 ++++++++-------- > include/linux/acpi.h | 6 +++--- > 2 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c > index e721863..0251c9b 100644 > --- a/drivers/acpi/osl.c > +++ b/drivers/acpi/osl.c > @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger); > extern char line_buf[80]; > #endif /*ENABLE_DEBUGGER */ > > -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl, > - u32 pm1b_ctrl); > +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a, u32 val_b, > + bool extended);So from here till patch 5 the build will be half broken because of the type mismatches? I think at least the types of the consumers need to be changed in this patch; leaving the meat of the Xen change to patch 4 is perhaps fine. Jan> > static acpi_osd_handler acpi_irq_handler; > static void *acpi_irq_context; > @@ -1757,13 +1757,13 @@ acpi_status acpi_os_terminate(void) > return AE_OK; > } > > -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) > { > int rc = 0; > if (__acpi_os_prepare_sleep) > - rc = __acpi_os_prepare_sleep(sleep_state, > - pm1a_control, pm1b_control); > + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b, > + extended); > if (rc < 0) > return AE_ERROR; > else if (rc > 0) > @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8 sleep_state, u32 > pm1a_control, > return AE_OK; > } > > -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, bool extended)) > { > __acpi_os_prepare_sleep = func; > } > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index 709a2f2..26f9996 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -477,8 +477,8 @@ 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, bool extended)); > #ifdef CONFIG_X86 > void arch_reserve_mem_area(acpi_physical_address addr, size_t size); > #else > @@ -488,7 +488,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) > -- > 1.7.9.5
Moore, Robert
2013-Jun-27 15:59 UTC
Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new extended parameter
I have not seen a discussion of the details on this, so can someone explain to me just why acpi_os_prepare_sleep is needed, what does it do, and why these changes are being made to ACPICA code? Thanks, Bob> -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Thursday, June 27, 2013 8:12 AM > To: Ben Guthro > Cc: Moore, Robert; xen-devel@lists.xen.org; Konrad Rzeszutek Wilk; Rafaell > J . Wysocki; linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new > extended parameter > > >>> On 27.06.13 at 17:02, Ben Guthro <benjamin.guthro@citrix.com> wrote: > > Change the function definitions of acpi_os_prepare_sleep() and > > acpi_os_set_prepare_sleep() to pass along the new extended sleep > > parameter. > > > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > > Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com> > > Cc: Bob Moore <robert.moore@intel.com> > > Cc: Rafaell J. Wysocki <rjw@sisk.pl> > > Cc: linux-acpi@vger.kernel.org > > --- > > drivers/acpi/osl.c | 16 ++++++++-------- > > include/linux/acpi.h | 6 +++--- > > 2 files changed, 11 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index > > e721863..0251c9b 100644 > > --- a/drivers/acpi/osl.c > > +++ b/drivers/acpi/osl.c > > @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger); extern char > > line_buf[80]; > > #endif /*ENABLE_DEBUGGER */ > > > > -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl, > > - u32 pm1b_ctrl); > > +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a, u32 > val_b, > > + bool extended); > > So from here till patch 5 the build will be half broken because of the > type mismatches? I think at least the types of the consumers need to be > changed in this patch; leaving the meat of the Xen change to patch 4 is > perhaps fine. > > Jan > > > > > static acpi_osd_handler acpi_irq_handler; static void > > *acpi_irq_context; @@ -1757,13 +1757,13 @@ acpi_status > > acpi_os_terminate(void) > > return AE_OK; > > } > > > > -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) > > { > > int rc = 0; > > if (__acpi_os_prepare_sleep) > > - rc = __acpi_os_prepare_sleep(sleep_state, > > - pm1a_control, pm1b_control); > > + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b, > > + extended); > > if (rc < 0) > > return AE_ERROR; > > else if (rc > 0) > > @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8 > > sleep_state, u32 pm1a_control, > > return AE_OK; > > } > > > > -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, bool extended)) > > { > > __acpi_os_prepare_sleep = func; > > } > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h index > > 709a2f2..26f9996 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -477,8 +477,8 @@ 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, bool extended)); > > #ifdef CONFIG_X86 > > void arch_reserve_mem_area(acpi_physical_address addr, size_t size); > > #else @@ -488,7 +488,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) > > -- > > 1.7.9.5 > >
Ben Guthro
2013-Jun-27 16:12 UTC
Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new extended parameter
On 06/27/2013 11:59 AM, Moore, Robert wrote:> I have not seen a discussion of the details on this, so can someone explain to me just why acpi_os_prepare_sleep is needed, what does it do, and why these changes are being made to ACPICA code?Hi Bob, I''m not familiar with your background here, so I apologize if I am stating obvious things below. acpi_os_prepare_sleep() has been in the acpica code for some time now, allowing for OS specific hooks to account for differences between OS architectures. Specifically, it has been in since: commit 09f98a825a821f7a3f1b162f9ed023f37213a63b Author: Tang Liang <liang.tang@oracle.com> Date: Fri Dec 9 10:05:54 2011 +0800 x86, acpi, tboot: Have a ACPI os prepare sleep instead of calling tboot_sleep. The ACPI suspend path makes a call to tboot_sleep right before it writes the PM1A, PM1B values. We replace the direct call to tboot via an registration callback similar to __acpi_register_gsi. CC: Len Brown <len.brown@intel.com> Acked-by: Joseph Cihula <joseph.cihula@intel.com> Acked-by: Rafael J. Wysocki <rjw@sisk.pl> [v1: Added __attribute__ ((unused))] [v2: Introduced a wrapper instead of changing tboot_sleep return values] [v3: Added return value AE_CTRL_SKIP for acpi_os_sleep_prepare] Signed-off-by: Tang Liang <liang.tang@oracle.com> [v1: Fix compile issues on IA64 and PPC64] [v2: Fix where __acpi_os_prepare_sleep==NULL and did not go in sleep properly] Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> In this case, the Xen hypervisor, and not linux needs to actually put the system into S3, so this hook gives the architecture to do so. This change does not introduce this functionality, but simply moves the code around into the proper locations, such that the acpica code no longer need to include linux specific headers. I hope this helps. If you have specific questions, please let me know. Regards, Ben> > Thanks, > Bob > > >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: Thursday, June 27, 2013 8:12 AM >> To: Ben Guthro >> Cc: Moore, Robert; xen-devel@lists.xen.org; Konrad Rzeszutek Wilk; Rafaell >> J . Wysocki; linux-acpi@vger.kernel.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new >> extended parameter >> >>>>> On 27.06.13 at 17:02, Ben Guthro <benjamin.guthro@citrix.com> wrote: >>> Change the function definitions of acpi_os_prepare_sleep() and >>> acpi_os_set_prepare_sleep() to pass along the new extended sleep >>> parameter. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com> >>> Cc: Bob Moore <robert.moore@intel.com> >>> Cc: Rafaell J. Wysocki <rjw@sisk.pl> >>> Cc: linux-acpi@vger.kernel.org >>> --- >>> drivers/acpi/osl.c | 16 ++++++++-------- >>> include/linux/acpi.h | 6 +++--- >>> 2 files changed, 11 insertions(+), 11 deletions(-) >>> >>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index >>> e721863..0251c9b 100644 >>> --- a/drivers/acpi/osl.c >>> +++ b/drivers/acpi/osl.c >>> @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger); extern char >>> line_buf[80]; >>> #endif /*ENABLE_DEBUGGER */ >>> >>> -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl, >>> - u32 pm1b_ctrl); >>> +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a, u32 >> val_b, >>> + bool extended); >> >> So from here till patch 5 the build will be half broken because of the >> type mismatches? I think at least the types of the consumers need to be >> changed in this patch; leaving the meat of the Xen change to patch 4 is >> perhaps fine. >> >> Jan >> >>> >>> static acpi_osd_handler acpi_irq_handler; static void >>> *acpi_irq_context; @@ -1757,13 +1757,13 @@ acpi_status >>> acpi_os_terminate(void) >>> return AE_OK; >>> } >>> >>> -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) >>> { >>> int rc = 0; >>> if (__acpi_os_prepare_sleep) >>> - rc = __acpi_os_prepare_sleep(sleep_state, >>> - pm1a_control, pm1b_control); >>> + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b, >>> + extended); >>> if (rc < 0) >>> return AE_ERROR; >>> else if (rc > 0) >>> @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8 >>> sleep_state, u32 pm1a_control, >>> return AE_OK; >>> } >>> >>> -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, bool extended)) >>> { >>> __acpi_os_prepare_sleep = func; >>> } >>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h index >>> 709a2f2..26f9996 100644 >>> --- a/include/linux/acpi.h >>> +++ b/include/linux/acpi.h >>> @@ -477,8 +477,8 @@ 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, bool extended)); >>> #ifdef CONFIG_X86 >>> void arch_reserve_mem_area(acpi_physical_address addr, size_t size); >>> #else @@ -488,7 +488,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) >>> -- >>> 1.7.9.5 >> >> >
Moore, Robert
2013-Jun-27 20:19 UTC
Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new extended parameter
I''m the ACPICA owner. The issue I have is that acpi_os_prepare_sleep causes a divergence between the raw ACPICA source code and the "linux" version of the ACPICA code. If it is possible to implement the functionality that is needed without changes to ACPICA, then that would be best. If not, we are willing to integrate this interface into ACPICA. However, this has an impact on all operating systems that use ACPICA, so this is always the last resort. Bob> -----Original Message----- > From: Ben Guthro [mailto:Benjamin.Guthro@citrix.com] > Sent: Thursday, June 27, 2013 9:13 AM > To: Moore, Robert > Cc: Jan Beulich; Zheng, Lv; Box, David E; Brown, Len; xen- > devel@lists.xen.org; Konrad Rzeszutek Wilk; Rafaell J . Wysocki; linux- > acpi@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new > extended parameter > > > > On 06/27/2013 11:59 AM, Moore, Robert wrote: > > I have not seen a discussion of the details on this, so can someone > explain to me just why acpi_os_prepare_sleep is needed, what does it do, > and why these changes are being made to ACPICA code? > > Hi Bob, > > I''m not familiar with your background here, so I apologize if I am stating > obvious things below. > > acpi_os_prepare_sleep() has been in the acpica code for some time now, > allowing for OS specific hooks to account for differences between OS > architectures. > > Specifically, it has been in since: > > commit 09f98a825a821f7a3f1b162f9ed023f37213a63b > Author: Tang Liang <liang.tang@oracle.com> > Date: Fri Dec 9 10:05:54 2011 +0800 > > x86, acpi, tboot: Have a ACPI os prepare sleep instead of calling > tboot_sleep. > > The ACPI suspend path makes a call to tboot_sleep right before > it writes the PM1A, PM1B values. We replace the direct call to > tboot via an registration callback similar to __acpi_register_gsi. > > CC: Len Brown <len.brown@intel.com> > Acked-by: Joseph Cihula <joseph.cihula@intel.com> > Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > [v1: Added __attribute__ ((unused))] > [v2: Introduced a wrapper instead of changing tboot_sleep return > values] > [v3: Added return value AE_CTRL_SKIP for acpi_os_sleep_prepare] > Signed-off-by: Tang Liang <liang.tang@oracle.com> > [v1: Fix compile issues on IA64 and PPC64] > [v2: Fix where __acpi_os_prepare_sleep==NULL and did not go in sleep > properly] > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > > > In this case, the Xen hypervisor, and not linux needs to actually put the > system into S3, so this hook gives the architecture to do so. > > This change does not introduce this functionality, but simply moves the > code around into the proper locations, such that the acpica code no longer > need to include linux specific headers. > > I hope this helps. > If you have specific questions, please let me know. > > Regards, > Ben > > > > > Thanks, > > Bob > > > > > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Thursday, June 27, 2013 8:12 AM > >> To: Ben Guthro > >> Cc: Moore, Robert; xen-devel@lists.xen.org; Konrad Rzeszutek Wilk; > >> Rafaell J . Wysocki; linux-acpi@vger.kernel.org; > >> linux-kernel@vger.kernel.org > >> Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to > >> new extended parameter > >> > >>>>> On 27.06.13 at 17:02, Ben Guthro <benjamin.guthro@citrix.com> wrote: > >>> Change the function definitions of acpi_os_prepare_sleep() and > >>> acpi_os_set_prepare_sleep() to pass along the new extended sleep > >>> parameter. > >>> > >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >>> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com> > >>> Cc: Bob Moore <robert.moore@intel.com> > >>> Cc: Rafaell J. Wysocki <rjw@sisk.pl> > >>> Cc: linux-acpi@vger.kernel.org > >>> --- > >>> drivers/acpi/osl.c | 16 ++++++++-------- > >>> include/linux/acpi.h | 6 +++--- > >>> 2 files changed, 11 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index > >>> e721863..0251c9b 100644 > >>> --- a/drivers/acpi/osl.c > >>> +++ b/drivers/acpi/osl.c > >>> @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger); extern char > >>> line_buf[80]; > >>> #endif /*ENABLE_DEBUGGER */ > >>> > >>> -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl, > >>> - u32 pm1b_ctrl); > >>> +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a, > >>> +u32 > >> val_b, > >>> + bool extended); > >> > >> So from here till patch 5 the build will be half broken because of > >> the type mismatches? I think at least the types of the consumers need > >> to be changed in this patch; leaving the meat of the Xen change to > >> patch 4 is perhaps fine. > >> > >> Jan > >> > >>> > >>> static acpi_osd_handler acpi_irq_handler; static void > >>> *acpi_irq_context; @@ -1757,13 +1757,13 @@ acpi_status > >>> acpi_os_terminate(void) > >>> return AE_OK; > >>> } > >>> > >>> -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) > >>> { > >>> int rc = 0; > >>> if (__acpi_os_prepare_sleep) > >>> - rc = __acpi_os_prepare_sleep(sleep_state, > >>> - pm1a_control, pm1b_control); > >>> + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b, > >>> + extended); > >>> if (rc < 0) > >>> return AE_ERROR; > >>> else if (rc > 0) > >>> @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8 > >>> sleep_state, u32 pm1a_control, > >>> return AE_OK; > >>> } > >>> > >>> -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, bool extended)) > >>> { > >>> __acpi_os_prepare_sleep = func; > >>> } > >>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h index > >>> 709a2f2..26f9996 100644 > >>> --- a/include/linux/acpi.h > >>> +++ b/include/linux/acpi.h > >>> @@ -477,8 +477,8 @@ 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, bool extended)); > >>> #ifdef CONFIG_X86 > >>> void arch_reserve_mem_area(acpi_physical_address addr, size_t > >>> size); #else @@ -488,7 +488,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) > >>> -- > >>> 1.7.9.5 > >> > >> > >
Ben Guthro
2013-Jun-27 20:35 UTC
Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new extended parameter
On 06/27/2013 04:19 PM, Moore, Robert wrote:> I''m the ACPICA owner. >Apologies, I should have looked that up when Rafael asked for you to be CC''ed> The issue I have is that acpi_os_prepare_sleep causes a divergence between the raw ACPICA source code and the "linux" version of the ACPICA code. >Understood. The v2 version of this patchset was discussed with Jan Beulich here: http://www.gossamer-threads.com/lists/xen/devel/274507 In this thread, Rafael suggests that these changes be made, and submitted through linux-acpi, with he, and you CC''ed. This is more of a cleanup effort to existing functionality, that did not get coordinated. That is, as the comment indicates, in linux 3.4, reduced hardware sleep was introduced as part of ACPIv5. In parallel, acpi_os_prepare_sleep() was also introduced, to allow for subsystems such as Xen, and tboot to take advantage of the vast majority of ACPICA, but hook some low level capabilities via function pointer, to do the specific things for those subsystems. Rafael rightly pointed out that inclusion of linux/acpi.h in ACPICA code is wrong, and we should make efforts to avoid doing so. This patch is to clean this up, and also to extend the functionality to also go down the same path for the reduced hardware sleep path, passing that "extended" flag down to that subsystem, so that it might make use of it at the very low levels. All efforts are being made to reduce the amount of churn in ACPICA, since it is an upstream project to linux.> If it is possible to implement the functionality that is needed without changes to ACPICA, then that would be best. If not, we are willing to integrate this interface into ACPICA. However, this has an impact on all operating systems that use ACPICA, so this is always the last resort. >I am unaware of a way to take advantage of the infrastructure that ACPICA provides, and also do that which is necessary to make it work in Xen / tboot without these hooks. It would be greatly appreciated if you would consider this new interface for inclusion. Thanks for your time. Ben> Bob > > >> -----Original Message----- >> From: Ben Guthro [mailto:Benjamin.Guthro@citrix.com] >> Sent: Thursday, June 27, 2013 9:13 AM >> To: Moore, Robert >> Cc: Jan Beulich; Zheng, Lv; Box, David E; Brown, Len; xen- >> devel@lists.xen.org; Konrad Rzeszutek Wilk; Rafaell J . Wysocki; linux- >> acpi@vger.kernel.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to new >> extended parameter >> >> >> >> On 06/27/2013 11:59 AM, Moore, Robert wrote: >>> I have not seen a discussion of the details on this, so can someone >> explain to me just why acpi_os_prepare_sleep is needed, what does it do, >> and why these changes are being made to ACPICA code? >> >> Hi Bob, >> >> I''m not familiar with your background here, so I apologize if I am stating >> obvious things below. >> >> acpi_os_prepare_sleep() has been in the acpica code for some time now, >> allowing for OS specific hooks to account for differences between OS >> architectures. >> >> Specifically, it has been in since: >> >> commit 09f98a825a821f7a3f1b162f9ed023f37213a63b >> Author: Tang Liang <liang.tang@oracle.com> >> Date: Fri Dec 9 10:05:54 2011 +0800 >> >> x86, acpi, tboot: Have a ACPI os prepare sleep instead of calling >> tboot_sleep. >> >> The ACPI suspend path makes a call to tboot_sleep right before >> it writes the PM1A, PM1B values. We replace the direct call to >> tboot via an registration callback similar to __acpi_register_gsi. >> >> CC: Len Brown <len.brown@intel.com> >> Acked-by: Joseph Cihula <joseph.cihula@intel.com> >> Acked-by: Rafael J. Wysocki <rjw@sisk.pl> >> [v1: Added __attribute__ ((unused))] >> [v2: Introduced a wrapper instead of changing tboot_sleep return >> values] >> [v3: Added return value AE_CTRL_SKIP for acpi_os_sleep_prepare] >> Signed-off-by: Tang Liang <liang.tang@oracle.com> >> [v1: Fix compile issues on IA64 and PPC64] >> [v2: Fix where __acpi_os_prepare_sleep==NULL and did not go in sleep >> properly] >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> >> >> >> In this case, the Xen hypervisor, and not linux needs to actually put the >> system into S3, so this hook gives the architecture to do so. >> >> This change does not introduce this functionality, but simply moves the >> code around into the proper locations, such that the acpica code no longer >> need to include linux specific headers. >> >> I hope this helps. >> If you have specific questions, please let me know. >> >> Regards, >> Ben >> >>> >>> Thanks, >>> Bob >>> >>> >>>> -----Original Message----- >>>> From: Jan Beulich [mailto:JBeulich@suse.com] >>>> Sent: Thursday, June 27, 2013 8:12 AM >>>> To: Ben Guthro >>>> Cc: Moore, Robert; xen-devel@lists.xen.org; Konrad Rzeszutek Wilk; >>>> Rafaell J . Wysocki; linux-acpi@vger.kernel.org; >>>> linux-kernel@vger.kernel.org >>>> Subject: Re: [PATCH v4 3/5] acpi: Adjust linux acpi OS functions to >>>> new extended parameter >>>> >>>>>>> On 27.06.13 at 17:02, Ben Guthro <benjamin.guthro@citrix.com> wrote: >>>>> Change the function definitions of acpi_os_prepare_sleep() and >>>>> acpi_os_set_prepare_sleep() to pass along the new extended sleep >>>>> parameter. >>>>> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>>> Signed-off-by: Ben Guthro <benjamin.guthro@citrix.com> >>>>> Cc: Bob Moore <robert.moore@intel.com> >>>>> Cc: Rafaell J. Wysocki <rjw@sisk.pl> >>>>> Cc: linux-acpi@vger.kernel.org >>>>> --- >>>>> drivers/acpi/osl.c | 16 ++++++++-------- >>>>> include/linux/acpi.h | 6 +++--- >>>>> 2 files changed, 11 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index >>>>> e721863..0251c9b 100644 >>>>> --- a/drivers/acpi/osl.c >>>>> +++ b/drivers/acpi/osl.c >>>>> @@ -77,8 +77,8 @@ EXPORT_SYMBOL(acpi_in_debugger); extern char >>>>> line_buf[80]; >>>>> #endif /*ENABLE_DEBUGGER */ >>>>> >>>>> -static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 pm1a_ctrl, >>>>> - u32 pm1b_ctrl); >>>>> +static int (*__acpi_os_prepare_sleep)(u8 sleep_state, u32 val_a, >>>>> +u32 >>>> val_b, >>>>> + bool extended); >>>> >>>> So from here till patch 5 the build will be half broken because of >>>> the type mismatches? I think at least the types of the consumers need >>>> to be changed in this patch; leaving the meat of the Xen change to >>>> patch 4 is perhaps fine. >>>> >>>> Jan >>>> >>>>> >>>>> static acpi_osd_handler acpi_irq_handler; static void >>>>> *acpi_irq_context; @@ -1757,13 +1757,13 @@ acpi_status >>>>> acpi_os_terminate(void) >>>>> return AE_OK; >>>>> } >>>>> >>>>> -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) >>>>> { >>>>> int rc = 0; >>>>> if (__acpi_os_prepare_sleep) >>>>> - rc = __acpi_os_prepare_sleep(sleep_state, >>>>> - pm1a_control, pm1b_control); >>>>> + rc = __acpi_os_prepare_sleep(sleep_state, val_a, val_b, >>>>> + extended); >>>>> if (rc < 0) >>>>> return AE_ERROR; >>>>> else if (rc > 0) >>>>> @@ -1772,8 +1772,8 @@ acpi_status acpi_os_prepare_sleep(u8 >>>>> sleep_state, u32 pm1a_control, >>>>> return AE_OK; >>>>> } >>>>> >>>>> -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, bool extended)) >>>>> { >>>>> __acpi_os_prepare_sleep = func; >>>>> } >>>>> diff --git a/include/linux/acpi.h b/include/linux/acpi.h index >>>>> 709a2f2..26f9996 100644 >>>>> --- a/include/linux/acpi.h >>>>> +++ b/include/linux/acpi.h >>>>> @@ -477,8 +477,8 @@ 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, bool extended)); >>>>> #ifdef CONFIG_X86 >>>>> void arch_reserve_mem_area(acpi_physical_address addr, size_t >>>>> size); #else @@ -488,7 +488,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) >>>>> -- >>>>> 1.7.9.5 >>>> >>>> >>>