[PATCH v6 0/6] Hardware Reduced Mode Cleanup for ACPI

[PATCH v6 0/6] Hardware Reduced Mode Cleanup for ACPI

[al.stone]

From: Al Stone <al.stone@linaro.org>

This series of patches starts with Hanjun's patch to create a kernel
config item for CONFIG_ACPI_REDUCED_HARDWARE [0].  Building on that, I
then reviewed all of the code that touched any of several fields in the
FADT that the OSPM is supposed to ignore when ACPI is in Hardware Reduced
mode [1].  Any time there was a use of one of the fields to be ignored,
I evaluated whether or not the code was implementing Hardware Reduced
mode correctly.  Similarly, for each the flags in the FADT flags field
that are to be ignored in Hardware Reduced mode, the kernel code was again
scanned for proper usage.  The remainder of the patches are to fix all of
the situations I could find where the kernel would not behave correctly
in this ACPI mode.

These seem to work just fine on the RTSM model for ARMv7, both with and
without ACPI enabled, and with and without ACPI_REDUCED_HARDWARE enabled;
similarly for the FVP model for ARMv8.  The patches for ACPI on ARM
hardware have been submitted elsewhere but they presume that reduced HW
mode is functioning correctly.  In the meantime, there's no way I can think
of to test all possible scenarios so feedback would be greatly appreciated.


[0] List at https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/AcpiReducedHw#Section_5:_ACPI_Software_Programming_Model
[1] Please see the ACPI Specification v5.0 for details on Hardware Reduced
    mode (sections 3.11.1, 4.1, 5.2.9, at a minimum).

Changes for v6:
   -- Allow selection of CONFIG_ACPI_REDUCED_HARDWARE_ONLY to be more
      architecture-independent, but make it harder to accidentally enable.
   -- Make sure that ACPI ECs do not use the ACPI global lock when in
      hardware reduced mode (I had missed this case in earlier reviews
      of the kernel tree).

Changes for v5:
   -- Clarify that if the kernel config option to build ACPI hardware reduced
      mode is used, it builds a hardware reduced *only* kernel (i.e., full
      legacy ACPI mode will no longer work).

Changes for v4:
   -- Given the current state of ACPICA, disable CONFIG_ACPI_REDUCED_HARDWARE
      for use on anything other than ARM.
   -- Replaced #ifdefs with run-time checking for hardware reduced mode,
      whenever possible

Changes for v3:
   -- Modified enabling ACPI_REDUCED_HARDWARE in ACPICA when using
      kernel config item CONFIG_ACPI_REDUCED_HARDWARE; now consistent
      with ACPICA code base where needed
   -- Enable X86 for CONFIG_ACPI_REDUCED_HARDWARE
   -- Minimize bus master reload patching
   -- Remove unneeded patch for dmi_check_system() (was 4/6)
   -- Correct the patch for removing unneeded map/unmap of FADT fields

Changes for v2:
   -- Remove patch that was outside of reduced HW mode changes
   -- Simplify CONFIG_ACPI_REDUCED_HARDWARE in Kconfig
   -- Simplify use of CONFIG_ACPI_REDUCED_HARDWARE in #ifdefs
   -- Ensure changelogs are present
   -- Combine and simplify previous patches 8 & 10


Al Stone (6):
  ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI
    mode
  ACPI: bus master reload not supported in reduced HW mode
  ACPI: HW reduced mode does not allow use of the FADT sci_interrupt
    field
  ACPI: in HW reduced mode, using FADT PM information is not allowed.
  ACPI: do not map/unmap memory regions for FADT entries in reduced HW
    mode
  ACPI: make sure ECs do not use the ACPI global lock

 drivers/acpi/Kconfig            | 12 ++++++++++++
 drivers/acpi/bus.c              | 30 ++++++++++++++++--------------
 drivers/acpi/ec.c               | 12 ++++++++++--
 drivers/acpi/osl.c              | 38 +++++++++++++++++++-------------------
 drivers/acpi/pci_link.c         |  2 ++
 drivers/acpi/processor_idle.c   | 14 ++++++++++++--
 include/acpi/platform/aclinux.h |  6 ++++++
 7 files changed, 77 insertions(+), 37 deletions(-)

-- 
1.8.4.2

[PATCH v6 0/6] Hardware Reduced Mode Cleanup for ACPI

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

This series of patches starts with Hanjun's patch to create a kernel
config item for CONFIG_ACPI_REDUCED_HARDWARE [0].  Building on that, I
then reviewed all of the code that touched any of several fields in the
FADT that the OSPM is supposed to ignore when ACPI is in Hardware Reduced
mode [1].  Any time there was a use of one of the fields to be ignored,
I evaluated whether or not the code was implementing Hardware Reduced
mode correctly.  Similarly, for each the flags in the FADT flags field
that are to be ignored in Hardware Reduced mode, the kernel code was again
scanned for proper usage.  The remainder of the patches are to fix all of
the situations I could find where the kernel would not behave correctly
in this ACPI mode.

These seem to work just fine on the RTSM model for ARMv7, both with and
without ACPI enabled, and with and without ACPI_REDUCED_HARDWARE enabled;
similarly for the FVP model for ARMv8.  The patches for ACPI on ARM
hardware have been submitted elsewhere but they presume that reduced HW
mode is functioning correctly.  In the meantime, there's no way I can think
of to test all possible scenarios so feedback would be greatly appreciated.


[0] List at https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/AcpiReducedHw#Section_5:_ACPI_Software_Programming_Model
[1] Please see the ACPI Specification v5.0 for details on Hardware Reduced
    mode (sections 3.11.1, 4.1, 5.2.9, at a minimum).

Changes for v6:
   -- Allow selection of CONFIG_ACPI_REDUCED_HARDWARE_ONLY to be more
      architecture-independent, but make it harder to accidentally enable.
   -- Make sure that ACPI ECs do not use the ACPI global lock when in
      hardware reduced mode (I had missed this case in earlier reviews
      of the kernel tree).

Changes for v5:
   -- Clarify that if the kernel config option to build ACPI hardware reduced
      mode is used, it builds a hardware reduced *only* kernel (i.e., full
      legacy ACPI mode will no longer work).

Changes for v4:
   -- Given the current state of ACPICA, disable CONFIG_ACPI_REDUCED_HARDWARE
      for use on anything other than ARM.
   -- Replaced #ifdefs with run-time checking for hardware reduced mode,
      whenever possible

Changes for v3:
   -- Modified enabling ACPI_REDUCED_HARDWARE in ACPICA when using
      kernel config item CONFIG_ACPI_REDUCED_HARDWARE; now consistent
      with ACPICA code base where needed
   -- Enable X86 for CONFIG_ACPI_REDUCED_HARDWARE
   -- Minimize bus master reload patching
   -- Remove unneeded patch for dmi_check_system() (was 4/6)
   -- Correct the patch for removing unneeded map/unmap of FADT fields

Changes for v2:
   -- Remove patch that was outside of reduced HW mode changes
   -- Simplify CONFIG_ACPI_REDUCED_HARDWARE in Kconfig
   -- Simplify use of CONFIG_ACPI_REDUCED_HARDWARE in #ifdefs
   -- Ensure changelogs are present
   -- Combine and simplify previous patches 8 & 10


Al Stone (6):
  ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI
    mode
  ACPI: bus master reload not supported in reduced HW mode
  ACPI: HW reduced mode does not allow use of the FADT sci_interrupt
    field
  ACPI: in HW reduced mode, using FADT PM information is not allowed.
  ACPI: do not map/unmap memory regions for FADT entries in reduced HW
    mode
  ACPI: make sure ECs do not use the ACPI global lock

 drivers/acpi/Kconfig            | 12 ++++++++++++
 drivers/acpi/bus.c              | 30 ++++++++++++++++--------------
 drivers/acpi/ec.c               | 12 ++++++++++--
 drivers/acpi/osl.c              | 38 +++++++++++++++++++-------------------
 drivers/acpi/pci_link.c         |  2 ++
 drivers/acpi/processor_idle.c   | 14 ++++++++++++--
 include/acpi/platform/aclinux.h |  6 ++++++
 7 files changed, 77 insertions(+), 37 deletions(-)

-- 
1.8.4.2

[PATCH v6 1/6] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI mode

[al.stone]

From: Al Stone <al.stone@linaro.org>

Hardware reduced mode, despite the name, exists primarily to allow
newer platforms to use a much simpler form of ACPI that does not
require supporting the legacy of previous versions of the specification.
This mode was first introduced in the ACPI 5.0 specification, but because
it is so much simpler and reduces the size of the object code needed to
support ACPI, it is likely to be used more often in the near future.

