ACPI: early _PDC eval and unify x86/ia64

PATCH Superseded Alex Chiang 4 participants 19 messages Dec 20, 2009 – Jan 16, 2010

Subsystems (3)

x86 ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

Alex Chiang

author

2009-12-20 19:19 UTC #0 lore raw

cover ACPI: early _PDC eval and unify x86/ia64

It was recently discovered that some BIOS implementations load dynamic SSDTs when a processor's _PDC method is evaluated.

In Linux, we do not evaluate _PDC until after we initialize the EC.

Unfortunately, that same BIOS references objects in the dynamic table during EC enablement. So we get undefined references to methods during enablement, and we fail to initialize certain pieces of hardware.

Windows works just fine on this machine, so to me, this is a hint that Linux should be evaluating _PDC much earlier.

The first patch in this series does just that and makes Linux more compatible with Windows' ACPI implementation.

The remaining 10 patches unify the x86/ia64 implementations of calling _PDC. I noticed the two archs were way more similar than they were different, so I thought it made sense to combine them as much as possible.

This patch series was built on both x86 and ia64 (checkpatch and sparse clean).

It was also boot tested on an HP Envy 15 (x86) and an HP rx6600 (ia64). It resolved the namespace failures on the Envy 15 and did not cause any regressions on the rx6600.

This does introduce a boot time namespace walk for all the CPUs in the system, looking for and evaluating _PDC. Hopefully that will not make life miserable for the giant SGI clusters. If worse comes to worse, maybe we can quirk them and avoid the namespace walk.

Thanks,
/ac

---

Alex Chiang (11):
ACPI: processor: call _PDC early
ACPI: processor: introduce arch_has_acpi_pdc
ACPI: processor: unify arch_acpi_processor_init_pdc
ACPI: processor: factor out common _PDC settings
ACPI: processor: finish unifying arch_acpi_processor_init_pdc()
ACPI: processor: unify arch_acpi_processor_cleanup_pdc
ACPI: processor: introduce acpi_processor_alloc_pdc()
ACPI: processor: change acpi_processor_eval_pdc interface
ACPI: processor: open code acpi_processor_cleanup_pdc
ACPI: processor: change acpi_processor_set_pdc() interface
ACPI: processor: remove _PDC object list from struct acpi_processor

 arch/ia64/include/asm/acpi.h      |    6 +
 arch/ia64/kernel/Makefile         |    4 -
 arch/ia64/kernel/acpi-processor.c |   85 -------------------
 arch/x86/include/asm/acpi.h       |   26 ++++++
 arch/x86/kernel/acpi/Makefile     |    2 
 arch/x86/kernel/acpi/processor.c  |  101 -----------------------
 drivers/acpi/Makefile             |    1 
 drivers/acpi/bus.c                |    2 
 drivers/acpi/internal.h           |    1 
 drivers/acpi/processor_core.c     |   71 ----------------
 drivers/acpi/processor_pdc.c      |  165 +++++++++++++++++++++++++++++++++++++
 include/acpi/processor.h          |    8 +-
 12 files changed, 206 insertions(+), 266 deletions(-)
 delete mode 100644 arch/ia64/kernel/acpi-processor.c
 delete mode 100644 arch/x86/kernel/acpi/processor.c
 create mode 100644 drivers/acpi/processor_pdc.c

Alex Chiang

author

2009-12-20 19:19 UTC #1 lore raw

PATCH 1/11 ACPI: processor: call _PDC early

We discovered that at least one machine (HP Envy), methods in the DSDT attempt to call external methods defined in a dynamically loaded SSDT.

Unfortunately, the DSDT methods we are trying to call are part of the EC initialization, which happens very early, and the the dynamic SSDT is only loaded when a processor _PDC method runs much later.

This results in namespace lookup errors for the (as of yet) undefined methods.

Since Windows doesn't have any issues with this machine, we take it as a hint that they must be evaluating _PDC much earlier than we are.

Thus, the proper thing for Linux to do should be to match the Windows implementation more closely.

Provide a mechanism to call _PDC before we enable the EC. Doing so loads the dynamic tables, and allows the EC to be enabled correctly.

The ACPI processor driver will still evaluate _PDC in its .add() method to cover the hotplug case.

Resolves: http://bugzilla.kernel.org/show_bug.cgi?id=14824

Cc [email protected]

Signed-off-by Alex Chiang <[email protected]>

---

 drivers/acpi/Makefile         |    1 
 drivers/acpi/bus.c            |    2 +
 drivers/acpi/internal.h       |    1 
 drivers/acpi/processor_core.c |   69 --------------------------
 drivers/acpi/processor_pdc.c  |  108 +++++++++++++++++++++++++++++++++++++++++
 include/acpi/processor.h      |    3 +
 6 files changed, 115 insertions(+), 69 deletions(-)
 create mode 100644 drivers/acpi/processor_pdc.c

drivers/acpi/Makefile +1 -0

index c7b10b4..66cc3f3 100644
@@ -32,6 +32,7 @@ acpi-$(CONFIG_ACPI_SLEEP)	+= proc.o
 #
 acpi-y				+= bus.o glue.o
 acpi-y				+= scan.o
+acpi-y				+= processor_pdc.o
 acpi-y				+= ec.o
 acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
 acpi-y				+= pci_root.o pci_link.o pci_irq.o pci_bind.o

drivers/acpi/bus.c +2 -0

index 65f7e33..0bdf24a 100644
@@ -888,6 +888,8 @@ static int __init acpi_bus_init(void)
 		goto error1;
 	}
 
