Skip to contentSkip to conversation

ACPI patches in test for Linux-2.6.22

PATCH In review Len Brown 7 participants 31 messages Mar 10, 2007 – Apr 19, 2007

Subsystems (4)
lore
Share & tools
lore

Len Brown

author reviewer
#0 lore raw

These patches are in test for Linux-2.6.22.

git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-acpi-2.6.git test

thanks,
-Len

author reviewer
#1 lore raw

PATCH 1/22 cpuidle take2: Core cpuidle infrastructure

From: Venkatesh Pallipadi <[email protected]>

Announcing 'cpuidle', a new CPU power management infrastructure to manage idle CPUs in a clean and efficient manner.
cpuidle separates out the drivers that can provide support for multiple types of idle states and policy governors that decide on what idle state to use at run time.
A cpuidle driver can support multiple idle states based on parameters like varying power consumption, wakeup latency, etc (ACPI C-states for example). A cpuidle governor can be usage model specific (laptop, server, laptop on battery etc).
Main advantage of the infrastructure being, it allows independent development of drivers and governors and allows for better CPU power management.

A huge thanks to Adam Belay and Shaohua Li who were part of this mini-project since its beginning and are greatly responsible for this patchset.

This patch:

Core cpuidle infrastructure.
Introduces a new abstraction layer for cpuidle:
* which manages drivers that can support multiple idles states. Drivers can be generic or particular to specific hardware/platform
* allows pluging in multiple policy governors that can take idle state policy decision
* The core also has a set of sysfs interfaces with which administrato can know about supported drivers and governors and switch them at run time.

---
 arch/i386/Kconfig                  |    2 +
 arch/x86_64/Kconfig                |    2 +
 drivers/Makefile                   |    1 +
 drivers/cpuidle/Kconfig            |   28 +++
 drivers/cpuidle/Makefile           |    5 +
 drivers/cpuidle/cpuidle.c          |  287 ++++++++++++++++++++++++++++++
 drivers/cpuidle/cpuidle.h          |   52 ++++++
 drivers/cpuidle/driver.c           |  219 +++++++++++++++++++++++
 drivers/cpuidle/governor.c         |  160 +++++++++++++++++
 drivers/cpuidle/governors/Makefile |    5 +
 drivers/cpuidle/governors/ladder.c |  229 ++++++++++++++++++++++++
 drivers/cpuidle/sysfs.c            |  340 ++++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h            |  160 +++++++++++++++++
 13 files changed, 1490 insertions(+), 0 deletions(-)
 create mode 100644 drivers/cpuidle/Kconfig
 create mode 100644 drivers/cpuidle/Makefile
 create mode 100644 drivers/cpuidle/cpuidle.c
 create mode 100644 drivers/cpuidle/cpuidle.h
 create mode 100644 drivers/cpuidle/driver.c
 create mode 100644 drivers/cpuidle/governor.c
 create mode 100644 drivers/cpuidle/governors/Makefile
 create mode 100644 drivers/cpuidle/governors/ladder.c
 create mode 100644 drivers/cpuidle/sysfs.c
 create mode 100644 include/linux/cpuidle.h
arch/i386/Kconfig +2 -0
index 2f76725..bf68fc2 100644
@@ -1066,6 +1066,8 @@ endmenu
 
 source "arch/i386/kernel/cpu/cpufreq/Kconfig"
 
+source "drivers/cpuidle/Kconfig"
+
 endmenu
 
 menu "Bus options (PCI, PCMCIA, EISA, MCA, ISA)"
arch/x86_64/Kconfig +2 -0
index 56eb14c..fd01428 100644
@@ -670,6 +670,8 @@ source "drivers/acpi/Kconfig"
 
 source "arch/x86_64/kernel/cpufreq/Kconfig"
 
+source "drivers/cpuidle/Kconfig"
+
 endmenu
 
 menu "Bus options (PCI etc.)"
drivers/Makefile +1 -0
index 3a718f5..7acd123 100644
@@ -69,6 +69,7 @@ obj-$(CONFIG_EDAC)		+= edac/
 obj-$(CONFIG_MCA)		+= mca/
 obj-$(CONFIG_EISA)		+= eisa/
 obj-$(CONFIG_CPU_FREQ)		+= cpufreq/
+obj-$(CONFIG_CPU_IDLE)		+= cpuidle/
 obj-$(CONFIG_MMC)		+= mmc/
 obj-$(CONFIG_NEW_LEDS)		+= leds/
 obj-$(CONFIG_INFINIBAND)	+= infiniband/
drivers/cpuidle/Kconfig new file +28 -0
new file mode 100644
index 0000000..b9ac217
@@ -0,0 +1,28 @@
+menu "CPU idle PM support"
+
+config CPU_IDLE
+	bool "CPU idle PM support"
+	help
+	  CPU idle is a generic framework for supporting software-controlled
+	  idle processor power management.  It includes modular cross-platform
+	  governors that can be swapped during runtime.
+
+	  If you're using a mobile platform that supports CPU idle PM (e.g.
+	  an ACPI-capable notebook), you should say Y here.
+
+if CPU_IDLE
+
+comment "Governors"
+
+config CPU_IDLE_GOV_LADDER
+	tristate "'ladder' governor"
+	depends on CPU_IDLE
+	default y
+	help
+	  This cpuidle governor promotes and demotes through the supported idle
+	  states using residency time and bus master activity as metrics.  This
+	  algorithm was originally introduced in the old ACPI processor driver.
+
+endif	# CPU_IDLE
+
+endmenu
drivers/cpuidle/Makefile new file +5 -0
new file mode 100644
index 0000000..5634f88
@@ -0,0 +1,5 @@
+#
+# Makefile for cpuidle.
+#
+
+obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
drivers/cpuidle/cpuidle.c new file +287 -0

287 lines not on this page · raw message

drivers/cpuidle/cpuidle.h new file +52 -0

52 lines not on this page · raw message

drivers/cpuidle/driver.c new file +219 -0

219 lines not on this page · raw message

drivers/cpuidle/governor.c new file +160 -0

160 lines not on this page · raw message

drivers/cpuidle/governors/Makefile new file +5 -0
new file mode 100644
index 0000000..becf470
@@ -0,0 +1,5 @@
+#
+# Makefile for cpuidle governors.
+#
+
+obj-$(CONFIG_CPU_IDLE_GOV_LADDER) += ladder.o
drivers/cpuidle/governors/ladder.c new file +229 -0

229 lines not on this page · raw message

drivers/cpuidle/sysfs.c new file +340 -0

340 lines not on this page · raw message

include/linux/cpuidle.h new file +160 -0

160 lines not on this page · raw message

Signature · 1 line
1.5.0.3.310.g05ef5
#2 lore raw

Re: [PATCH 01/22] cpuidle take2: Core cpuidle infrastructure

drivers/cpuidle/cpuidle.c line 33

Quoted 6 lines · Len Brown
drivers/cpuidle/cpuidle.c new file +6 -0
+
+/**
+ * cpuidle_idle_call - the main idle loop
+ *
+ * NOTE: no locks or semaphores should be used here
+ * FIXME: DYNTICKS handling

I don't think you can merge anything anymore with such a FIXME.

Besides cpuidle is a pretty radical change outside ACPI and I would recommend to make a few review cycles over linux-kernel and likely linux-arch.

For me it's not clear it has enough advantages for its amount of code

-Andi

author reviewer
#3 lore raw

Re: [PATCH 01/22] cpuidle take2: Core cpuidle infrastructure

Quoted 9 lines · Andi Kleen

drivers/cpuidle/cpuidle.c line 33

Quoted 6 lines · Len Brown
drivers/cpuidle/cpuidle.c new file +6 -0
+
+/**
+ * cpuidle_idle_call - the main idle loop
+ *
+ * NOTE: no locks or semaphores should be used here
+ * FIXME: DYNTICKS handling

I don't think you can merge anything anymore with such a FIXME.

Certainly it need to work well with NOHZ before it can go upstream. Indeed, NOHZ is one of the reasons for doing cpuidle in the first place.

But then NOHZ is probably going to take some time to mature also, certainly it can't be considered mature until it is also in the 64-bit kernel.

The question becomes if NOHZ matures w/o cpuidle -- do we need to modify the existing tick-based ACPI idle code before cpuidle?

Quoted 2 lines

Besides cpuidle is a pretty radical change outside ACPI and I would recommend to make a few review cycles over linux-kernel and likely linux-arch.

I agree. acpi-test and -mm would not be sufficient exposure for generic code.

Quoted 1 line

For me it's not clear it has enough advantages for its amount of code

Understood. Measurements will tell.
This is the very reason we created tools which can simultaneously measure performance and battery life: http://sourceforge.net/projects/bltk

I think that for cpuidle to go upstream, its default should not require any user-space changes to work as well as the old code. If some sysfs hooks are available to get added value, such as useful statistics or modifying policy, that is something that user-space can safely ignore and not break.

-Len

#4 lore raw

Re: [PATCH 01/22] cpuidle take2: Core cpuidle infrastructure

In reply to Len Brown
Quoted 5 lines · Len Brown
 arch/i386/Kconfig                  |    2 +
 arch/x86_64/Kconfig                |    2 +
 drivers/Makefile                   |    1 +
 drivers/cpuidle/Kconfig            |   28 +++
 drivers/cpuidle/Makefile           |    5 +
Quoted 8 lines
 drivers/cpuidle/cpuidle.c          |  287 ++++++++++++++++++++++++++++++
 drivers/cpuidle/cpuidle.h          |   52 ++++++
 drivers/cpuidle/driver.c           |  219 +++++++++++++++++++++++
 drivers/cpuidle/governor.c         |  160 +++++++++++++++++
 drivers/cpuidle/governors/Makefile |    5 +
 drivers/cpuidle/governors/ladder.c |  229 ++++++++++++++++++++++++
 drivers/cpuidle/sysfs.c            |  340 ++++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h            |  160 +++++++++++++++++

What about some documentation so that userspace knows what to do with this new framework?

Richard.

author reviewer
#5 lore raw

Re: [PATCH 01/22] cpuidle take2: Core cpuidle infrastructure

Quoted 2 lines · Richard Hughes

What about some documentation so that userspace knows what to do with this new framework?

What does userspace need to know in addition to what is in Documentation/cpuidle/?

#6 lore raw

Re: [PATCH 01/22] cpuidle take2: Core cpuidle infrastructure

Quoted 6 lines · Len Brown
Quoted 2 lines · Richard Hughes

What about some documentation so that userspace knows what to do with this new framework?

What does userspace need to know in addition to what is in Documentation/cpuidle/?

I didn't see that file in the patch, apologies.

Richard.

author reviewer
#7 lore raw

PATCH 2/22 cpuidle take2: Hookup ACPI C-states driver with cpuidle

In reply to Len Brown

From: Venkatesh Pallipadi <[email protected]>

Hookup ACPI C-states onto generic cpuidle infrastructure.

drivers/acpi/procesor_idle.c is now a ACPI C-states driver that registers as a driver in cpuidle infrastructure and the policy part is removed from drivers/acpi/processor_idle.c. We use governor in cpuidle instead.

---
 drivers/acpi/processor_core.c |    3 +
 drivers/acpi/processor_idle.c |  868 ++++++++++++++++-------------------------
 include/acpi/processor.h      |    2 +
 3 files changed, 337 insertions(+), 536 deletions(-)
drivers/acpi/processor_core.c +3 -0
index 99d1516..ddc9e20 100644
@@ -44,6 +44,7 @@
 #include <linux/seq_file.h>
 #include <linux/dmi.h>
 #include <linux/moduleparam.h>
+#include <linux/cpuidle.h>
 
 #include <asm/io.h>
 #include <asm/system.h>
@@ -1024,11 +1025,13 @@ static int __init acpi_processor_init(void)
 
 	acpi_processor_ppc_init();
 
+	cpuidle_register_driver(&acpi_idle_driver);
 	return 0;
 }
 
 static void __exit acpi_processor_exit(void)
 {
+	cpuidle_unregister_driver(&acpi_idle_driver);
 
 	acpi_processor_ppc_exit();
 
drivers/acpi/processor_idle.c +332 -536

961 lines not on this page · raw message

include/acpi/processor.h +2 -0
index 916c010..e375570 100644
@@ -161,6 +161,7 @@ struct acpi_processor_flags {
 	u8 bm_check:1;
 	u8 has_cst:1;
 	u8 power_setup_done:1;
+	u8 bm_rld_set:1;
 };
 
 struct acpi_processor {
@@ -275,6 +276,7 @@ int acpi_processor_power_init(struct acpi_processor *pr,
 int acpi_processor_cst_has_changed(struct acpi_processor *pr);
 int acpi_processor_power_exit(struct acpi_processor *pr,
 			      struct acpi_device *device);
+extern struct cpuidle_driver acpi_idle_driver;
 
 /* in processor_thermal.c */
 int acpi_processor_get_limit_info(struct acpi_processor *pr);
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#8 lore raw

PATCH 3/22 cpuidle take2: Basic documentation for cpuidle

In reply to Len Brown

From: Venkatesh Pallipadi <[email protected]>

Documentation for cpuidle infrastructure

---
 Documentation/cpuidle/core.txt     |   17 +++++++++++++++++
 Documentation/cpuidle/driver.txt   |   24 ++++++++++++++++++++++++
 Documentation/cpuidle/governor.txt |   24 ++++++++++++++++++++++++
 Documentation/cpuidle/sysfs.txt    |   27 +++++++++++++++++++++++++++
 4 files changed, 92 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/cpuidle/core.txt
 create mode 100644 Documentation/cpuidle/driver.txt
 create mode 100644 Documentation/cpuidle/governor.txt
 create mode 100644 Documentation/cpuidle/sysfs.txt