To enable the hardware reduced mode of ACPI on some platforms (such as
ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
to TRUE in the ACPICA source.  For ARM/ARM64, hardware reduced ACPI
should be the only mode used; legacy mode would require modifications
to SoCs in order to provide several x86-specific hardware features (e.g.,
an NMI and SMI support).

We set ACPI_REDUCED_HARDWARE to TRUE in the ACPICA source by introducing
a kernel config item to enable/disable ACPI_REDUCED_HARDWARE.  We can then
change the kernel config instead of having to modify the kernel source
directly to enable the reduced hardware mode of ACPI.

Lv Zheng suggested that this configuration item does not belong in ACPICA,
the upstream source for much of the ACPI internals, but rather to the
Linux kernel itself.  Hence, we introduce this flag so that we can make
ACPI_REDUCED_HARDWARE configurable.  For the details of the discussion,
please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html

Even though support for X86 in hardware reduced mode is possible, it
is NOT enabled.  Extensive effort has gone into the Linux kernel so that
there is a single kernel image than can run on all x86 hardware; the kernel
changes run-time behavior to adapt to the hardware being used.  This is not
currently possible with the existing ACPICA infrastructure but only presents
a problem on achitectures supporting both hardware-reduced and legacy modes
of ACPI -- i.e., on x86 only.

The problem with the current ACPICA code base is that if one builds legacy
ACPI (a proper superset of hardware-reduced), the kernel can run in hardware-
reduced with the proper ACPI tables, but there is still ACPICA code that could
be executed even though it is not allowed by the specification.  If one builds
a hardware-reduced only ACPI, the kernel cannot run with ACPI tables that are
for legacy mode.  To ensure compliance with ACPI, one must therefore build
two separate kernels.  Once this problem has been properly fixed, we can then
enable x86 hardware-reduced mode and use a single kernel.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/Kconfig            | 12 ++++++++++++
 include/acpi/platform/aclinux.h |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 4770de5..75dd38a 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -343,6 +343,18 @@ config ACPI_BGRT
 	  data from the firmware boot splash. It will appear under
 	  /sys/firmware/acpi/bgrt/ .
 
+config ACPI_REDUCED_HARDWARE_ONLY
+	bool "Hardware-reduced ACPI support only"
+	depends on !X86 && !IA64 || EXPERT
+	help
+	This config item changes the way the ACPI code is built.  When this
+	option is selected, the kernel will use a specialized version of
+	ACPICA that ONLY supports the ACPI "reduced hardware" mode.  The
+	resulting kernel will be smaller but it will also be restricted to
+	running in ACPI reduced hardware mode ONLY.
+
+	If you are unsure what to do, do not enable this option.
+
 source "drivers/acpi/apei/Kconfig"
 
 config ACPI_EXTLOG
diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
index 28f4f4d..7d71f08 100644
--- a/include/acpi/platform/aclinux.h
+++ b/include/acpi/platform/aclinux.h
@@ -52,6 +52,12 @@
 
 #ifdef __KERNEL__
 
+/* Compile for reduced hardware mode only with this kernel config */
+
+#ifdef CONFIG_ACPI_REDUCED_HARDWARE_ONLY
+#define ACPI_REDUCED_HARDWARE 1
+#endif
+
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/ctype.h>
-- 
1.8.4.2

[PATCH v6 1/6] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI mode

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

Hardware reduced mode, despite the name, exists primarily to allow
newer platforms to use a much simpler form of ACPI that does not
require supporting the legacy of previous versions of the specification.
This mode was first introduced in the ACPI 5.0 specification, but because
it is so much simpler and reduces the size of the object code needed to
support ACPI, it is likely to be used more often in the near future.

To enable the hardware reduced mode of ACPI on some platforms (such as
ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
to TRUE in the ACPICA source.  For ARM/ARM64, hardware reduced ACPI
should be the only mode used; legacy mode would require modifications
to SoCs in order to provide several x86-specific hardware features (e.g.,
an NMI and SMI support).

We set ACPI_REDUCED_HARDWARE to TRUE in the ACPICA source by introducing
a kernel config item to enable/disable ACPI_REDUCED_HARDWARE.  We can then
change the kernel config instead of having to modify the kernel source
directly to enable the reduced hardware mode of ACPI.

Lv Zheng suggested that this configuration item does not belong in ACPICA,
the upstream source for much of the ACPI internals, but rather to the
Linux kernel itself.  Hence, we introduce this flag so that we can make
ACPI_REDUCED_HARDWARE configurable.  For the details of the discussion,
please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html

Even though support for X86 in hardware reduced mode is possible, it
is NOT enabled.  Extensive effort has gone into the Linux kernel so that
there is a single kernel image than can run on all x86 hardware; the kernel
changes run-time behavior to adapt to the hardware being used.  This is not
currently possible with the existing ACPICA infrastructure but only presents
a problem on achitectures supporting both hardware-reduced and legacy modes
of ACPI -- i.e., on x86 only.

The problem with the current ACPICA code base is that if one builds legacy
ACPI (a proper superset of hardware-reduced), the kernel can run in hardware-
reduced with the proper ACPI tables, but there is still ACPICA code that could
be executed even though it is not allowed by the specification.  If one builds
a hardware-reduced only ACPI, the kernel cannot run with ACPI tables that are
for legacy mode.  To ensure compliance with ACPI, one must therefore build
two separate kernels.  Once this problem has been properly fixed, we can then
enable x86 hardware-reduced mode and use a single kernel.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/Kconfig            | 12 ++++++++++++
 include/acpi/platform/aclinux.h |  6 ++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 4770de5..75dd38a 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -343,6 +343,18 @@ config ACPI_BGRT
 	  data from the firmware boot splash. It will appear under
 	  /sys/firmware/acpi/bgrt/ .
 
+config ACPI_REDUCED_HARDWARE_ONLY
+	bool "Hardware-reduced ACPI support only"
+	depends on !X86 && !IA64 || EXPERT
+	help
+	This config item changes the way the ACPI code is built.  When this
+	option is selected, the kernel will use a specialized version of
+	ACPICA that ONLY supports the ACPI "reduced hardware" mode.  The
+	resulting kernel will be smaller but it will also be restricted to
+	running in ACPI reduced hardware mode ONLY.
+
+	If you are unsure what to do, do not enable this option.
+
 source "drivers/acpi/apei/Kconfig"
 
 config ACPI_EXTLOG
diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
index 28f4f4d..7d71f08 100644
--- a/include/acpi/platform/aclinux.h
+++ b/include/acpi/platform/aclinux.h
@@ -52,6 +52,12 @@
 
 #ifdef __KERNEL__
 
+/* Compile for reduced hardware mode only with this kernel config */
+
+#ifdef CONFIG_ACPI_REDUCED_HARDWARE_ONLY
+#define ACPI_REDUCED_HARDWARE 1
+#endif
+
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/ctype.h>
-- 
1.8.4.2

[PATCH v6 2/6] ACPI: bus master reload not supported in reduced HW mode

[al.stone]

From: Al Stone <al.stone@linaro.org>

Do not save and restore bus master reload registers in suspend/resume
when in reduced HW mode; according to the spec, no such registers should
exist

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/processor_idle.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 597cdab..eac984a 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -229,12 +229,14 @@ static struct syscore_ops acpi_processor_syscore_ops = {
 
 void acpi_processor_syscore_init(void)
 {
-	register_syscore_ops(&acpi_processor_syscore_ops);
+	if (!acpi_gbl_reduced_hardware)
+		register_syscore_ops(&acpi_processor_syscore_ops);
 }
 
 void acpi_processor_syscore_exit(void)
 {
-	unregister_syscore_ops(&acpi_processor_syscore_ops);
+	if (!acpi_gbl_reduced_hardware)
+		unregister_syscore_ops(&acpi_processor_syscore_ops);
 }
 #endif /* CONFIG_PM_SLEEP */
 
-- 
1.8.4.2

[PATCH v6 2/6] ACPI: bus master reload not supported in reduced HW mode

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

Do not save and restore bus master reload registers in suspend/resume
when in reduced HW mode; according to the spec, no such registers should
exist

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/processor_idle.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 597cdab..eac984a 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -229,12 +229,14 @@ static struct syscore_ops acpi_processor_syscore_ops = {
 
 void acpi_processor_syscore_init(void)
 {
-	register_syscore_ops(&acpi_processor_syscore_ops);
+	if (!acpi_gbl_reduced_hardware)
+		register_syscore_ops(&acpi_processor_syscore_ops);
 }
 
 void acpi_processor_syscore_exit(void)
 {
-	unregister_syscore_ops(&acpi_processor_syscore_ops);
+	if (!acpi_gbl_reduced_hardware)
+		unregister_syscore_ops(&acpi_processor_syscore_ops);
 }
 #endif /* CONFIG_PM_SLEEP */
 
-- 
1.8.4.2

[PATCH v6 3/6] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field

[al.stone]

From: Al Stone <al.stone@linaro.org>

In HW reduced mode, the use of the SCI interrupt is not allowed.  In
all those places that use the FADT sci_interrupt field, make sure we
do not execute that path when in HW reduced mode.

In the case of acpi_os_install_interrupt_handler() in osl.c, this allows
us to open up the routine to installing interrupt handlers other than
acpi_gbl_FADT.sci_interrupt regardless of whether we are in ACPI legacy
mode or reduced HW mode; acpi_os_remove_interrupt_handler() changes to
maintain symmetry.

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/bus.c      | 30 ++++++++++++++++--------------
 drivers/acpi/osl.c      | 18 +++++++-----------
 drivers/acpi/pci_link.c |  2 ++
 3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 0710004..d871859 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -552,21 +552,23 @@ void __init acpi_early_init(void)
 	}
 
 #ifdef CONFIG_X86
-	if (!acpi_ioapic) {
-		/* compatible (0) means level (3) */
-		if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
-			acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
-			acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
+	if (!acpi_gbl_reduced_hardware) {
+		if (!acpi_ioapic) {
+			/* compatible (0) means level (3) */
+			if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
+				acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
+				acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
+			}
+			/* Set PIC-mode SCI trigger type */
+			acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
+				(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
+		} else {
+			/*
+			 * now that acpi_gbl_FADT is initialized,
+			 * update it with result from INT_SRC_OVR parsing
+			 */
+			acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
 		}
-		/* Set PIC-mode SCI trigger type */
-		acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
-					 (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
-	} else {
-		/*
-		 * now that acpi_gbl_FADT is initialized,
-		 * update it with result from INT_SRC_OVR parsing
-		 */
-		acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
 	}
 #endif
 
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 44c07eb..c946a3a 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
 
 static acpi_osd_handler acpi_irq_handler;
 static void *acpi_irq_context;
+static u32 acpi_irq_number;
 static struct workqueue_struct *kacpid_wq;
 static struct workqueue_struct *kacpi_notify_wq;
 static struct workqueue_struct *kacpi_hotplug_wq;
@@ -178,6 +179,10 @@ static void __init acpi_request_region (struct acpi_generic_address *gas,
 
 static int __init acpi_reserve_resources(void)
 {
+	/* Handle hardware reduced mode: i.e., do nothing. */
+	if (acpi_gbl_reduced_hardware)
+		return 0;
+
 	acpi_request_region(&acpi_gbl_FADT.xpm1a_event_block, acpi_gbl_FADT.pm1_event_length,
 		"ACPI PM1a_EVT_BLK");
 
@@ -795,13 +800,6 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
 
 	acpi_irq_stats_init();
 
-	/*
-	 * ACPI interrupts different from the SCI in our copy of the FADT are
-	 * not supported.
-	 */
-	if (gsi != acpi_gbl_FADT.sci_interrupt)
-		return AE_BAD_PARAMETER;
-
 	if (acpi_irq_handler)
 		return AE_ALREADY_ACQUIRED;
 
@@ -818,15 +816,13 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
 		acpi_irq_handler = NULL;
 		return AE_NOT_ACQUIRED;
 	}
+	acpi_irq_number = irq;
 
 	return AE_OK;
 }
 
 acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
 {
-	if (irq != acpi_gbl_FADT.sci_interrupt)
-		return AE_BAD_PARAMETER;
-
 	free_irq(irq, acpi_irq);
 	acpi_irq_handler = NULL;
 
@@ -1806,7 +1802,7 @@ acpi_status __init acpi_os_initialize1(void)
 acpi_status acpi_os_terminate(void)
 {
 	if (acpi_irq_handler) {
-		acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
+		acpi_os_remove_interrupt_handler(acpi_irq_number,
 						 acpi_irq_handler);
 	}
 
diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 2652a61..d5c155e 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -505,6 +505,8 @@ int __init acpi_irq_penalty_init(void)
 		}
 	}
 	/* Add a penalty for the SCI */
+	if (acpi_gbl_reduced_hardware)
+		return 0;
 	acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
 	return 0;
 }
-- 
1.8.4.2

[PATCH v6 3/6] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

In HW reduced mode, the use of the SCI interrupt is not allowed.  In
all those places that use the FADT sci_interrupt field, make sure we
do not execute that path when in HW reduced mode.

In the case of acpi_os_install_interrupt_handler() in osl.c, this allows
us to open up the routine to installing interrupt handlers other than
acpi_gbl_FADT.sci_interrupt regardless of whether we are in ACPI legacy
mode or reduced HW mode; acpi_os_remove_interrupt_handler() changes to
maintain symmetry.

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/bus.c      | 30 ++++++++++++++++--------------
 drivers/acpi/osl.c      | 18 +++++++-----------
 drivers/acpi/pci_link.c |  2 ++
 3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 0710004..d871859 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -552,21 +552,23 @@ void __init acpi_early_init(void)
 	}
 
 #ifdef CONFIG_X86
-	if (!acpi_ioapic) {
-		/* compatible (0) means level (3) */
-		if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
-			acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
-			acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
+	if (!acpi_gbl_reduced_hardware) {
+		if (!acpi_ioapic) {
+			/* compatible (0) means level (3) */
+			if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
+				acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
+				acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
+			}
+			/* Set PIC-mode SCI trigger type */
+			acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
+				(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
+		} else {
+			/*
+			 * now that acpi_gbl_FADT is initialized,
+			 * update it with result from INT_SRC_OVR parsing
+			 */
+			acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
 		}
-		/* Set PIC-mode SCI trigger type */
-		acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
-					 (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
-	} else {
-		/*
-		 * now that acpi_gbl_FADT is initialized,
-		 * update it with result from INT_SRC_OVR parsing
-		 */
-		acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
 	}
 #endif
 
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 44c07eb..c946a3a 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
 
 static acpi_osd_handler acpi_irq_handler;
 static void *acpi_irq_context;
+static u32 acpi_irq_number;
 static struct workqueue_struct *kacpid_wq;
 static struct workqueue_struct *kacpi_notify_wq;
 static struct workqueue_struct *kacpi_hotplug_wq;
@@ -178,6 +179,10 @@ static void __init acpi_request_region (struct acpi_generic_address *gas,
 
 static int __init acpi_reserve_resources(void)
 {
+	/* Handle hardware reduced mode: i.e., do nothing. */
+	if (acpi_gbl_reduced_hardware)
+		return 0;
+
 	acpi_request_region(&acpi_gbl_FADT.xpm1a_event_block, acpi_gbl_FADT.pm1_event_length,
 		"ACPI PM1a_EVT_BLK");
 
@@ -795,13 +800,6 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
 
 	acpi_irq_stats_init();
 
-	/*
-	 * ACPI interrupts different from the SCI in our copy of the FADT are
-	 * not supported.
-	 */
-	if (gsi != acpi_gbl_FADT.sci_interrupt)
-		return AE_BAD_PARAMETER;
-
 	if (acpi_irq_handler)
 		return AE_ALREADY_ACQUIRED;
 
@@ -818,15 +816,13 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
 		acpi_irq_handler = NULL;
 		return AE_NOT_ACQUIRED;
 	}
+	acpi_irq_number = irq;
 
 	return AE_OK;
 }
 
 acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
 {
-	if (irq != acpi_gbl_FADT.sci_interrupt)
-		return AE_BAD_PARAMETER;
-
 	free_irq(irq, acpi_irq);
 	acpi_irq_handler = NULL;
 
@@ -1806,7 +1802,7 @@ acpi_status __init acpi_os_initialize1(void)
 acpi_status acpi_os_terminate(void)
 {
 	if (acpi_irq_handler) {
-		acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
+		acpi_os_remove_interrupt_handler(acpi_irq_number,
 						 acpi_irq_handler);
 	}
 
diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 2652a61..d5c155e 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -505,6 +505,8 @@ int __init acpi_irq_penalty_init(void)
 		}
 	}
 	/* Add a penalty for the SCI */
+	if (acpi_gbl_reduced_hardware)
+		return 0;
 	acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
 	return 0;
 }
-- 
1.8.4.2

[PATCH v6 4/6] ACPI: in HW reduced mode, using FADT PM information is not allowed.

[al.stone]

From: Al Stone <al.stone@linaro.org>

Per the ACPI 5.0 specification, section 5.2.9, none of the power
management fields in the FADT are to be used.  Short-circuit using
any of those fields in acpi_processor_get_power_info_fadt().

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/processor_idle.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index eac984a..89f0a0f 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -270,6 +270,14 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
 	if (!pr->pblk)
 		return -ENODEV;
 
+	/*
+	 * Using power management information from the FADT is not
+	 * allowed when in HW reduced mode.  See ACPI 5.0, section
+	 * 5.2.9.
+	 */
+	if (acpi_gbl_reduced_hardware)
+		return -ENODEV;
+
 	/* if info is obtained from pblk/fadt, type equals state */
 	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
 	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
-- 
1.8.4.2

[PATCH v6 4/6] ACPI: in HW reduced mode, using FADT PM information is not allowed.

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

Per the ACPI 5.0 specification, section 5.2.9, none of the power
management fields in the FADT are to be used.  Short-circuit using
any of those fields in acpi_processor_get_power_info_fadt().

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/processor_idle.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index eac984a..89f0a0f 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -270,6 +270,14 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
 	if (!pr->pblk)
 		return -ENODEV;
 
+	/*
+	 * Using power management information from the FADT is not
+	 * allowed when in HW reduced mode.  See ACPI 5.0, section
+	 * 5.2.9.
+	 */
+	if (acpi_gbl_reduced_hardware)
+		return -ENODEV;
+
 	/* if info is obtained from pblk/fadt, type equals state */
 	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
 	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
-- 
1.8.4.2

[PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[al.stone]

From: Al Stone <al.stone@linaro.org>

Several of the FADT fields are normally kept in specific memory
regions.  Since these fields are to be ignored in hardware reduced
ACPI mode, do not map those addresses when in that mode, and of
course do not release the mappings that have not been made.

The function acpi_os_initialize() could become a stub in the header
file but is left here in case it can be of further use.

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/osl.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index c946a3a..7822821 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -1778,10 +1778,12 @@ __setup("acpi_no_auto_ssdt", acpi_no_auto_ssdt_setup);
 
 acpi_status __init acpi_os_initialize(void)
 {
-	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
-	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
-	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
-	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
+	if (!acpi_gbl_reduced_hardware) {
+		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
+		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
+		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
+		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
+	}
 
 	return AE_OK;
 }
@@ -1806,10 +1808,12 @@ acpi_status acpi_os_terminate(void)
 						 acpi_irq_handler);
 	}
 
-	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
-	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
-	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
-	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
+	if (!acpi_gbl_reduced_hardware) {
+		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
+		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
+		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
+		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
+	}
 
 	destroy_workqueue(kacpid_wq);
 	destroy_workqueue(kacpi_notify_wq);
-- 
1.8.4.2

[PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

Several of the FADT fields are normally kept in specific memory
regions.  Since these fields are to be ignored in hardware reduced
ACPI mode, do not map those addresses when in that mode, and of
course do not release the mappings that have not been made.

The function acpi_os_initialize() could become a stub in the header
file but is left here in case it can be of further use.

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/osl.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index c946a3a..7822821 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -1778,10 +1778,12 @@ __setup("acpi_no_auto_ssdt", acpi_no_auto_ssdt_setup);
 
 acpi_status __init acpi_os_initialize(void)
 {
-	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
-	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
-	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
-	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
+	if (!acpi_gbl_reduced_hardware) {
+		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
+		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
+		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
+		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
+	}
 
 	return AE_OK;
 }
@@ -1806,10 +1808,12 @@ acpi_status acpi_os_terminate(void)
 						 acpi_irq_handler);
 	}
 
-	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
-	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
-	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
-	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
+	if (!acpi_gbl_reduced_hardware) {
+		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
+		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
+		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
+		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
+	}
 
 	destroy_workqueue(kacpid_wq);
 	destroy_workqueue(kacpi_notify_wq);
-- 
1.8.4.2

[PATCH v6 6/6] ACPI: make sure ECs do not use the ACPI global lock

[al.stone]

From: Al Stone <al.stone@linaro.org>

ACPI ECs (Embedded Controllers) are allowed to use the ACPI global
lock in legacy mode.  Since there is no global lock in hardware
reduced mode, make sure that ECs cannot inadvertently use it with
older ACPI tables that may have defined an _GLK method for the
EC device.  Since an individual lock can and should be defined for
each EC, access to each EC should still be properly controlled.

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/ec.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index ba5b56d..3f15110 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -745,9 +745,17 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 	if (ACPI_FAILURE(status))
 		return status;
 	ec->gpe = tmp;
-	/* Use the global lock for all EC transactions? */
+	/*
+	 * Use the global lock for all EC transactions?
+	 *
+	 * If ACPI is in hardware reduced mode, there is no global lock.
+	 * If a _GLK method is defined, it needs to be ignored, just
+	 * in case it returns "true", which could happen if the ASL is
+	 * written incorrectly.
+	 */
 	tmp = 0;
-	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
+	if (!acpi_gbl_reduced_hardware)
+		acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
 	ec->global_lock = tmp;
 	ec->handle = handle;
 	return AE_CTRL_TERMINATE;
-- 
1.8.4.2

[PATCH v6 6/6] ACPI: make sure ECs do not use the ACPI global lock

[al.stone at linaro.org]

From: Al Stone <al.stone@linaro.org>

ACPI ECs (Embedded Controllers) are allowed to use the ACPI global
lock in legacy mode.  Since there is no global lock in hardware
reduced mode, make sure that ECs cannot inadvertently use it with
older ACPI tables that may have defined an _GLK method for the
EC device.  Since an individual lock can and should be defined for
each EC, access to each EC should still be properly controlled.

Signed-off-by: Al Stone <al.stone@linaro.org>
---
 drivers/acpi/ec.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index ba5b56d..3f15110 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -745,9 +745,17 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 	if (ACPI_FAILURE(status))
 		return status;
 	ec->gpe = tmp;
-	/* Use the global lock for all EC transactions? */
+	/*
+	 * Use the global lock for all EC transactions?
+	 *
+	 * If ACPI is in hardware reduced mode, there is no global lock.
+	 * If a _GLK method is defined, it needs to be ignored, just
+	 * in case it returns "true", which could happen if the ASL is
+	 * written incorrectly.
+	 */
 	tmp = 0;
-	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
+	if (!acpi_gbl_reduced_hardware)
+		acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
 	ec->global_lock = tmp;
 	ec->handle = handle;
 	return AE_CTRL_TERMINATE;
-- 
1.8.4.2

Re: [PATCH v6 1/6] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI mode

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:15 PM al.stone@linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> Hardware reduced mode, despite the name, exists primarily to allow
> newer platforms to use a much simpler form of ACPI that does not
> require supporting the legacy of previous versions of the specification.
> This mode was first introduced in the ACPI 5.0 specification, but because
> it is so much simpler and reduces the size of the object code needed to
> support ACPI, it is likely to be used more often in the near future.
> 
> To enable the hardware reduced mode of ACPI on some platforms (such as
> ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
> to TRUE in the ACPICA source.  For ARM/ARM64, hardware reduced ACPI
> should be the only mode used; legacy mode would require modifications
> to SoCs in order to provide several x86-specific hardware features (e.g.,
> an NMI and SMI support).
> 
> We set ACPI_REDUCED_HARDWARE to TRUE in the ACPICA source by introducing
> a kernel config item to enable/disable ACPI_REDUCED_HARDWARE.  We can then
> change the kernel config instead of having to modify the kernel source
> directly to enable the reduced hardware mode of ACPI.
> 
> Lv Zheng suggested that this configuration item does not belong in ACPICA,
> the upstream source for much of the ACPI internals, but rather to the
> Linux kernel itself.  Hence, we introduce this flag so that we can make
> ACPI_REDUCED_HARDWARE configurable.  For the details of the discussion,
> please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html
> 
> Even though support for X86 in hardware reduced mode is possible, it
> is NOT enabled.  Extensive effort has gone into the Linux kernel so that
> there is a single kernel image than can run on all x86 hardware; the kernel
> changes run-time behavior to adapt to the hardware being used.  This is not
> currently possible with the existing ACPICA infrastructure but only presents
> a problem on achitectures supporting both hardware-reduced and legacy modes
> of ACPI -- i.e., on x86 only.
> 
> The problem with the current ACPICA code base is that if one builds legacy
> ACPI (a proper superset of hardware-reduced), the kernel can run in hardware-
> reduced with the proper ACPI tables, but there is still ACPICA code that could
> be executed even though it is not allowed by the specification.  If one builds
> a hardware-reduced only ACPI, the kernel cannot run with ACPI tables that are
> for legacy mode.  To ensure compliance with ACPI, one must therefore build
> two separate kernels.  Once this problem has been properly fixed, we can then
> enable x86 hardware-reduced mode and use a single kernel.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  drivers/acpi/Kconfig            | 12 ++++++++++++
>  include/acpi/platform/aclinux.h |  6 ++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 4770de5..75dd38a 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -343,6 +343,18 @@ config ACPI_BGRT
>  	  data from the firmware boot splash. It will appear under
>  	  /sys/firmware/acpi/bgrt/ .
>  
> +config ACPI_REDUCED_HARDWARE_ONLY
> +	bool "Hardware-reduced ACPI support only"
> +	depends on !X86 && !IA64 || EXPERT
> +	help
> +	This config item changes the way the ACPI code is built.  When this
> +	option is selected, the kernel will use a specialized version of
> +	ACPICA that ONLY supports the ACPI "reduced hardware" mode.  The
> +	resulting kernel will be smaller but it will also be restricted to
> +	running in ACPI reduced hardware mode ONLY.
> +
> +	If you are unsure what to do, do not enable this option.

I'm still not exactly convinced this is the best way to do that.

In particular, I wonder if it would be viable to make it disabled by default
and then make the interested architectures do

	select ACPI_REDUCED_HARDWARE_ONLY if ACPI

in their top-level Kconfig files?

> +
>  source "drivers/acpi/apei/Kconfig"
>  
>  config ACPI_EXTLOG
> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> index 28f4f4d..7d71f08 100644
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -52,6 +52,12 @@
>  
>  #ifdef __KERNEL__
>  
> +/* Compile for reduced hardware mode only with this kernel config */
> +
> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE_ONLY
> +#define ACPI_REDUCED_HARDWARE 1
> +#endif
> +
>  #include <linux/string.h>
>  #include <linux/kernel.h>
>  #include <linux/ctype.h>
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v6 1/6] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI mode

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:15 PM al.stone at linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> Hardware reduced mode, despite the name, exists primarily to allow
> newer platforms to use a much simpler form of ACPI that does not
> require supporting the legacy of previous versions of the specification.
> This mode was first introduced in the ACPI 5.0 specification, but because
> it is so much simpler and reduces the size of the object code needed to
> support ACPI, it is likely to be used more often in the near future.
> 
> To enable the hardware reduced mode of ACPI on some platforms (such as
> ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
> to TRUE in the ACPICA source.  For ARM/ARM64, hardware reduced ACPI
> should be the only mode used; legacy mode would require modifications
> to SoCs in order to provide several x86-specific hardware features (e.g.,
> an NMI and SMI support).
> 
> We set ACPI_REDUCED_HARDWARE to TRUE in the ACPICA source by introducing
> a kernel config item to enable/disable ACPI_REDUCED_HARDWARE.  We can then
> change the kernel config instead of having to modify the kernel source
> directly to enable the reduced hardware mode of ACPI.
> 
> Lv Zheng suggested that this configuration item does not belong in ACPICA,
> the upstream source for much of the ACPI internals, but rather to the
> Linux kernel itself.  Hence, we introduce this flag so that we can make
> ACPI_REDUCED_HARDWARE configurable.  For the details of the discussion,
> please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html
> 
> Even though support for X86 in hardware reduced mode is possible, it
> is NOT enabled.  Extensive effort has gone into the Linux kernel so that
> there is a single kernel image than can run on all x86 hardware; the kernel
> changes run-time behavior to adapt to the hardware being used.  This is not
> currently possible with the existing ACPICA infrastructure but only presents
> a problem on achitectures supporting both hardware-reduced and legacy modes
> of ACPI -- i.e., on x86 only.
> 
> The problem with the current ACPICA code base is that if one builds legacy
> ACPI (a proper superset of hardware-reduced), the kernel can run in hardware-
> reduced with the proper ACPI tables, but there is still ACPICA code that could
> be executed even though it is not allowed by the specification.  If one builds
> a hardware-reduced only ACPI, the kernel cannot run with ACPI tables that are
> for legacy mode.  To ensure compliance with ACPI, one must therefore build
> two separate kernels.  Once this problem has been properly fixed, we can then
> enable x86 hardware-reduced mode and use a single kernel.
> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  drivers/acpi/Kconfig            | 12 ++++++++++++
>  include/acpi/platform/aclinux.h |  6 ++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 4770de5..75dd38a 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -343,6 +343,18 @@ config ACPI_BGRT
>  	  data from the firmware boot splash. It will appear under
>  	  /sys/firmware/acpi/bgrt/ .
>  
> +config ACPI_REDUCED_HARDWARE_ONLY
> +	bool "Hardware-reduced ACPI support only"
> +	depends on !X86 && !IA64 || EXPERT
> +	help
> +	This config item changes the way the ACPI code is built.  When this
> +	option is selected, the kernel will use a specialized version of
> +	ACPICA that ONLY supports the ACPI "reduced hardware" mode.  The
> +	resulting kernel will be smaller but it will also be restricted to
> +	running in ACPI reduced hardware mode ONLY.
> +
> +	If you are unsure what to do, do not enable this option.

I'm still not exactly convinced this is the best way to do that.

In particular, I wonder if it would be viable to make it disabled by default
and then make the interested architectures do

	select ACPI_REDUCED_HARDWARE_ONLY if ACPI

in their top-level Kconfig files?

> +
>  source "drivers/acpi/apei/Kconfig"
>  
>  config ACPI_EXTLOG
> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
> index 28f4f4d..7d71f08 100644
> --- a/include/acpi/platform/aclinux.h
> +++ b/include/acpi/platform/aclinux.h
> @@ -52,6 +52,12 @@
>  
>  #ifdef __KERNEL__
>  
> +/* Compile for reduced hardware mode only with this kernel config */
> +
> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE_ONLY
> +#define ACPI_REDUCED_HARDWARE 1
> +#endif
> +
>  #include <linux/string.h>
>  #include <linux/kernel.h>
>  #include <linux/ctype.h>
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 1/6] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI mode

[Al Stone]

On 01/10/2014 04:11 PM, Rafael J. Wysocki wrote:
> On Friday, January 10, 2014 03:52:15 PM al.stone@linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> Hardware reduced mode, despite the name, exists primarily to allow
>> newer platforms to use a much simpler form of ACPI that does not
>> require supporting the legacy of previous versions of the specification.
>> This mode was first introduced in the ACPI 5.0 specification, but because
>> it is so much simpler and reduces the size of the object code needed to
>> support ACPI, it is likely to be used more often in the near future.
>>
>> To enable the hardware reduced mode of ACPI on some platforms (such as
>> ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
>> to TRUE in the ACPICA source.  For ARM/ARM64, hardware reduced ACPI
>> should be the only mode used; legacy mode would require modifications
>> to SoCs in order to provide several x86-specific hardware features (e.g.,
>> an NMI and SMI support).
>>
>> We set ACPI_REDUCED_HARDWARE to TRUE in the ACPICA source by introducing
>> a kernel config item to enable/disable ACPI_REDUCED_HARDWARE.  We can then
>> change the kernel config instead of having to modify the kernel source
>> directly to enable the reduced hardware mode of ACPI.
>>
>> Lv Zheng suggested that this configuration item does not belong in ACPICA,
>> the upstream source for much of the ACPI internals, but rather to the
>> Linux kernel itself.  Hence, we introduce this flag so that we can make
>> ACPI_REDUCED_HARDWARE configurable.  For the details of the discussion,
>> please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html
>>
>> Even though support for X86 in hardware reduced mode is possible, it
>> is NOT enabled.  Extensive effort has gone into the Linux kernel so that
>> there is a single kernel image than can run on all x86 hardware; the kernel
>> changes run-time behavior to adapt to the hardware being used.  This is not
>> currently possible with the existing ACPICA infrastructure but only presents
>> a problem on achitectures supporting both hardware-reduced and legacy modes
>> of ACPI -- i.e., on x86 only.
>>
>> The problem with the current ACPICA code base is that if one builds legacy
>> ACPI (a proper superset of hardware-reduced), the kernel can run in hardware-
>> reduced with the proper ACPI tables, but there is still ACPICA code that could
>> be executed even though it is not allowed by the specification.  If one builds
>> a hardware-reduced only ACPI, the kernel cannot run with ACPI tables that are
>> for legacy mode.  To ensure compliance with ACPI, one must therefore build
>> two separate kernels.  Once this problem has been properly fixed, we can then
>> enable x86 hardware-reduced mode and use a single kernel.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   drivers/acpi/Kconfig            | 12 ++++++++++++
>>   include/acpi/platform/aclinux.h |  6 ++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 4770de5..75dd38a 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -343,6 +343,18 @@ config ACPI_BGRT
>>   	  data from the firmware boot splash. It will appear under
>>   	  /sys/firmware/acpi/bgrt/ .
>>
>> +config ACPI_REDUCED_HARDWARE_ONLY
>> +	bool "Hardware-reduced ACPI support only"
>> +	depends on !X86 && !IA64 || EXPERT
>> +	help
>> +	This config item changes the way the ACPI code is built.  When this
>> +	option is selected, the kernel will use a specialized version of
>> +	ACPICA that ONLY supports the ACPI "reduced hardware" mode.  The
>> +	resulting kernel will be smaller but it will also be restricted to
>> +	running in ACPI reduced hardware mode ONLY.
>> +
>> +	If you are unsure what to do, do not enable this option.
>
> I'm still not exactly convinced this is the best way to do that.
>
> In particular, I wonder if it would be viable to make it disabled by default
> and then make the interested architectures do
>
> 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>
> in their top-level Kconfig files?

Hrm.  Do you mean something like this:

config ACPI_REDUCED_HARDWARE_ONLY
	bool "Hardware-reduced ACPI support only"
	default n
	depends on EXPERT
	help
	...

and remove any mention of architecture?  Or forgo the "depends"
entirely?  From my perspective, any of the above is sufficient
so I'll defer to your broader view of ACPI.

>> +
>>   source "drivers/acpi/apei/Kconfig"
>>
>>   config ACPI_EXTLOG
>> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
>> index 28f4f4d..7d71f08 100644
>> --- a/include/acpi/platform/aclinux.h
>> +++ b/include/acpi/platform/aclinux.h
>> @@ -52,6 +52,12 @@
>>
>>   #ifdef __KERNEL__
>>
>> +/* Compile for reduced hardware mode only with this kernel config */
>> +
>> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE_ONLY
>> +#define ACPI_REDUCED_HARDWARE 1
>> +#endif
>> +
>>   #include <linux/string.h>
>>   #include <linux/kernel.h>
>>   #include <linux/ctype.h>
>>
>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

[PATCH v6 1/6] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI mode

[Al Stone]

On 01/10/2014 04:11 PM, Rafael J. Wysocki wrote:
> On Friday, January 10, 2014 03:52:15 PM al.stone at linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> Hardware reduced mode, despite the name, exists primarily to allow
>> newer platforms to use a much simpler form of ACPI that does not
>> require supporting the legacy of previous versions of the specification.
>> This mode was first introduced in the ACPI 5.0 specification, but because
>> it is so much simpler and reduces the size of the object code needed to
>> support ACPI, it is likely to be used more often in the near future.
>>
>> To enable the hardware reduced mode of ACPI on some platforms (such as
>> ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
>> to TRUE in the ACPICA source.  For ARM/ARM64, hardware reduced ACPI
>> should be the only mode used; legacy mode would require modifications
>> to SoCs in order to provide several x86-specific hardware features (e.g.,
>> an NMI and SMI support).
>>
>> We set ACPI_REDUCED_HARDWARE to TRUE in the ACPICA source by introducing
>> a kernel config item to enable/disable ACPI_REDUCED_HARDWARE.  We can then
>> change the kernel config instead of having to modify the kernel source
>> directly to enable the reduced hardware mode of ACPI.
>>
>> Lv Zheng suggested that this configuration item does not belong in ACPICA,
>> the upstream source for much of the ACPI internals, but rather to the
>> Linux kernel itself.  Hence, we introduce this flag so that we can make
>> ACPI_REDUCED_HARDWARE configurable.  For the details of the discussion,
>> please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html
>>
>> Even though support for X86 in hardware reduced mode is possible, it
>> is NOT enabled.  Extensive effort has gone into the Linux kernel so that
>> there is a single kernel image than can run on all x86 hardware; the kernel
>> changes run-time behavior to adapt to the hardware being used.  This is not
>> currently possible with the existing ACPICA infrastructure but only presents
>> a problem on achitectures supporting both hardware-reduced and legacy modes
>> of ACPI -- i.e., on x86 only.
>>
>> The problem with the current ACPICA code base is that if one builds legacy
>> ACPI (a proper superset of hardware-reduced), the kernel can run in hardware-
>> reduced with the proper ACPI tables, but there is still ACPICA code that could
>> be executed even though it is not allowed by the specification.  If one builds
>> a hardware-reduced only ACPI, the kernel cannot run with ACPI tables that are
>> for legacy mode.  To ensure compliance with ACPI, one must therefore build
>> two separate kernels.  Once this problem has been properly fixed, we can then
>> enable x86 hardware-reduced mode and use a single kernel.
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   drivers/acpi/Kconfig            | 12 ++++++++++++
>>   include/acpi/platform/aclinux.h |  6 ++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 4770de5..75dd38a 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -343,6 +343,18 @@ config ACPI_BGRT
>>   	  data from the firmware boot splash. It will appear under
>>   	  /sys/firmware/acpi/bgrt/ .
>>
>> +config ACPI_REDUCED_HARDWARE_ONLY
>> +	bool "Hardware-reduced ACPI support only"
>> +	depends on !X86 && !IA64 || EXPERT
>> +	help
>> +	This config item changes the way the ACPI code is built.  When this
>> +	option is selected, the kernel will use a specialized version of
>> +	ACPICA that ONLY supports the ACPI "reduced hardware" mode.  The
>> +	resulting kernel will be smaller but it will also be restricted to
>> +	running in ACPI reduced hardware mode ONLY.
>> +
>> +	If you are unsure what to do, do not enable this option.
>
> I'm still not exactly convinced this is the best way to do that.
>
> In particular, I wonder if it would be viable to make it disabled by default
> and then make the interested architectures do
>
> 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
>
> in their top-level Kconfig files?

Hrm.  Do you mean something like this:

config ACPI_REDUCED_HARDWARE_ONLY
	bool "Hardware-reduced ACPI support only"
	default n
	depends on EXPERT
	help
	...

and remove any mention of architecture?  Or forgo the "depends"
entirely?  From my perspective, any of the above is sufficient
so I'll defer to your broader view of ACPI.

>> +
>>   source "drivers/acpi/apei/Kconfig"
>>
>>   config ACPI_EXTLOG
>> diff --git a/include/acpi/platform/aclinux.h b/include/acpi/platform/aclinux.h
>> index 28f4f4d..7d71f08 100644
>> --- a/include/acpi/platform/aclinux.h
>> +++ b/include/acpi/platform/aclinux.h
>> @@ -52,6 +52,12 @@
>>
>>   #ifdef __KERNEL__
>>
>> +/* Compile for reduced hardware mode only with this kernel config */
>> +
>> +#ifdef CONFIG_ACPI_REDUCED_HARDWARE_ONLY
>> +#define ACPI_REDUCED_HARDWARE 1
>> +#endif
>> +
>>   #include <linux/string.h>
>>   #include <linux/kernel.h>
>>   #include <linux/ctype.h>
>>
>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3 at redhat.com
-----------------------------------

Re: [PATCH v6 3/6] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:17 PM al.stone@linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> In HW reduced mode, the use of the SCI interrupt is not allowed.  In
> all those places that use the FADT sci_interrupt field, make sure we
> do not execute that path when in HW reduced mode.
> 
> In the case of acpi_os_install_interrupt_handler() in osl.c, this allows
> us to open up the routine to installing interrupt handlers other than
> acpi_gbl_FADT.sci_interrupt regardless of whether we are in ACPI legacy
> mode or reduced HW mode; acpi_os_remove_interrupt_handler() changes to
> maintain symmetry.
> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  drivers/acpi/bus.c      | 30 ++++++++++++++++--------------
>  drivers/acpi/osl.c      | 18 +++++++-----------
>  drivers/acpi/pci_link.c |  2 ++
>  3 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 0710004..d871859 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -552,21 +552,23 @@ void __init acpi_early_init(void)
>  	}
>  
>  #ifdef CONFIG_X86
> -	if (!acpi_ioapic) {
> -		/* compatible (0) means level (3) */
> -		if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> -			acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
> -			acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;

I wonder why exactly you want to make this change.  It surely doesn't matter
for ARM and do you have any HW-reduced x86 hardware to test it?

> +	if (!acpi_gbl_reduced_hardware) {
> +		if (!acpi_ioapic) {
> +			/* compatible (0) means level (3) */
> +			if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> +				acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
> +				acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
> +			}
> +			/* Set PIC-mode SCI trigger type */
> +			acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> +				(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> +		} else {
> +			/*
> +			 * now that acpi_gbl_FADT is initialized,
> +			 * update it with result from INT_SRC_OVR parsing
> +			 */
> +			acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
>  		}
> -		/* Set PIC-mode SCI trigger type */
> -		acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> -					 (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> -	} else {
> -		/*
> -		 * now that acpi_gbl_FADT is initialized,
> -		 * update it with result from INT_SRC_OVR parsing
> -		 */
> -		acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
>  	}
>  #endif
>  
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 44c07eb..c946a3a 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
>  
>  static acpi_osd_handler acpi_irq_handler;
>  static void *acpi_irq_context;
> +static u32 acpi_irq_number;
>  static struct workqueue_struct *kacpid_wq;
>  static struct workqueue_struct *kacpi_notify_wq;
>  static struct workqueue_struct *kacpi_hotplug_wq;
> @@ -178,6 +179,10 @@ static void __init acpi_request_region (struct acpi_generic_address *gas,
>  
>  static int __init acpi_reserve_resources(void)
>  {
> +	/* Handle hardware reduced mode: i.e., do nothing. */
> +	if (acpi_gbl_reduced_hardware)
> +		return 0;

Does it actually break things if we do that?

> +
>  	acpi_request_region(&acpi_gbl_FADT.xpm1a_event_block, acpi_gbl_FADT.pm1_event_length,
>  		"ACPI PM1a_EVT_BLK");
>  
> @@ -795,13 +800,6 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>  
>  	acpi_irq_stats_init();
>  
> -	/*
> -	 * ACPI interrupts different from the SCI in our copy of the FADT are
> -	 * not supported.
> -	 */
> -	if (gsi != acpi_gbl_FADT.sci_interrupt)
> -		return AE_BAD_PARAMETER;
> -
>  	if (acpi_irq_handler)
>  		return AE_ALREADY_ACQUIRED;
>  
> @@ -818,15 +816,13 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>  		acpi_irq_handler = NULL;
>  		return AE_NOT_ACQUIRED;
>  	}
> +	acpi_irq_number = irq;
>  
>  	return AE_OK;
>  }
>  
>  acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
>  {
> -	if (irq != acpi_gbl_FADT.sci_interrupt)
> -		return AE_BAD_PARAMETER;
> -
>  	free_irq(irq, acpi_irq);
>  	acpi_irq_handler = NULL;
>  
> @@ -1806,7 +1802,7 @@ acpi_status __init acpi_os_initialize1(void)
>  acpi_status acpi_os_terminate(void)
>  {
>  	if (acpi_irq_handler) {
> -		acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
> +		acpi_os_remove_interrupt_handler(acpi_irq_number,
>  						 acpi_irq_handler);
>  	}
>  
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index 2652a61..d5c155e 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -505,6 +505,8 @@ int __init acpi_irq_penalty_init(void)
>  		}
>  	}
>  	/* Add a penalty for the SCI */
> +	if (acpi_gbl_reduced_hardware)
> +		return 0;
>  	acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
>  	return 0;
>  }

Is ARM really going to use the code in pci_link.c?  If so, then how exactly?

Rafael

[PATCH v6 3/6] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:17 PM al.stone at linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> In HW reduced mode, the use of the SCI interrupt is not allowed.  In
> all those places that use the FADT sci_interrupt field, make sure we
> do not execute that path when in HW reduced mode.
> 
> In the case of acpi_os_install_interrupt_handler() in osl.c, this allows
> us to open up the routine to installing interrupt handlers other than
> acpi_gbl_FADT.sci_interrupt regardless of whether we are in ACPI legacy
> mode or reduced HW mode; acpi_os_remove_interrupt_handler() changes to
> maintain symmetry.
> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  drivers/acpi/bus.c      | 30 ++++++++++++++++--------------
>  drivers/acpi/osl.c      | 18 +++++++-----------
>  drivers/acpi/pci_link.c |  2 ++
>  3 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 0710004..d871859 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -552,21 +552,23 @@ void __init acpi_early_init(void)
>  	}
>  
>  #ifdef CONFIG_X86
> -	if (!acpi_ioapic) {
> -		/* compatible (0) means level (3) */
> -		if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> -			acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
> -			acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;

I wonder why exactly you want to make this change.  It surely doesn't matter
for ARM and do you have any HW-reduced x86 hardware to test it?

> +	if (!acpi_gbl_reduced_hardware) {
> +		if (!acpi_ioapic) {
> +			/* compatible (0) means level (3) */
> +			if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> +				acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
> +				acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
> +			}
> +			/* Set PIC-mode SCI trigger type */
> +			acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> +				(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> +		} else {
> +			/*
> +			 * now that acpi_gbl_FADT is initialized,
> +			 * update it with result from INT_SRC_OVR parsing
> +			 */
> +			acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
>  		}
> -		/* Set PIC-mode SCI trigger type */
> -		acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> -					 (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> -	} else {
> -		/*
> -		 * now that acpi_gbl_FADT is initialized,
> -		 * update it with result from INT_SRC_OVR parsing
> -		 */
> -		acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
>  	}
>  #endif
>  
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 44c07eb..c946a3a 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
>  
>  static acpi_osd_handler acpi_irq_handler;
>  static void *acpi_irq_context;
> +static u32 acpi_irq_number;
>  static struct workqueue_struct *kacpid_wq;
>  static struct workqueue_struct *kacpi_notify_wq;
>  static struct workqueue_struct *kacpi_hotplug_wq;
> @@ -178,6 +179,10 @@ static void __init acpi_request_region (struct acpi_generic_address *gas,
>  
>  static int __init acpi_reserve_resources(void)
>  {
> +	/* Handle hardware reduced mode: i.e., do nothing. */
> +	if (acpi_gbl_reduced_hardware)
> +		return 0;

Does it actually break things if we do that?

> +
>  	acpi_request_region(&acpi_gbl_FADT.xpm1a_event_block, acpi_gbl_FADT.pm1_event_length,
>  		"ACPI PM1a_EVT_BLK");
>  
> @@ -795,13 +800,6 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>  
>  	acpi_irq_stats_init();
>  
> -	/*
> -	 * ACPI interrupts different from the SCI in our copy of the FADT are
> -	 * not supported.
> -	 */
> -	if (gsi != acpi_gbl_FADT.sci_interrupt)
> -		return AE_BAD_PARAMETER;
> -
>  	if (acpi_irq_handler)
>  		return AE_ALREADY_ACQUIRED;
>  
> @@ -818,15 +816,13 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>  		acpi_irq_handler = NULL;
>  		return AE_NOT_ACQUIRED;
>  	}
> +	acpi_irq_number = irq;
>  
>  	return AE_OK;
>  }
>  
>  acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
>  {
> -	if (irq != acpi_gbl_FADT.sci_interrupt)
> -		return AE_BAD_PARAMETER;
> -
>  	free_irq(irq, acpi_irq);
>  	acpi_irq_handler = NULL;
>  
> @@ -1806,7 +1802,7 @@ acpi_status __init acpi_os_initialize1(void)
>  acpi_status acpi_os_terminate(void)
>  {
>  	if (acpi_irq_handler) {
> -		acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
> +		acpi_os_remove_interrupt_handler(acpi_irq_number,
>  						 acpi_irq_handler);
>  	}
>  
> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> index 2652a61..d5c155e 100644
> --- a/drivers/acpi/pci_link.c
> +++ b/drivers/acpi/pci_link.c
> @@ -505,6 +505,8 @@ int __init acpi_irq_penalty_init(void)
>  		}
>  	}
>  	/* Add a penalty for the SCI */
> +	if (acpi_gbl_reduced_hardware)
> +		return 0;
>  	acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
>  	return 0;
>  }

Is ARM really going to use the code in pci_link.c?  If so, then how exactly?

Rafael

Re: [PATCH v6 0/6] Hardware Reduced Mode Cleanup for ACPI

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:14 PM al.stone@linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> This series of patches starts with Hanjun's patch to create a kernel
> config item for CONFIG_ACPI_REDUCED_HARDWARE [0].  Building on that, I
> then reviewed all of the code that touched any of several fields in the
> FADT that the OSPM is supposed to ignore when ACPI is in Hardware Reduced
> mode [1].  Any time there was a use of one of the fields to be ignored,
> I evaluated whether or not the code was implementing Hardware Reduced
> mode correctly.  Similarly, for each the flags in the FADT flags field
> that are to be ignored in Hardware Reduced mode, the kernel code was again
> scanned for proper usage.  The remainder of the patches are to fix all of
> the situations I could find where the kernel would not behave correctly
> in this ACPI mode.
> 
> These seem to work just fine on the RTSM model for ARMv7, both with and
> without ACPI enabled, and with and without ACPI_REDUCED_HARDWARE enabled;
> similarly for the FVP model for ARMv8.  The patches for ACPI on ARM
> hardware have been submitted elsewhere but they presume that reduced HW
> mode is functioning correctly.  In the meantime, there's no way I can think
> of to test all possible scenarios so feedback would be greatly appreciated.

Quite frankly, I'm seeing a pattern in this patchset that I'm not really
liking, which is making changes on a hunch.  Especially things like
changing code under #ifdef CONFIG_X86 and similar.

Can we please focus on making changes that are *known* to be necessary and
where we know *why* this is the case?

> [0] List at https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/AcpiReducedHw#Section_5:_ACPI_Software_Programming_Model
> [1] Please see the ACPI Specification v5.0 for details on Hardware Reduced
>     mode (sections 3.11.1, 4.1, 5.2.9, at a minimum).
> 
> Changes for v6:
>    -- Allow selection of CONFIG_ACPI_REDUCED_HARDWARE_ONLY to be more
>       architecture-independent, but make it harder to accidentally enable.
>    -- Make sure that ACPI ECs do not use the ACPI global lock when in
>       hardware reduced mode (I had missed this case in earlier reviews
>       of the kernel tree).
> 
> Changes for v5:
>    -- Clarify that if the kernel config option to build ACPI hardware reduced
>       mode is used, it builds a hardware reduced *only* kernel (i.e., full
>       legacy ACPI mode will no longer work).
> 
> Changes for v4:
>    -- Given the current state of ACPICA, disable CONFIG_ACPI_REDUCED_HARDWARE
>       for use on anything other than ARM.
>    -- Replaced #ifdefs with run-time checking for hardware reduced mode,
>       whenever possible
> 
> Changes for v3:
>    -- Modified enabling ACPI_REDUCED_HARDWARE in ACPICA when using
>       kernel config item CONFIG_ACPI_REDUCED_HARDWARE; now consistent
>       with ACPICA code base where needed
>    -- Enable X86 for CONFIG_ACPI_REDUCED_HARDWARE
>    -- Minimize bus master reload patching
>    -- Remove unneeded patch for dmi_check_system() (was 4/6)
>    -- Correct the patch for removing unneeded map/unmap of FADT fields
> 
> Changes for v2:
>    -- Remove patch that was outside of reduced HW mode changes
>    -- Simplify CONFIG_ACPI_REDUCED_HARDWARE in Kconfig
>    -- Simplify use of CONFIG_ACPI_REDUCED_HARDWARE in #ifdefs
>    -- Ensure changelogs are present
>    -- Combine and simplify previous patches 8 & 10
> 
> 
> Al Stone (6):
>   ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI
>     mode
>   ACPI: bus master reload not supported in reduced HW mode
>   ACPI: HW reduced mode does not allow use of the FADT sci_interrupt
>     field
>   ACPI: in HW reduced mode, using FADT PM information is not allowed.
>   ACPI: do not map/unmap memory regions for FADT entries in reduced HW
>     mode
>   ACPI: make sure ECs do not use the ACPI global lock
> 
>  drivers/acpi/Kconfig            | 12 ++++++++++++
>  drivers/acpi/bus.c              | 30 ++++++++++++++++--------------
>  drivers/acpi/ec.c               | 12 ++++++++++--
>  drivers/acpi/osl.c              | 38 +++++++++++++++++++-------------------
>  drivers/acpi/pci_link.c         |  2 ++
>  drivers/acpi/processor_idle.c   | 14 ++++++++++++--
>  include/acpi/platform/aclinux.h |  6 ++++++
>  7 files changed, 77 insertions(+), 37 deletions(-)
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v6 0/6] Hardware Reduced Mode Cleanup for ACPI

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:14 PM al.stone at linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> This series of patches starts with Hanjun's patch to create a kernel
> config item for CONFIG_ACPI_REDUCED_HARDWARE [0].  Building on that, I
> then reviewed all of the code that touched any of several fields in the
> FADT that the OSPM is supposed to ignore when ACPI is in Hardware Reduced
> mode [1].  Any time there was a use of one of the fields to be ignored,
> I evaluated whether or not the code was implementing Hardware Reduced
> mode correctly.  Similarly, for each the flags in the FADT flags field
> that are to be ignored in Hardware Reduced mode, the kernel code was again
> scanned for proper usage.  The remainder of the patches are to fix all of
> the situations I could find where the kernel would not behave correctly
> in this ACPI mode.
> 
> These seem to work just fine on the RTSM model for ARMv7, both with and
> without ACPI enabled, and with and without ACPI_REDUCED_HARDWARE enabled;
> similarly for the FVP model for ARMv8.  The patches for ACPI on ARM
> hardware have been submitted elsewhere but they presume that reduced HW
> mode is functioning correctly.  In the meantime, there's no way I can think
> of to test all possible scenarios so feedback would be greatly appreciated.

Quite frankly, I'm seeing a pattern in this patchset that I'm not really
liking, which is making changes on a hunch.  Especially things like
changing code under #ifdef CONFIG_X86 and similar.

Can we please focus on making changes that are *known* to be necessary and
where we know *why* this is the case?

> [0] List at https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/AcpiReducedHw#Section_5:_ACPI_Software_Programming_Model
> [1] Please see the ACPI Specification v5.0 for details on Hardware Reduced
>     mode (sections 3.11.1, 4.1, 5.2.9, at a minimum).
> 
> Changes for v6:
>    -- Allow selection of CONFIG_ACPI_REDUCED_HARDWARE_ONLY to be more
>       architecture-independent, but make it harder to accidentally enable.
>    -- Make sure that ACPI ECs do not use the ACPI global lock when in
>       hardware reduced mode (I had missed this case in earlier reviews
>       of the kernel tree).
> 
> Changes for v5:
>    -- Clarify that if the kernel config option to build ACPI hardware reduced
>       mode is used, it builds a hardware reduced *only* kernel (i.e., full
>       legacy ACPI mode will no longer work).
> 
> Changes for v4:
>    -- Given the current state of ACPICA, disable CONFIG_ACPI_REDUCED_HARDWARE
>       for use on anything other than ARM.
>    -- Replaced #ifdefs with run-time checking for hardware reduced mode,
>       whenever possible
> 
> Changes for v3:
>    -- Modified enabling ACPI_REDUCED_HARDWARE in ACPICA when using
>       kernel config item CONFIG_ACPI_REDUCED_HARDWARE; now consistent
>       with ACPICA code base where needed
>    -- Enable X86 for CONFIG_ACPI_REDUCED_HARDWARE
>    -- Minimize bus master reload patching
>    -- Remove unneeded patch for dmi_check_system() (was 4/6)
>    -- Correct the patch for removing unneeded map/unmap of FADT fields
> 
> Changes for v2:
>    -- Remove patch that was outside of reduced HW mode changes
>    -- Simplify CONFIG_ACPI_REDUCED_HARDWARE in Kconfig
>    -- Simplify use of CONFIG_ACPI_REDUCED_HARDWARE in #ifdefs
>    -- Ensure changelogs are present
>    -- Combine and simplify previous patches 8 & 10
> 
> 
> Al Stone (6):
>   ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI
>     mode
>   ACPI: bus master reload not supported in reduced HW mode
>   ACPI: HW reduced mode does not allow use of the FADT sci_interrupt
>     field
>   ACPI: in HW reduced mode, using FADT PM information is not allowed.
>   ACPI: do not map/unmap memory regions for FADT entries in reduced HW
>     mode
>   ACPI: make sure ECs do not use the ACPI global lock
> 
>  drivers/acpi/Kconfig            | 12 ++++++++++++
>  drivers/acpi/bus.c              | 30 ++++++++++++++++--------------
>  drivers/acpi/ec.c               | 12 ++++++++++--
>  drivers/acpi/osl.c              | 38 +++++++++++++++++++-------------------
>  drivers/acpi/pci_link.c         |  2 ++
>  drivers/acpi/processor_idle.c   | 14 ++++++++++++--
>  include/acpi/platform/aclinux.h |  6 ++++++
>  7 files changed, 77 insertions(+), 37 deletions(-)
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 4/6] ACPI: in HW reduced mode, using FADT PM information is not allowed.

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:18 PM al.stone@linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> Per the ACPI 5.0 specification, section 5.2.9, none of the power
> management fields in the FADT are to be used.  Short-circuit using
> any of those fields in acpi_processor_get_power_info_fadt().

So the spec says that we're supposed to ignore the information in
certain FADT fields if the HW_REDUCED_ACPI flag is set.

However, what about systems in which that information is actually
valid even though HW_REDUCED_ACPI is set?

Does it break things if we attempt to use that information?

> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  drivers/acpi/processor_idle.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index eac984a..89f0a0f 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -270,6 +270,14 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
>  	if (!pr->pblk)
>  		return -ENODEV;
>  
> +	/*
> +	 * Using power management information from the FADT is not
> +	 * allowed when in HW reduced mode.  See ACPI 5.0, section
> +	 * 5.2.9.
> +	 */
> +	if (acpi_gbl_reduced_hardware)
> +		return -ENODEV;
> +
>  	/* if info is obtained from pblk/fadt, type equals state */
>  	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
>  	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v6 4/6] ACPI: in HW reduced mode, using FADT PM information is not allowed.

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:18 PM al.stone at linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> Per the ACPI 5.0 specification, section 5.2.9, none of the power
> management fields in the FADT are to be used.  Short-circuit using
> any of those fields in acpi_processor_get_power_info_fadt().

So the spec says that we're supposed to ignore the information in
certain FADT fields if the HW_REDUCED_ACPI flag is set.

However, what about systems in which that information is actually
valid even though HW_REDUCED_ACPI is set?

Does it break things if we attempt to use that information?

> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  drivers/acpi/processor_idle.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index eac984a..89f0a0f 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -270,6 +270,14 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
>  	if (!pr->pblk)
>  		return -ENODEV;
>  
> +	/*
> +	 * Using power management information from the FADT is not
> +	 * allowed when in HW reduced mode.  See ACPI 5.0, section
> +	 * 5.2.9.
> +	 */
> +	if (acpi_gbl_reduced_hardware)
> +		return -ENODEV;
> +
>  	/* if info is obtained from pblk/fadt, type equals state */
>  	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
>  	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:19 PM al.stone@linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> Several of the FADT fields are normally kept in specific memory
> regions.  Since these fields are to be ignored in hardware reduced
> ACPI mode, do not map those addresses when in that mode, and of
> course do not release the mappings that have not been made.
> 
> The function acpi_os_initialize() could become a stub in the header
> file but is left here in case it can be of further use.

Why exactly is this change necessary?

Will things work incorrectly on HW-reduced ACPI systems if we don't make it?

> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  drivers/acpi/osl.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index c946a3a..7822821 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -1778,10 +1778,12 @@ __setup("acpi_no_auto_ssdt", acpi_no_auto_ssdt_setup);
>  
>  acpi_status __init acpi_os_initialize(void)
>  {
> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
> +	if (!acpi_gbl_reduced_hardware) {
> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
> +	}
>  
>  	return AE_OK;
>  }
> @@ -1806,10 +1808,12 @@ acpi_status acpi_os_terminate(void)
>  						 acpi_irq_handler);
>  	}
>  
> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> +	if (!acpi_gbl_reduced_hardware) {
> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> +	}
>  
>  	destroy_workqueue(kacpid_wq);
>  	destroy_workqueue(kacpi_notify_wq);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:19 PM al.stone at linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> Several of the FADT fields are normally kept in specific memory
> regions.  Since these fields are to be ignored in hardware reduced
> ACPI mode, do not map those addresses when in that mode, and of
> course do not release the mappings that have not been made.
> 
> The function acpi_os_initialize() could become a stub in the header
> file but is left here in case it can be of further use.