+	acpi_early_processor_set_pdc();
+
 	/*
 	 * Maybe EC region is required at bus_scan/acpi_get_devices. So it
 	 * is necessary to enable it as early as possible.

drivers/acpi/internal.h +1 -0

index 074cf86..cb28e05 100644
@@ -43,6 +43,7 @@ int acpi_power_transition(struct acpi_device *device, int state);
 extern int acpi_power_nocheck;
 
 int acpi_wakeup_device_init(void);
+void acpi_early_processor_set_pdc(void);
 
 /* --------------------------------------------------------------------------
                                   Embedded Controller

drivers/acpi/processor_core.c +0 ~~-69~~

94 lines not on this page · raw message

drivers/acpi/processor_pdc.c new file +108 -0

108 lines not on this page · raw message

include/acpi/processor.h +3 -0

index 29245c6..a1b748a 100644
@@ -325,6 +325,9 @@ static inline int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
 
 #endif				/* CONFIG_CPU_FREQ */
 
+/* in processor_pdc.c */
+void acpi_processor_set_pdc(struct acpi_processor *pr);
+
 /* in processor_throttling.c */
 int acpi_processor_tstate_has_changed(struct acpi_processor *pr);
 int acpi_processor_get_throttling_info(struct acpi_processor *pr);

Luck, Tony

2009-12-28 18:39 UTC #2 lore raw

PATCH 1/1 Fix section mismatch error for acpi_early_processor_set_pdc()

Alex Chiang introduced acpi_early_processor_set_pdc() in commit:
ACPI: processor: call _PDC early 78f1699659963fff97975df44db6d5dbe7218e55

But this results in a section mismatch:

WARNING: drivers/acpi/acpi.o(.text+0xa9c1): Section mismatch in reference from the function acpi_early_processor_set_pdc() to the variable .cpuinit.data:processor_idle_dmi_table The function acpi_early_processor_set_pdc() references
the variable __cpuinitdata processor_idle_dmi_table.
This is often because acpi_early_processor_set_pdc lacks a __cpuinitdata annotation or the annotation of processor_idle_dmi_table is wrong.

The only caller of acpi_early_processor_set_pdc() is acpi_bus_init() which is an "__init" function. So the correct fix here is to mark acpi_early_processor_set_pdc() "__init" too.

Signed-off-by Tony Luck <[email protected]>

---

drivers/acpi/processor_pdc.c +1 -1

index 30e4dc0..7d4ee39 100644
@@ -151,7 +151,7 @@ early_init_pdc(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK;
 }
 
-void acpi_early_processor_set_pdc(void)
+void __init acpi_early_processor_set_pdc(void)
 {
 	/*
 	 * Check whether the system is DMI table. If yes, OSPM

Alex Chiang

author Acked-by on v1 1/1

2010-01-06 22:52 UTC #3 lore raw

Re: [PATCH] Fix section mismatch error for acpi_early_processor_set_pdc()

* Luck, Tony <[email protected]>:

Quoted 13 lines

Alex Chiang introduced acpi_early_processor_set_pdc() in commit:
ACPI: processor: call _PDC early 78f1699659963fff97975df44db6d5dbe7218e55
But this results in a section mismatch:
WARNING: drivers/acpi/acpi.o(.text+0xa9c1): Section mismatch in reference from the function acpi_early_processor_set_pdc() to the variable .cpuinit.data:processor_idle_dmi_table The function acpi_early_processor_set_pdc() references
the variable __cpuinitdata processor_idle_dmi_table.
This is often because acpi_early_processor_set_pdc lacks a __cpuinitdata annotation or the annotation of processor_idle_dmi_table is wrong.

Quoted 5 lines

The only caller of acpi_early_processor_set_pdc() is acpi_bus_init() which is an "__init" function. So the correct fix here is to mark acpi_early_processor_set_pdc() "__init" too.
Signed-off-by Tony Luck <[email protected]>

Acked-by Alex Chiang <[email protected]>

Sorry for not catching this. Not sure why I didn't notice it myself.

/ac

Quoted 17 lines

---

drivers/acpi/processor_pdc.c +1 -1

index 30e4dc0..7d4ee39 100644
@@ -151,7 +151,7 @@ early_init_pdc(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK;
 }
 
-void acpi_early_processor_set_pdc(void)
+void __init acpi_early_processor_set_pdc(void)
 {
 	/*
 	 * Check whether the system is DMI table. If yes, OSPM

Len Brown

reviewer Applied

2010-01-16 07:01 UTC #4 lore raw

Re: [PATCH] Fix section mismatch error for acpi_early_processor_set_pdc()

applied

thanks,
Len Brown, Intel Open Source Technology Center

Alex Chiang

author

2009-12-20 19:19 UTC #5 lore raw

PATCH 2/11 ACPI: processor: introduce arch_has_acpi_pdc

In reply to Alex Chiang

arch dependent helper function that tells us if we should attempt to evaluate _PDC on this machine or not.

The x86 implementation assumes that the CPUs in the machine must be homogeneous, and that you cannot mix CPUs of different vendors.

Cc Tony Luck <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

---

 arch/ia64/include/asm/acpi.h     |    2 ++
 arch/x86/include/asm/acpi.h      |    7 +++++++
 arch/x86/kernel/acpi/processor.c |    4 +---
 drivers/acpi/processor_pdc.c     |    3 +++
 4 files changed, 13 insertions(+), 3 deletions(-)

arch/ia64/include/asm/acpi.h +2 -0

index 91df968..3b78882 100644
@@ -132,6 +132,8 @@ extern int __devinitdata pxm_to_nid_map[MAX_PXM_DOMAINS];
 extern int __initdata nid_to_pxm_map[MAX_NUMNODES];
 #endif
 
+static inline bool arch_has_acpi_pdc(void) { return true; }
+
 #define acpi_unlazy_tlb(x)
 
 #ifdef CONFIG_ACPI_NUMA

arch/x86/include/asm/acpi.h +7 -0

index 60d2b2d..d787e6e 100644
@@ -142,6 +142,13 @@ static inline unsigned int acpi_processor_cstate_check(unsigned int max_cstate)
 		return max_cstate;
 }
 
+static inline bool arch_has_acpi_pdc(void)
+{
+	struct cpuinfo_x86 *c = &cpu_data(0);
+	return (c->x86_vendor = X86_VENDOR_INTEL ||
+		c->x86_vendor = X86_VENDOR_CENTAUR);
+}
+
 #else /* !CONFIG_ACPI */
 
 #define acpi_lapic 0

arch/x86/kernel/acpi/processor.c +1 -3

index d85d1b2..bcb6efe 100644
@@ -79,9 +79,7 @@ void arch_acpi_processor_init_pdc(struct acpi_processor *pr)
 	struct cpuinfo_x86 *c = &cpu_data(pr->id);
 
 	pr->pdc = NULL;
-	if (c->x86_vendor = X86_VENDOR_INTEL ||
-	    c->x86_vendor = X86_VENDOR_CENTAUR)
-		init_intel_pdc(pr, c);
+	init_intel_pdc(pr, c);
 
 	return;
 }