Documentation/cpuidle/core.txt new file +17 -0
new file mode 100644
index 0000000..e686cfc
@@ -0,0 +1,17 @@
+
+		Supporting multiple CPU idle levels in kernel
+
+				cpuidle
+
+General Information:
+
+Various CPUs today support multiple idle levels that are differentiated
+by varying exit latencies and power consumption during idle.
+cpuidle is a generic in-kernel infrastructure that separates
+idle policy (governor) from idle mechanism (driver) and provides a
+standardized infrastructure to support independent development of
+governors and drivers.
+
+cpuidle resides under /drivers/cpuidle.
+
+
Documentation/cpuidle/driver.txt new file +24 -0
new file mode 100644
index 0000000..2dbee4b
@@ -0,0 +1,24 @@
+
+
+		Supporting multiple CPU idle levels in kernel
+
+				cpuidle drivers
+
+
+
+
+cpuidle driver supports capability detection for a particular system. The
+init and exit routines will be called for each online CPU, with a percpu
+cpuidle_driver object and driver should fill in cpuidle_states inside
+cpuidle_driver depending on the CPU capability.
+
+Driver can handle dynamic state changes (like battery<->AC), by calling
+force_redetect interface.
+
+It is possible to have more than one driver registered at the same time and
+user can switch between drivers using /sysfs interface.
+
+Interfaces:
+int cpuidle_register_driver(struct cpuidle_driver *drv);
+void cpuidle_unregister_driver(struct cpuidle_driver *drv);
+int cpuidle_force_redetect(struct cpuidle_device *dev);
Documentation/cpuidle/governor.txt new file +24 -0
new file mode 100644
index 0000000..a0fc3e5
@@ -0,0 +1,24 @@
+
+
+
+		Supporting multiple CPU idle levels in kernel
+
+				cpuidle governors
+
+
+
+
+cpuidle governor is policy routine that decides what idle state to enter at
+any given time. cpuidle core uses different callbacks to governor while
+handling idle entry.
+* select_state callback where governor can determine next idle state to enter
+* prepare_idle callback is called before entering an idle state
+* scan callback is called after a driver forces redetection of the states
+
+More than one governor can be registered at the same time and
+user can switch between drivers using /sysfs interface.
+
+Interfaces:
+int cpuidle_register_governor(struct cpuidle_governor *gov);
+void cpuidle_unregister_governor(struct cpuidle_governor *gov);
+
Documentation/cpuidle/sysfs.txt new file +27 -0
new file mode 100644
index 0000000..7fbf644
@@ -0,0 +1,27 @@
+
+
+		Supporting multiple CPU idle levels in kernel
+
+				cpuidle sysfs
+
+System global cpuidle information are under
+/sys/devices/system/cpu/cpuidle
+
+The current interfaces in this directory has self-explanatory names:
+* available_drivers
+* available_governors
+* current_driver
+* current_governor
+
+Per logical CPU specific cpuidle information are under
+/sys/devices/system/cpu/cpuX/cpuidle
+for each online cpu X
+
+Under this percpu directory, there is a directory for each idle state supported
+by the driver, which in turn has
+* latency
+* power
+* time
+* usage
+
+
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#9 lore raw

PATCH 4/22 ACPI: battery: make internal names consistent with battery "state"

In reply to Len Brown

From: Vladimir Lebedev <[email protected]>

Cleanup -- No functional changes.

Battery state is currently exported in a proc "state" file. Update associated #defines and routines to be consistent.

---
 drivers/acpi/battery.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)
drivers/acpi/battery.c +9 -9
index e64c76c..df04f87 100644
@@ -44,7 +44,7 @@
 #define ACPI_BATTERY_HID		"PNP0C0A"
 #define ACPI_BATTERY_DEVICE_NAME	"Battery"
 #define ACPI_BATTERY_FILE_INFO		"info"
-#define ACPI_BATTERY_FILE_STATUS	"state"
+#define ACPI_BATTERY_FILE_STATE		"state"
 #define ACPI_BATTERY_FILE_ALARM		"alarm"
 #define ACPI_BATTERY_NOTIFY_STATUS	0x80
 #define ACPI_BATTERY_NOTIFY_INFO	0x81
@@ -76,7 +76,7 @@ static struct acpi_driver acpi_battery_driver = {
 		},
 };
 