Why exactly is this change necessary?

Will things work incorrectly on HW-reduced ACPI systems if we don't make it?

> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  drivers/acpi/osl.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index c946a3a..7822821 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -1778,10 +1778,12 @@ __setup("acpi_no_auto_ssdt", acpi_no_auto_ssdt_setup);
>  
>  acpi_status __init acpi_os_initialize(void)
>  {
> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
> +	if (!acpi_gbl_reduced_hardware) {
> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
> +	}
>  
>  	return AE_OK;
>  }
> @@ -1806,10 +1808,12 @@ acpi_status acpi_os_terminate(void)
>  						 acpi_irq_handler);
>  	}
>  
> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> +	if (!acpi_gbl_reduced_hardware) {
> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
> +	}
>  
>  	destroy_workqueue(kacpid_wq);
>  	destroy_workqueue(kacpi_notify_wq);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 6/6] ACPI: make sure ECs do not use the ACPI global lock

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:20 PM al.stone@linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> ACPI ECs (Embedded Controllers) are allowed to use the ACPI global
> lock in legacy mode.  Since there is no global lock in hardware
> reduced mode, make sure that ECs cannot inadvertently use it with
> older ACPI tables that may have defined an _GLK method for the
> EC device.  Since an individual lock can and should be defined for
> each EC, access to each EC should still be properly controlled.
> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  drivers/acpi/ec.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index ba5b56d..3f15110 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -745,9 +745,17 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
>  	if (ACPI_FAILURE(status))
>  		return status;
>  	ec->gpe = tmp;
> -	/* Use the global lock for all EC transactions? */
> +	/*
> +	 * Use the global lock for all EC transactions?
> +	 *
> +	 * If ACPI is in hardware reduced mode, there is no global lock.
> +	 * If a _GLK method is defined, it needs to be ignored, just
> +	 * in case it returns "true", which could happen if the ASL is
> +	 * written incorrectly.

What is going to happen if it is defined and works correctly in turn?

> +	 */
>  	tmp = 0;
> -	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
> +	if (!acpi_gbl_reduced_hardware)
> +		acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
>  	ec->global_lock = tmp;
>  	ec->handle = handle;
>  	return AE_CTRL_TERMINATE;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v6 6/6] ACPI: make sure ECs do not use the ACPI global lock