drivers/acpi/processor_pdc.c +3 -0

index b416c32..931e735 100644
@@ -69,6 +69,9 @@ static int acpi_processor_eval_pdc(struct acpi_processor *pr)
 
 void acpi_processor_set_pdc(struct acpi_processor *pr)
 {
+	if (arch_has_acpi_pdc() = false)
+		return;
+
 	arch_acpi_processor_init_pdc(pr);
 	acpi_processor_eval_pdc(pr);
 	arch_acpi_processor_cleanup_pdc(pr);

Alex Chiang

author

2009-12-20 19:19 UTC #6 lore raw

PATCH 3/11 ACPI: processor: unify arch_acpi_processor_init_pdc

In reply to Alex Chiang

The x86 and ia64 implementations of arch_acpi_processor_init_pdc() are almost exactly the same. The only difference is in what bits they set in obj_list buffer.

Combine the boilerplate memory management code, and leave the arch-specific bit twiddling in separate implementations.

Cc Tony Luck <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

---

 arch/ia64/kernel/acpi-processor.c |   34 +---------------------------
 arch/x86/kernel/acpi/processor.c  |   34 +---------------------------
 drivers/acpi/processor_pdc.c      |   45 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 46 insertions(+), 67 deletions(-)

arch/ia64/kernel/acpi-processor.c +1 ~~-33~~

index dbda7bd..ab72d46 100644
@@ -16,31 +16,7 @@
 
 static void init_intel_pdc(struct acpi_processor *pr)
 {
-	struct acpi_object_list *obj_list;
-	union acpi_object *obj;
-	u32 *buf;
-
-	/* allocate and initialize pdc. It will be used later. */
-	obj_list = kmalloc(sizeof(struct acpi_object_list), GFP_KERNEL);
-	if (!obj_list) {
-		printk(KERN_ERR "Memory allocation error\n");
-		return;
-	}
-
-	obj = kmalloc(sizeof(union acpi_object), GFP_KERNEL);
-	if (!obj) {
-		printk(KERN_ERR "Memory allocation error\n");
-		kfree(obj_list);
-		return;
-	}
-
-	buf = kmalloc(12, GFP_KERNEL);
-	if (!buf) {
-		printk(KERN_ERR "Memory allocation error\n");
-		kfree(obj);
-		kfree(obj_list);
-		return;
-	}
+	u32 *buf = (u32 *)pr->pdc->pointer->buffer.pointer;
 
 	buf[0] = ACPI_PDC_REVISION_ID;
 	buf[1] = 1;

@@ -52,20 +28,12 @@ static void init_intel_pdc(struct acpi_processor *pr)
 	 */
 	buf[2] |= ACPI_PDC_SMP_T_SWCOORD;
 
-	obj->type = ACPI_TYPE_BUFFER;
-	obj->buffer.length = 12;
-	obj->buffer.pointer = (u8 *) buf;
-	obj_list->count = 1;
-	obj_list->pointer = obj;
-	pr->pdc = obj_list;
-
 	return;
 }
 
 /* Initialize _PDC data based on the CPU vendor */
 void arch_acpi_processor_init_pdc(struct acpi_processor *pr)
 {
-	pr->pdc = NULL;
 	init_intel_pdc(pr);
 	return;
 }

arch/x86/kernel/acpi/processor.c +1 ~~-33~~

index bcb6efe..967860b 100644
@@ -14,31 +14,7 @@
 
 static void init_intel_pdc(struct acpi_processor *pr, struct cpuinfo_x86 *c)
 {
-	struct acpi_object_list *obj_list;
-	union acpi_object *obj;
-	u32 *buf;
-
-	/* allocate and initialize pdc. It will be used later. */
-	obj_list = kmalloc(sizeof(struct acpi_object_list), GFP_KERNEL);
-	if (!obj_list) {
-		printk(KERN_ERR "Memory allocation error\n");
-		return;
-	}
-
-	obj = kmalloc(sizeof(union acpi_object), GFP_KERNEL);
-	if (!obj) {
-		printk(KERN_ERR "Memory allocation error\n");
-		kfree(obj_list);
-		return;
-	}
-
-	buf = kmalloc(12, GFP_KERNEL);
-	if (!buf) {
-		printk(KERN_ERR "Memory allocation error\n");
-		kfree(obj);
-		kfree(obj_list);
-		return;
-	}
+	u32 *buf = (u32 *)pr->pdc->pointer->buffer.pointer;
 
 	buf[0] = ACPI_PDC_REVISION_ID;
 	buf[1] = 1;

@@ -62,13 +38,6 @@ static void init_intel_pdc(struct acpi_processor *pr, struct cpuinfo_x86 *c)
 	if (!cpu_has(c, X86_FEATURE_MWAIT))
 		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
 
-	obj->type = ACPI_TYPE_BUFFER;
-	obj->buffer.length = 12;
-	obj->buffer.pointer = (u8 *) buf;
-	obj_list->count = 1;
-	obj_list->pointer = obj;
-	pr->pdc = obj_list;
-
 	return;
 }

@@ -78,7 +47,6 @@ void arch_acpi_processor_init_pdc(struct acpi_processor *pr)
 {
 	struct cpuinfo_x86 *c = &cpu_data(pr->id);
 
-	pr->pdc = NULL;
 	init_intel_pdc(pr, c);
 
 	return;

drivers/acpi/processor_pdc.c +44 -1

57 lines not on this page · raw message

Alex Chiang

author

2009-12-20 19:19 UTC #7 lore raw

PATCH 4/11 ACPI: processor: factor out common _PDC settings

In reply to Alex Chiang

Both x86 and ia64 initialize _PDC with mostly common bit settings.

Factor out the common settings and leave the arch-specific ones alone.

Cc Tony Luck <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

---

 arch/ia64/kernel/acpi-processor.c |   10 +---------
 arch/x86/kernel/acpi/processor.c  |   10 +---------
 drivers/acpi/processor_pdc.c      |   11 +++++++++++
 3 files changed, 13 insertions(+), 18 deletions(-)

arch/ia64/kernel/acpi-processor.c +1 -9

index ab72d46..ebe23f5 100644
@@ -18,15 +18,7 @@ static void init_intel_pdc(struct acpi_processor *pr)
 {
 	u32 *buf = (u32 *)pr->pdc->pointer->buffer.pointer;
 
-	buf[0] = ACPI_PDC_REVISION_ID;
-	buf[1] = 1;
-	buf[2] = ACPI_PDC_EST_CAPABILITY_SMP;
-	/*
-	 * The default of PDC_SMP_T_SWCOORD bit is set for IA64 cpu so
-	 * that OSPM is capable of native ACPI throttling software
-	 * coordination using BIOS supplied _TSD info.
-	 */
-	buf[2] |= ACPI_PDC_SMP_T_SWCOORD;
+	buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;
 
 	return;
 }

arch/x86/kernel/acpi/processor.c +1 -9

index 967860b..d722ca8 100644
@@ -16,16 +16,8 @@ static void init_intel_pdc(struct acpi_processor *pr, struct cpuinfo_x86 *c)
 {
 	u32 *buf = (u32 *)pr->pdc->pointer->buffer.pointer;
 
-	buf[0] = ACPI_PDC_REVISION_ID;
-	buf[1] = 1;
-	buf[2] = ACPI_PDC_C_CAPABILITY_SMP;
+	buf[2] |= ACPI_PDC_C_CAPABILITY_SMP;
 
-	/*
-	 * The default of PDC_SMP_T_SWCOORD bit is set for intel x86 cpu so
-	 * that OSPM is capable of native ACPI throttling software
-	 * coordination using BIOS supplied _TSD info.
-	 */
-	buf[2] |= ACPI_PDC_SMP_T_SWCOORD;
 	if (cpu_has(c, X86_FEATURE_EST))
 		buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;

drivers/acpi/processor_pdc.c +11 -0

index 87946b6..ccda7c9 100644
@@ -33,6 +33,15 @@ static struct dmi_system_id __cpuinitdata processor_idle_dmi_table[] = {
 	{},
 };
 
+static void acpi_set_pdc_bits(u32 *buf)
+{
+	buf[0] = ACPI_PDC_REVISION_ID;
+	buf[1] = 1;
+
+	/* Enable coordination with firmware's _TSD info */
+	buf[2] = ACPI_PDC_SMP_T_SWCOORD;
+}
+
 static void acpi_processor_init_pdc(struct acpi_processor *pr)
 {
 	struct acpi_object_list *obj_list;

@@ -63,6 +72,8 @@ static void acpi_processor_init_pdc(struct acpi_processor *pr)
 		return;
 	}
 
+	acpi_set_pdc_bits(buf);
+
 	obj->type = ACPI_TYPE_BUFFER;
 	obj->buffer.length = 12;
 	obj->buffer.pointer = (u8 *) buf;

Alex Chiang

author

2009-12-20 19:19 UTC #8 lore raw

PATCH 5/11 ACPI: processor: finish unifying arch_acpi_processor_init_pdc()

In reply to Alex Chiang

The only thing arch-specific about calling _PDC is what bits get set in the input obj_list buffer.

There's no need for several levels of indirection to twiddle those bits. Additionally, since we're just messing around with a buffer, we can simplify the interface; no need to pass around the entire struct acpi_processor * just to get at the buffer.

Cc Tony Luck <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

---

 arch/ia64/include/asm/acpi.h      |    4 ++++
 arch/ia64/kernel/acpi-processor.c |   18 ------------------
 arch/x86/include/asm/acpi.h       |   19 +++++++++++++++++++
 arch/x86/kernel/acpi/processor.c  |   34 ----------------------------------
 drivers/acpi/processor_pdc.c      |    6 +++---
 include/acpi/processor.h          |    1 -
 6 files changed, 26 insertions(+), 56 deletions(-)

arch/ia64/include/asm/acpi.h +4 -0

index 3b78882..7ae5889 100644
@@ -133,6 +133,10 @@ extern int __initdata nid_to_pxm_map[MAX_NUMNODES];
 #endif
 
 static inline bool arch_has_acpi_pdc(void) { return true; }
+static inline void arch_acpi_set_pdc_bits(u32 *buf)
+{
+	buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;
+}
 
 #define acpi_unlazy_tlb(x)

arch/ia64/kernel/acpi-processor.c +0 ~~-18~~

index ebe23f5..7ba5acc 100644
@@ -14,24 +14,6 @@
 #include <acpi/processor.h>
 #include <asm/acpi.h>
 
-static void init_intel_pdc(struct acpi_processor *pr)
-{
-	u32 *buf = (u32 *)pr->pdc->pointer->buffer.pointer;
-
-	buf[2] |= ACPI_PDC_EST_CAPABILITY_SMP;
-
-	return;
-}
-
-/* Initialize _PDC data based on the CPU vendor */
-void arch_acpi_processor_init_pdc(struct acpi_processor *pr)
-{
-	init_intel_pdc(pr);
-	return;
-}
-
-EXPORT_SYMBOL(arch_acpi_processor_init_pdc);
-
 void arch_acpi_processor_cleanup_pdc(struct acpi_processor *pr)
 {
 	if (pr->pdc) {

arch/x86/include/asm/acpi.h +19 -0

index d787e6e..56f462c 100644
@@ -149,6 +149,25 @@ static inline bool arch_has_acpi_pdc(void)
 		c->x86_vendor == X86_VENDOR_CENTAUR);
 }
 
+static inline void arch_acpi_set_pdc_bits(u32 *buf)
+{
+	struct cpuinfo_x86 *c = &cpu_data(0);
+
+	buf[2] |= ACPI_PDC_C_CAPABILITY_SMP;
+
+	if (cpu_has(c, X86_FEATURE_EST))
+		buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
+
+	if (cpu_has(c, X86_FEATURE_ACPI))
+		buf[2] |= ACPI_PDC_T_FFH;
+
+	/*
+	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
+	 */
+	if (!cpu_has(c, X86_FEATURE_MWAIT))
+		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
+}
+
 #else /* !CONFIG_ACPI */
 
 #define acpi_lapic 0

arch/x86/kernel/acpi/processor.c +0 ~~-34~~

index d722ca8..0f57307 100644
@@ -12,40 +12,6 @@
 #include <acpi/processor.h>
 #include <asm/acpi.h>
 
-static void init_intel_pdc(struct acpi_processor *pr, struct cpuinfo_x86 *c)
-{
-	u32 *buf = (u32 *)pr->pdc->pointer->buffer.pointer;
-
-	buf[2] |= ACPI_PDC_C_CAPABILITY_SMP;
-
-	if (cpu_has(c, X86_FEATURE_EST))
-		buf[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP;
-
-	if (cpu_has(c, X86_FEATURE_ACPI))
-		buf[2] |= ACPI_PDC_T_FFH;
-
-	/*
-	 * If mwait/monitor is unsupported, C2/C3_FFH will be disabled
-	 */
-	if (!cpu_has(c, X86_FEATURE_MWAIT))
-		buf[2] &= ~(ACPI_PDC_C_C2C3_FFH);
-
-	return;
-}
-
-
-/* Initialize _PDC data based on the CPU vendor */
-void arch_acpi_processor_init_pdc(struct acpi_processor *pr)
-{
-	struct cpuinfo_x86 *c = &cpu_data(pr->id);
-
-	init_intel_pdc(pr, c);
-
-	return;
-}
-
-EXPORT_SYMBOL(arch_acpi_processor_init_pdc);
-
 void arch_acpi_processor_cleanup_pdc(struct acpi_processor *pr)
 {
 	if (pr->pdc) {

drivers/acpi/processor_pdc.c +3 -3

index ccda7c9..48df08e 100644
@@ -40,6 +40,9 @@ static void acpi_set_pdc_bits(u32 *buf)
 
 	/* Enable coordination with firmware's _TSD info */
 	buf[2] = ACPI_PDC_SMP_T_SWCOORD;
+
+	/* Twiddle arch-specific bits needed for _PDC */
+	arch_acpi_set_pdc_bits(buf);
 }
 
 static void acpi_processor_init_pdc(struct acpi_processor *pr)

@@ -81,9 +84,6 @@ static void acpi_processor_init_pdc(struct acpi_processor *pr)
 	obj_list->pointer = obj;
 	pr->pdc = obj_list;
 
-	/* Now let the arch do the bit-twiddling to buf[] */
-	arch_acpi_processor_init_pdc(pr);
-
 	return;
 }

include/acpi/processor.h +0 -1

index a1b748a..50edd73 100644
@@ -257,7 +257,6 @@ int acpi_processor_notify_smm(struct module *calling_module);
 DECLARE_PER_CPU(struct acpi_processor *, processors);
 extern struct acpi_processor_errata errata;
 
-void arch_acpi_processor_init_pdc(struct acpi_processor *pr);
 void arch_acpi_processor_cleanup_pdc(struct acpi_processor *pr);
 
 #ifdef ARCH_HAS_POWER_INIT

Alex Chiang

author

2009-12-20 19:19 UTC #9 lore raw

PATCH 6/11 ACPI: processor: unify arch_acpi_processor_cleanup_pdc

In reply to Alex Chiang

The x86 and ia64 implementations of the function in $subject are exactly the same.

Also, since the arch-specific implementations of setting _PDC have been completely hollowed out, remove the empty shells.

Cc Tony Luck <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

---

 arch/ia64/kernel/Makefile         |    4 ----
 arch/ia64/kernel/acpi-processor.c |   27 ---------------------------
 arch/x86/kernel/acpi/Makefile     |    2 +-
 arch/x86/kernel/acpi/processor.c  |   25 -------------------------
 drivers/acpi/processor_pdc.c      |   21 ++++++++++++++++++++-
 include/acpi/processor.h          |    2 --
 6 files changed, 21 insertions(+), 60 deletions(-)
 delete mode 100644 arch/ia64/kernel/acpi-processor.c
 delete mode 100644 arch/x86/kernel/acpi/processor.c

arch/ia64/kernel/Makefile +0 -4

index 2a75e93..e123634 100644
@@ -18,10 +18,6 @@ obj-$(CONFIG_IA64_GENERIC)	+= acpi-ext.o
 obj-$(CONFIG_IA64_HP_ZX1)	+= acpi-ext.o
 obj-$(CONFIG_IA64_HP_ZX1_SWIOTLB) += acpi-ext.o
 
-ifneq ($(CONFIG_ACPI_PROCESSOR),)
-obj-y				+= acpi-processor.o
-endif
-
 obj-$(CONFIG_IA64_PALINFO)	+= palinfo.o
 obj-$(CONFIG_IOSAPIC)		+= iosapic.o
 obj-$(CONFIG_MODULES)		+= module.o

arch/ia64/kernel/acpi-processor.c deleted +0 ~~-27~~

deleted file mode 100644
index 7ba5acc..0000000
@@ -1,27 +0,0 @@
-/*
- * arch/ia64/kernel/acpi-processor.c
- *
- * Copyright (C) 2005 Intel Corporation
- * 	Venkatesh Pallipadi <[email protected]>
- * 	- Added _PDC for platforms with Intel CPUs
- */
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/acpi.h>
-
-#include <acpi/processor.h>
-#include <asm/acpi.h>
-
-void arch_acpi_processor_cleanup_pdc(struct acpi_processor *pr)
-{
-	if (pr->pdc) {
-		kfree(pr->pdc->pointer->buffer.pointer);
-		kfree(pr->pdc->pointer);
-		kfree(pr->pdc);
-		pr->pdc = NULL;
-	}
-}
-
-EXPORT_SYMBOL(arch_acpi_processor_cleanup_pdc);

arch/x86/kernel/acpi/Makefile +1 -1

index fd5ca97..6f35260 100644
@@ -4,7 +4,7 @@ obj-$(CONFIG_ACPI)		+= boot.o
 obj-$(CONFIG_ACPI_SLEEP)	+= sleep.o wakeup_rm.o wakeup_$(BITS).o
 
 ifneq ($(CONFIG_ACPI_PROCESSOR),)
-obj-y				+= cstate.o processor.o
+obj-y				+= cstate.o
 endif
 
 $(obj)/wakeup_rm.o:    $(obj)/realmode/wakeup.bin

arch/x86/kernel/acpi/processor.c deleted +0 ~~-25~~

deleted file mode 100644
index 0f57307..0000000
@@ -1,25 +0,0 @@
-/*
- * Copyright (C) 2005 Intel Corporation
- * 	Venkatesh Pallipadi <[email protected]>
- * 	- Added _PDC for platforms with Intel CPUs
- */
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/acpi.h>
-
-#include <acpi/processor.h>
-#include <asm/acpi.h>
-
-void arch_acpi_processor_cleanup_pdc(struct acpi_processor *pr)
-{
-	if (pr->pdc) {
-		kfree(pr->pdc->pointer->buffer.pointer);
-		kfree(pr->pdc->pointer);
-		kfree(pr->pdc);
-		pr->pdc = NULL;
-	}
-}
-
-EXPORT_SYMBOL(arch_acpi_processor_cleanup_pdc);

drivers/acpi/processor_pdc.c +20 -1

index 48df08e..e786e2c 100644
@@ -1,3 +1,12 @@
+/*
+ * Copyright (C) 2005 Intel Corporation
+ * Copyright (C) 2009 Hewlett-Packard Development Company, L.P.
+ *
+ *	Alex Chiang <[email protected]>
+ *	- Unified x86/ia64 implementations
+ *	Venkatesh Pallipadi <[email protected]>
+ *	- Added _PDC for platforms with Intel CPUs
+ */
 #include <linux/dmi.h>
 
 #include <acpi/acpi_drivers.h>

@@ -121,6 +130,16 @@ static int acpi_processor_eval_pdc(struct acpi_processor *pr)
 	return status;
 }
 
+static void acpi_processor_cleanup_pdc(struct acpi_processor *pr)
+{
+	if (pr->pdc) {
+		kfree(pr->pdc->pointer->buffer.pointer);
+		kfree(pr->pdc->pointer);
+		kfree(pr->pdc);
+		pr->pdc = NULL;
+	}
+}
+
 void acpi_processor_set_pdc(struct acpi_processor *pr)
 {
 	if (arch_has_acpi_pdc() == false)

@@ -128,7 +147,7 @@ void acpi_processor_set_pdc(struct acpi_processor *pr)
 
 	acpi_processor_init_pdc(pr);
 	acpi_processor_eval_pdc(pr);
-	arch_acpi_processor_cleanup_pdc(pr);
+	acpi_processor_cleanup_pdc(pr);
 }
 EXPORT_SYMBOL_GPL(acpi_processor_set_pdc);

include/acpi/processor.h +0 -2

index 50edd73..0873cd5 100644
@@ -257,8 +257,6 @@ int acpi_processor_notify_smm(struct module *calling_module);
 DECLARE_PER_CPU(struct acpi_processor *, processors);
 extern struct acpi_processor_errata errata;
 
-void arch_acpi_processor_cleanup_pdc(struct acpi_processor *pr);
-
 #ifdef ARCH_HAS_POWER_INIT
 void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags,
 					unsigned int cpu);

Alex Chiang

author

2009-12-20 19:19 UTC #10 lore raw

PATCH 7/11 ACPI: processor: introduce acpi_processor_alloc_pdc()

In reply to Alex Chiang

acpi_processor_init_pdc() isn't really doing anything interesting with the struct acpi_processor * parameter. Its real job is to allocate the buffer for the _PDC bits.

So rename the function to acpi_processor_alloc_pdc(), and just return the struct acpi_object_list * it's supposed to allocate.

Signed-off-by Alex Chiang <[email protected]>

---

 drivers/acpi/processor_pdc.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

drivers/acpi/processor_pdc.c +13 -9

index e786e2c..23d2828 100644
@@ -54,26 +54,24 @@ static void acpi_set_pdc_bits(u32 *buf)
 	arch_acpi_set_pdc_bits(buf);
 }
 
-static void acpi_processor_init_pdc(struct acpi_processor *pr)
+static struct acpi_object_list *acpi_processor_alloc_pdc(void)
 {
 	struct acpi_object_list *obj_list;
 	union acpi_object *obj;
 	u32 *buf;
 
-	pr->pdc = NULL;
-
 	/* allocate and initialize pdc. It will be used later. */
 	obj_list = kmalloc(sizeof(struct acpi_object_list), GFP_KERNEL);
 	if (!obj_list) {
 		printk(KERN_ERR "Memory allocation error\n");
-		return;
+		return NULL;
 	}
 
 	obj = kmalloc(sizeof(union acpi_object), GFP_KERNEL);
 	if (!obj) {
 		printk(KERN_ERR "Memory allocation error\n");
 		kfree(obj_list);
-		return;
+		return NULL;
 	}
 
 	buf = kmalloc(12, GFP_KERNEL);

@@ -81,7 +79,7 @@ static void acpi_processor_init_pdc(struct acpi_processor *pr)
 		printk(KERN_ERR "Memory allocation error\n");
 		kfree(obj);
 		kfree(obj_list);
-		return;
+		return NULL;
 	}
 
 	acpi_set_pdc_bits(buf);

@@ -91,9 +89,8 @@ static void acpi_processor_init_pdc(struct acpi_processor *pr)
 	obj->buffer.pointer = (u8 *) buf;
 	obj_list->count = 1;
 	obj_list->pointer = obj;
-	pr->pdc = obj_list;
 
-	return;
+	return obj_list;
 }
 
 /*

@@ -142,10 +139,17 @@ static void acpi_processor_cleanup_pdc(struct acpi_processor *pr)
 
 void acpi_processor_set_pdc(struct acpi_processor *pr)
 {
+	struct acpi_object_list *obj_list;
+
 	if (arch_has_acpi_pdc() == false)
 		return;
 
-	acpi_processor_init_pdc(pr);
+	obj_list = acpi_processor_alloc_pdc();
+	if (!obj_list)
+		return;
+
+	pr->pdc = obj_list;
+
 	acpi_processor_eval_pdc(pr);
 	acpi_processor_cleanup_pdc(pr);
 }

Alex Chiang

author

2009-12-20 19:19 UTC #11 lore raw

PATCH 8/11 ACPI: processor: change acpi_processor_eval_pdc interface

In reply to Alex Chiang

acpi_processor_eval_pdc() really only needs a handle and an acpi_object_list * to do its work.

No need to pass in a struct acpi_processor *, so let's be more specific about what we want.

Signed-off-by Alex Chiang <[email protected]>

---

 drivers/acpi/processor_pdc.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

drivers/acpi/processor_pdc.c +4 -8

index 23d2828..974de4e 100644
@@ -97,13 +97,11 @@ static struct acpi_object_list *acpi_processor_alloc_pdc(void)
  * _PDC is required for a BIOS-OS handshake for most of the newer
  * ACPI processor features.
  */