-struct acpi_battery_status {
+struct acpi_battery_state {
 	acpi_integer state;
 	acpi_integer present_rate;
 	acpi_integer remaining_capacity;
@@ -183,8 +183,8 @@ acpi_battery_get_info(struct acpi_battery *battery,
 }
 
 static int
-acpi_battery_get_status(struct acpi_battery *battery,
-			struct acpi_battery_status **bst)
+acpi_battery_get_state(struct acpi_battery *battery,
+			struct acpi_battery_state **bst)
 {
 	int result = 0;
 	acpi_status status = 0;
@@ -425,7 +425,7 @@ static int acpi_battery_read_state(struct seq_file *seq, void *offset)
 {
 	int result = 0;
 	struct acpi_battery *battery = seq->private;
-	struct acpi_battery_status *bst = NULL;
+	struct acpi_battery_state *bst = NULL;
 	char *units = "?";
 
 
@@ -449,9 +449,9 @@ static int acpi_battery_read_state(struct seq_file *seq, void *offset)
 
 	/* Battery Status (_BST) */
 
-	result = acpi_battery_get_status(battery, &bst);
+	result = acpi_battery_get_state(battery, &bst);
 	if (result || !bst) {
-		seq_printf(seq, "ERROR: Unable to read battery status\n");
+		seq_printf(seq, "ERROR: Unable to read battery state\n");
 		goto end;
 	}
 
@@ -621,7 +621,7 @@ static int acpi_battery_add_fs(struct acpi_device *device)
 	}
 
 	/* 'status' [R] */
-	entry = create_proc_entry(ACPI_BATTERY_FILE_STATUS,
+	entry = create_proc_entry(ACPI_BATTERY_FILE_STATE,
 				  S_IRUGO, acpi_device_dir(device));
 	if (!entry)
 		return -ENODEV;
@@ -652,7 +652,7 @@ static int acpi_battery_remove_fs(struct acpi_device *device)
 	if (acpi_device_dir(device)) {
 		remove_proc_entry(ACPI_BATTERY_FILE_ALARM,
 				  acpi_device_dir(device));
-		remove_proc_entry(ACPI_BATTERY_FILE_STATUS,
+		remove_proc_entry(ACPI_BATTERY_FILE_STATE,
 				  acpi_device_dir(device));
 		remove_proc_entry(ACPI_BATTERY_FILE_INFO,
 				  acpi_device_dir(device));
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#10 lore raw

PATCH 5/22 ACPI: Cache battery status instead of re-evaluating AML

In reply to Len Brown

From: Vladimir Lebedev <[email protected]>

/proc exports _BST in a single file, and _BST is re-evaulated whenever that file is read.

Sometimes user-space reads this file frequently, and on some systems _BST takes a long time to evaluate due to a slow EC.

Further, when we move to sysfs, the values returned from _BST will be in multiple files, and evaluating _BST for each file read would make matters worse.

Here code is added to support caching the results of _BST. A new module parameter "update_time" tells how many seconds the cached _BST should be used before it is re-evaluated. Currently, update_time defaults to 0, and so the existing behaviour of re-evaluating on each read retained.

---
 drivers/acpi/battery.c |  613 ++++++++++++++++++++++++++++++++++--------------
 1 files changed, 441 insertions(+), 172 deletions(-)
drivers/acpi/battery.c +441 -172
index df04f87..f3b0024 100644
@@ -52,12 +52,24 @@
 #define ACPI_BATTERY_UNITS_AMPS		"mA"
 
 #define _COMPONENT		ACPI_BATTERY_COMPONENT
+
+#define ACPI_BATTERY_UPDATE_TIME	0
+
+#define ACPI_BATTERY_NONE_UPDATE	0
+#define ACPI_BATTERY_EASY_UPDATE	1
+#define ACPI_BATTERY_INIT_UPDATE	2
+
 ACPI_MODULE_NAME("battery");
 
 MODULE_AUTHOR("Paul Diefenbaugh");
 MODULE_DESCRIPTION("ACPI Battery Driver");
 MODULE_LICENSE("GPL");
 
+static unsigned int update_time = ACPI_BATTERY_UPDATE_TIME;
+
+/* 0 - every time, > 0 - by update_time */
+module_param(update_time, uint, 0644);
+
 extern struct proc_dir_entry *acpi_lock_battery_dir(void);
 extern void *acpi_unlock_battery_dir(struct proc_dir_entry *acpi_battery_dir);
 
@@ -100,32 +112,117 @@ struct acpi_battery_info {
 };
 
 struct acpi_battery_flags {
-	u8 present:1;		/* Bay occupied? */
-	u8 power_unit:1;	/* 0=watts, 1=apms */
-	u8 alarm:1;		/* _BTP present? */
-	u8 reserved:5;
-};
-
-struct acpi_battery_trips {
-	unsigned long warning;
-	unsigned long low;
+	u8 battery_present_prev;
+	u8 alarm_present;
+	u8 init_update;
+	u8 info_update;
+	u8 state_update;
+	u8 alarm_update;
+	u8 power_unit;
 };
 
 struct acpi_battery {
+	struct mutex mutex;
 	struct acpi_device * device;
 	struct acpi_battery_flags flags;
-	struct acpi_battery_trips trips;
+	struct acpi_buffer bif_data;
+	struct acpi_buffer bst_data;
 	unsigned long alarm;
-	struct acpi_battery_info *info;
+	unsigned long info_update_time;
+	unsigned long state_update_time;
+	unsigned long alarm_update_time;
 };
 
+#define acpi_battery_present(battery) battery->device->status.battery_present
+#define acpi_battery_present_prev(battery) battery->flags.battery_present_prev
+#define acpi_battery_alarm_present(battery) battery->flags.alarm_present
+#define acpi_battery_init_update_flag(battery) battery->flags.init_update
+#define acpi_battery_info_update_flag(battery) battery->flags.info_update
+#define acpi_battery_state_update_flag(battery) battery->flags.state_update
+#define acpi_battery_alarm_update_flag(battery) battery->flags.alarm_update
+#define acpi_battery_power_units(battery) battery->flags.power_unit ? \
+		ACPI_BATTERY_UNITS_AMPS : ACPI_BATTERY_UNITS_WATTS
+#define acpi_battery_handle(battery) battery->device->handle
+#define acpi_battery_inserted(battery) (!acpi_battery_present_prev(battery) & acpi_battery_present(battery))
+#define acpi_battery_removed(battery) (acpi_battery_present_prev(battery) & !acpi_battery_present(battery))
+#define acpi_battery_bid(battery) acpi_device_bid(battery->device)
+#define acpi_battery_status_str(battery) acpi_battery_present(battery) ? "present" : "absent"
+
 /* --------------------------------------------------------------------------
                                Battery Management
    -------------------------------------------------------------------------- */
 
-static int
-acpi_battery_get_info(struct acpi_battery *battery,
-		      struct acpi_battery_info **bif)
+static void acpi_battery_mutex_lock(struct acpi_battery *battery)
+{
+	mutex_lock(&battery->mutex);
+}
+
+static void acpi_battery_mutex_unlock(struct acpi_battery *battery)
+{
+	mutex_unlock(&battery->mutex);
+}
+
+static void acpi_battery_check_result(struct acpi_battery *battery, int result)
+{
+	if (!battery)
+		return;
+
+	if (result) {
+		acpi_battery_init_update_flag(battery) = 1;
+	}
+}
+
+static int acpi_battery_extract_package(struct acpi_battery *battery,
+					union acpi_object *package,
+					struct acpi_buffer *format,
+					struct acpi_buffer *data,
+					char *package_name)
+{
+	acpi_status status = AE_OK;
+	struct acpi_buffer data_null = { 0, NULL };
+
+	status = acpi_extract_package(package, format, &data_null);
+	if (status != AE_BUFFER_OVERFLOW) {
+		ACPI_EXCEPTION((AE_INFO, status, "Extracting size %s",
+				package_name));
+		return -ENODEV;
+	}
+
+	if (data_null.length != data->length) {
+		if (data->pointer) {
+			kfree(data->pointer);
+		}
+		data->pointer = kzalloc(data_null.length, GFP_KERNEL);
+		if (!data->pointer) {
+			ACPI_EXCEPTION((AE_INFO, AE_NO_MEMORY, "kzalloc()"));
+			return -ENOMEM;
+		}
+		data->length = data_null.length;
+	}
+
+	status = acpi_extract_package(package, format, data);
+	if (ACPI_FAILURE(status)) {
+		ACPI_EXCEPTION((AE_INFO, status, "Extracting %s",
+				package_name));
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int acpi_battery_get_status(struct acpi_battery *battery)
+{
+	int result = 0;
+
+	result = acpi_bus_get_status(battery->device);
+	if (result) {
+		ACPI_EXCEPTION((AE_INFO, AE_ERROR, "Evaluating _STA"));
+		return -ENODEV;
+	}
+	return result;
+}
+
+static int acpi_battery_get_info(struct acpi_battery *battery)
 {
 	int result = 0;
 	acpi_status status = 0;
@@ -133,16 +230,18 @@ acpi_battery_get_info(struct acpi_battery *battery,
 	struct acpi_buffer format = { sizeof(ACPI_BATTERY_FORMAT_BIF),
 		ACPI_BATTERY_FORMAT_BIF
 	};
-	struct acpi_buffer data = { 0, NULL };
 	union acpi_object *package = NULL;
+	struct acpi_buffer *data = NULL;
+	struct acpi_battery_info *bif = NULL;
 
+	battery->info_update_time = get_seconds();
 
-	if (!battery || !bif)
-		return -EINVAL;
+	if (!acpi_battery_present(battery))
+		return 0;
 
 	/* Evalute _BIF */
 
-	status = acpi_evaluate_object(battery->device->handle, "_BIF", NULL, &buffer);
+	status = acpi_evaluate_object(acpi_battery_handle(battery), "_BIF", NULL, &buffer);
 	if (ACPI_FAILURE(status)) {
 		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BIF"));
 		return -ENODEV;
@@ -150,41 +249,29 @@ acpi_battery_get_info(struct acpi_battery *battery,
 
 	package = buffer.pointer;
 
+	data = &battery->bif_data;
+
 	/* Extract Package Data */
 
-	status = acpi_extract_package(package, &format, &data);
-	if (status != AE_BUFFER_OVERFLOW) {
-		ACPI_EXCEPTION((AE_INFO, status, "Extracting _BIF"));
-		result = -ENODEV;
+	result = acpi_battery_extract_package(battery, package, &format, data, "_BIF");
+	if (result)
 		goto end;
-	}
 
-	data.pointer = kzalloc(data.length, GFP_KERNEL);
-	if (!data.pointer) {
-		result = -ENOMEM;
-		goto end;
-	}
+      end:
 
-	status = acpi_extract_package(package, &format, &data);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Extracting _BIF"));
-		kfree(data.pointer);
-		result = -ENODEV;
-		goto end;
+	if (buffer.pointer) {
+		kfree(buffer.pointer);
 	}
 
-      end:
-	kfree(buffer.pointer);
-
-	if (!result)
-		(*bif) = data.pointer;
+	if (!result) {
+		bif = data->pointer;
+		battery->flags.power_unit = bif->power_unit;
+	}
 
 	return result;
 }
 
-static int
-acpi_battery_get_state(struct acpi_battery *battery,
-			struct acpi_battery_state **bst)
+static int acpi_battery_get_state(struct acpi_battery *battery)
 {
 	int result = 0;
 	acpi_status status = 0;
@@ -192,16 +279,17 @@ acpi_battery_get_state(struct acpi_battery *battery,
 	struct acpi_buffer format = { sizeof(ACPI_BATTERY_FORMAT_BST),
 		ACPI_BATTERY_FORMAT_BST
 	};
-	struct acpi_buffer data = { 0, NULL };
 	union acpi_object *package = NULL;
+	struct acpi_buffer *data = NULL;
 
+	battery->state_update_time = get_seconds();
 
-	if (!battery || !bst)
-		return -EINVAL;
+	if (!acpi_battery_present(battery))
+		return 0;
 
 	/* Evalute _BST */
 
-	status = acpi_evaluate_object(battery->device->handle, "_BST", NULL, &buffer);
+	status = acpi_evaluate_object(acpi_battery_handle(battery), "_BST", NULL, &buffer);
 	if (ACPI_FAILURE(status)) {
 		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BST"));
 		return -ENODEV;
@@ -209,55 +297,46 @@ acpi_battery_get_state(struct acpi_battery *battery,
 
 	package = buffer.pointer;
 
-	/* Extract Package Data */
+	data = &battery->bst_data;
 
-	status = acpi_extract_package(package, &format, &data);
-	if (status != AE_BUFFER_OVERFLOW) {
-		ACPI_EXCEPTION((AE_INFO, status, "Extracting _BST"));
-		result = -ENODEV;
-		goto end;
-	}
+	/* Extract Package Data */
 
-	data.pointer = kzalloc(data.length, GFP_KERNEL);
-	if (!data.pointer) {
-		result = -ENOMEM;
+	result = acpi_battery_extract_package(battery, package, &format, data, "_BST");
+	if (result)
 		goto end;
-	}
 
-	status = acpi_extract_package(package, &format, &data);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status, "Extracting _BST"));
-		kfree(data.pointer);
-		result = -ENODEV;
-		goto end;
+      end:
+	if (buffer.pointer) {
+		kfree(buffer.pointer);
 	}
 
-      end:
-	kfree(buffer.pointer);
+	return result;
+}
 
-	if (!result)
-		(*bst) = data.pointer;
+static int acpi_battery_get_alarm(struct acpi_battery *battery)
+{
+	battery->alarm_update_time = get_seconds();
 
-	return result;
+	return 0;
 }
 
-static int
-acpi_battery_set_alarm(struct acpi_battery *battery, unsigned long alarm)
+static int acpi_battery_set_alarm(struct acpi_battery *battery, unsigned long alarm)
 {
 	acpi_status status = 0;
 	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
 	struct acpi_object_list arg_list = { 1, &arg0 };
 
+	battery->alarm_update_time = get_seconds();
 
-	if (!battery)
-		return -EINVAL;
+	if (!acpi_battery_present(battery))
+		return -ENODEV;
 
-	if (!battery->flags.alarm)
+	if (!acpi_battery_alarm_present(battery))
 		return -ENODEV;
 
 	arg0.integer.value = alarm;
 
-	status = acpi_evaluate_object(battery->device->handle, "_BTP", &arg_list, NULL);
+	status = acpi_evaluate_object(acpi_battery_handle(battery), "_BTP", &arg_list, NULL);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
@@ -268,65 +347,110 @@ acpi_battery_set_alarm(struct acpi_battery *battery, unsigned long alarm)
 	return 0;
 }
 
-static int acpi_battery_check(struct acpi_battery *battery)
+static int acpi_battery_init_alarm(struct acpi_battery *battery)
 {
 	int result = 0;
 	acpi_status status = AE_OK;
 	acpi_handle handle = NULL;
-	struct acpi_device *device = NULL;
-	struct acpi_battery_info *bif = NULL;
-
+	struct acpi_battery_info *bif = battery->bif_data.pointer;
+	unsigned long alarm = battery->alarm;
 
-	if (!battery)
-		return -EINVAL;
+	/* See if alarms are supported, and if so, set default */
 
-	device = battery->device;
+	status = acpi_get_handle(acpi_battery_handle(battery), "_BTP", &handle);
+	if (ACPI_SUCCESS(status)) {
+		acpi_battery_alarm_present(battery) = 1;
+		if (!alarm && bif) {
+			alarm = bif->design_capacity_warning;
+		}
+		result = acpi_battery_set_alarm(battery, alarm);
+		if (result)
+			goto end;
+	} else {
+		acpi_battery_alarm_present(battery) = 0;
+	}
 
-	result = acpi_bus_get_status(device);
-	if (result)
-		return result;
+      end:
 
-	/* Insertion? */
+	return result;
+}
 
-	if (!battery->flags.present && device->status.battery_present) {
+static int acpi_battery_init_update(struct acpi_battery *battery)
+{
+	int result = 0;
 
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Battery inserted\n"));
+	result = acpi_battery_get_status(battery);
+	if (result)
+		return result;
 
-		/* Evalute _BIF to get certain static information */
+	acpi_battery_present_prev(battery) = acpi_battery_present(battery);
 
-		result = acpi_battery_get_info(battery, &bif);
+	if (acpi_battery_present(battery)) {
+		result = acpi_battery_get_info(battery);
+		if (result)
+			return result;
+		result = acpi_battery_get_state(battery);
 		if (result)
 			return result;
 
-		battery->flags.power_unit = bif->power_unit;
-		battery->trips.warning = bif->design_capacity_warning;
-		battery->trips.low = bif->design_capacity_low;
-		kfree(bif);
+		acpi_battery_init_alarm(battery);
+	}
+
+	return result;
+}
+
+static int acpi_battery_update(struct acpi_battery *battery,
+			   int update, int *update_result_ptr)
+{
+	int result = 0;
+	int update_result = ACPI_BATTERY_NONE_UPDATE;
 
-		/* See if alarms are supported, and if so, set default */
+	if (!acpi_battery_present(battery)) {
+		update = 1;
+	}
 
-		status = acpi_get_handle(battery->device->handle, "_BTP", &handle);
-		if (ACPI_SUCCESS(status)) {
-			battery->flags.alarm = 1;
-			acpi_battery_set_alarm(battery, battery->trips.warning);
+	if (acpi_battery_init_update_flag(battery)) {
+		result = acpi_battery_init_update(battery);
+		if (result)
+			goto end;;
+		update_result = ACPI_BATTERY_INIT_UPDATE;
+	} else if (update) {
+		result = acpi_battery_get_status(battery);
+		if (result)
+			goto end;;
+		if (acpi_battery_inserted(battery) || acpi_battery_removed(battery)) {
+			result = acpi_battery_init_update(battery);
+			if (result)
+				goto end;;
+			update_result = ACPI_BATTERY_INIT_UPDATE;
+		} else {
+			update_result = ACPI_BATTERY_EASY_UPDATE;
 		}
 	}
 
-	/* Removal? */
+      end:
 
-	else if (battery->flags.present && !device->status.battery_present) {
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Battery removed\n"));
-	}
+	acpi_battery_init_update_flag(battery) = (result != 0);
 
-	battery->flags.present = device->status.battery_present;
+	*update_result_ptr = update_result;
 
 	return result;
 }
 
-static void acpi_battery_check_present(struct acpi_battery *battery)
+static void acpi_battery_notify_update(struct acpi_battery *battery)
 {
-	if (!battery->flags.present) {
-		acpi_battery_check(battery);
+	acpi_battery_get_status(battery);
+
+	if (acpi_battery_init_update_flag(battery)) {
+		return;
+	}
+
+	if (acpi_battery_inserted(battery) || acpi_battery_removed(battery)) {
+		acpi_battery_init_update_flag(battery) = 1;
+	} else {
+		acpi_battery_info_update_flag(battery) = 1;
+		acpi_battery_state_update_flag(battery) = 1;
+		acpi_battery_alarm_update_flag(battery) = 1;
 	}
 }
 
@@ -335,37 +459,33 @@ static void acpi_battery_check_present(struct acpi_battery *battery)
    -------------------------------------------------------------------------- */
 
 static struct proc_dir_entry *acpi_battery_dir;
-static int acpi_battery_read_info(struct seq_file *seq, void *offset)
+
+static int acpi_battery_read_info_print(struct seq_file *seq, int result)
 {
-	int result = 0;
 	struct acpi_battery *battery = seq->private;
 	struct acpi_battery_info *bif = NULL;
 	char *units = "?";
 
-
-	if (!battery)
+	if (result)
 		goto end;
 
-	acpi_battery_check_present(battery);
-
-	if (battery->flags.present)
+	if (acpi_battery_present(battery))
 		seq_printf(seq, "present:                 yes\n");
 	else {
 		seq_printf(seq, "present:                 no\n");
 		goto end;
 	}
 
-	/* Battery Info (_BIF) */
-
-	result = acpi_battery_get_info(battery, &bif);
-	if (result || !bif) {
-		seq_printf(seq, "ERROR: Unable to read battery information\n");
+	bif = battery->bif_data.pointer;
+	if (!bif) {
+		ACPI_EXCEPTION((AE_INFO, AE_ERROR, "BIF buffer is NULL"));
+		result = -ENODEV;
 		goto end;
 	}
 
-	units =
-	    bif->
-	    power_unit ? ACPI_BATTERY_UNITS_AMPS : ACPI_BATTERY_UNITS_WATTS;
+	/* Battery Units */
+
+	units = acpi_battery_power_units(battery);
 
 	if (bif->design_capacity == ACPI_BATTERY_VALUE_UNKNOWN)
 		seq_printf(seq, "design capacity:         unknown\n");
@@ -396,7 +516,6 @@ static int acpi_battery_read_info(struct seq_file *seq, void *offset)
 	else
 		seq_printf(seq, "design voltage:          %d mV\n",
 			   (u32) bif->design_voltage);
-
 	seq_printf(seq, "design capacity warning: %d %sh\n",
 		   (u32) bif->design_capacity_warning, units);
 	seq_printf(seq, "design capacity low:     %d %sh\n",
@@ -411,9 +530,48 @@ static int acpi_battery_read_info(struct seq_file *seq, void *offset)
 	seq_printf(seq, "OEM info:                %s\n", bif->oem_info);
 
       end:
-	kfree(bif);
 
-	return 0;
+	if (result)
+		seq_printf(seq, "ERROR: Unable to read battery info\n");
+
+	return result;
+}
+
+static int acpi_battery_read_info(struct seq_file *seq, void *offset)
+{
+	struct acpi_battery *battery = seq->private;
+	int result = 0;
+	int update_result = ACPI_BATTERY_NONE_UPDATE;
+	int update = 0;
+
+	acpi_battery_mutex_lock(battery);
+
+	update = (get_seconds() - battery->info_update_time >= update_time);
+	update = (update | acpi_battery_info_update_flag(battery));
+
+	result = acpi_battery_update(battery, update, &update_result);
+	if (result)
+		goto end;
+
+	/* Battery Info (_BIF) */
+
+	if (update_result == ACPI_BATTERY_EASY_UPDATE) {
+		result = acpi_battery_get_info(battery);
+		if (result)
+			goto end;
+	}
+
+      end:
+
+	result = acpi_battery_read_info_print(seq, result);
+
+	acpi_battery_check_result(battery, result);
+
+	acpi_battery_info_update_flag(battery) = result;
+
+	acpi_battery_mutex_unlock(battery);
+
+	return result;
 }
 
 static int acpi_battery_info_open_fs(struct inode *inode, struct file *file)
@@ -421,40 +579,33 @@ static int acpi_battery_info_open_fs(struct inode *inode, struct file *file)
 	return single_open(file, acpi_battery_read_info, PDE(inode)->data);
 }
 
-static int acpi_battery_read_state(struct seq_file *seq, void *offset)
+static int acpi_battery_read_state_print(struct seq_file *seq, int result)
 {
-	int result = 0;
 	struct acpi_battery *battery = seq->private;
 	struct acpi_battery_state *bst = NULL;
 	char *units = "?";
 
-
-	if (!battery)
+	if (result)
 		goto end;
 
-	acpi_battery_check_present(battery);
-
-	if (battery->flags.present)
+	if (acpi_battery_present(battery))
 		seq_printf(seq, "present:                 yes\n");
 	else {
 		seq_printf(seq, "present:                 no\n");
 		goto end;
 	}
 
-	/* Battery Units */
-
-	units =
-	    battery->flags.
-	    power_unit ? ACPI_BATTERY_UNITS_AMPS : ACPI_BATTERY_UNITS_WATTS;
-
-	/* Battery Status (_BST) */
-
-	result = acpi_battery_get_state(battery, &bst);
-	if (result || !bst) {
-		seq_printf(seq, "ERROR: Unable to read battery state\n");
+	bst = battery->bst_data.pointer;
+	if (!bst) {
+		ACPI_EXCEPTION((AE_INFO, AE_ERROR, "BST buffer is NULL"));
+		result = -ENODEV;
 		goto end;
 	}
 
+	/* Battery Units */
+
+	units = acpi_battery_power_units(battery);
+
 	if (!(bst->state & 0x04))
 		seq_printf(seq, "capacity state:          ok\n");
 	else
@@ -490,9 +641,49 @@ static int acpi_battery_read_state(struct seq_file *seq, void *offset)
 			   (u32) bst->present_voltage);
 
       end:
-	kfree(bst);
 
-	return 0;
+	if (result) {
+		seq_printf(seq, "ERROR: Unable to read battery state\n");
+	}
+
+	return result;
+}
+
+static int acpi_battery_read_state(struct seq_file *seq, void *offset)
+{
+	struct acpi_battery *battery = seq->private;
+	int result = 0;
+	int update_result = ACPI_BATTERY_NONE_UPDATE;
+	int update = 0;
+
+	acpi_battery_mutex_lock(battery);
+
+	update = (get_seconds() - battery->state_update_time >= update_time);
+	update = (update | acpi_battery_state_update_flag(battery));
+
+	result = acpi_battery_update(battery, update, &update_result);
+	if (result)
+		goto end;
+
+	/* Battery State (_BST) */
+
+	if (update_result == ACPI_BATTERY_EASY_UPDATE) {
+		result = acpi_battery_get_state(battery);
+		if (result)
+			goto end;
+	}
+
+      end:
+
+	result = acpi_battery_read_state_print(seq, result);
+
+	acpi_battery_check_result(battery, result);
+
+	acpi_battery_state_update_flag(battery) = result;
+
+	acpi_battery_mutex_unlock(battery);
+
+	return result;
 }
 
 static int acpi_battery_state_open_fs(struct inode *inode, struct file *file)
@@ -500,38 +691,72 @@ static int acpi_battery_state_open_fs(struct inode *inode, struct file *file)
 	return single_open(file, acpi_battery_read_state, PDE(inode)->data);
 }
 
-static int acpi_battery_read_alarm(struct seq_file *seq, void *offset)
+static int acpi_battery_read_alarm_print(struct seq_file *seq, int result)
 {
 	struct acpi_battery *battery = seq->private;
 	char *units = "?";
 
-
-	if (!battery)
+	if (result)
 		goto end;
 
-	acpi_battery_check_present(battery);
-
-	if (!battery->flags.present) {
+	if (!acpi_battery_present(battery)) {
 		seq_printf(seq, "present:                 no\n");
 		goto end;
 	}
 
 	/* Battery Units */
 
-	units =
-	    battery->flags.
-	    power_unit ? ACPI_BATTERY_UNITS_AMPS : ACPI_BATTERY_UNITS_WATTS;
-
-	/* Battery Alarm */
+	units = acpi_battery_power_units(battery);
 
 	seq_printf(seq, "alarm:                   ");
 	if (!battery->alarm)
 		seq_printf(seq, "unsupported\n");
 	else
-		seq_printf(seq, "%d %sh\n", (u32) battery->alarm, units);
+		seq_printf(seq, "%lu %sh\n", battery->alarm, units);
 
       end:
-	return 0;
+
+	if (result)
+		seq_printf(seq, "ERROR: Unable to read battery alarm\n");
+
+	return result;
+}
+
+static int acpi_battery_read_alarm(struct seq_file *seq, void *offset)
+{
+	struct acpi_battery *battery = seq->private;
+	int result = 0;
+	int update_result = ACPI_BATTERY_NONE_UPDATE;
+	int update = 0;
+
+	acpi_battery_mutex_lock(battery);
+
+	update = (get_seconds() - battery->alarm_update_time >= update_time);
+	update = (update | acpi_battery_alarm_update_flag(battery));
+
+	result = acpi_battery_update(battery, update, &update_result);
+	if (result)
+		goto end;
+
+	/* Battery Alarm */
+
+	if (update_result == ACPI_BATTERY_EASY_UPDATE) {
+		result = acpi_battery_get_alarm(battery);
+		if (result)
+			goto end;
+	}
+
+      end:
+
+	result = acpi_battery_read_alarm_print(seq, result);
+
+	acpi_battery_check_result(battery, result);
+
+	acpi_battery_alarm_update_flag(battery) = result;
+
+	acpi_battery_mutex_unlock(battery);
+
+	return result;
 }
 
 static ssize_t
@@ -543,27 +768,47 @@ acpi_battery_write_alarm(struct file *file,
 	char alarm_string[12] = { '\0' };
 	struct seq_file *m = file->private_data;
 	struct acpi_battery *battery = m->private;
+	int update_result = ACPI_BATTERY_NONE_UPDATE;
 
 
 	if (!battery || (count > sizeof(alarm_string) - 1))
 		return -EINVAL;
 
-	acpi_battery_check_present(battery);
+	acpi_battery_mutex_lock(battery);
 
-	if (!battery->flags.present)
-		return -ENODEV;
+	result = acpi_battery_update(battery, 1, &update_result);
+	if (result) {
+		result = -ENODEV;
+		goto end;
+	}
 
-	if (copy_from_user(alarm_string, buffer, count))
-		return -EFAULT;
+	if (!acpi_battery_present(battery)) {
+		result = -ENODEV;
+		goto end;
+	}
+
+	if (copy_from_user(alarm_string, buffer, count)) {
+		result = -EFAULT;
+		goto end;
+	}
 
 	alarm_string[count] = '\0';
 
 	result = acpi_battery_set_alarm(battery,
 					simple_strtoul(alarm_string, NULL, 0));
 	if (result)
-		return result;
+		goto end;
+
+      end:
+
+	acpi_battery_check_result(battery, result);
 
-	return count;
+	if (!result)
+		result = count;
+
+	acpi_battery_mutex_unlock(battery);
+
+	return result;
 }
 
 static int acpi_battery_alarm_open_fs(struct inode *inode, struct file *file)
@@ -648,7 +893,6 @@ static int acpi_battery_add_fs(struct acpi_device *device)
 
 static int acpi_battery_remove_fs(struct acpi_device *device)
 {
-
 	if (acpi_device_dir(device)) {
 		remove_proc_entry(ACPI_BATTERY_FILE_ALARM,
 				  acpi_device_dir(device));
@@ -684,8 +928,11 @@ static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
 	case ACPI_BATTERY_NOTIFY_INFO:
 	case ACPI_NOTIFY_BUS_CHECK:
 	case ACPI_NOTIFY_DEVICE_CHECK:
-		acpi_battery_check(battery);
-		acpi_bus_generate_event(device, event, battery->flags.present);
+		acpi_battery_mutex_lock(battery);
+		device = battery->device;
+		acpi_battery_notify_update(battery);
+		acpi_battery_mutex_unlock(battery);
+		acpi_bus_generate_event(device, event, acpi_battery_present(battery));
 		break;
 	default:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
@@ -702,7 +949,6 @@ static int acpi_battery_add(struct acpi_device *device)
 	acpi_status status = 0;
 	struct acpi_battery *battery = NULL;
 
-
 	if (!device)
 		return -EINVAL;
 
@@ -710,15 +956,21 @@ static int acpi_battery_add(struct acpi_device *device)
 	if (!battery)
 		return -ENOMEM;
 
+	mutex_init(&battery->mutex);
+
+	acpi_battery_mutex_lock(battery);
+
 	battery->device = device;
 	strcpy(acpi_device_name(device), ACPI_BATTERY_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_BATTERY_CLASS);
 	acpi_driver_data(device) = battery;
 
-	result = acpi_battery_check(battery);
+	result = acpi_battery_get_status(battery);
 	if (result)
 		goto end;
 
+	acpi_battery_init_update_flag(battery) = 1;
+
 	result = acpi_battery_add_fs(device);
 	if (result)
 		goto end;
@@ -727,6 +979,7 @@ static int acpi_battery_add(struct acpi_device *device)
 					     ACPI_ALL_NOTIFY,
 					     acpi_battery_notify, battery);
 	if (ACPI_FAILURE(status)) {
+		ACPI_EXCEPTION((AE_INFO, status, "Installing notify handler"));
 		result = -ENODEV;
 		goto end;
 	}
@@ -736,11 +989,14 @@ static int acpi_battery_add(struct acpi_device *device)
 	       device->status.battery_present ? "present" : "absent");
 
       end:
+
 	if (result) {
 		acpi_battery_remove_fs(device);
 		kfree(battery);
 	}
 
+	acpi_battery_mutex_unlock(battery);
+
 	return result;
 }
 
@@ -749,18 +1005,29 @@ static int acpi_battery_remove(struct acpi_device *device, int type)
 	acpi_status status = 0;
 	struct acpi_battery *battery = NULL;
 
-
 	if (!device || !acpi_driver_data(device))
 		return -EINVAL;
 
 	battery = acpi_driver_data(device);
 
+	acpi_battery_mutex_lock(battery);
+
 	status = acpi_remove_notify_handler(device->handle,
 					    ACPI_ALL_NOTIFY,
 					    acpi_battery_notify);
 
 	acpi_battery_remove_fs(device);
 
+	if (battery->bif_data.pointer)
+		kfree(battery->bif_data.pointer);
+
+	if (battery->bst_data.pointer)
+		kfree(battery->bst_data.pointer);
+
+	acpi_battery_mutex_unlock(battery);
+
+	mutex_destroy(&battery->mutex);
+
 	kfree(battery);
 
 	return 0;
@@ -775,7 +1042,10 @@ static int acpi_battery_resume(struct acpi_device *device)
 		return -EINVAL;
 
 	battery = device->driver_data;
-	return acpi_battery_check(battery);
+
+	acpi_battery_init_update_flag(battery) = 1;
+
+	return 0;
 }
 
 static int __init acpi_battery_init(void)
@@ -800,7 +1070,6 @@ static int __init acpi_battery_init(void)
 
 static void __exit acpi_battery_exit(void)
 {
-
 	acpi_bus_unregister_driver(&acpi_battery_driver);
 
 	acpi_unlock_battery_dir(acpi_battery_dir);
Signature · 1 line
1.5.0.3.310.g05ef5
#11 lore raw

Re: [PATCH 05/22] ACPI: Cache battery status instead of re-evaluating AML

Quoted 24 lines · On Fri, 9 Mar 2007 23:00:42 -0500 Len Brown <[email protected]> wrote:

From: Vladimir Lebedev <[email protected]>

/proc exports _BST in a single file, and _BST is re-evaulated whenever that file is read.

Sometimes user-space reads this file frequently, and on some systems _BST takes a long time to evaluate due to a slow EC.

Further, when we move to sysfs, the values returned from _BST will be in multiple files, and evaluating _BST for each file read would make matters worse.

Here code is added to support caching the results of _BST. A new module parameter "update_time" tells how many seconds the cached _BST should be used before it is re-evaluated. Currently, update_time defaults to 0, and so the existing behaviour of re-evaluating on each read retained.

...

drivers/acpi/battery.c +4 -0
+#define acpi_battery_present(battery) battery->device->status.battery_present
+#define acpi_battery_present_prev(battery) battery->flags.battery_present_prev
+#define acpi_battery_alarm_present(battery) battery->flags.alarm_present
+#define acpi_battery_init_update_flag(battery) battery->flags.init_update

drivers/acpi/battery.c struct acpi_battery_info { · line 149

Quoted 10 lines
drivers/acpi/battery.c +10 -0
+#define acpi_battery_info_update_flag(battery) battery->flags.info_update
+#define acpi_battery_state_update_flag(battery) battery->flags.state_update
+#define acpi_battery_alarm_update_flag(battery) battery->flags.alarm_update
+#define acpi_battery_power_units(battery) battery->flags.power_unit ? \
+		ACPI_BATTERY_UNITS_AMPS : ACPI_BATTERY_UNITS_WATTS
+#define acpi_battery_handle(battery) battery->device->handle
+#define acpi_battery_inserted(battery) (!acpi_battery_present_prev(battery) & acpi_battery_present(battery))
+#define acpi_battery_removed(battery) (acpi_battery_present_prev(battery) & !acpi_battery_present(battery))
+#define acpi_battery_bid(battery) acpi_device_bid(battery->device)
+#define acpi_battery_status_str(battery) acpi_battery_present(battery) ? "present" : "absent"

These macros are

a) insufficiently parenthesised: try evaluating

	acpi_battery_present(battery + 1);

b) ugly. It's a real bad sign when we see

	acpi_battery_init_update_flag(battery) = 1;

which simply isn't C.

If we really insist on this obfuscation layer then it would be much better to do

static inline int acpi_battery_init_update_flag(struct acpi_battery *battery)

{
	return battery->flags.init_update;
}

static inline void
acpi_battery_set_init_update_flag(struct acpi_battery *battery, int value)

{
	battery->flags.init_update = value;
}

drivers/acpi/battery.c struct acpi_battery_info { · line 163

Quoted 9 lines
drivers/acpi/battery.c +9 -0
+static void acpi_battery_mutex_lock(struct acpi_battery *battery)
+{
+	mutex_lock(&battery->mutex);
+}
+
+static void acpi_battery_mutex_unlock(struct acpi_battery *battery)
+{
+	mutex_unlock(&battery->mutex);
+}

These wrappers don't add any value and should be removed.

drivers/acpi/battery.c struct acpi_battery_info { · line 173

Quoted 9 lines
drivers/acpi/battery.c +9 -0
+static void acpi_battery_check_result(struct acpi_battery *battery, int result)
+{
+	if (!battery)
+		return;
+
+	if (result) {
+		acpi_battery_init_update_flag(battery) = 1;
+	}
+}

We prefer not to have braces around a single statement like this.

Quoted 10 lines
drivers/acpi/battery.c +10 -0
+static int acpi_battery_extract_package(struct acpi_battery *battery,
+					union acpi_object *package,
+					struct acpi_buffer *format,
+					struct acpi_buffer *data,
+					char *package_name)
+{
+	acpi_status status = AE_OK;
+	struct acpi_buffer data_null = { 0, NULL };
+
+	status = acpi_extract_package(package, format, &data_null);

drivers/acpi/battery.c struct acpi_battery_info { · line 194

Quoted 10 lines
drivers/acpi/battery.c +10 -0
+	if (status != AE_BUFFER_OVERFLOW) {
+		ACPI_EXCEPTION((AE_INFO, status, "Extracting size %s",
+				package_name));
+		return -ENODEV;
+	}
+
+	if (data_null.length != data->length) {
+		if (data->pointer) {
+			kfree(data->pointer);
+		}

kfree(NULL) is legal and preferred kernel style is to not duplicate the test fro NULL in the kfree() caller like this (there are multiple instances of this in this patch).

Quoted 47 lines
(quoted diff) +44 -0
+		data->pointer = kzalloc(data_null.length, GFP_KERNEL);
+		if (!data->pointer) {
+			ACPI_EXCEPTION((AE_INFO, AE_NO_MEMORY, "kzalloc()"));
+			return -ENOMEM;
+		}
+		data->length = data_null.length;
+	}
+
+	status = acpi_extract_package(package, format, data);
+	if (ACPI_FAILURE(status)) {
+		ACPI_EXCEPTION((AE_INFO, status, "Extracting %s",
+				package_name));
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 
...
 
+static int acpi_battery_read_info(struct seq_file *seq, void *offset)
+{
+	struct acpi_battery *battery = seq->private;
+	int result = 0;
+	int update_result = ACPI_BATTERY_NONE_UPDATE;
+	int update = 0;
+
+	acpi_battery_mutex_lock(battery);
+
+	update = (get_seconds() - battery->info_update_time >= update_time);
+	update = (update | acpi_battery_info_update_flag(battery));
+
+	result = acpi_battery_update(battery, update, &update_result);
+	if (result)
+		goto end;
+
+	/* Battery Info (_BIF) */
+
+	if (update_result == ACPI_BATTERY_EASY_UPDATE) {
+		result = acpi_battery_get_info(battery);
+		if (result)
+			goto end;
+	}
+
+      end:
+

Review

Quoted 10 lines
(quoted diff) +9 -0
+	result = acpi_battery_read_info_print(seq, result);
+
+	acpi_battery_check_result(battery, result);
+
+	acpi_battery_info_update_flag(battery) = result;
+
+	acpi_battery_mutex_unlock(battery);
+
+	return result;
 }

We have several copies of this function, each with tiny variations:

Quoted 26 lines
drivers/acpi/battery.c +26 -0
+static int acpi_battery_read_state(struct seq_file *seq, void *offset)
+{
+	struct acpi_battery *battery = seq->private;
+	int result = 0;
+	int update_result = ACPI_BATTERY_NONE_UPDATE;
+	int update = 0;
+
+	acpi_battery_mutex_lock(battery);
+
+	update = (get_seconds() - battery->state_update_time >= update_time);
+	update = (update | acpi_battery_state_update_flag(battery));
+
+	result = acpi_battery_update(battery, update, &update_result);
+	if (result)
+		goto end;
+
+	/* Battery State (_BST) */
+
+	if (update_result == ACPI_BATTERY_EASY_UPDATE) {
+		result = acpi_battery_get_state(battery);
+		if (result)
+			goto end;
+	}
+
+      end:
+

drivers/acpi/battery.c static int acpi_battery_read_state(struct seq_file *seq, void *offset) · line 687

Quoted 10 lines
drivers/acpi/battery.c +9 -0
+	result = acpi_battery_read_state_print(seq, result);
+
+	acpi_battery_check_result(battery, result);
+
+	acpi_battery_state_update_flag(battery) = result;
+
+	acpi_battery_mutex_unlock(battery);
+
+	return result;
 }

Can we not find a way of consolidating all this duplication?

Quoted 26 lines
drivers/acpi/battery.c +26 -0
+static int acpi_battery_read_alarm(struct seq_file *seq, void *offset)
+{
+	struct acpi_battery *battery = seq->private;
+	int result = 0;
+	int update_result = ACPI_BATTERY_NONE_UPDATE;
+	int update = 0;
+
+	acpi_battery_mutex_lock(battery);
+
+	update = (get_seconds() - battery->alarm_update_time >= update_time);
+	update = (update | acpi_battery_alarm_update_flag(battery));
+
+	result = acpi_battery_update(battery, update, &update_result);
+	if (result)
+		goto end;
+
+	/* Battery Alarm */
+
+	if (update_result == ACPI_BATTERY_EASY_UPDATE) {
+		result = acpi_battery_get_alarm(battery);
+		if (result)
+			goto end;
+	}
+
+      end:
+

drivers/acpi/battery.c static int acpi_battery_state_open_fs(struct inode *inode, struct file *file) · line 760

Quoted 10 lines
drivers/acpi/battery.c +9 -0
+	result = acpi_battery_read_alarm_print(seq, result);
+
+	acpi_battery_check_result(battery, result);
+
+	acpi_battery_alarm_update_flag(battery) = result;
+
+	acpi_battery_mutex_unlock(battery);
+
+	return result;
 }

More here.

author reviewer
#12 lore raw

PATCH 6/22 ACPI: battery: Lindent

In reply to Len Brown

From: Vladimir Lebedev <[email protected]>

---
 drivers/acpi/battery.c |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)
drivers/acpi/battery.c +23 -13
index f3b0024..fc9c50a 100644
@@ -123,7 +123,7 @@ struct acpi_battery_flags {
 
 struct acpi_battery {
 	struct mutex mutex;
-	struct acpi_device * device;
+	struct acpi_device *device;
 	struct acpi_battery_flags flags;
 	struct acpi_buffer bif_data;
 	struct acpi_buffer bst_data;
@@ -241,7 +241,9 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
 
 	/* Evalute _BIF */
 
-	status = acpi_evaluate_object(acpi_battery_handle(battery), "_BIF", NULL, &buffer);
+	status =
+	    acpi_evaluate_object(acpi_battery_handle(battery), "_BIF", NULL,
+				 &buffer);
 	if (ACPI_FAILURE(status)) {
 		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BIF"));
 		return -ENODEV;
@@ -253,7 +255,9 @@ static int acpi_battery_get_info(struct acpi_battery *battery)
 
 	/* Extract Package Data */
 
-	result = acpi_battery_extract_package(battery, package, &format, data, "_BIF");
+	result =
+	    acpi_battery_extract_package(battery, package, &format, data,
+					 "_BIF");
 	if (result)
 		goto end;
 
@@ -289,7 +293,9 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
 
 	/* Evalute _BST */
 
-	status = acpi_evaluate_object(acpi_battery_handle(battery), "_BST", NULL, &buffer);
+	status =
+	    acpi_evaluate_object(acpi_battery_handle(battery), "_BST", NULL,
+				 &buffer);
 	if (ACPI_FAILURE(status)) {
 		ACPI_EXCEPTION((AE_INFO, status, "Evaluating _BST"));
 		return -ENODEV;
@@ -301,7 +307,9 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
 
 	/* Extract Package Data */
 
-	result = acpi_battery_extract_package(battery, package, &format, data, "_BST");
+	result =
+	    acpi_battery_extract_package(battery, package, &format, data,
+					 "_BST");
 	if (result)
 		goto end;
 
@@ -320,7 +328,8 @@ static int acpi_battery_get_alarm(struct acpi_battery *battery)
 	return 0;
 }
 
-static int acpi_battery_set_alarm(struct acpi_battery *battery, unsigned long alarm)
+static int acpi_battery_set_alarm(struct acpi_battery *battery,
+				  unsigned long alarm)
 {
 	acpi_status status = 0;
 	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
@@ -336,7 +345,9 @@ static int acpi_battery_set_alarm(struct acpi_battery *battery, unsigned long al
 
 	arg0.integer.value = alarm;
 
-	status = acpi_evaluate_object(acpi_battery_handle(battery), "_BTP", &arg_list, NULL);
+	status =
+	    acpi_evaluate_object(acpi_battery_handle(battery), "_BTP",
+				 &arg_list, NULL);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
@@ -400,7 +411,7 @@ static int acpi_battery_init_update(struct acpi_battery *battery)
 }
 
 static int acpi_battery_update(struct acpi_battery *battery,
-			   int update, int *update_result_ptr)
+			       int update, int *update_result_ptr)
 {
 	int result = 0;
 	int update_result = ACPI_BATTERY_NONE_UPDATE;
@@ -418,7 +429,8 @@ static int acpi_battery_update(struct acpi_battery *battery,
 		result = acpi_battery_get_status(battery);
 		if (result)
 			goto end;;
-		if (acpi_battery_inserted(battery) || acpi_battery_removed(battery)) {
+		if (acpi_battery_inserted(battery)
+		    || acpi_battery_removed(battery)) {
 			result = acpi_battery_init_update(battery);
 			if (result)
 				goto end;;
@@ -770,7 +782,6 @@ acpi_battery_write_alarm(struct file *file,
 	struct acpi_battery *battery = m->private;
 	int update_result = ACPI_BATTERY_NONE_UPDATE;
 
-
 	if (!battery || (count > sizeof(alarm_string) - 1))
 		return -EINVAL;
 
@@ -845,7 +856,6 @@ static int acpi_battery_add_fs(struct acpi_device *device)
 {
 	struct proc_dir_entry *entry = NULL;
 
-
 	if (!acpi_device_dir(device)) {
 		acpi_device_dir(device) = proc_mkdir(acpi_device_bid(device),
 						     acpi_battery_dir);
@@ -917,7 +927,6 @@ static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
 	struct acpi_battery *battery = data;
 	struct acpi_device *device = NULL;
 
-
 	if (!battery)
 		return;
 
@@ -932,7 +941,8 @@ static void acpi_battery_notify(acpi_handle handle, u32 event, void *data)
 		device = battery->device;
 		acpi_battery_notify_update(battery);
 		acpi_battery_mutex_unlock(battery);
-		acpi_bus_generate_event(device, event, acpi_battery_present(battery));
+		acpi_bus_generate_event(device, event,
+					acpi_battery_present(battery));
 		break;
 	default:
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#13 lore raw

PATCH 7/22 cpuidle: build fix for !CPU_IDLE

In reply to Len Brown

From: Venkatesh Pallipadi <[email protected]>

Fix the compile issues when CPU_IDLE is not configured.

---
 include/linux/cpuidle.h |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)
include/linux/cpuidle.h +23 -0
index d26a82e..4a41f42 100644
@@ -130,10 +130,21 @@ struct cpuidle_driver {
 	struct module 		*owner;
 };
 
+#ifdef CONFIG_CPU_IDLE
+
 extern int cpuidle_register_driver(struct cpuidle_driver *drv);
 extern void cpuidle_unregister_driver(struct cpuidle_driver *drv);
 extern int cpuidle_force_redetect(struct cpuidle_device *dev);
 
+#else
+
+static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
+{return 0;}
+static inline void cpuidle_unregister_driver(struct cpuidle_driver *drv) { }
+static inline int cpuidle_force_redetect(struct cpuidle_device *dev)
+{return 0;}
+
+#endif
 
 /******************************
  * CPUIDLE GOVERNOR INTERFACE *
@@ -153,8 +164,20 @@ struct cpuidle_governor {
 	struct module 		*owner;
 };
 
+#ifdef CONFIG_CPU_IDLE
+
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
 extern int cpuidle_get_bm_activity(void);
 
+#else
+
+static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
+{return 0;}
+static inline void cpuidle_unregister_governor(struct cpuidle_governor *gov) { }
+static inline int cpuidle_get_bm_activity(void)
+{return 0;}
+
+#endif
+
 #endif /* _LINUX_CPUIDLE_H */
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#14 lore raw

PATCH 8/22 cpu_idle: fix build break

In reply to Len Brown

From: Venkatesh Pallipadi <[email protected]>

This patch fixes a build breakage with !CONFIG_HOTPLUG_CPU and CONFIG_CPU_IDLE.

---
 drivers/cpuidle/cpuidle.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)
drivers/cpuidle/cpuidle.c +1 -5
index dc626d9..4d2071e 100644
@@ -189,10 +189,6 @@ static struct sysdev_driver cpuidle_sysdev_driver = {
 	.remove		= cpuidle_remove_device,
 };
 
-#ifdef CONFIG_SMP
-
-#ifdef CONFIG_HOTPLUG_CPU
-
 static int cpuidle_cpu_callback(struct notifier_block *nfb,
 					unsigned long action, void *hcpu)
 {
@@ -224,7 +220,7 @@ static struct notifier_block __cpuinitdata cpuidle_cpu_notifier =
     .notifier_call = cpuidle_cpu_callback,
 };
 
-#endif /* CONFIG_HOTPLUG_CPU */
+#ifdef CONFIG_SMP
 
 static void smp_callback(void *v)
 {
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#15 lore raw

PATCH 9/22 cpuidle: make code static

In reply to Len Brown

From: Adrian Bunk <[email protected]>

This patch makes the following needlessly global code static:
- driver.c: __cpuidle_find_driver()
- governor.c: __cpuidle_find_governor()
- ladder.c: struct ladder_governor

---
 drivers/cpuidle/cpuidle.h          |    2 --
 drivers/cpuidle/driver.c           |    2 +-
 drivers/cpuidle/governor.c         |    2 +-
 drivers/cpuidle/governors/ladder.c |    2 +-
 4 files changed, 3 insertions(+), 5 deletions(-)
drivers/cpuidle/cpuidle.h +0 -2
index 2defa39..8bbc090 100644
@@ -23,13 +23,11 @@ extern void cpuidle_rescan_device(struct cpuidle_device *dev);
 /* drivers */
 extern int cpuidle_attach_driver(struct cpuidle_device *dev);
 extern void cpuidle_detach_driver(struct cpuidle_device *dev);
-extern struct cpuidle_driver * __cpuidle_find_driver(const char *str);
 extern int cpuidle_switch_driver(struct cpuidle_driver *drv);
 
 /* governors */
 extern int cpuidle_attach_governor(struct cpuidle_device *dev);
 extern void cpuidle_detach_governor(struct cpuidle_device *dev);
-extern struct cpuidle_governor * __cpuidle_find_governor(const char *str);
 extern int cpuidle_switch_governor(struct cpuidle_governor *gov);
 
 /* sysfs */
drivers/cpuidle/driver.c +1 -1
index 1111279..de8c627 100644
@@ -73,7 +73,7 @@ void cpuidle_detach_driver(struct cpuidle_device *dev)
  *
  * Must be called with cpuidle_lock aquired.
  */
-struct cpuidle_driver * __cpuidle_find_driver(const char *str)
+static struct cpuidle_driver * __cpuidle_find_driver(const char *str)
 {
 	struct cpuidle_driver *drv;
 
drivers/cpuidle/governor.c +1 -1
index b2d6a69..fa637fa 100644
@@ -72,7 +72,7 @@ void cpuidle_detach_governor(struct cpuidle_device *dev)
  *
  * Must be called with cpuidle_lock aquired.
  */
-struct cpuidle_governor * __cpuidle_find_governor(const char *str)
+static struct cpuidle_governor * __cpuidle_find_governor(const char *str)
 {
 	struct cpuidle_governor *gov;
 
drivers/cpuidle/governors/ladder.c +1 -1
index d999393..87d2e54 100644
@@ -199,7 +199,7 @@ static void ladder_exit_device(struct cpuidle_device *dev)
 	kfree(dev->governor_data);
 }
 
-struct cpuidle_governor ladder_governor = {
+static struct cpuidle_governor ladder_governor = {
 	.name =		"ladder",
 	.init =		ladder_init_device,
 	.exit =		ladder_exit_device,
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#16 lore raw

PATCH 10/22 cpuidle: ladder does not depend on ACPI

In reply to Len Brown

From: Len Brown <[email protected]>

build fix for CONFIG_ACPI=n

In file included from drivers/cpuidle/governors/ladder.c:21:
include/acpi/processor.h:88: error: expected specifier-qualifier-list before ‘acpi_integer’ include/acpi/processor.h:106: error: expected specifier-qualifier-list before ‘acpi_integer’ include/acpi/processor.h:168: error: expected specifier-qualifier-list before ‘acpi_handle’

---
 drivers/cpuidle/governors/ladder.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)
drivers/cpuidle/governors/ladder.c +0 -2
index 87d2e54..cf5a7a7 100644
@@ -14,11 +14,9 @@
 
 #include <linux/kernel.h>
 #include <linux/cpuidle.h>
-#include <linux/acpi.h>
 #include <linux/latency.h>
 #include <linux/moduleparam.h>
 #include <linux/jiffies.h>
-#include <acpi/processor.h>
 
 #include <asm/io.h>
 #include <asm/uaccess.h>
Signature · 5 lines
1.5.0.3.310.g05ef5
-
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author reviewer
#17 lore raw

PATCH 11/22 ACPI: EC: Don't use Global Lock if not asked to do so

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/ec.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)
drivers/acpi/ec.c +0 -3
index 1e525dd..0e06ba8 100644
@@ -827,7 +827,6 @@ acpi_fake_ecdt_callback(acpi_handle handle,
 	status = acpi_evaluate_integer(handle, "_GPE", NULL, &ec_ecdt->gpe);
 	if (ACPI_FAILURE(status))
 		return status;
-	ec_ecdt->global_lock = TRUE;
 	ec_ecdt->handle = handle;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "GPE=0x%02lx, ports=0x%2lx, 0x%2lx",
@@ -901,8 +900,6 @@ static int __init acpi_ec_get_real_ecdt(void)
 	ec_ecdt->command_addr = ecdt_ptr->control.address;
 	ec_ecdt->data_addr = ecdt_ptr->data.address;
 	ec_ecdt->gpe = ecdt_ptr->gpe;
-	/* use the GL just to be safe */
-	ec_ecdt->global_lock = TRUE;
 	ec_ecdt->uid = ecdt_ptr->uid;
 
 	status = acpi_get_handle(NULL, ecdt_ptr->id, &ec_ecdt->handle);
Signature · 1 line
1.5.0.3.310.g05ef5
#18 lore raw

Re: [PATCH 11/22] ACPI: EC: Don't use Global Lock if not asked to do so

Does this patch obsolete the patch to "release global lock in acpi_ec_wait"? It's part of bugzilla 6749 "Thinkpad A21m freeze after second supedn to RAM"
<http://bugzilla.kernel.org/show_bug.cgi?id=6749> and its direct url is <http://bugzilla.kernel.org/attachment.cgi?id=8481&action=view>.

Or did that patch get integrated via some other flow of code? It fixed the S3 suspend hangs on my TP 600X. It might also help the T60:
Once in a long while it would hang on suspend and it reminded me of the 600X's behavior (though the T60 has an ancient kernel, 2.6.17-10 from ubuntu, which is why I never filed a report). I work around the problem by unloading the fan and thermal modules before suspending. It also hangs once in a while on resume, but those problems will hopefully be resolved by a more recent kernel.

-Sanjoy

`Not all those who wander are lost.' (J.R.R. Tolkien)

author reviewer
#19 lore raw

PATCH 12/22 ACPI: EC: Make EC to initialize first in ACPI

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/Makefile |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
drivers/acpi/Makefile +4 -3
index 5956e9f..9623aac 100644
@@ -1,6 +1,6 @@
 #
 # Makefile for the Linux ACPI interpreter
-# 
+#
 
 export ACPI_CFLAGS
 
@@ -32,16 +32,17 @@ obj-y				+= osl.o utils.o \
 processor-objs	+= processor_core.o processor_throttling.o \
 				processor_idle.o processor_thermal.o
 ifdef CONFIG_CPU_FREQ
-processor-objs	+= processor_perflib.o			
+processor-objs	+= processor_perflib.o
 endif
 
 obj-y				+= sleep/
 obj-y				+= bus.o glue.o
 obj-y				+= scan.o
+# Keep EC driver first. Initialization of others depend on it.
+obj-$(CONFIG_ACPI_EC)		+= ec.o
 obj-$(CONFIG_ACPI_AC) 		+= ac.o
 obj-$(CONFIG_ACPI_BATTERY)	+= battery.o
 obj-$(CONFIG_ACPI_BUTTON)	+= button.o
-obj-$(CONFIG_ACPI_EC)		+= ec.o
 obj-$(CONFIG_ACPI_FAN)		+= fan.o
 obj-$(CONFIG_ACPI_DOCK)		+= dock.o
 obj-$(CONFIG_ACPI_BAY)		+= bay.o
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#20 lore raw

PATCH 13/22 ACPI: EC: "Fake ECDT" workaround is not needed any longer.

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/ec.c |   81 -----------------------------------------------------
 1 files changed, 0 insertions(+), 81 deletions(-)
drivers/acpi/ec.c +0 -81
index 0e06ba8..5999abd 100644
@@ -805,74 +805,6 @@ static int acpi_ec_stop(struct acpi_device *device, int type)
 	return 0;
 }
 
-static acpi_status __init
-acpi_fake_ecdt_callback(acpi_handle handle,
-			u32 Level, void *context, void **retval)
-{
-	acpi_status status;
-
-	mutex_init(&ec_ecdt->lock);
-	atomic_set(&ec->event_count, 1);
-	if (acpi_ec_mode == EC_INTR) {
-		init_waitqueue_head(&ec_ecdt->wait);
-	}
-	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
-				     acpi_ec_io_ports, ec_ecdt);
-	if (ACPI_FAILURE(status))
-		return status;
-
-	ec_ecdt->uid = -1;
-	acpi_evaluate_integer(handle, "_UID", NULL, &ec_ecdt->uid);
-
-	status = acpi_evaluate_integer(handle, "_GPE", NULL, &ec_ecdt->gpe);
-	if (ACPI_FAILURE(status))
-		return status;
-	ec_ecdt->handle = handle;
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "GPE=0x%02lx, ports=0x%2lx, 0x%2lx",
-			  ec_ecdt->gpe, ec_ecdt->command_addr,
-			  ec_ecdt->data_addr));
-
-	return AE_CTRL_TERMINATE;
-}
-
-/*
- * Some BIOS (such as some from Gateway laptops) access EC region very early
- * such as in BAT0._INI or EC._INI before an EC device is found and
- * do not provide an ECDT. According to ACPI spec, ECDT isn't mandatorily
- * required, but if EC regison is accessed early, it is required.
- * The routine tries to workaround the BIOS bug by pre-scan EC device
- * It assumes that _CRS, _HID, _GPE, _UID methods of EC don't touch any
- * op region (since _REG isn't invoked yet). The assumption is true for
- * all systems found.
- */
-static int __init acpi_ec_fake_ecdt(void)
-{
-	acpi_status status;
-	int ret = 0;
-
-	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Try to make an fake ECDT"));
-
-	ec_ecdt = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
-	if (!ec_ecdt) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	status = acpi_get_devices(ACPI_EC_HID,
-				  acpi_fake_ecdt_callback, NULL, NULL);
-	if (ACPI_FAILURE(status)) {
-		kfree(ec_ecdt);
-		ec_ecdt = NULL;
-		ret = -ENODEV;
-		ACPI_EXCEPTION((AE_INFO, status, "Can't make an fake ECDT"));
-		goto error;
-	}
-	return 0;
-      error:
-	return ret;
-}
-
 static int __init acpi_ec_get_real_ecdt(void)
 {
 	acpi_status status;
@@ -916,18 +848,12 @@ static int __init acpi_ec_get_real_ecdt(void)
 	return -ENODEV;
 }
 
-static int __initdata acpi_fake_ecdt_enabled;
 int __init acpi_ec_ecdt_probe(void)
 {
 	acpi_status status;
 	int ret;
 
 	ret = acpi_ec_get_real_ecdt();
-	/* Try to make a fake ECDT */
-	if (ret && acpi_fake_ecdt_enabled) {
-		ret = acpi_ec_fake_ecdt();
-	}
-
 	if (ret)
 		return 0;
 
@@ -1000,13 +926,6 @@ static void __exit acpi_ec_exit(void)
 }
 #endif				/* 0 */
 
-static int __init acpi_fake_ecdt_setup(char *str)
-{
-	acpi_fake_ecdt_enabled = 1;
-	return 1;
-}
-
-__setup("acpi_fake_ecdt", acpi_fake_ecdt_setup);
 static int __init acpi_ec_set_intr_mode(char *str)
 {
 	int intr;
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#21 lore raw

PATCH 14/22 ACPI: EC: enable burst functionality in EC.

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/ec.c |   89 ++++++++++++++++++++++-------------------------------
 1 files changed, 37 insertions(+), 52 deletions(-)
drivers/acpi/ec.c +37 -52
index 5999abd..e7ce6e4 100644
@@ -101,7 +101,6 @@ static struct acpi_ec {
 	struct mutex lock;
 	atomic_t query_pending;
 	atomic_t event_count;
-	atomic_t leaving_burst;	/* 0 : No, 1 : Yes, 2: abort */
 	wait_queue_head_t wait;
 } *ec_ecdt;
 
@@ -173,56 +172,6 @@ static int acpi_ec_wait(struct acpi_ec *ec, enum ec_event event, unsigned count)
 	return -ETIME;
 }
 
-#ifdef ACPI_FUTURE_USAGE
-/*
- * Note: samsung nv5000 doesn't work with ec burst mode.
- * http://bugzilla.kernel.org/show_bug.cgi?id=4980
- */
-int acpi_ec_enter_burst_mode(struct acpi_ec *ec)
-{
-	u8 tmp = 0;
-	u8 status = 0;
-
-	status = acpi_ec_read_status(ec);
-	if (status != -EINVAL && !(status & ACPI_EC_FLAG_BURST)) {
-		status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0);
-		if (status)
-			goto end;
-		acpi_ec_write_cmd(ec, ACPI_EC_BURST_ENABLE);
-		status = acpi_ec_wait(ec, ACPI_EC_EVENT_OBF_1);
-		tmp = acpi_ec_read_data(ec);
-		if (tmp != 0x90) {	/* Burst ACK byte */
-			return -EINVAL;
-		}
-	}
-
-	atomic_set(&ec->leaving_burst, 0);
-	return 0;
-      end:
-	ACPI_EXCEPTION((AE_INFO, status, "EC wait, burst mode"));
-	return -1;
-}
-
-int acpi_ec_leave_burst_mode(struct acpi_ec *ec)
-{
-	u8 status = 0;
-
-	status = acpi_ec_read_status(ec);
-	if (status != -EINVAL && (status & ACPI_EC_FLAG_BURST)) {
-		status = acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0);
-		if (status)
-			goto end;
-		acpi_ec_write_cmd(ec, ACPI_EC_BURST_DISABLE);
-		acpi_ec_wait(ec, ACPI_EC_EVENT_IBF_0);
-	}
-	atomic_set(&ec->leaving_burst, 1);
-	return 0;
-      end:
-	ACPI_EXCEPTION((AE_INFO, status, "EC leave burst mode"));
-	return -1;
-}
-#endif				/* ACPI_FUTURE_USAGE */
-
 static int acpi_ec_transaction_unlocked(struct acpi_ec *ec, u8 command,
 					const u8 * wdata, unsigned wdata_len,
 					u8 * rdata, unsigned rdata_len)
@@ -312,6 +261,21 @@ static int acpi_ec_transaction(struct acpi_ec *ec, u8 command,
 	return status;
 }
 
+/*
+ * Note: samsung nv5000 doesn't work with ec burst mode.
+ * http://bugzilla.kernel.org/show_bug.cgi?id=4980
+ */
+int acpi_ec_burst_enable(struct acpi_ec *ec)
+{
+	u8 d;
+	return acpi_ec_transaction(ec, ACPI_EC_BURST_ENABLE, NULL, 0, &d, 1);
+}
+
+int acpi_ec_burst_disable(struct acpi_ec *ec)
+{
+	return acpi_ec_transaction(ec, ACPI_EC_BURST_DISABLE, NULL, 0, NULL, 0);
+}
+
 static int acpi_ec_read(struct acpi_ec *ec, u8 address, u8 * data)
 {
 	int result;
@@ -333,6 +297,28 @@ static int acpi_ec_write(struct acpi_ec *ec, u8 address, u8 data)
 /*
  * Externally callable EC access functions. For now, assume 1 EC only
  */
+int ec_burst_enable(void)
+{
+	struct acpi_ec *ec;
+	if (!first_ec)
+		return -ENODEV;
+	ec = acpi_driver_data(first_ec);
+	return acpi_ec_burst_enable(ec);
+}
+
+EXPORT_SYMBOL(ec_burst_enable);
+
+int ec_burst_disable(void)
+{
+	struct acpi_ec *ec;
+	if (!first_ec)
+		return -ENODEV;
+	ec = acpi_driver_data(first_ec);
+	return acpi_ec_burst_disable(ec);
+}
+
+EXPORT_SYMBOL(ec_burst_disable);
+
 int ec_read(u8 addr, u8 * val)
 {
 	struct acpi_ec *ec;
@@ -639,7 +625,6 @@ static int acpi_ec_add(struct acpi_device *device)
 	atomic_set(&ec->query_pending, 0);
 	atomic_set(&ec->event_count, 1);
 	if (acpi_ec_mode == EC_INTR) {
-		atomic_set(&ec->leaving_burst, 1);
 		init_waitqueue_head(&ec->wait);
 	}
 	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#22 lore raw

PATCH 15/22 ACPI: EC: Remove casts to/from void* from ec.c

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/ec.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)
drivers/acpi/ec.c +6 -7
index e7ce6e4..1989631 100644
@@ -406,7 +406,7 @@ static int acpi_ec_query(struct acpi_ec *ec, u8 * data)
 
 static void acpi_ec_gpe_query(void *ec_cxt)
 {
-	struct acpi_ec *ec = (struct acpi_ec *)ec_cxt;
+	struct acpi_ec *ec = ec_cxt;
 	u8 value = 0;
 	char object_name[8];
 
@@ -424,8 +424,9 @@ static u32 acpi_ec_gpe_handler(void *data)
 {
 	acpi_status status = AE_OK;
 	u8 value;
-	struct acpi_ec *ec = (struct acpi_ec *)data;
+	struct acpi_ec *ec = data;
 	atomic_inc(&ec->event_count);
+
 	if (acpi_ec_mode == EC_INTR) {
 		wake_up(&ec->wait);
 	}
@@ -468,7 +469,7 @@ acpi_ec_space_handler(u32 function,
 		      void *handler_context, void *region_context)
 {
 	int result = 0;
-	struct acpi_ec *ec = NULL;
+	struct acpi_ec *ec = handler_context;
 	u64 temp = *value;
 	acpi_integer f_v = 0;
 	int i = 0;
@@ -480,8 +481,6 @@ acpi_ec_space_handler(u32 function,
 		return AE_BAD_PARAMETER;
 	}
 
-	ec = (struct acpi_ec *)handler_context;
-
       next_byte:
 	switch (function) {
 	case ACPI_READ:
@@ -537,7 +536,7 @@ static struct proc_dir_entry *acpi_ec_dir;
 
 static int acpi_ec_read_info(struct seq_file *seq, void *offset)
 {
-	struct acpi_ec *ec = (struct acpi_ec *)seq->private;
+	struct acpi_ec *ec = seq->private;
 
 	if (!ec)
 		goto end;
@@ -694,7 +693,7 @@ static int acpi_ec_remove(struct acpi_device *device, int type)
 static acpi_status
 acpi_ec_io_ports(struct acpi_resource *resource, void *context)
 {
-	struct acpi_ec *ec = (struct acpi_ec *)context;
+	struct acpi_ec *ec = context;
 
 	if (resource->type != ACPI_RESOURCE_TYPE_IO) {
 		return AE_OK;
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#23 lore raw

PATCH 16/22 ACPI: EC: Put install handlers into separate function.

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/ec.c |   87 ++++++++++++++++------------------------------------
 1 files changed, 27 insertions(+), 60 deletions(-)
drivers/acpi/ec.c +27 -60
index 1989631..399cedf 100644
@@ -715,6 +715,28 @@ acpi_ec_io_ports(struct acpi_resource *resource, void *context)
 	return AE_OK;
 }
 
+static int ec_install_handlers(struct acpi_ec *ec)
+{
+	acpi_status status = acpi_install_gpe_handler(NULL, ec->gpe,
+						      ACPI_GPE_EDGE_TRIGGERED,
+						      &acpi_ec_gpe_handler, ec);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+	acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
+	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
+
+	status = acpi_install_address_space_handler(ec->handle,
+						    ACPI_ADR_SPACE_EC,
+						    &acpi_ec_space_handler,
+						    &acpi_ec_space_setup, ec);
+	if (ACPI_FAILURE(status)) {
+		acpi_remove_gpe_handler(NULL, ec->gpe, &acpi_ec_gpe_handler);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static int acpi_ec_start(struct acpi_device *device)
 {
 	acpi_status status = AE_OK;
@@ -742,28 +764,7 @@ static int acpi_ec_start(struct acpi_device *device)
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "gpe=0x%02lx, ports=0x%2lx,0x%2lx",
 			  ec->gpe, ec->command_addr, ec->data_addr));
 
-	/*
-	 * Install GPE handler
-	 */
-	status = acpi_install_gpe_handler(NULL, ec->gpe,
-					  ACPI_GPE_EDGE_TRIGGERED,
-					  &acpi_ec_gpe_handler, ec);
-	if (ACPI_FAILURE(status)) {
-		return -ENODEV;
-	}
-	acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
-	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
-
-	status = acpi_install_address_space_handler(ec->handle,
-						    ACPI_ADR_SPACE_EC,
-						    &acpi_ec_space_handler,
-						    &acpi_ec_space_setup, ec);
-	if (ACPI_FAILURE(status)) {
-		acpi_remove_gpe_handler(NULL, ec->gpe, &acpi_ec_gpe_handler);
-		return -ENODEV;
-	}
-
-	return AE_OK;
+	return ec_install_handlers(ec);
 }
 
 static int acpi_ec_stop(struct acpi_device *device, int type)
@@ -818,56 +819,22 @@ static int __init acpi_ec_get_real_ecdt(void)
 	ec_ecdt->gpe = ecdt_ptr->gpe;
 	ec_ecdt->uid = ecdt_ptr->uid;
 
-	status = acpi_get_handle(NULL, ecdt_ptr->id, &ec_ecdt->handle);
-	if (ACPI_FAILURE(status)) {
-		goto error;
-	}
-
+	ec_ecdt->handle = ACPI_ROOT_OBJECT;
 	return 0;
-      error:
-	ACPI_EXCEPTION((AE_INFO, status, "Could not use ECDT"));
-	kfree(ec_ecdt);
-	ec_ecdt = NULL;
-
-	return -ENODEV;
 }
 
 int __init acpi_ec_ecdt_probe(void)
 {
-	acpi_status status;
 	int ret;
 
 	ret = acpi_ec_get_real_ecdt();
 	if (ret)
 		return 0;
 
-	/*
-	 * Install GPE handler
-	 */
-	status = acpi_install_gpe_handler(NULL, ec_ecdt->gpe,
-					  ACPI_GPE_EDGE_TRIGGERED,
-					  &acpi_ec_gpe_handler, ec_ecdt);
-	if (ACPI_FAILURE(status)) {
-		goto error;
-	}
-	acpi_set_gpe_type(NULL, ec_ecdt->gpe, ACPI_GPE_TYPE_RUNTIME);
-	acpi_enable_gpe(NULL, ec_ecdt->gpe, ACPI_NOT_ISR);
-
-	status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT,
-						    ACPI_ADR_SPACE_EC,
-						    &acpi_ec_space_handler,
-						    &acpi_ec_space_setup,
-						    ec_ecdt);
-	if (ACPI_FAILURE(status)) {
-		acpi_remove_gpe_handler(NULL, ec_ecdt->gpe,
-					&acpi_ec_gpe_handler);
-		goto error;
-	}
-
-	return 0;
+	ret = ec_install_handlers(ec_ecdt);
+	if (!ret)
+		return 0;
 
-      error:
-	ACPI_EXCEPTION((AE_INFO, status, "Could not use ECDT"));
 	kfree(ec_ecdt);
 	ec_ecdt = NULL;
 
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#24 lore raw

PATCH 17/22 ACPI: EC: Clean ECDT and namespace parsing.

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/ec.c |  205 ++++++++++++++++++++++++++++-------------------------
 1 files changed, 109 insertions(+), 96 deletions(-)
drivers/acpi/ec.c +109 -96
index 399cedf..c9dcf9a 100644
@@ -604,74 +604,73 @@ static int acpi_ec_remove_fs(struct acpi_device *device)
 /* --------------------------------------------------------------------------
                                Driver Interface
    -------------------------------------------------------------------------- */
+static acpi_status
+ec_parse_io_ports(struct acpi_resource *resource, void *context);
+
+static acpi_status
+ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval);
+
+static struct acpi_ec *make_acpi_ec(void)
+{
+	struct acpi_ec *ec = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
+	if (!ec)
+		return NULL;
+
+	atomic_set(&ec->query_pending, 0);
+	atomic_set(&ec->event_count, 1);
+	mutex_init(&ec->lock);
+	init_waitqueue_head(&ec->wait);
+
+	return ec;
+}
 
 static int acpi_ec_add(struct acpi_device *device)
 {
-	int result = 0;
 	acpi_status status = AE_OK;
 	struct acpi_ec *ec = NULL;
 
 	if (!device)
 		return -EINVAL;
 
-	ec = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
+	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
+	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
+
+	ec = make_acpi_ec();
 	if (!ec)
 		return -ENOMEM;
 
-	ec->handle = device->handle;
-	ec->uid = -1;
-	mutex_init(&ec->lock);
-	atomic_set(&ec->query_pending, 0);
-	atomic_set(&ec->event_count, 1);
-	if (acpi_ec_mode == EC_INTR) {
-		init_waitqueue_head(&ec->wait);
+	status = ec_parse_device(device->handle, 0, ec, NULL);
+	if (status != AE_CTRL_TERMINATE) {
+		kfree(ec);
+		return -EINVAL;
 	}
-	strcpy(acpi_device_name(device), ACPI_EC_DEVICE_NAME);
-	strcpy(acpi_device_class(device), ACPI_EC_CLASS);
-	acpi_driver_data(device) = ec;
 
-	/* Use the global lock for all EC transactions? */
-	acpi_evaluate_integer(ec->handle, "_GLK", NULL, &ec->global_lock);
-
-	/* XXX we don't test uids, because on some boxes ecdt uid = 0, see:
-	   http://bugzilla.kernel.org/show_bug.cgi?id=6111 */
+	/* Check if we found the boot EC */
 	if (ec_ecdt) {
-		acpi_remove_address_space_handler(ACPI_ROOT_OBJECT,
-						  ACPI_ADR_SPACE_EC,
-						  &acpi_ec_space_handler);
+		if (ec_ecdt->gpe == ec->gpe) {
+			/* We might have incorrect info for GL at boot time */
+			mutex_lock(&ec_ecdt->lock);
+			ec_ecdt->global_lock = ec->global_lock;
+			mutex_unlock(&ec_ecdt->lock);
+			kfree(ec);
+			ec = ec_ecdt;
+		}
+	}
 
-		acpi_remove_gpe_handler(NULL, ec_ecdt->gpe,
-					&acpi_ec_gpe_handler);
+	ec->handle = device->handle;
 
-		kfree(ec_ecdt);
-	}
+	acpi_driver_data(device) = ec;
 
-	/* Get GPE bit assignment (EC events). */
-	/* TODO: Add support for _GPE returning a package */
-	status = acpi_evaluate_integer(ec->handle, "_GPE", NULL, &ec->gpe);
-	if (ACPI_FAILURE(status)) {
-		ACPI_EXCEPTION((AE_INFO, status,
-				"Obtaining GPE bit assignment"));
-		result = -ENODEV;
-		goto end;
-	}
+	if (!first_ec)
+		first_ec = device;
 
-	result = acpi_ec_add_fs(device);
-	if (result)
-		goto end;
+	acpi_ec_add_fs(device);
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "%s [%s] (gpe %d) interrupt mode.",
 			  acpi_device_name(device), acpi_device_bid(device),
 			  (u32) ec->gpe));
 
-	if (!first_ec)
-		first_ec = device;
-
-      end:
-	if (result)
-		kfree(ec);
-
-	return result;
+	return 0;
 }
 
 static int acpi_ec_remove(struct acpi_device *device, int type)
@@ -685,13 +684,19 @@ static int acpi_ec_remove(struct acpi_device *device, int type)
 
 	acpi_ec_remove_fs(device);
 
-	kfree(ec);
+	acpi_driver_data(device) = NULL;
+	if (device == first_ec)
+		first_ec = NULL;
+
+	/* Don't touch boot EC */
+	if (ec_ecdt != ec)
+		kfree(ec);
 
 	return 0;
 }
 
 static acpi_status
-acpi_ec_io_ports(struct acpi_resource *resource, void *context)
+ec_parse_io_ports(struct acpi_resource *resource, void *context)
 {
 	struct acpi_ec *ec = context;
 
@@ -717,9 +722,10 @@ acpi_ec_io_ports(struct acpi_resource *resource, void *context)
 
 static int ec_install_handlers(struct acpi_ec *ec)
 {
-	acpi_status status = acpi_install_gpe_handler(NULL, ec->gpe,
-						      ACPI_GPE_EDGE_TRIGGERED,
-						      &acpi_ec_gpe_handler, ec);
+	acpi_status status;
+	status = acpi_install_gpe_handler(NULL, ec->gpe,
+					  ACPI_GPE_EDGE_TRIGGERED,
+					  &acpi_ec_gpe_handler, ec);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 	acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
@@ -739,8 +745,7 @@ static int ec_install_handlers(struct acpi_ec *ec)
 
 static int acpi_ec_start(struct acpi_device *device)
 {
-	acpi_status status = AE_OK;
-	struct acpi_ec *ec = NULL;
+	struct acpi_ec *ec;
 
 	if (!device)
 		return -EINVAL;
@@ -750,32 +755,31 @@ static int acpi_ec_start(struct acpi_device *device)
 	if (!ec)
 		return -EINVAL;
 
-	/*
-	 * Get I/O port addresses. Convert to GAS format.
-	 */
-	status = acpi_walk_resources(ec->handle, METHOD_NAME__CRS,
-				     acpi_ec_io_ports, ec);
-	if (ACPI_FAILURE(status) || ec->command_addr == 0) {
-		ACPI_EXCEPTION((AE_INFO, status,
-				"Error getting I/O port addresses"));
-		return -ENODEV;
-	}
-
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "gpe=0x%02lx, ports=0x%2lx,0x%2lx",
 			  ec->gpe, ec->command_addr, ec->data_addr));
 
+	/* Boot EC is already working */
+	if (ec == ec_ecdt)
+		return 0;
+
 	return ec_install_handlers(ec);
 }
 
 static int acpi_ec_stop(struct acpi_device *device, int type)
 {
-	acpi_status status = AE_OK;
-	struct acpi_ec *ec = NULL;
+	acpi_status status;
+	struct acpi_ec *ec;
 
 	if (!device)
 		return -EINVAL;
 
 	ec = acpi_driver_data(device);
+	if (!ec)
+		return -EINVAL;
+
+	/* Don't touch boot EC */
+	if (ec == ec_ecdt)
+		return 0;
 
 	status = acpi_remove_address_space_handler(ec->handle,
 						   ACPI_ADR_SPACE_EC,
@@ -790,51 +794,64 @@ static int acpi_ec_stop(struct acpi_device *device, int type)
 	return 0;
 }
 
-static int __init acpi_ec_get_real_ecdt(void)
+static acpi_status
+ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
 {
 	acpi_status status;
+
+	struct acpi_ec *ec = context;
+	status = acpi_walk_resources(handle, METHOD_NAME__CRS,
+				     ec_parse_io_ports, ec);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	/* Get GPE bit assignment (EC events). */
+	/* TODO: Add support for _GPE returning a package */
+	status = acpi_evaluate_integer(handle, "_GPE", NULL, &ec->gpe);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	/* Use the global lock for all EC transactions? */
+	acpi_evaluate_integer(handle, "_GLK", NULL, &ec->global_lock);
+
+	ec->handle = handle;
+
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "GPE=0x%02lx, ports=0x%2lx, 0x%2lx",
+			  ec->gpe, ec->command_addr, ec->data_addr));
+
+	return AE_CTRL_TERMINATE;
+}
+
+int __init acpi_ec_ecdt_probe(void)
+{
+	int ret;
+	acpi_status status;
 	struct acpi_table_ecdt *ecdt_ptr;
 
+	ec_ecdt = make_acpi_ec();
+	if (!ec_ecdt)
+		return -ENOMEM;
+	/*
+	 * Generate a boot ec context
+	 */
+
 	status = acpi_get_table(ACPI_SIG_ECDT, 1,
 				(struct acpi_table_header **)&ecdt_ptr);
 	if (ACPI_FAILURE(status))
-		return -ENODEV;
+		goto error;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found ECDT"));
 
-	/*
-	 * Generate a temporary ec context to use until the namespace is scanned
-	 */
-	ec_ecdt = kzalloc(sizeof(struct acpi_ec), GFP_KERNEL);
-	if (!ec_ecdt)
-		return -ENOMEM;
-
-	mutex_init(&ec_ecdt->lock);
-	atomic_set(&ec_ecdt->event_count, 1);
-	if (acpi_ec_mode == EC_INTR) {
-		init_waitqueue_head(&ec_ecdt->wait);
-	}
 	ec_ecdt->command_addr = ecdt_ptr->control.address;
 	ec_ecdt->data_addr = ecdt_ptr->data.address;
 	ec_ecdt->gpe = ecdt_ptr->gpe;
 	ec_ecdt->uid = ecdt_ptr->uid;
-
 	ec_ecdt->handle = ACPI_ROOT_OBJECT;
-	return 0;
-}
-
-int __init acpi_ec_ecdt_probe(void)
-{
-	int ret;
-
-	ret = acpi_ec_get_real_ecdt();
-	if (ret)
-		return 0;
 
 	ret = ec_install_handlers(ec_ecdt);
 	if (!ret)
 		return 0;
-
+      error:
 	kfree(ec_ecdt);
 	ec_ecdt = NULL;
 
@@ -884,12 +901,8 @@ static int __init acpi_ec_set_intr_mode(char *str)
 	if (!get_option(&str, &intr))
 		return 0;
 
-	if (intr) {
-		acpi_ec_mode = EC_INTR;
-	} else {
-		acpi_ec_mode = EC_POLL;
-	}
-	acpi_ec_driver.ops.add = acpi_ec_add;
+	acpi_ec_mode = (intr) ? EC_INTR : EC_POLL;
+
 	printk(KERN_NOTICE PREFIX "%s mode.\n", intr ? "interrupt" : "polling");
 
 	return 1;
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#25 lore raw

PATCH 18/22 ACPI: EC: Rename ec_ecdt to more informative boot_ec

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/ec.c |   43 ++++++++++++++++++++-----------------------
 1 files changed, 20 insertions(+), 23 deletions(-)
drivers/acpi/ec.c +20 -23
index c9dcf9a..1250468 100644
@@ -102,7 +102,7 @@ static struct acpi_ec {
 	atomic_t query_pending;
 	atomic_t event_count;
 	wait_queue_head_t wait;
-} *ec_ecdt;
+} *boot_ec;
 
 /* External interfaces use first EC only, so remember */
 static struct acpi_device *first_ec;
@@ -646,21 +646,18 @@ static int acpi_ec_add(struct acpi_device *device)
 	}
 
 	/* Check if we found the boot EC */
-	if (ec_ecdt) {
-		if (ec_ecdt->gpe == ec->gpe) {
+	if (boot_ec) {
+		if (boot_ec->gpe == ec->gpe) {
 			/* We might have incorrect info for GL at boot time */
-			mutex_lock(&ec_ecdt->lock);
-			ec_ecdt->global_lock = ec->global_lock;
-			mutex_unlock(&ec_ecdt->lock);
+			mutex_lock(&boot_ec->lock);
+			boot_ec->global_lock = ec->global_lock;
+			mutex_unlock(&boot_ec->lock);
 			kfree(ec);
-			ec = ec_ecdt;
+			ec = boot_ec;
 		}
 	}
-
 	ec->handle = device->handle;
-
 	acpi_driver_data(device) = ec;
-
 	if (!first_ec)
 		first_ec = device;
 
@@ -689,7 +686,7 @@ static int acpi_ec_remove(struct acpi_device *device, int type)
 		first_ec = NULL;
 
 	/* Don't touch boot EC */
-	if (ec_ecdt != ec)
+	if (boot_ec != ec)
 		kfree(ec);
 
 	return 0;
@@ -759,7 +756,7 @@ static int acpi_ec_start(struct acpi_device *device)
 			  ec->gpe, ec->command_addr, ec->data_addr));
 
 	/* Boot EC is already working */
-	if (ec == ec_ecdt)
+	if (ec == boot_ec)
 		return 0;
 
 	return ec_install_handlers(ec);
@@ -778,7 +775,7 @@ static int acpi_ec_stop(struct acpi_device *device, int type)
 		return -EINVAL;
 
 	/* Don't touch boot EC */
-	if (ec == ec_ecdt)
+	if (ec == boot_ec)
 		return 0;
 
 	status = acpi_remove_address_space_handler(ec->handle,
@@ -828,8 +825,8 @@ int __init acpi_ec_ecdt_probe(void)
 	acpi_status status;
 	struct acpi_table_ecdt *ecdt_ptr;
 
-	ec_ecdt = make_acpi_ec();
-	if (!ec_ecdt)
+	boot_ec = make_acpi_ec();
+	if (!boot_ec)
 		return -ENOMEM;
 	/*
 	 * Generate a boot ec context
@@ -842,18 +839,18 @@ int __init acpi_ec_ecdt_probe(void)
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found ECDT"));
 
-	ec_ecdt->command_addr = ecdt_ptr->control.address;
-	ec_ecdt->data_addr = ecdt_ptr->data.address;
-	ec_ecdt->gpe = ecdt_ptr->gpe;
-	ec_ecdt->uid = ecdt_ptr->uid;
-	ec_ecdt->handle = ACPI_ROOT_OBJECT;
+	boot_ec->command_addr = ecdt_ptr->control.address;
+	boot_ec->data_addr = ecdt_ptr->data.address;
+	boot_ec->gpe = ecdt_ptr->gpe;
+	boot_ec->uid = ecdt_ptr->uid;
+	boot_ec->handle = ACPI_ROOT_OBJECT;
 
-	ret = ec_install_handlers(ec_ecdt);
+	ret = ec_install_handlers(boot_ec);
 	if (!ret)
 		return 0;
       error:
-	kfree(ec_ecdt);
-	ec_ecdt = NULL;
+	kfree(boot_ec);
+	boot_ec = NULL;
 
 	return -ENODEV;
 }
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#26 lore raw

PATCH 19/22 ACPI: EC: first_ec is better to be acpi_ec than acpi_device.

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/ec.c |   41 +++++++++++++----------------------------
 1 files changed, 13 insertions(+), 28 deletions(-)
drivers/acpi/ec.c +13 -28
index 1250468..187981a 100644
@@ -91,6 +91,7 @@ static struct acpi_driver acpi_ec_driver = {
 };
 
 /* If we find an EC via the ECDT, we need to keep a ptr to its context */
+/* External interfaces use first EC only, so remember */
 static struct acpi_ec {
 	acpi_handle handle;
 	unsigned long uid;
@@ -102,10 +103,7 @@ static struct acpi_ec {
 	atomic_t query_pending;
 	atomic_t event_count;
 	wait_queue_head_t wait;
-} *boot_ec;
-
-/* External interfaces use first EC only, so remember */
-static struct acpi_device *first_ec;
+} *boot_ec, *first_ec;
 
 /* --------------------------------------------------------------------------
                              Transaction Management
@@ -299,38 +297,31 @@ static int acpi_ec_write(struct acpi_ec *ec, u8 address, u8 data)
  */
 int ec_burst_enable(void)
 {
-	struct acpi_ec *ec;
 	if (!first_ec)
 		return -ENODEV;
-	ec = acpi_driver_data(first_ec);
-	return acpi_ec_burst_enable(ec);
+	return acpi_ec_burst_enable(first_ec);
 }
 
 EXPORT_SYMBOL(ec_burst_enable);
 
 int ec_burst_disable(void)
 {
-	struct acpi_ec *ec;
 	if (!first_ec)
 		return -ENODEV;
-	ec = acpi_driver_data(first_ec);
-	return acpi_ec_burst_disable(ec);
+	return acpi_ec_burst_disable(first_ec);
 }
 
 EXPORT_SYMBOL(ec_burst_disable);
 
 int ec_read(u8 addr, u8 * val)
 {
-	struct acpi_ec *ec;
 	int err;
 	u8 temp_data;
 
 	if (!first_ec)
 		return -ENODEV;
 
-	ec = acpi_driver_data(first_ec);
-
-	err = acpi_ec_read(ec, addr, &temp_data);
+	err = acpi_ec_read(first_ec, addr, &temp_data);
 
 	if (!err) {
 		*val = temp_data;
@@ -343,15 +334,12 @@ EXPORT_SYMBOL(ec_read);
 
 int ec_write(u8 addr, u8 val)
 {
-	struct acpi_ec *ec;
 	int err;
 
 	if (!first_ec)
 		return -ENODEV;
 
-	ec = acpi_driver_data(first_ec);
-
-	err = acpi_ec_write(ec, addr, val);
+	err = acpi_ec_write(first_ec, addr, val);
 
 	return err;
 }
@@ -362,14 +350,10 @@ int ec_transaction(u8 command,
 		   const u8 * wdata, unsigned wdata_len,
 		   u8 * rdata, unsigned rdata_len)
 {
-	struct acpi_ec *ec;
-
 	if (!first_ec)
 		return -ENODEV;
 
-	ec = acpi_driver_data(first_ec);
-
-	return acpi_ec_transaction(ec, command, wdata,
+	return acpi_ec_transaction(first_ec, command, wdata,
 				   wdata_len, rdata, rdata_len);
 }
 
@@ -655,11 +639,10 @@ static int acpi_ec_add(struct acpi_device *device)
 			kfree(ec);
 			ec = boot_ec;
 		}
-	}
+	} else
+		first_ec = ec;
 	ec->handle = device->handle;
 	acpi_driver_data(device) = ec;
-	if (!first_ec)
-		first_ec = device;
 
 	acpi_ec_add_fs(device);
 
@@ -682,7 +665,7 @@ static int acpi_ec_remove(struct acpi_device *device, int type)
 	acpi_ec_remove_fs(device);
 
 	acpi_driver_data(device) = NULL;
-	if (device == first_ec)
+	if (ec == first_ec)
 		first_ec = NULL;
 
 	/* Don't touch boot EC */
@@ -846,8 +829,10 @@ int __init acpi_ec_ecdt_probe(void)
 	boot_ec->handle = ACPI_ROOT_OBJECT;
 
 	ret = ec_install_handlers(boot_ec);
-	if (!ret)
+	if (!ret) {
+		first_ec = boot_ec;
 		return 0;
+	}
       error:
 	kfree(boot_ec);
 	boot_ec = NULL;
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#27 lore raw

PATCH 20/22 ACPI: EC: Cleanup of EC initialization

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/ec.c |   32 +++++++++++++-------------------
 1 files changed, 13 insertions(+), 19 deletions(-)
drivers/acpi/ec.c +13 -19
index 187981a..2a17229 100644
@@ -1,6 +1,8 @@
 /*
- *  acpi_ec.c - ACPI Embedded Controller Driver ($Revision: 38 $)
+ *  ec.c - ACPI Embedded Controller Driver (v2.0)
  *
+ *  Copyright (C) 2006, 2007 Alexey Starikovskiy <[email protected]>
+ *  Copyright (C) 2006 Denis Sadykov <[email protected]>
  *  Copyright (C) 2004 Luming Yu <[email protected]>
  *  Copyright (C) 2001, 2002 Andy Grover <[email protected]>
  *  Copyright (C) 2001, 2002 Paul Diefenbaugh <[email protected]>
@@ -94,7 +96,6 @@ static struct acpi_driver acpi_ec_driver = {
 /* External interfaces use first EC only, so remember */
 static struct acpi_ec {
 	acpi_handle handle;
-	unsigned long uid;
 	unsigned long gpe;
 	unsigned long command_addr;
 	unsigned long data_addr;
@@ -525,13 +526,11 @@ static int acpi_ec_read_info(struct seq_file *seq, void *offset)
 	if (!ec)
 		goto end;
 
-	seq_printf(seq, "gpe:                 0x%02x\n", (u32) ec->gpe);
-	seq_printf(seq, "ports:                   0x%02x, 0x%02x\n",
-		   (u32) ec->command_addr, (u32) ec->data_addr);
-	seq_printf(seq, "use global lock:         %s\n",
+	seq_printf(seq, "gpe:\t\t\t0x%02x\n", (u32) ec->gpe);
+	seq_printf(seq, "ports:\t\t\t0x%02x, 0x%02x\n",
+		   (unsigned)ec->command_addr, (unsigned)ec->data_addr);
+	seq_printf(seq, "use global lock:\t%s\n",
 		   ec->global_lock ? "yes" : "no");
-	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
-
       end:
 	return 0;
 }
@@ -655,15 +654,13 @@ static int acpi_ec_add(struct acpi_device *device)
 
 static int acpi_ec_remove(struct acpi_device *device, int type)
 {
-	struct acpi_ec *ec = NULL;
+	struct acpi_ec *ec;
 
 	if (!device)
 		return -EINVAL;
 
 	ec = acpi_driver_data(device);
-
 	acpi_ec_remove_fs(device);
-
 	acpi_driver_data(device) = NULL;
 	if (ec == first_ec)
 		first_ec = NULL;
@@ -671,7 +668,6 @@ static int acpi_ec_remove(struct acpi_device *device, int type)
 	/* Don't touch boot EC */
 	if (boot_ec != ec)
 		kfree(ec);
-
 	return 0;
 }
 
@@ -680,22 +676,20 @@ ec_parse_io_ports(struct acpi_resource *resource, void *context)
 {
 	struct acpi_ec *ec = context;
 
-	if (resource->type != ACPI_RESOURCE_TYPE_IO) {
+	if (resource->type != ACPI_RESOURCE_TYPE_IO)
 		return AE_OK;
-	}
 
 	/*
 	 * The first address region returned is the data port, and
 	 * the second address region returned is the status/command
 	 * port.
 	 */
-	if (ec->data_addr == 0) {
+	if (ec->data_addr == 0)
 		ec->data_addr = resource->data.io.minimum;
-	} else if (ec->command_addr == 0) {
+	else if (ec->command_addr == 0)
 		ec->command_addr = resource->data.io.minimum;
-	} else {
+	else
 		return AE_CTRL_TERMINATE;
-	}
 
 	return AE_OK;
 }
@@ -708,6 +702,7 @@ static int ec_install_handlers(struct acpi_ec *ec)
 					  &acpi_ec_gpe_handler, ec);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
+
 	acpi_set_gpe_type(NULL, ec->gpe, ACPI_GPE_TYPE_RUNTIME);
 	acpi_enable_gpe(NULL, ec->gpe, ACPI_NOT_ISR);
 
@@ -825,7 +820,6 @@ int __init acpi_ec_ecdt_probe(void)
 	boot_ec->command_addr = ecdt_ptr->control.address;
 	boot_ec->data_addr = ecdt_ptr->data.address;
 	boot_ec->gpe = ecdt_ptr->gpe;
-	boot_ec->uid = ecdt_ptr->uid;
 	boot_ec->handle = ACPI_ROOT_OBJECT;
 
 	ret = ec_install_handlers(boot_ec);
Signature · 1 line
1.5.0.3.310.g05ef5
author reviewer
#28 lore raw

PATCH 21/22 ACPI: EC: Block queries until EC is fully initialized

In reply to Len Brown

From: Alexey Starikovskiy <[email protected]>

---
 drivers/acpi/ec.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
drivers/acpi/ec.c +4 -1
index 2a17229..e08cf98 100644
@@ -599,7 +599,7 @@ static struct acpi_ec *make_acpi_ec(void)
 	if (!ec)
 		return NULL;
 
-	atomic_set(&ec->query_pending, 0);
+	atomic_set(&ec->query_pending, 1);
 	atomic_set(&ec->event_count, 1);
 	mutex_init(&ec->lock);
 	init_waitqueue_head(&ec->wait);
@@ -715,6 +715,9 @@ static int ec_install_handlers(struct acpi_ec *ec)
 		return -ENODEV;
 	}
 
+	/* EC is fully operational, allow queries */
+	atomic_set(&ec->query_pending, 0);
+
 	return 0;
 }
 
Signature · 1 line
1.5.0.3.310.g05ef5
#29 lore raw
In reply to Len Brown
Quoted 2 lines · Len Brown

These patches are in test for Linux-2.6.22.

Since we have big changes on ACPI for 2.6.21 , why we don't test all at once (why we don't also include this on 6.21) ?

Best regards,

Signature · 1 line
Sérgio M. B.
#30 lore raw
Quoted 10 lines · Sergio Monteiro Basto
Quoted 3 lines · Len Brown

These patches are in test for Linux-2.6.22.

Since we have big changes on ACPI for 2.6.21 , why we don't test all at once (why we don't also include this on 6.21) ?

Best regards,

Because testing is expected to be done in Andrew Morton' -mm and not in Linus' stable kernel.