[Rafael J. Wysocki]

On Friday, January 10, 2014 03:52:20 PM al.stone at linaro.org wrote:
> From: Al Stone <al.stone@linaro.org>
> 
> ACPI ECs (Embedded Controllers) are allowed to use the ACPI global
> lock in legacy mode.  Since there is no global lock in hardware
> reduced mode, make sure that ECs cannot inadvertently use it with
> older ACPI tables that may have defined an _GLK method for the
> EC device.  Since an individual lock can and should be defined for
> each EC, access to each EC should still be properly controlled.
> 
> Signed-off-by: Al Stone <al.stone@linaro.org>
> ---
>  drivers/acpi/ec.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index ba5b56d..3f15110 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -745,9 +745,17 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
>  	if (ACPI_FAILURE(status))
>  		return status;
>  	ec->gpe = tmp;
> -	/* Use the global lock for all EC transactions? */
> +	/*
> +	 * Use the global lock for all EC transactions?
> +	 *
> +	 * If ACPI is in hardware reduced mode, there is no global lock.
> +	 * If a _GLK method is defined, it needs to be ignored, just
> +	 * in case it returns "true", which could happen if the ASL is
> +	 * written incorrectly.

What is going to happen if it is defined and works correctly in turn?

> +	 */
>  	tmp = 0;
> -	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
> +	if (!acpi_gbl_reduced_hardware)
> +		acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
>  	ec->global_lock = tmp;
>  	ec->handle = handle;
>  	return AE_CTRL_TERMINATE;
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 1/6] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI mode