-static int acpi_processor_eval_pdc(struct acpi_processor *pr)
+static int
+acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
 {
-	struct acpi_object_list *pdc_in = pr->pdc;
 	acpi_status status = AE_OK;
 
-	if (!pdc_in)
-		return status;
 	if (idle_nomwait) {
 		/*
 		 * If mwait is disabled for CPU C-states, the C2C3_FFH access

@@ -118,7 +116,7 @@ static int acpi_processor_eval_pdc(struct acpi_processor *pr)
 		buffer[2] &= ~(ACPI_PDC_C_C2C3_FFH | ACPI_PDC_C_C1_FFH);
 
 	}
-	status = acpi_evaluate_object(pr->handle, "_PDC", pdc_in, NULL);
+	status = acpi_evaluate_object(handle, "_PDC", pdc_in, NULL);
 
 	if (ACPI_FAILURE(status))
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,

@@ -148,9 +146,7 @@ void acpi_processor_set_pdc(struct acpi_processor *pr)
 	if (!obj_list)
 		return;
 
-	pr->pdc = obj_list;
-
-	acpi_processor_eval_pdc(pr);
+	acpi_processor_eval_pdc(pr->handle, obj_list);
 	acpi_processor_cleanup_pdc(pr);
 }
 EXPORT_SYMBOL_GPL(acpi_processor_set_pdc);

Alex Chiang

author

2009-12-20 19:23 UTC #12 lore raw

PATCH 1/3 ACPI: processor: open code acpi_processor_cleanup_pdc

In reply to Alex Chiang

We have the acpi_object_list * right there in acpi_processor_set_pdc() so it doesn't seem necessary for an entire helper function just to free it.

Signed-off-by Alex Chiang <[email protected]>

---

 drivers/acpi/processor_pdc.c |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

drivers/acpi/processor_pdc.c +4 ~~-11~~

index 974de4e..deeba22 100644
@@ -125,16 +125,6 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
 	return status;
 }
 
-static void acpi_processor_cleanup_pdc(struct acpi_processor *pr)
-{
-	if (pr->pdc) {
-		kfree(pr->pdc->pointer->buffer.pointer);
-		kfree(pr->pdc->pointer);
-		kfree(pr->pdc);
-		pr->pdc = NULL;
-	}
-}
-
 void acpi_processor_set_pdc(struct acpi_processor *pr)
 {
 	struct acpi_object_list *obj_list;

@@ -147,7 +137,10 @@ void acpi_processor_set_pdc(struct acpi_processor *pr)
 		return;
 
 	acpi_processor_eval_pdc(pr->handle, obj_list);
-	acpi_processor_cleanup_pdc(pr);
+
+	kfree(obj_list->pointer->buffer.pointer);
+	kfree(obj_list->pointer);
+	kfree(obj_list);
 }
 EXPORT_SYMBOL_GPL(acpi_processor_set_pdc);

Alex Chiang

author

2009-12-20 19:23 UTC #13 lore raw

PATCH 2/3 ACPI: processor: change acpi_processor_set_pdc() interface

In reply to Alex Chiang

When calling _PDC, we really only need the handle to the processor to call the method; we don't look at any other parts of the struct acpi_processor * given to us.

In the early path, when we walk the namespace, we are given the handle directly, so just pass it through to acpi_processor_set_pdc() without stuffing it into a wasteful struct acpi_processor allocated on the stack each time

This saves 2834 bytes of stack.

Update the interface accordingly.

Signed-off-by Alex Chiang <[email protected]>

---

 drivers/acpi/processor_core.c |    2 +-
 drivers/acpi/processor_pdc.c  |   18 +++---------------
 include/acpi/processor.h      |    2 +-
 3 files changed, 5 insertions(+), 17 deletions(-)

drivers/acpi/processor_core.c +1 -1

index a19a4ff..9863c98 100644
@@ -763,7 +763,7 @@ static int __cpuinit acpi_processor_add(struct acpi_device *device)
 	}
 
 	/* _PDC call should be done before doing anything else (if reqd.). */
-	acpi_processor_set_pdc(pr);
+	acpi_processor_set_pdc(pr->handle);
 
 #ifdef CONFIG_CPU_FREQ
 	acpi_processor_ppc_has_changed(pr, 0);

drivers/acpi/processor_pdc.c +3 ~~-15~~

index deeba22..30e4dc0 100644
@@ -125,7 +125,7 @@ acpi_processor_eval_pdc(acpi_handle handle, struct acpi_object_list *pdc_in)
 	return status;
 }
 
-void acpi_processor_set_pdc(struct acpi_processor *pr)
+void acpi_processor_set_pdc(acpi_handle handle)
 {
 	struct acpi_object_list *obj_list;

@@ -136,7 +136,7 @@ void acpi_processor_set_pdc(struct acpi_processor *pr)
 	if (!obj_list)
 		return;
 
-	acpi_processor_eval_pdc(pr->handle, obj_list);
+	acpi_processor_eval_pdc(handle, obj_list);
 
 	kfree(obj_list->pointer->buffer.pointer);
 	kfree(obj_list->pointer);

@@ -147,19 +147,7 @@ EXPORT_SYMBOL_GPL(acpi_processor_set_pdc);
 static acpi_status
 early_init_pdc(acpi_handle handle, u32 lvl, void *context, void **rv)
 {
-	struct acpi_processor pr;
-
-	pr.handle = handle;
-
-	/* x86 implementation looks at pr.id to determine some
-	 * CPU capabilites. We can just hard code to 0 since we're
-	 * assuming the CPUs in the system are homogenous and all
-	 * have the same capabilities.
-	 */
-	pr.id = 0;
-
-	acpi_processor_set_pdc(&pr);
-
+	acpi_processor_set_pdc(handle);
 	return AE_OK;
 }

include/acpi/processor.h +1 -1

index 0873cd5..7cc433d 100644
@@ -323,7 +323,7 @@ static inline int acpi_processor_get_bios_limit(int cpu, unsigned int *limit)
 #endif				/* CONFIG_CPU_FREQ */
 
 /* in processor_pdc.c */
-void acpi_processor_set_pdc(struct acpi_processor *pr);
+void acpi_processor_set_pdc(acpi_handle handle);
 
 /* in processor_throttling.c */
 int acpi_processor_tstate_has_changed(struct acpi_processor *pr);

Alex Chiang

author

2009-12-20 19:23 UTC #14 lore raw

PATCH 3/3 ACPI: processor: remove _PDC object list from struct acpi_processor

In reply to Alex Chiang

When we call _PDC, we get a handle to the processor, allocate the object list buffer as needed, and free it immediately after calling _PDC.

There's no need to drag around this object list with us everywhere else, so let's just get rid of it.

Signed-off-by Alex Chiang <[email protected]>

---

 include/acpi/processor.h |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

include/acpi/processor.h +0 -2

index 7cc433d..0ea5ef4 100644
@@ -224,8 +224,6 @@ struct acpi_processor {
 	struct acpi_processor_throttling throttling;
 	struct acpi_processor_limit limit;
 	struct thermal_cooling_device *cdev;
-	/* the _PDC objects for this processor, if any */
-	struct acpi_object_list *pdc;
 };
 
 struct acpi_processor_errata {

Alex Chiang

author

2009-12-20 19:25 UTC #15 lore raw

In reply to Alex Chiang

* Alex Chiang <[email protected]>:

Quoted 2 lines

It was recently discovered that some BIOS implementations load dynamic SSDTs when a processor's _PDC method is evaluated.

D'oh, I knew I'd forget something. Here are some bz references:

	http://bugzilla.kernel.org/show_bug.cgi?id=14824
	http://bugzilla.kernel.org/show_bug.cgi?id=14710

/ac

Dominik Brodowski

2009-12-21 08:15 UTC #16 lore raw

In reply to Alex Chiang

Hey,

Quoted 5 lines · Alex Chiang

This does introduce a boot time namespace walk for all the CPUs in the system, looking for and evaluating _PDC. Hopefully that will not make life miserable for the giant SGI clusters. If worse comes to worse, maybe we can quirk them and avoid the namespace walk.

Could this be done async, with completion before EC initialization?

Best,
Dominik

Alex Chiang

author

2009-12-21 21:17 UTC #17 lore raw

* Dominik Brodowski <[email protected]>:

Quoted 9 lines · Alex Chiang

Quoted 5 lines
This does introduce a boot time namespace walk for all the CPUs in the system, looking for and evaluating _PDC. Hopefully that will not make life miserable for the giant SGI clusters. If worse comes to worse, maybe we can quirk them and avoid the namespace walk.
Could this be done async, with completion before EC initialization?

I don't think I understand your suggestion.

On some platforms, we need to finish evaluating _PDC before we initialize the EC. That's what I discovered with the Envy 15. At that point in boot, we're single-threaded and going through early ACPI initialization. Each step has to finish before the next, in order for later initialization to succeed.

There's nothing else going on except for waiting for _PDC to finish so that we can initialize the EC.

My fear was that on a huge compute cluster, walking the namespace for every CPU object to evaluate _PDC might take a long time, and is also probably unnecessary, since their hardware/firmware probably doesn't load dynamic tables in _PDC.

In that case, my suggestion was to avoid the walk with a quirk.

But maybe it's not necessary to worry about that until it's actually proven to be a problem.

Thanks,
/ac

Len Brown

reviewer

2009-12-22 08:43 UTC #18 lore raw

In reply to Alex Chiang

entire series applied to acpi-test

nice cleanup, Alex.
Unless Venki knows of something I don't about upcoming PDC-foo, I think it will stick.

thanks,
Len Brown, Intel Open Source Technology Center