[v4,3/5] ACPI: HW reduced mode does not allow use of the FADT sci_interrupt field

Commit Message

[e-mail] Dec. 17, 2013, 12:16 a.m. UTC

From: Al Stone <[e-mail]>

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 <[e-mail]>
---
 drivers/acpi/bus.c      | 30 ++++++++++++++++--------------
 drivers/acpi/osl.c      | 18 +++++++-----------
 drivers/acpi/pci_link.c |  2 ++
 3 files changed, 25 insertions(+), 25 deletions(-)

Comments

Matthew Garrett Dec. 27, 2013, 4:56 a.m. UTC | #1

On Mon, Dec 16, 2013 at 05:16:39PM -0700, [e-mail] wrote:

> +	if (!acpi_gbl_reduced_hardware) {

I don't really understand this. You've explained that runtime hardware 
reduced mode switching isn't expected to work, but you're making this 
conditional on a runtime flag?

[e-mail] Jan. 6, 2014, 11:14 p.m. UTC | #2

On 12/26/2013 09:56 PM, Matthew Garrett wrote:
> On Mon, Dec 16, 2013 at 05:16:39PM -0700, [e-mail] wrote:
>
>> +	if (!acpi_gbl_reduced_hardware) {
>
> I don't really understand this. You've explained that runtime hardware
> reduced mode switching isn't expected to work, but you're making this
> conditional on a runtime flag?
>

Correct.  My apologies if I've made this unclear.

Suppose I build a kernel with full ACPI support.  The kernel would
then include legacy and hardware-reduced support.  If I pass that
kernel ACPI tables specifying legacy mode, things work fine.  If I
pass that same kernel ACPI tables with the hardware-reduced flag
set in the FADT, that should also work -- iff the ACPI driver checks
the acpi_gbl_reduced_hardware flag at run-time.  This patch is adding
some of those checks that were missing so that at least the files in
drivers/acpi/*.[ch] are correct; I contend that there are still places
in the drivers/acpi/acpica/*.[ch] files where a Linux driver can call
code it should not be allowed to when in hardware-reduced (e.g., when
using the ACPI global lock [0]).  Patching the ACPICA code I'm viewing
as a separate issue yet to be done.

If I build a kernel with only hardware-reduced ACPI support, using
ACPI tables without the hardware-reduced flag set in the FADT will
clearly not work -- nor do I expect it to in this case since most of
the legacy code gets removed at compile time.  If the ACPI tables do
have hardware-reduced mode set in the FADT, I expect the kernel to
behave correctly.  It may turn out to be exactly the same patch for
ACPICA as mentioned above, but I contend that ACPICA does not
completely handle this case properly at runtime, either.

Does that make sense?



[0] No driver should be doing this but it cannot currently be precluded
     by the kernel.  You can call me paranoid but I think if it can be
     abused by a driver, it will be.

Patch

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index bba9b72..33ff87f 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -542,21 +542,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;
 }