[Rafael J. Wysocki]

On Friday, January 10, 2014 04:13:54 PM Al Stone wrote:
> On 01/10/2014 04:11 PM, Rafael J. Wysocki wrote:
> > On Friday, January 10, 2014 03:52:15 PM al.stone@linaro.org wrote:
> >> From: Al Stone <al.stone@linaro.org>
> >>
> >> Hardware reduced mode, despite the name, exists primarily to allow
> >> newer platforms to use a much simpler form of ACPI that does not
> >> require supporting the legacy of previous versions of the specification.
> >> This mode was first introduced in the ACPI 5.0 specification, but because
> >> it is so much simpler and reduces the size of the object code needed to
> >> support ACPI, it is likely to be used more often in the near future.
> >>
> >> To enable the hardware reduced mode of ACPI on some platforms (such as
> >> ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
> >> to TRUE in the ACPICA source.  For ARM/ARM64, hardware reduced ACPI
> >> should be the only mode used; legacy mode would require modifications
> >> to SoCs in order to provide several x86-specific hardware features (e.g.,
> >> an NMI and SMI support).
> >>
> >> We set ACPI_REDUCED_HARDWARE to TRUE in the ACPICA source by introducing
> >> a kernel config item to enable/disable ACPI_REDUCED_HARDWARE.  We can then
> >> change the kernel config instead of having to modify the kernel source
> >> directly to enable the reduced hardware mode of ACPI.
> >>
> >> Lv Zheng suggested that this configuration item does not belong in ACPICA,
> >> the upstream source for much of the ACPI internals, but rather to the
> >> Linux kernel itself.  Hence, we introduce this flag so that we can make
> >> ACPI_REDUCED_HARDWARE configurable.  For the details of the discussion,
> >> please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html
> >>
> >> Even though support for X86 in hardware reduced mode is possible, it
> >> is NOT enabled.  Extensive effort has gone into the Linux kernel so that
> >> there is a single kernel image than can run on all x86 hardware; the kernel
> >> changes run-time behavior to adapt to the hardware being used.  This is not
> >> currently possible with the existing ACPICA infrastructure but only presents
> >> a problem on achitectures supporting both hardware-reduced and legacy modes
> >> of ACPI -- i.e., on x86 only.
> >>
> >> The problem with the current ACPICA code base is that if one builds legacy
> >> ACPI (a proper superset of hardware-reduced), the kernel can run in hardware-
> >> reduced with the proper ACPI tables, but there is still ACPICA code that could
> >> be executed even though it is not allowed by the specification.  If one builds
> >> a hardware-reduced only ACPI, the kernel cannot run with ACPI tables that are
> >> for legacy mode.  To ensure compliance with ACPI, one must therefore build
> >> two separate kernels.  Once this problem has been properly fixed, we can then
> >> enable x86 hardware-reduced mode and use a single kernel.
> >>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> ---
> >>   drivers/acpi/Kconfig            | 12 ++++++++++++
> >>   include/acpi/platform/aclinux.h |  6 ++++++
> >>   2 files changed, 18 insertions(+)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index 4770de5..75dd38a 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -343,6 +343,18 @@ config ACPI_BGRT
> >>   	  data from the firmware boot splash. It will appear under
> >>   	  /sys/firmware/acpi/bgrt/ .
> >>
> >> +config ACPI_REDUCED_HARDWARE_ONLY
> >> +	bool "Hardware-reduced ACPI support only"
> >> +	depends on !X86 && !IA64 || EXPERT
> >> +	help
> >> +	This config item changes the way the ACPI code is built.  When this
> >> +	option is selected, the kernel will use a specialized version of
> >> +	ACPICA that ONLY supports the ACPI "reduced hardware" mode.  The
> >> +	resulting kernel will be smaller but it will also be restricted to
> >> +	running in ACPI reduced hardware mode ONLY.
> >> +
> >> +	If you are unsure what to do, do not enable this option.
> >
> > I'm still not exactly convinced this is the best way to do that.
> >
> > In particular, I wonder if it would be viable to make it disabled by default
> > and then make the interested architectures do
> >
> > 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
> >
> > in their top-level Kconfig files?
> 
> Hrm.  Do you mean something like this:
> 
> config ACPI_REDUCED_HARDWARE_ONLY
> 	bool "Hardware-reduced ACPI support only"
> 	default n
> 	depends on EXPERT
> 	help
> 	...
> 
> and remove any mention of architecture?

Yes.

> Or forgo the "depends" entirely?  From my perspective, any of the above is sufficient
> so I'll defer to your broader view of ACPI.

Well, I thought about something like this:

config ACPI_REDUCED_HARDWARE_ONLY
	def_bool n
	depends on ACPI && EXPERT

and then do the above select in arch-level Kconfigs where necessary.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v6 1/6] ACPI: introduce CONFIG_ACPI_REDUCED_HARDWARE_ONLY to enforce this ACPI mode

[Rafael J. Wysocki]

On Friday, January 10, 2014 04:13:54 PM Al Stone wrote:
> On 01/10/2014 04:11 PM, Rafael J. Wysocki wrote:
> > On Friday, January 10, 2014 03:52:15 PM al.stone at linaro.org wrote:
> >> From: Al Stone <al.stone@linaro.org>
> >>
> >> Hardware reduced mode, despite the name, exists primarily to allow
> >> newer platforms to use a much simpler form of ACPI that does not
> >> require supporting the legacy of previous versions of the specification.
> >> This mode was first introduced in the ACPI 5.0 specification, but because
> >> it is so much simpler and reduces the size of the object code needed to
> >> support ACPI, it is likely to be used more often in the near future.
> >>
> >> To enable the hardware reduced mode of ACPI on some platforms (such as
> >> ARM), we need to modify the kernel code and set ACPI_REDUCED_HARDWARE
> >> to TRUE in the ACPICA source.  For ARM/ARM64, hardware reduced ACPI
> >> should be the only mode used; legacy mode would require modifications
> >> to SoCs in order to provide several x86-specific hardware features (e.g.,
> >> an NMI and SMI support).
> >>
> >> We set ACPI_REDUCED_HARDWARE to TRUE in the ACPICA source by introducing
> >> a kernel config item to enable/disable ACPI_REDUCED_HARDWARE.  We can then
> >> change the kernel config instead of having to modify the kernel source
> >> directly to enable the reduced hardware mode of ACPI.
> >>
> >> Lv Zheng suggested that this configuration item does not belong in ACPICA,
> >> the upstream source for much of the ACPI internals, but rather to the
> >> Linux kernel itself.  Hence, we introduce this flag so that we can make
> >> ACPI_REDUCED_HARDWARE configurable.  For the details of the discussion,
> >> please refer to: http://www.spinics.net/lists/linux-acpi/msg46369.html
> >>
> >> Even though support for X86 in hardware reduced mode is possible, it
> >> is NOT enabled.  Extensive effort has gone into the Linux kernel so that
> >> there is a single kernel image than can run on all x86 hardware; the kernel
> >> changes run-time behavior to adapt to the hardware being used.  This is not
> >> currently possible with the existing ACPICA infrastructure but only presents
> >> a problem on achitectures supporting both hardware-reduced and legacy modes
> >> of ACPI -- i.e., on x86 only.
> >>
> >> The problem with the current ACPICA code base is that if one builds legacy
> >> ACPI (a proper superset of hardware-reduced), the kernel can run in hardware-
> >> reduced with the proper ACPI tables, but there is still ACPICA code that could
> >> be executed even though it is not allowed by the specification.  If one builds
> >> a hardware-reduced only ACPI, the kernel cannot run with ACPI tables that are
> >> for legacy mode.  To ensure compliance with ACPI, one must therefore build
> >> two separate kernels.  Once this problem has been properly fixed, we can then
> >> enable x86 hardware-reduced mode and use a single kernel.
> >>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> ---
> >>   drivers/acpi/Kconfig            | 12 ++++++++++++
> >>   include/acpi/platform/aclinux.h |  6 ++++++
> >>   2 files changed, 18 insertions(+)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index 4770de5..75dd38a 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -343,6 +343,18 @@ config ACPI_BGRT
> >>   	  data from the firmware boot splash. It will appear under
> >>   	  /sys/firmware/acpi/bgrt/ .
> >>
> >> +config ACPI_REDUCED_HARDWARE_ONLY
> >> +	bool "Hardware-reduced ACPI support only"
> >> +	depends on !X86 && !IA64 || EXPERT
> >> +	help
> >> +	This config item changes the way the ACPI code is built.  When this
> >> +	option is selected, the kernel will use a specialized version of
> >> +	ACPICA that ONLY supports the ACPI "reduced hardware" mode.  The
> >> +	resulting kernel will be smaller but it will also be restricted to
> >> +	running in ACPI reduced hardware mode ONLY.
> >> +
> >> +	If you are unsure what to do, do not enable this option.
> >
> > I'm still not exactly convinced this is the best way to do that.
> >
> > In particular, I wonder if it would be viable to make it disabled by default
> > and then make the interested architectures do
> >
> > 	select ACPI_REDUCED_HARDWARE_ONLY if ACPI
> >
> > in their top-level Kconfig files?
> 
> Hrm.  Do you mean something like this:
> 
> config ACPI_REDUCED_HARDWARE_ONLY
> 	bool "Hardware-reduced ACPI support only"
> 	default n
> 	depends on EXPERT
> 	help
> 	...
> 
> and remove any mention of architecture?

Yes.

> Or forgo the "depends" entirely?  From my perspective, any of the above is sufficient
> so I'll defer to your broader view of ACPI.

Well, I thought about something like this:

config ACPI_REDUCED_HARDWARE_ONLY
	def_bool n
	depends on ACPI && EXPERT

and then do the above select in arch-level Kconfigs where necessary.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 3/6] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field

[Al Stone]

On 01/10/2014 04:20 PM, Rafael J. Wysocki wrote:
> On Friday, January 10, 2014 03:52:17 PM al.stone@linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> In HW reduced mode, the use of the SCI interrupt is not allowed.  In
>> all those places that use the FADT sci_interrupt field, make sure we
>> do not execute that path when in HW reduced mode.
>>
>> In the case of acpi_os_install_interrupt_handler() in osl.c, this allows
>> us to open up the routine to installing interrupt handlers other than
>> acpi_gbl_FADT.sci_interrupt regardless of whether we are in ACPI legacy
>> mode or reduced HW mode; acpi_os_remove_interrupt_handler() changes to
>> maintain symmetry.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   drivers/acpi/bus.c      | 30 ++++++++++++++++--------------
>>   drivers/acpi/osl.c      | 18 +++++++-----------
>>   drivers/acpi/pci_link.c |  2 ++
>>   3 files changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 0710004..d871859 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -552,21 +552,23 @@ void __init acpi_early_init(void)
>>   	}
>>
>>   #ifdef CONFIG_X86
>> -	if (!acpi_ioapic) {
>> -		/* compatible (0) means level (3) */
>> -		if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
>> -			acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
>> -			acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
>
> I wonder why exactly you want to make this change.  It surely doesn't matter
> for ARM and do you have any HW-reduced x86 hardware to test it?

The point of this exercise was not to make hardware reduced work for
just ARM; the point was to ensure hardware reduced is working correctly
for any architecture using ACPI.

No, I do not have hardware reduced x86 hardware to test with.  Nor can
I predict if or when Intel will make such a thing.  If you would rather
wait until they do, so be it.  My intent was to avoid problems before
they occur, rather than waiting until they actually break.

I chose to make this change because (a) it seemed silly to me to
execute code you don't have to, (b) since the kernel code does end
up acting as documentation for how ACPI is implemented and how it
is compliant with the spec, it made sense to me to ensure all usage
was compliant with the spec, and (c) if the machine has an IOAPIC
and acpi_sci=n (n is non-zero) is supplied on the command line, the
FADT SCI interrupt will end being initialized and used which is not
allowed in hardware reduced mode.

>> +	if (!acpi_gbl_reduced_hardware) {
>> +		if (!acpi_ioapic) {
>> +			/* compatible (0) means level (3) */
>> +			if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
>> +				acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
>> +				acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
>> +			}
>> +			/* Set PIC-mode SCI trigger type */
>> +			acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
>> +				(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
>> +		} else {
>> +			/*
>> +			 * now that acpi_gbl_FADT is initialized,
>> +			 * update it with result from INT_SRC_OVR parsing
>> +			 */
>> +			acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
>>   		}
>> -		/* Set PIC-mode SCI trigger type */
>> -		acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
>> -					 (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
>> -	} else {
>> -		/*
>> -		 * now that acpi_gbl_FADT is initialized,
>> -		 * update it with result from INT_SRC_OVR parsing
>> -		 */
>> -		acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
>>   	}
>>   #endif
>>
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index 44c07eb..c946a3a 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
>>
>>   static acpi_osd_handler acpi_irq_handler;
>>   static void *acpi_irq_context;
>> +static u32 acpi_irq_number;
>>   static struct workqueue_struct *kacpid_wq;
>>   static struct workqueue_struct *kacpi_notify_wq;
>>   static struct workqueue_struct *kacpi_hotplug_wq;
>> @@ -178,6 +179,10 @@ static void __init acpi_request_region (struct acpi_generic_address *gas,
>>
>>   static int __init acpi_reserve_resources(void)
>>   {
>> +	/* Handle hardware reduced mode: i.e., do nothing. */
>> +	if (acpi_gbl_reduced_hardware)
>> +		return 0;
>
> Does it actually break things if we do that?

Not that I can see anywhere.  All of the code I have looked at appears
to behave correctly if the ignored fields in the FADT are set to zero
-- in this case, not using or checking bits in the xpm1a_event_blocks
if the FADT field is zero.

>> +
>>   	acpi_request_region(&acpi_gbl_FADT.xpm1a_event_block, acpi_gbl_FADT.pm1_event_length,
>>   		"ACPI PM1a_EVT_BLK");
>>
>> @@ -795,13 +800,6 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>>
>>   	acpi_irq_stats_init();
>>
>> -	/*
>> -	 * ACPI interrupts different from the SCI in our copy of the FADT are
>> -	 * not supported.
>> -	 */
>> -	if (gsi != acpi_gbl_FADT.sci_interrupt)
>> -		return AE_BAD_PARAMETER;
>> -
>>   	if (acpi_irq_handler)
>>   		return AE_ALREADY_ACQUIRED;
>>
>> @@ -818,15 +816,13 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>>   		acpi_irq_handler = NULL;
>>   		return AE_NOT_ACQUIRED;
>>   	}
>> +	acpi_irq_number = irq;
>>
>>   	return AE_OK;
>>   }
>>
>>   acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
>>   {
>> -	if (irq != acpi_gbl_FADT.sci_interrupt)
>> -		return AE_BAD_PARAMETER;
>> -
>>   	free_irq(irq, acpi_irq);
>>   	acpi_irq_handler = NULL;
>>
>> @@ -1806,7 +1802,7 @@ acpi_status __init acpi_os_initialize1(void)
>>   acpi_status acpi_os_terminate(void)
>>   {
>>   	if (acpi_irq_handler) {
>> -		acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
>> +		acpi_os_remove_interrupt_handler(acpi_irq_number,
>>   						 acpi_irq_handler);
>>   	}
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index 2652a61..d5c155e 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -505,6 +505,8 @@ int __init acpi_irq_penalty_init(void)
>>   		}
>>   	}
>>   	/* Add a penalty for the SCI */
>> +	if (acpi_gbl_reduced_hardware)
>> +		return 0;
>>   	acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
>>   	return 0;
>>   }
>
> Is ARM really going to use the code in pci_link.c?  If so, then how exactly?

I have no idea; I wasn't thinking strictly ARM in the first place.
Just the same, why execute code if it is irrelevant?  Further, if
hardware reduced ACPI tables have the SCI interrupt field set to
zero as they should, IRQ 0 gets a hefty penalty assessed; or, if the
hardware reduced tables were made by taking a shortcut and just toggling
the hardware reduced flag while leaving the SCI interrupt field alone,
some other IRQ gets a penalty assessed, neither of which seemed to me
to be correct either.

> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

[PATCH v6 3/6] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field

[Al Stone]

On 01/10/2014 04:20 PM, Rafael J. Wysocki wrote:
> On Friday, January 10, 2014 03:52:17 PM al.stone at linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> In HW reduced mode, the use of the SCI interrupt is not allowed.  In
>> all those places that use the FADT sci_interrupt field, make sure we
>> do not execute that path when in HW reduced mode.
>>
>> In the case of acpi_os_install_interrupt_handler() in osl.c, this allows
>> us to open up the routine to installing interrupt handlers other than
>> acpi_gbl_FADT.sci_interrupt regardless of whether we are in ACPI legacy
>> mode or reduced HW mode; acpi_os_remove_interrupt_handler() changes to
>> maintain symmetry.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   drivers/acpi/bus.c      | 30 ++++++++++++++++--------------
>>   drivers/acpi/osl.c      | 18 +++++++-----------
>>   drivers/acpi/pci_link.c |  2 ++
>>   3 files changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 0710004..d871859 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -552,21 +552,23 @@ void __init acpi_early_init(void)
>>   	}
>>
>>   #ifdef CONFIG_X86
>> -	if (!acpi_ioapic) {
>> -		/* compatible (0) means level (3) */
>> -		if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
>> -			acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
>> -			acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
>
> I wonder why exactly you want to make this change.  It surely doesn't matter
> for ARM and do you have any HW-reduced x86 hardware to test it?

The point of this exercise was not to make hardware reduced work for
just ARM; the point was to ensure hardware reduced is working correctly
for any architecture using ACPI.

No, I do not have hardware reduced x86 hardware to test with.  Nor can
I predict if or when Intel will make such a thing.  If you would rather
wait until they do, so be it.  My intent was to avoid problems before
they occur, rather than waiting until they actually break.

I chose to make this change because (a) it seemed silly to me to
execute code you don't have to, (b) since the kernel code does end
up acting as documentation for how ACPI is implemented and how it
is compliant with the spec, it made sense to me to ensure all usage
was compliant with the spec, and (c) if the machine has an IOAPIC
and acpi_sci=n (n is non-zero) is supplied on the command line, the
FADT SCI interrupt will end being initialized and used which is not
allowed in hardware reduced mode.

>> +	if (!acpi_gbl_reduced_hardware) {
>> +		if (!acpi_ioapic) {
>> +			/* compatible (0) means level (3) */
>> +			if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
>> +				acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
>> +				acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
>> +			}
>> +			/* Set PIC-mode SCI trigger type */
>> +			acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
>> +				(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
>> +		} else {
>> +			/*
>> +			 * now that acpi_gbl_FADT is initialized,
>> +			 * update it with result from INT_SRC_OVR parsing
>> +			 */
>> +			acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
>>   		}
>> -		/* Set PIC-mode SCI trigger type */
>> -		acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
>> -					 (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
>> -	} else {
>> -		/*
>> -		 * now that acpi_gbl_FADT is initialized,
>> -		 * update it with result from INT_SRC_OVR parsing
>> -		 */
>> -		acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
>>   	}
>>   #endif
>>
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index 44c07eb..c946a3a 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
>>
>>   static acpi_osd_handler acpi_irq_handler;
>>   static void *acpi_irq_context;
>> +static u32 acpi_irq_number;
>>   static struct workqueue_struct *kacpid_wq;
>>   static struct workqueue_struct *kacpi_notify_wq;
>>   static struct workqueue_struct *kacpi_hotplug_wq;
>> @@ -178,6 +179,10 @@ static void __init acpi_request_region (struct acpi_generic_address *gas,
>>
>>   static int __init acpi_reserve_resources(void)
>>   {
>> +	/* Handle hardware reduced mode: i.e., do nothing. */
>> +	if (acpi_gbl_reduced_hardware)
>> +		return 0;
>
> Does it actually break things if we do that?

Not that I can see anywhere.  All of the code I have looked at appears
to behave correctly if the ignored fields in the FADT are set to zero
-- in this case, not using or checking bits in the xpm1a_event_blocks
if the FADT field is zero.

>> +
>>   	acpi_request_region(&acpi_gbl_FADT.xpm1a_event_block, acpi_gbl_FADT.pm1_event_length,
>>   		"ACPI PM1a_EVT_BLK");
>>
>> @@ -795,13 +800,6 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>>
>>   	acpi_irq_stats_init();
>>
>> -	/*
>> -	 * ACPI interrupts different from the SCI in our copy of the FADT are
>> -	 * not supported.
>> -	 */
>> -	if (gsi != acpi_gbl_FADT.sci_interrupt)
>> -		return AE_BAD_PARAMETER;
>> -
>>   	if (acpi_irq_handler)
>>   		return AE_ALREADY_ACQUIRED;
>>
>> @@ -818,15 +816,13 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
>>   		acpi_irq_handler = NULL;
>>   		return AE_NOT_ACQUIRED;
>>   	}
>> +	acpi_irq_number = irq;
>>
>>   	return AE_OK;
>>   }
>>
>>   acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
>>   {
>> -	if (irq != acpi_gbl_FADT.sci_interrupt)
>> -		return AE_BAD_PARAMETER;
>> -
>>   	free_irq(irq, acpi_irq);
>>   	acpi_irq_handler = NULL;
>>
>> @@ -1806,7 +1802,7 @@ acpi_status __init acpi_os_initialize1(void)
>>   acpi_status acpi_os_terminate(void)
>>   {
>>   	if (acpi_irq_handler) {
>> -		acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
>> +		acpi_os_remove_interrupt_handler(acpi_irq_number,
>>   						 acpi_irq_handler);
>>   	}
>>
>> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
>> index 2652a61..d5c155e 100644
>> --- a/drivers/acpi/pci_link.c
>> +++ b/drivers/acpi/pci_link.c
>> @@ -505,6 +505,8 @@ int __init acpi_irq_penalty_init(void)
>>   		}
>>   	}
>>   	/* Add a penalty for the SCI */
>> +	if (acpi_gbl_reduced_hardware)
>> +		return 0;
>>   	acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
>>   	return 0;
>>   }
>
> Is ARM really going to use the code in pci_link.c?  If so, then how exactly?

I have no idea; I wasn't thinking strictly ARM in the first place.
Just the same, why execute code if it is irrelevant?  Further, if
hardware reduced ACPI tables have the SCI interrupt field set to
zero as they should, IRQ 0 gets a hefty penalty assessed; or, if the
hardware reduced tables were made by taking a shortcut and just toggling
the hardware reduced flag while leaving the SCI interrupt field alone,
some other IRQ gets a penalty assessed, neither of which seemed to me
to be correct either.

> Rafael
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3 at redhat.com
-----------------------------------

Re: [PATCH v6 4/6] ACPI: in HW reduced mode, using FADT PM information is not allowed.

[Al Stone]

On 01/10/2014 04:31 PM, Rafael J. Wysocki wrote:
> On Friday, January 10, 2014 03:52:18 PM al.stone@linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> Per the ACPI 5.0 specification, section 5.2.9, none of the power
>> management fields in the FADT are to be used.  Short-circuit using
>> any of those fields in acpi_processor_get_power_info_fadt().
>
> So the spec says that we're supposed to ignore the information in
> certain FADT fields if the HW_REDUCED_ACPI flag is set.
>
> However, what about systems in which that information is actually
> valid even though HW_REDUCED_ACPI is set?
>
> Does it break things if we attempt to use that information?

The way I see it in the spec, we just don't use that info _if_ we wish
to remain compliant with the spec.  If we insist on using it anyway, we
can't say we're implementing hardware reduced mode correctly.  That
being said, from my read of the code it appears things would operate
the same as they did before if this part of the spec was ignored _and_
the info in the tables was still valid (same control register addresses
and content and so on).

>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   drivers/acpi/processor_idle.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index eac984a..89f0a0f 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -270,6 +270,14 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
>>   	if (!pr->pblk)
>>   		return -ENODEV;
>>
>> +	/*
>> +	 * Using power management information from the FADT is not
>> +	 * allowed when in HW reduced mode.  See ACPI 5.0, section
>> +	 * 5.2.9.
>> +	 */
>> +	if (acpi_gbl_reduced_hardware)
>> +		return -ENODEV;
>> +
>>   	/* if info is obtained from pblk/fadt, type equals state */
>>   	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
>>   	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
>>
>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------

[PATCH v6 4/6] ACPI: in HW reduced mode, using FADT PM information is not allowed.

[Al Stone]

On 01/10/2014 04:31 PM, Rafael J. Wysocki wrote:
> On Friday, January 10, 2014 03:52:18 PM al.stone at linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> Per the ACPI 5.0 specification, section 5.2.9, none of the power
>> management fields in the FADT are to be used.  Short-circuit using
>> any of those fields in acpi_processor_get_power_info_fadt().
>
> So the spec says that we're supposed to ignore the information in
> certain FADT fields if the HW_REDUCED_ACPI flag is set.
>
> However, what about systems in which that information is actually
> valid even though HW_REDUCED_ACPI is set?
>
> Does it break things if we attempt to use that information?

The way I see it in the spec, we just don't use that info _if_ we wish
to remain compliant with the spec.  If we insist on using it anyway, we
can't say we're implementing hardware reduced mode correctly.  That
being said, from my read of the code it appears things would operate
the same as they did before if this part of the spec was ignored _and_
the info in the tables was still valid (same control register addresses
and content and so on).

>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   drivers/acpi/processor_idle.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
>> index eac984a..89f0a0f 100644
>> --- a/drivers/acpi/processor_idle.c
>> +++ b/drivers/acpi/processor_idle.c
>> @@ -270,6 +270,14 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
>>   	if (!pr->pblk)
>>   		return -ENODEV;
>>
>> +	/*
>> +	 * Using power management information from the FADT is not
>> +	 * allowed when in HW reduced mode.  See ACPI 5.0, section
>> +	 * 5.2.9.
>> +	 */
>> +	if (acpi_gbl_reduced_hardware)
>> +		return -ENODEV;
>> +
>>   	/* if info is obtained from pblk/fadt, type equals state */
>>   	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
>>   	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
>>
>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone at linaro.org
-----------------------------------

Re: [PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[Al Stone]

On 01/10/2014 04:32 PM, Rafael J. Wysocki wrote:
> On Friday, January 10, 2014 03:52:19 PM al.stone@linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> Several of the FADT fields are normally kept in specific memory
>> regions.  Since these fields are to be ignored in hardware reduced
>> ACPI mode, do not map those addresses when in that mode, and of
>> course do not release the mappings that have not been made.
>>
>> The function acpi_os_initialize() could become a stub in the header
>> file but is left here in case it can be of further use.
>
> Why exactly is this change necessary?

Two reasons: (1) why do work we do not have to do?  and (2) it
seemed to make sense to me to have the code reflect the spec
accurately.

> Will things work incorrectly on HW-reduced ACPI systems if we don't make it?

If the ACPI tables have all of these fields properly set to zero
in hardware reduced, this change does not need to be made.  If a
vendor provides broken ACPI tables where these values are valid,
but still sets hardware reduced in the FADT, these fields could
then be used as before -- but allowing them to be used would mean
we can no longer claim we are implementing hardware reduced correctly.
So things would work, but the system would by definition be in some
sort of undefined hybrid ACPI mode.

>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   drivers/acpi/osl.c | 20 ++++++++++++--------
>>   1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index c946a3a..7822821 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -1778,10 +1778,12 @@ __setup("acpi_no_auto_ssdt", acpi_no_auto_ssdt_setup);
>>
>>   acpi_status __init acpi_os_initialize(void)
>>   {
>> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
>> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
>> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
>> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
>> +	if (!acpi_gbl_reduced_hardware) {
>> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
>> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
>> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
>> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
>> +	}
>>
>>   	return AE_OK;
>>   }
>> @@ -1806,10 +1808,12 @@ acpi_status acpi_os_terminate(void)
>>   						 acpi_irq_handler);
>>   	}
>>
>> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
>> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
>> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
>> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
>> +	if (!acpi_gbl_reduced_hardware) {
>> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
>> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
>> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
>> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
>> +	}
>>
>>   	destroy_workqueue(kacpid_wq);
>>   	destroy_workqueue(kacpi_notify_wq);
>>
>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------

[PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[Al Stone]

On 01/10/2014 04:32 PM, Rafael J. Wysocki wrote:
> On Friday, January 10, 2014 03:52:19 PM al.stone at linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> Several of the FADT fields are normally kept in specific memory
>> regions.  Since these fields are to be ignored in hardware reduced
>> ACPI mode, do not map those addresses when in that mode, and of
>> course do not release the mappings that have not been made.
>>
>> The function acpi_os_initialize() could become a stub in the header
>> file but is left here in case it can be of further use.
>
> Why exactly is this change necessary?

Two reasons: (1) why do work we do not have to do?  and (2) it
seemed to make sense to me to have the code reflect the spec
accurately.

> Will things work incorrectly on HW-reduced ACPI systems if we don't make it?

If the ACPI tables have all of these fields properly set to zero
in hardware reduced, this change does not need to be made.  If a
vendor provides broken ACPI tables where these values are valid,
but still sets hardware reduced in the FADT, these fields could
then be used as before -- but allowing them to be used would mean
we can no longer claim we are implementing hardware reduced correctly.
So things would work, but the system would by definition be in some
sort of undefined hybrid ACPI mode.

>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   drivers/acpi/osl.c | 20 ++++++++++++--------
>>   1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> index c946a3a..7822821 100644
>> --- a/drivers/acpi/osl.c
>> +++ b/drivers/acpi/osl.c
>> @@ -1778,10 +1778,12 @@ __setup("acpi_no_auto_ssdt", acpi_no_auto_ssdt_setup);
>>
>>   acpi_status __init acpi_os_initialize(void)
>>   {
>> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
>> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
>> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
>> -	acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
>> +	if (!acpi_gbl_reduced_hardware) {
>> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
>> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
>> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe0_block);
>> +		acpi_os_map_generic_address(&acpi_gbl_FADT.xgpe1_block);
>> +	}
>>
>>   	return AE_OK;
>>   }
>> @@ -1806,10 +1808,12 @@ acpi_status acpi_os_terminate(void)
>>   						 acpi_irq_handler);
>>   	}
>>
>> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
>> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
>> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
>> -	acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
>> +	if (!acpi_gbl_reduced_hardware) {
>> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe1_block);
>> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xgpe0_block);
>> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1b_event_block);
>> +		acpi_os_unmap_generic_address(&acpi_gbl_FADT.xpm1a_event_block);
>> +	}
>>
>>   	destroy_workqueue(kacpid_wq);
>>   	destroy_workqueue(kacpi_notify_wq);
>>
>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone at linaro.org
-----------------------------------

Re: [PATCH v6 6/6] ACPI: make sure ECs do not use the ACPI global lock

[Al Stone]

On 01/10/2014 04:33 PM, Rafael J. Wysocki wrote:
> On Friday, January 10, 2014 03:52:20 PM al.stone@linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> ACPI ECs (Embedded Controllers) are allowed to use the ACPI global
>> lock in legacy mode.  Since there is no global lock in hardware
>> reduced mode, make sure that ECs cannot inadvertently use it with
>> older ACPI tables that may have defined an _GLK method for the
>> EC device.  Since an individual lock can and should be defined for
>> each EC, access to each EC should still be properly controlled.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   drivers/acpi/ec.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>> index ba5b56d..3f15110 100644
>> --- a/drivers/acpi/ec.c
>> +++ b/drivers/acpi/ec.c
>> @@ -745,9 +745,17 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
>>   	if (ACPI_FAILURE(status))
>>   		return status;
>>   	ec->gpe = tmp;
>> -	/* Use the global lock for all EC transactions? */
>> +	/*
>> +	 * Use the global lock for all EC transactions?
>> +	 *
>> +	 * If ACPI is in hardware reduced mode, there is no global lock.
>> +	 * If a _GLK method is defined, it needs to be ignored, just
>> +	 * in case it returns "true", which could happen if the ASL is
>> +	 * written incorrectly.
>
> What is going to happen if it is defined and works correctly in turn?

If for any reason the ec->global_lock field is non-zero, then every
time an EC transaction is being run (acpi_ec_transaction()), there will
be an attempt to acquire the ACPI global lock in violation of the
definition of hardware reduced mode -- the spec says that there is
no ACPI global lock in this mode.

Since the ACPICA code acpi_acquire_global_lock() doesn't check if it
is in hardware reduced or not, it will go ahead and make the attempt.
And again, things will work, but we will not be in hardware reduced
mode by definition.

If the ACPICA code is compiled for hardware reduced only, then
acpi_acquire_global_lock() is properly stubbed out and we would
stay in compliance with the spec's definition of hardware reduced.

>> +	 */
>>   	tmp = 0;
>> -	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
>> +	if (!acpi_gbl_reduced_hardware)
>> +		acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
>>   	ec->global_lock = tmp;
>>   	ec->handle = handle;
>>   	return AE_CTRL_TERMINATE;
>>
>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------

[PATCH v6 6/6] ACPI: make sure ECs do not use the ACPI global lock

[Al Stone]

On 01/10/2014 04:33 PM, Rafael J. Wysocki wrote:
> On Friday, January 10, 2014 03:52:20 PM al.stone at linaro.org wrote:
>> From: Al Stone <al.stone@linaro.org>
>>
>> ACPI ECs (Embedded Controllers) are allowed to use the ACPI global
>> lock in legacy mode.  Since there is no global lock in hardware
>> reduced mode, make sure that ECs cannot inadvertently use it with
>> older ACPI tables that may have defined an _GLK method for the
>> EC device.  Since an individual lock can and should be defined for
>> each EC, access to each EC should still be properly controlled.
>>
>> Signed-off-by: Al Stone <al.stone@linaro.org>
>> ---
>>   drivers/acpi/ec.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
>> index ba5b56d..3f15110 100644
>> --- a/drivers/acpi/ec.c
>> +++ b/drivers/acpi/ec.c
>> @@ -745,9 +745,17 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
>>   	if (ACPI_FAILURE(status))
>>   		return status;
>>   	ec->gpe = tmp;
>> -	/* Use the global lock for all EC transactions? */
>> +	/*
>> +	 * Use the global lock for all EC transactions?
>> +	 *
>> +	 * If ACPI is in hardware reduced mode, there is no global lock.
>> +	 * If a _GLK method is defined, it needs to be ignored, just
>> +	 * in case it returns "true", which could happen if the ASL is
>> +	 * written incorrectly.
>
> What is going to happen if it is defined and works correctly in turn?

If for any reason the ec->global_lock field is non-zero, then every
time an EC transaction is being run (acpi_ec_transaction()), there will
be an attempt to acquire the ACPI global lock in violation of the
definition of hardware reduced mode -- the spec says that there is
no ACPI global lock in this mode.

Since the ACPICA code acpi_acquire_global_lock() doesn't check if it
is in hardware reduced or not, it will go ahead and make the attempt.
And again, things will work, but we will not be in hardware reduced
mode by definition.

If the ACPICA code is compiled for hardware reduced only, then
acpi_acquire_global_lock() is properly stubbed out and we would
stay in compliance with the spec's definition of hardware reduced.

>> +	 */
>>   	tmp = 0;
>> -	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
>> +	if (!acpi_gbl_reduced_hardware)
>> +		acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
>>   	ec->global_lock = tmp;
>>   	ec->handle = handle;
>>   	return AE_CTRL_TERMINATE;
>>
>


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone at linaro.org
-----------------------------------

Re: [PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[Rafael J. Wysocki]

On Monday, January 13, 2014 04:07:19 PM Al Stone wrote:
> On 01/10/2014 04:32 PM, Rafael J. Wysocki wrote:
> > On Friday, January 10, 2014 03:52:19 PM al.stone@linaro.org wrote:
> >> From: Al Stone <al.stone@linaro.org>
> >>
> >> Several of the FADT fields are normally kept in specific memory
> >> regions.  Since these fields are to be ignored in hardware reduced
> >> ACPI mode, do not map those addresses when in that mode, and of
> >> course do not release the mappings that have not been made.
> >>
> >> The function acpi_os_initialize() could become a stub in the header
> >> file but is left here in case it can be of further use.
> >
> > Why exactly is this change necessary?
> 
> Two reasons: (1) why do work we do not have to do?  and (2) it
> seemed to make sense to me to have the code reflect the spec
> accurately.
> 
> > Will things work incorrectly on HW-reduced ACPI systems if we don't make it?
> 
> If the ACPI tables have all of these fields properly set to zero
> in hardware reduced, this change does not need to be made.  If a
> vendor provides broken ACPI tables where these values are valid,
> but still sets hardware reduced in the FADT, these fields could
> then be used as before -- but allowing them to be used would mean
> we can no longer claim we are implementing hardware reduced correctly.
> So things would work, but the system would by definition be in some
> sort of undefined hybrid ACPI mode.

So this is how it goes.  I'm being told that there are systems in existence
where the HW-reduced bit is set for Windows RT compatibility, but otherwise
the ACPI HW is fully functional on them.  Apparently, people are able to
install and run Linux on those systems today.

Question is, are they still going to be able to run Linux on them after the
changes in this set?

Rafael

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[Rafael J. Wysocki]

On Monday, January 13, 2014 04:07:19 PM Al Stone wrote:
> On 01/10/2014 04:32 PM, Rafael J. Wysocki wrote:
> > On Friday, January 10, 2014 03:52:19 PM al.stone at linaro.org wrote:
> >> From: Al Stone <al.stone@linaro.org>
> >>
> >> Several of the FADT fields are normally kept in specific memory
> >> regions.  Since these fields are to be ignored in hardware reduced
> >> ACPI mode, do not map those addresses when in that mode, and of
> >> course do not release the mappings that have not been made.
> >>
> >> The function acpi_os_initialize() could become a stub in the header
> >> file but is left here in case it can be of further use.
> >
> > Why exactly is this change necessary?
> 
> Two reasons: (1) why do work we do not have to do?  and (2) it
> seemed to make sense to me to have the code reflect the spec
> accurately.
> 
> > Will things work incorrectly on HW-reduced ACPI systems if we don't make it?
> 
> If the ACPI tables have all of these fields properly set to zero
> in hardware reduced, this change does not need to be made.  If a
> vendor provides broken ACPI tables where these values are valid,
> but still sets hardware reduced in the FADT, these fields could
> then be used as before -- but allowing them to be used would mean
> we can no longer claim we are implementing hardware reduced correctly.
> So things would work, but the system would by definition be in some
> sort of undefined hybrid ACPI mode.

So this is how it goes.  I'm being told that there are systems in existence
where the HW-reduced bit is set for Windows RT compatibility, but otherwise
the ACPI HW is fully functional on them.  Apparently, people are able to
install and run Linux on those systems today.

Question is, are they still going to be able to run Linux on them after the
changes in this set?

Rafael

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 4/6] ACPI: in HW reduced mode, using FADT PM information is not allowed.

[Rafael J. Wysocki]

On Monday, January 13, 2014 03:46:58 PM Al Stone wrote:
> On 01/10/2014 04:31 PM, Rafael J. Wysocki wrote:
> > On Friday, January 10, 2014 03:52:18 PM al.stone@linaro.org wrote:
> >> From: Al Stone <al.stone@linaro.org>
> >>
> >> Per the ACPI 5.0 specification, section 5.2.9, none of the power
> >> management fields in the FADT are to be used.  Short-circuit using
> >> any of those fields in acpi_processor_get_power_info_fadt().
> >
> > So the spec says that we're supposed to ignore the information in
> > certain FADT fields if the HW_REDUCED_ACPI flag is set.
> >
> > However, what about systems in which that information is actually
> > valid even though HW_REDUCED_ACPI is set?
> >
> > Does it break things if we attempt to use that information?
> 
> The way I see it in the spec, we just don't use that info _if_ we wish
> to remain compliant with the spec.  If we insist on using it anyway, we
> can't say we're implementing hardware reduced mode correctly.  That
> being said, from my read of the code it appears things would operate
> the same as they did before if this part of the spec was ignored _and_
> the info in the tables was still valid (same control register addresses
> and content and so on).

Yes.

And we are at the point where we've already been violating the spec
(formally) by not ignoring that information for quite a while, so we risk
regressions if we suddenly try to become "exactly compliant".  That's
my main concern here.

> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> ---
> >>   drivers/acpi/processor_idle.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >> index eac984a..89f0a0f 100644
> >> --- a/drivers/acpi/processor_idle.c
> >> +++ b/drivers/acpi/processor_idle.c
> >> @@ -270,6 +270,14 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
> >>   	if (!pr->pblk)
> >>   		return -ENODEV;
> >>
> >> +	/*
> >> +	 * Using power management information from the FADT is not
> >> +	 * allowed when in HW reduced mode.  See ACPI 5.0, section
> >> +	 * 5.2.9.
> >> +	 */
> >> +	if (acpi_gbl_reduced_hardware)
> >> +		return -ENODEV;
> >> +
> >>   	/* if info is obtained from pblk/fadt, type equals state */
> >>   	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
> >>   	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
> >>
> >

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v6 4/6] ACPI: in HW reduced mode, using FADT PM information is not allowed.

[Rafael J. Wysocki]

On Monday, January 13, 2014 03:46:58 PM Al Stone wrote:
> On 01/10/2014 04:31 PM, Rafael J. Wysocki wrote:
> > On Friday, January 10, 2014 03:52:18 PM al.stone at linaro.org wrote:
> >> From: Al Stone <al.stone@linaro.org>
> >>
> >> Per the ACPI 5.0 specification, section 5.2.9, none of the power
> >> management fields in the FADT are to be used.  Short-circuit using
> >> any of those fields in acpi_processor_get_power_info_fadt().
> >
> > So the spec says that we're supposed to ignore the information in
> > certain FADT fields if the HW_REDUCED_ACPI flag is set.
> >
> > However, what about systems in which that information is actually
> > valid even though HW_REDUCED_ACPI is set?
> >
> > Does it break things if we attempt to use that information?
> 
> The way I see it in the spec, we just don't use that info _if_ we wish
> to remain compliant with the spec.  If we insist on using it anyway, we
> can't say we're implementing hardware reduced mode correctly.  That
> being said, from my read of the code it appears things would operate
> the same as they did before if this part of the spec was ignored _and_
> the info in the tables was still valid (same control register addresses
> and content and so on).

Yes.

And we are at the point where we've already been violating the spec
(formally) by not ignoring that information for quite a while, so we risk
regressions if we suddenly try to become "exactly compliant".  That's
my main concern here.

> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> ---
> >>   drivers/acpi/processor_idle.c | 8 ++++++++
> >>   1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> >> index eac984a..89f0a0f 100644
> >> --- a/drivers/acpi/processor_idle.c
> >> +++ b/drivers/acpi/processor_idle.c
> >> @@ -270,6 +270,14 @@ static int acpi_processor_get_power_info_fadt(struct acpi_processor *pr)
> >>   	if (!pr->pblk)
> >>   		return -ENODEV;
> >>
> >> +	/*
> >> +	 * Using power management information from the FADT is not
> >> +	 * allowed when in HW reduced mode.  See ACPI 5.0, section
> >> +	 * 5.2.9.
> >> +	 */
> >> +	if (acpi_gbl_reduced_hardware)
> >> +		return -ENODEV;
> >> +
> >>   	/* if info is obtained from pblk/fadt, type equals state */
> >>   	pr->power.states[ACPI_STATE_C2].type = ACPI_STATE_C2;
> >>   	pr->power.states[ACPI_STATE_C3].type = ACPI_STATE_C3;
> >>
> >

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 3/6] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field

[Rafael J. Wysocki]

On Monday, January 13, 2014 03:28:53 PM Al Stone wrote:
> On 01/10/2014 04:20 PM, Rafael J. Wysocki wrote:
> > On Friday, January 10, 2014 03:52:17 PM al.stone@linaro.org wrote:
> >> From: Al Stone <al.stone@linaro.org>
> >>
> >> In HW reduced mode, the use of the SCI interrupt is not allowed.  In
> >> all those places that use the FADT sci_interrupt field, make sure we
> >> do not execute that path when in HW reduced mode.
> >>
> >> In the case of acpi_os_install_interrupt_handler() in osl.c, this allows
> >> us to open up the routine to installing interrupt handlers other than
> >> acpi_gbl_FADT.sci_interrupt regardless of whether we are in ACPI legacy
> >> mode or reduced HW mode; acpi_os_remove_interrupt_handler() changes to
> >> maintain symmetry.
> >>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> ---
> >>   drivers/acpi/bus.c      | 30 ++++++++++++++++--------------
> >>   drivers/acpi/osl.c      | 18 +++++++-----------
> >>   drivers/acpi/pci_link.c |  2 ++
> >>   3 files changed, 25 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> >> index 0710004..d871859 100644
> >> --- a/drivers/acpi/bus.c
> >> +++ b/drivers/acpi/bus.c
> >> @@ -552,21 +552,23 @@ void __init acpi_early_init(void)
> >>   	}
> >>
> >>   #ifdef CONFIG_X86
> >> -	if (!acpi_ioapic) {
> >> -		/* compatible (0) means level (3) */
> >> -		if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> >> -			acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
> >> -			acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
> >
> > I wonder why exactly you want to make this change.  It surely doesn't matter
> > for ARM and do you have any HW-reduced x86 hardware to test it?
> 
> The point of this exercise was not to make hardware reduced work for
> just ARM; the point was to ensure hardware reduced is working correctly
> for any architecture using ACPI.
> 
> No, I do not have hardware reduced x86 hardware to test with.  Nor can
> I predict if or when Intel will make such a thing.  If you would rather
> wait until they do, so be it.  My intent was to avoid problems before
> they occur, rather than waiting until they actually break.
> 
> I chose to make this change because (a) it seemed silly to me to
> execute code you don't have to, (b) since the kernel code does end
> up acting as documentation for how ACPI is implemented and how it
> is compliant with the spec, it made sense to me to ensure all usage
> was compliant with the spec, and (c) if the machine has an IOAPIC
> and acpi_sci=n (n is non-zero) is supplied on the command line, the
> FADT SCI interrupt will end being initialized and used which is not
> allowed in hardware reduced mode.
> 
> >> +	if (!acpi_gbl_reduced_hardware) {
> >> +		if (!acpi_ioapic) {
> >> +			/* compatible (0) means level (3) */
> >> +			if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> >> +				acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
> >> +				acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
> >> +			}
> >> +			/* Set PIC-mode SCI trigger type */
> >> +			acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> >> +				(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> >> +		} else {
> >> +			/*
> >> +			 * now that acpi_gbl_FADT is initialized,
> >> +			 * update it with result from INT_SRC_OVR parsing
> >> +			 */
> >> +			acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
> >>   		}
> >> -		/* Set PIC-mode SCI trigger type */
> >> -		acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> >> -					 (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> >> -	} else {
> >> -		/*
> >> -		 * now that acpi_gbl_FADT is initialized,
> >> -		 * update it with result from INT_SRC_OVR parsing
> >> -		 */
> >> -		acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
> >>   	}
> >>   #endif
> >>
> >> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> >> index 44c07eb..c946a3a 100644
> >> --- a/drivers/acpi/osl.c
> >> +++ b/drivers/acpi/osl.c
> >> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
> >>
> >>   static acpi_osd_handler acpi_irq_handler;
> >>   static void *acpi_irq_context;
> >> +static u32 acpi_irq_number;
> >>   static struct workqueue_struct *kacpid_wq;
> >>   static struct workqueue_struct *kacpi_notify_wq;
> >>   static struct workqueue_struct *kacpi_hotplug_wq;
> >> @@ -178,6 +179,10 @@ static void __init acpi_request_region (struct acpi_generic_address *gas,
> >>
> >>   static int __init acpi_reserve_resources(void)
> >>   {
> >> +	/* Handle hardware reduced mode: i.e., do nothing. */
> >> +	if (acpi_gbl_reduced_hardware)
> >> +		return 0;
> >
> > Does it actually break things if we do that?
> 
> Not that I can see anywhere.  All of the code I have looked at appears
> to behave correctly if the ignored fields in the FADT are set to zero
> -- in this case, not using or checking bits in the xpm1a_event_blocks
> if the FADT field is zero.
> 
> >> +
> >>   	acpi_request_region(&acpi_gbl_FADT.xpm1a_event_block, acpi_gbl_FADT.pm1_event_length,
> >>   		"ACPI PM1a_EVT_BLK");
> >>
> >> @@ -795,13 +800,6 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
> >>
> >>   	acpi_irq_stats_init();
> >>
> >> -	/*
> >> -	 * ACPI interrupts different from the SCI in our copy of the FADT are
> >> -	 * not supported.
> >> -	 */
> >> -	if (gsi != acpi_gbl_FADT.sci_interrupt)
> >> -		return AE_BAD_PARAMETER;
> >> -
> >>   	if (acpi_irq_handler)
> >>   		return AE_ALREADY_ACQUIRED;
> >>
> >> @@ -818,15 +816,13 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
> >>   		acpi_irq_handler = NULL;
> >>   		return AE_NOT_ACQUIRED;
> >>   	}
> >> +	acpi_irq_number = irq;
> >>
> >>   	return AE_OK;
> >>   }
> >>
> >>   acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
> >>   {
> >> -	if (irq != acpi_gbl_FADT.sci_interrupt)
> >> -		return AE_BAD_PARAMETER;
> >> -
> >>   	free_irq(irq, acpi_irq);
> >>   	acpi_irq_handler = NULL;
> >>
> >> @@ -1806,7 +1802,7 @@ acpi_status __init acpi_os_initialize1(void)
> >>   acpi_status acpi_os_terminate(void)
> >>   {
> >>   	if (acpi_irq_handler) {
> >> -		acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
> >> +		acpi_os_remove_interrupt_handler(acpi_irq_number,
> >>   						 acpi_irq_handler);
> >>   	}
> >>
> >> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> >> index 2652a61..d5c155e 100644
> >> --- a/drivers/acpi/pci_link.c
> >> +++ b/drivers/acpi/pci_link.c
> >> @@ -505,6 +505,8 @@ int __init acpi_irq_penalty_init(void)
> >>   		}
> >>   	}
> >>   	/* Add a penalty for the SCI */
> >> +	if (acpi_gbl_reduced_hardware)
> >> +		return 0;
> >>   	acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
> >>   	return 0;
> >>   }
> >
> > Is ARM really going to use the code in pci_link.c?  If so, then how exactly?
> 
> I have no idea; I wasn't thinking strictly ARM in the first place.
> Just the same, why execute code if it is irrelevant?

Becase we don't know if it really is irrelevant.  Had I known for a fact that
it had been irrelevant, I wouldn't have had any concerns here.

> Further, if hardware reduced ACPI tables have the SCI interrupt field set to
> zero as they should, IRQ 0 gets a hefty penalty assessed; or, if the
> hardware reduced tables were made by taking a shortcut and just toggling
> the hardware reduced flag while leaving the SCI interrupt field alone,
> some other IRQ gets a penalty assessed, neither of which seemed to me
> to be correct either.

That is a good point.  I think it will be safe to avoid using the SCI if
the HW reduced bit is set and the SCI interrupt field is 0 at the same time.

So in my opinion it generally is better to avoid interpreting the spec
literally in this particular respect and to treat the ACPI HW features as
optional if the the HW reduced bit is set.  I think it is OK to check if they
are present (arguably, we should be doing that anyway, because there may be
broken BIOSes that don't set HW-reduced and still don't implement those features
although they should in that case), but changing the code to always ignore them
altogether may be going a bit too far.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v6 3/6] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field

[Rafael J. Wysocki]

On Monday, January 13, 2014 03:28:53 PM Al Stone wrote:
> On 01/10/2014 04:20 PM, Rafael J. Wysocki wrote:
> > On Friday, January 10, 2014 03:52:17 PM al.stone at linaro.org wrote:
> >> From: Al Stone <al.stone@linaro.org>
> >>
> >> In HW reduced mode, the use of the SCI interrupt is not allowed.  In
> >> all those places that use the FADT sci_interrupt field, make sure we
> >> do not execute that path when in HW reduced mode.
> >>
> >> In the case of acpi_os_install_interrupt_handler() in osl.c, this allows
> >> us to open up the routine to installing interrupt handlers other than
> >> acpi_gbl_FADT.sci_interrupt regardless of whether we are in ACPI legacy
> >> mode or reduced HW mode; acpi_os_remove_interrupt_handler() changes to
> >> maintain symmetry.
> >>
> >> Signed-off-by: Al Stone <al.stone@linaro.org>
> >> ---
> >>   drivers/acpi/bus.c      | 30 ++++++++++++++++--------------
> >>   drivers/acpi/osl.c      | 18 +++++++-----------
> >>   drivers/acpi/pci_link.c |  2 ++
> >>   3 files changed, 25 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> >> index 0710004..d871859 100644
> >> --- a/drivers/acpi/bus.c
> >> +++ b/drivers/acpi/bus.c
> >> @@ -552,21 +552,23 @@ void __init acpi_early_init(void)
> >>   	}
> >>
> >>   #ifdef CONFIG_X86
> >> -	if (!acpi_ioapic) {
> >> -		/* compatible (0) means level (3) */
> >> -		if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> >> -			acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
> >> -			acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
> >
> > I wonder why exactly you want to make this change.  It surely doesn't matter
> > for ARM and do you have any HW-reduced x86 hardware to test it?
> 
> The point of this exercise was not to make hardware reduced work for
> just ARM; the point was to ensure hardware reduced is working correctly
> for any architecture using ACPI.
> 
> No, I do not have hardware reduced x86 hardware to test with.  Nor can
> I predict if or when Intel will make such a thing.  If you would rather
> wait until they do, so be it.  My intent was to avoid problems before
> they occur, rather than waiting until they actually break.
> 
> I chose to make this change because (a) it seemed silly to me to
> execute code you don't have to, (b) since the kernel code does end
> up acting as documentation for how ACPI is implemented and how it
> is compliant with the spec, it made sense to me to ensure all usage
> was compliant with the spec, and (c) if the machine has an IOAPIC
> and acpi_sci=n (n is non-zero) is supplied on the command line, the
> FADT SCI interrupt will end being initialized and used which is not
> allowed in hardware reduced mode.
> 
> >> +	if (!acpi_gbl_reduced_hardware) {
> >> +		if (!acpi_ioapic) {
> >> +			/* compatible (0) means level (3) */
> >> +			if (!(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK)) {
> >> +				acpi_sci_flags &= ~ACPI_MADT_TRIGGER_MASK;
> >> +				acpi_sci_flags |= ACPI_MADT_TRIGGER_LEVEL;
> >> +			}
> >> +			/* Set PIC-mode SCI trigger type */
> >> +			acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> >> +				(acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> >> +		} else {
> >> +			/*
> >> +			 * now that acpi_gbl_FADT is initialized,
> >> +			 * update it with result from INT_SRC_OVR parsing
> >> +			 */
> >> +			acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
> >>   		}
> >> -		/* Set PIC-mode SCI trigger type */
> >> -		acpi_pic_sci_set_trigger(acpi_gbl_FADT.sci_interrupt,
> >> -					 (acpi_sci_flags & ACPI_MADT_TRIGGER_MASK) >> 2);
> >> -	} else {
> >> -		/*
> >> -		 * now that acpi_gbl_FADT is initialized,
> >> -		 * update it with result from INT_SRC_OVR parsing
> >> -		 */
> >> -		acpi_gbl_FADT.sci_interrupt = acpi_sci_override_gsi;
> >>   	}
> >>   #endif
> >>
> >> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> >> index 44c07eb..c946a3a 100644
> >> --- a/drivers/acpi/osl.c
> >> +++ b/drivers/acpi/osl.c
> >> @@ -84,6 +84,7 @@ static int (*__acpi_os_prepare_extended_sleep)(u8 sleep_state, u32 val_a,
> >>
> >>   static acpi_osd_handler acpi_irq_handler;
> >>   static void *acpi_irq_context;
> >> +static u32 acpi_irq_number;
> >>   static struct workqueue_struct *kacpid_wq;
> >>   static struct workqueue_struct *kacpi_notify_wq;
> >>   static struct workqueue_struct *kacpi_hotplug_wq;
> >> @@ -178,6 +179,10 @@ static void __init acpi_request_region (struct acpi_generic_address *gas,
> >>
> >>   static int __init acpi_reserve_resources(void)
> >>   {
> >> +	/* Handle hardware reduced mode: i.e., do nothing. */
> >> +	if (acpi_gbl_reduced_hardware)
> >> +		return 0;
> >
> > Does it actually break things if we do that?
> 
> Not that I can see anywhere.  All of the code I have looked at appears
> to behave correctly if the ignored fields in the FADT are set to zero
> -- in this case, not using or checking bits in the xpm1a_event_blocks
> if the FADT field is zero.
> 
> >> +
> >>   	acpi_request_region(&acpi_gbl_FADT.xpm1a_event_block, acpi_gbl_FADT.pm1_event_length,
> >>   		"ACPI PM1a_EVT_BLK");
> >>
> >> @@ -795,13 +800,6 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
> >>
> >>   	acpi_irq_stats_init();
> >>
> >> -	/*
> >> -	 * ACPI interrupts different from the SCI in our copy of the FADT are
> >> -	 * not supported.
> >> -	 */
> >> -	if (gsi != acpi_gbl_FADT.sci_interrupt)
> >> -		return AE_BAD_PARAMETER;
> >> -
> >>   	if (acpi_irq_handler)
> >>   		return AE_ALREADY_ACQUIRED;
> >>
> >> @@ -818,15 +816,13 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler,
> >>   		acpi_irq_handler = NULL;
> >>   		return AE_NOT_ACQUIRED;
> >>   	}
> >> +	acpi_irq_number = irq;
> >>
> >>   	return AE_OK;
> >>   }
> >>
> >>   acpi_status acpi_os_remove_interrupt_handler(u32 irq, acpi_osd_handler handler)
> >>   {
> >> -	if (irq != acpi_gbl_FADT.sci_interrupt)
> >> -		return AE_BAD_PARAMETER;
> >> -
> >>   	free_irq(irq, acpi_irq);
> >>   	acpi_irq_handler = NULL;
> >>
> >> @@ -1806,7 +1802,7 @@ acpi_status __init acpi_os_initialize1(void)
> >>   acpi_status acpi_os_terminate(void)
> >>   {
> >>   	if (acpi_irq_handler) {
> >> -		acpi_os_remove_interrupt_handler(acpi_gbl_FADT.sci_interrupt,
> >> +		acpi_os_remove_interrupt_handler(acpi_irq_number,
> >>   						 acpi_irq_handler);
> >>   	}
> >>
> >> diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
> >> index 2652a61..d5c155e 100644
> >> --- a/drivers/acpi/pci_link.c
> >> +++ b/drivers/acpi/pci_link.c
> >> @@ -505,6 +505,8 @@ int __init acpi_irq_penalty_init(void)
> >>   		}
> >>   	}
> >>   	/* Add a penalty for the SCI */
> >> +	if (acpi_gbl_reduced_hardware)
> >> +		return 0;
> >>   	acpi_irq_penalty[acpi_gbl_FADT.sci_interrupt] += PIRQ_PENALTY_PCI_USING;
> >>   	return 0;
> >>   }
> >
> > Is ARM really going to use the code in pci_link.c?  If so, then how exactly?
> 
> I have no idea; I wasn't thinking strictly ARM in the first place.
> Just the same, why execute code if it is irrelevant?

Becase we don't know if it really is irrelevant.  Had I known for a fact that
it had been irrelevant, I wouldn't have had any concerns here.

> Further, if hardware reduced ACPI tables have the SCI interrupt field set to
> zero as they should, IRQ 0 gets a hefty penalty assessed; or, if the
> hardware reduced tables were made by taking a shortcut and just toggling
> the hardware reduced flag while leaving the SCI interrupt field alone,
> some other IRQ gets a penalty assessed, neither of which seemed to me
> to be correct either.

That is a good point.  I think it will be safe to avoid using the SCI if
the HW reduced bit is set and the SCI interrupt field is 0 at the same time.

So in my opinion it generally is better to avoid interpreting the spec
literally in this particular respect and to treat the ACPI HW features as
optional if the the HW reduced bit is set.  I think it is OK to check if they
are present (arguably, we should be doing that anyway, because there may be
broken BIOSes that don't set HW-reduced and still don't implement those features
although they should in that case), but changing the code to always ignore them
altogether may be going a bit too far.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[Al Stone]

On 01/13/2014 05:02 PM, Rafael J. Wysocki wrote:
> On Monday, January 13, 2014 04:07:19 PM Al Stone wrote:
>> On 01/10/2014 04:32 PM, Rafael J. Wysocki wrote:
>>> On Friday, January 10, 2014 03:52:19 PM al.stone@linaro.org wrote:
>>>> From: Al Stone <al.stone@linaro.org>
>>>>
>>>> Several of the FADT fields are normally kept in specific memory
>>>> regions.  Since these fields are to be ignored in hardware reduced
>>>> ACPI mode, do not map those addresses when in that mode, and of
>>>> course do not release the mappings that have not been made.
>>>>
>>>> The function acpi_os_initialize() could become a stub in the header
>>>> file but is left here in case it can be of further use.
>>>
>>> Why exactly is this change necessary?
>>
>> Two reasons: (1) why do work we do not have to do?  and (2) it
>> seemed to make sense to me to have the code reflect the spec
>> accurately.
>>
>>> Will things work incorrectly on HW-reduced ACPI systems if we don't make it?
>>
>> If the ACPI tables have all of these fields properly set to zero
>> in hardware reduced, this change does not need to be made.  If a
>> vendor provides broken ACPI tables where these values are valid,
>> but still sets hardware reduced in the FADT, these fields could
>> then be used as before -- but allowing them to be used would mean
>> we can no longer claim we are implementing hardware reduced correctly.
>> So things would work, but the system would by definition be in some
>> sort of undefined hybrid ACPI mode.
>
> So this is how it goes.  I'm being told that there are systems in existence
> where the HW-reduced bit is set for Windows RT compatibility, but otherwise
> the ACPI HW is fully functional on them.  Apparently, people are able to
> install and run Linux on those systems today.
>
> Question is, are they still going to be able to run Linux on them after the
> changes in this set?

Hrm.  This would have been incredibly useful to know earlier.  I
might have taken a completely different approach.  Or perhaps not
even have bothered.

I'm not naive enough to think all vendors will fully or rigorously
comply with standards.  Down that path madness lies.  But at face
value, it sounds like they didn't even try.

Without access to the ACPI tables and the hardware itself, there is
no way to know if Linux will run; I have yet to see any such system.
The phrase "...the ACPI HW is fully functional..." could mean way too
many things -- it could mean anything from strict compliance with
hardware reduced mode to completely compliant with legacy mode but all
we did was toggle the hardware reduced flag so we could use GPIOs 
instead of an SCI.

As far as I can tell, that makes the question undecidable.  I can't
prove a negative -- I can't prove these patches won't break an unknown
set of systems that have implemented an unknown hybrid of legacy and
hardware reduced modes.

If someone can tell me that these mongrel ACPI systems continue
to run correctly when they run a Linux built with the
ACPI_REDUCED_HARDWARE flag set in the ACPICA code, I might at least
have a clue as to where the boundaries of compliance are.

Or, if such hardware is commercially available, where does one get it?

Otherwise, the only safe patch is 1/6, the Kconfig changes.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone@linaro.org
-----------------------------------

[PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[Al Stone]

On 01/13/2014 05:02 PM, Rafael J. Wysocki wrote:
> On Monday, January 13, 2014 04:07:19 PM Al Stone wrote:
>> On 01/10/2014 04:32 PM, Rafael J. Wysocki wrote:
>>> On Friday, January 10, 2014 03:52:19 PM al.stone at linaro.org wrote:
>>>> From: Al Stone <al.stone@linaro.org>
>>>>
>>>> Several of the FADT fields are normally kept in specific memory
>>>> regions.  Since these fields are to be ignored in hardware reduced
>>>> ACPI mode, do not map those addresses when in that mode, and of
>>>> course do not release the mappings that have not been made.
>>>>
>>>> The function acpi_os_initialize() could become a stub in the header
>>>> file but is left here in case it can be of further use.
>>>
>>> Why exactly is this change necessary?
>>
>> Two reasons: (1) why do work we do not have to do?  and (2) it
>> seemed to make sense to me to have the code reflect the spec
>> accurately.
>>
>>> Will things work incorrectly on HW-reduced ACPI systems if we don't make it?
>>
>> If the ACPI tables have all of these fields properly set to zero
>> in hardware reduced, this change does not need to be made.  If a
>> vendor provides broken ACPI tables where these values are valid,
>> but still sets hardware reduced in the FADT, these fields could
>> then be used as before -- but allowing them to be used would mean
>> we can no longer claim we are implementing hardware reduced correctly.
>> So things would work, but the system would by definition be in some
>> sort of undefined hybrid ACPI mode.
>
> So this is how it goes.  I'm being told that there are systems in existence
> where the HW-reduced bit is set for Windows RT compatibility, but otherwise
> the ACPI HW is fully functional on them.  Apparently, people are able to
> install and run Linux on those systems today.
>
> Question is, are they still going to be able to run Linux on them after the
> changes in this set?

Hrm.  This would have been incredibly useful to know earlier.  I
might have taken a completely different approach.  Or perhaps not
even have bothered.

I'm not naive enough to think all vendors will fully or rigorously
comply with standards.  Down that path madness lies.  But at face
value, it sounds like they didn't even try.

Without access to the ACPI tables and the hardware itself, there is
no way to know if Linux will run; I have yet to see any such system.
The phrase "...the ACPI HW is fully functional..." could mean way too
many things -- it could mean anything from strict compliance with
hardware reduced mode to completely compliant with legacy mode but all
we did was toggle the hardware reduced flag so we could use GPIOs 
instead of an SCI.

As far as I can tell, that makes the question undecidable.  I can't
prove a negative -- I can't prove these patches won't break an unknown
set of systems that have implemented an unknown hybrid of legacy and
hardware reduced modes.

If someone can tell me that these mongrel ACPI systems continue
to run correctly when they run a Linux built with the
ACPI_REDUCED_HARDWARE flag set in the ACPICA code, I might at least
have a clue as to where the boundaries of compliance are.

Or, if such hardware is commercially available, where does one get it?

Otherwise, the only safe patch is 1/6, the Kconfig changes.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Linaro Enterprise Group
al.stone at linaro.org
-----------------------------------

Re: [PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[Al Stone]

On 01/13/2014 05:59 PM, Al Stone wrote:
> On 01/13/2014 05:02 PM, Rafael J. Wysocki wrote:
>> On Monday, January 13, 2014 04:07:19 PM Al Stone wrote:
>>> On 01/10/2014 04:32 PM, Rafael J. Wysocki wrote:
>>>> On Friday, January 10, 2014 03:52:19 PM al.stone@linaro.org wrote:
>>>>> From: Al Stone <al.stone@linaro.org>
>>>>>
>>>>> Several of the FADT fields are normally kept in specific memory
>>>>> regions.  Since these fields are to be ignored in hardware reduced
>>>>> ACPI mode, do not map those addresses when in that mode, and of
>>>>> course do not release the mappings that have not been made.
>>>>>
>>>>> The function acpi_os_initialize() could become a stub in the header
>>>>> file but is left here in case it can be of further use.
>>>>
>>>> Why exactly is this change necessary?
>>>
>>> Two reasons: (1) why do work we do not have to do?  and (2) it
>>> seemed to make sense to me to have the code reflect the spec
>>> accurately.
>>>
>>>> Will things work incorrectly on HW-reduced ACPI systems if we don't
>>>> make it?
>>>
>>> If the ACPI tables have all of these fields properly set to zero
>>> in hardware reduced, this change does not need to be made.  If a
>>> vendor provides broken ACPI tables where these values are valid,
>>> but still sets hardware reduced in the FADT, these fields could
>>> then be used as before -- but allowing them to be used would mean
>>> we can no longer claim we are implementing hardware reduced correctly.
>>> So things would work, but the system would by definition be in some
>>> sort of undefined hybrid ACPI mode.
>>
>> So this is how it goes.  I'm being told that there are systems in
>> existence
>> where the HW-reduced bit is set for Windows RT compatibility, but
>> otherwise
>> the ACPI HW is fully functional on them.  Apparently, people are able to
>> install and run Linux on those systems today.
>>
>> Question is, are they still going to be able to run Linux on them
>> after the
>> changes in this set?
>
> Hrm.  This would have been incredibly useful to know earlier.  I
> might have taken a completely different approach.  Or perhaps not
> even have bothered.
>
> I'm not naive enough to think all vendors will fully or rigorously
> comply with standards.  Down that path madness lies.  But at face
> value, it sounds like they didn't even try.
>
> Without access to the ACPI tables and the hardware itself, there is
> no way to know if Linux will run; I have yet to see any such system.
> The phrase "...the ACPI HW is fully functional..." could mean way too
> many things -- it could mean anything from strict compliance with
> hardware reduced mode to completely compliant with legacy mode but all
> we did was toggle the hardware reduced flag so we could use GPIOs
> instead of an SCI.
>
> As far as I can tell, that makes the question undecidable.  I can't
> prove a negative -- I can't prove these patches won't break an unknown
> set of systems that have implemented an unknown hybrid of legacy and
> hardware reduced modes.
>
> If someone can tell me that these mongrel ACPI systems continue
> to run correctly when they run a Linux built with the
> ACPI_REDUCED_HARDWARE flag set in the ACPICA code, I might at least
> have a clue as to where the boundaries of compliance are.
>
> Or, if such hardware is commercially available, where does one get it?
>
> Otherwise, the only safe patch is 1/6, the Kconfig changes.
>

Thinking about this over night, I'll re-submit just the Kconfig changes
for now, and rethink the approach.

In the meantime... seriously, what devices are these with the weird
ACPI tables and hardware?  How do I get one?  Or access to one?  A part
number or a vendor or something is kind of essential here; otherwise, I
can only surmise they are being done behind closed doors somewhere.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

[PATCH v6 5/6] ACPI: do not map/unmap memory regions for FADT entries in reduced HW mode

[Al Stone]

On 01/13/2014 05:59 PM, Al Stone wrote:
> On 01/13/2014 05:02 PM, Rafael J. Wysocki wrote:
>> On Monday, January 13, 2014 04:07:19 PM Al Stone wrote:
>>> On 01/10/2014 04:32 PM, Rafael J. Wysocki wrote:
>>>> On Friday, January 10, 2014 03:52:19 PM al.stone at linaro.org wrote:
>>>>> From: Al Stone <al.stone@linaro.org>
>>>>>
>>>>> Several of the FADT fields are normally kept in specific memory
>>>>> regions.  Since these fields are to be ignored in hardware reduced
>>>>> ACPI mode, do not map those addresses when in that mode, and of
>>>>> course do not release the mappings that have not been made.
>>>>>
>>>>> The function acpi_os_initialize() could become a stub in the header
>>>>> file but is left here in case it can be of further use.
>>>>
>>>> Why exactly is this change necessary?
>>>
>>> Two reasons: (1) why do work we do not have to do?  and (2) it
>>> seemed to make sense to me to have the code reflect the spec
>>> accurately.
>>>
>>>> Will things work incorrectly on HW-reduced ACPI systems if we don't
>>>> make it?
>>>
>>> If the ACPI tables have all of these fields properly set to zero
>>> in hardware reduced, this change does not need to be made.  If a
>>> vendor provides broken ACPI tables where these values are valid,
>>> but still sets hardware reduced in the FADT, these fields could
>>> then be used as before -- but allowing them to be used would mean
>>> we can no longer claim we are implementing hardware reduced correctly.
>>> So things would work, but the system would by definition be in some
>>> sort of undefined hybrid ACPI mode.
>>
>> So this is how it goes.  I'm being told that there are systems in
>> existence
>> where the HW-reduced bit is set for Windows RT compatibility, but
>> otherwise
>> the ACPI HW is fully functional on them.  Apparently, people are able to
>> install and run Linux on those systems today.
>>
>> Question is, are they still going to be able to run Linux on them
>> after the
>> changes in this set?
>
> Hrm.  This would have been incredibly useful to know earlier.  I
> might have taken a completely different approach.  Or perhaps not
> even have bothered.
>
> I'm not naive enough to think all vendors will fully or rigorously
> comply with standards.  Down that path madness lies.  But at face
> value, it sounds like they didn't even try.
>
> Without access to the ACPI tables and the hardware itself, there is
> no way to know if Linux will run; I have yet to see any such system.
> The phrase "...the ACPI HW is fully functional..." could mean way too
> many things -- it could mean anything from strict compliance with
> hardware reduced mode to completely compliant with legacy mode but all
> we did was toggle the hardware reduced flag so we could use GPIOs
> instead of an SCI.
>
> As far as I can tell, that makes the question undecidable.  I can't
> prove a negative -- I can't prove these patches won't break an unknown
> set of systems that have implemented an unknown hybrid of legacy and
> hardware reduced modes.
>
> If someone can tell me that these mongrel ACPI systems continue
> to run correctly when they run a Linux built with the
> ACPI_REDUCED_HARDWARE flag set in the ACPICA code, I might at least
> have a clue as to where the boundaries of compliance are.
>
> Or, if such hardware is commercially available, where does one get it?
>
> Otherwise, the only safe patch is 1/6, the Kconfig changes.
>

Thinking about this over night, I'll re-submit just the Kconfig changes
for now, and rethink the approach.

In the meantime... seriously, what devices are these with the weird
ACPI tables and hardware?  How do I get one?  Or access to one?  A part
number or a vendor or something is kind of essential here; otherwise, I
can only surmise they are being done behind closed doors somewhere.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3 at redhat.com
-----------------------------------