[PATCH v3 00/17] ACPI: cleanups for hotplug

[PATCH v3 00/17] ACPI: cleanups for hotplug

[Bjorn Helgaas]

I'm working to make the Linux/ACPI code handle hotplug notification
events.  This should ultimately allow us to remove the existing
hotplug code scattered through the ACPI device drivers.

This is a long series of cleanups that should cause no functional
change, but will make it easier to call acpi_bus_scan() from the
notification path.

Comments and testing reports welcome.  Boot logs with
CONFIG_ACPI_DEBUG=y and "acpi.debug_layer=0x00010000" will be
helpful in debugging any issues.

Original posting: http://marc.info/?l=linux-acpi&m=124907619701194&w=2

Changes from v2 to v3:
    - refreshed to apply on current acpi-test (193a6dec1c0246a).
    - removed deferred execution patches (already in acpi-test)
    - added fix for acpiphp_ibm.c bug introduced by ACPICA
      acpi_get_object_info() changes (unrelated to this series, only
      here for convenience)

Changes from initial post to v2:
    - refreshed to apply on current acpi-test (0beab01e5b2c448).
    - fixed memory leak in acpi_add_single_object() debug code
    - added ACPI_IS_ROOT_DEVICE() macro (Len)

---

Bjorn Helgaas (17):
      ACPICA: fixup after acpi_get_object_info() change
      ACPI: add debug for device addition
      ACPI: remove unused acpi_bus_scan_fixed() argument
      ACPI: remove redundant "handle" and "parent" arguments
      ACPI: save device_type in acpi_device
      ACPI: use device_type rather than comparing HID
      ACPI: remove acpi_device_set_context() "type" argument
      ACPI: remove redundant "type" arguments
      ACPI: remove unnecessary argument checking
      ACPI: add acpi_bus_get_parent() and remove "parent" arguments
      ACPI: convert acpi_bus_scan() to operate on an acpi_handle
      ACPI: enumerate namespace before adding functional fixed hardware devices
      ACPI: identify device tree root by null parent pointer, not ACPI_BUS_TYPE
      ACPI: use acpi_walk_namespace() to enumerate devices
      ACPI: add acpi_bus_get_status_handle()
      ACPI: factor out device type and status checking
      ACPI: handle re-enumeration, when acpi_devices might already exist


 drivers/acpi/bus.c                |   49 ++--
 drivers/acpi/scan.c               |  427 +++++++++++++++++--------------------
 drivers/pci/hotplug/acpiphp_ibm.c |    1 
 include/acpi/acpi_bus.h           |    6 -
 4 files changed, 217 insertions(+), 266 deletions(-)

-- 
Bjorn

[PATCH v3 01/17] ACPICA: fixup after acpi_get_object_info() change

[Bjorn Helgaas]

Commit 15b8dd53f5ffa changed info->hardware_id from a static array to
a pointer.  If hardware_id is non-NULL, it points to a NULL-terminated
string, so we don't need to terminate it explicitly.  However, it may
be NULL; in that case, we *can't* add a NULL terminator.

This causes a NULL pointer dereference oops for devices without _HID.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
CC: Lin Ming <ming.m.lin@intel.com>
CC: Bob Moore <robert.moore@intel.com>
CC: Gary Hade <garyhade@us.ibm.com>
---
 drivers/pci/hotplug/acpiphp_ibm.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_ibm.c b/drivers/pci/hotplug/acpiphp_ibm.c
index a9d926b..e7be66d 100644
--- a/drivers/pci/hotplug/acpiphp_ibm.c
+++ b/drivers/pci/hotplug/acpiphp_ibm.c
@@ -406,7 +406,6 @@ static acpi_status __init ibm_find_acpi_device(acpi_handle handle,
 			__func__, status);
 		return retval;
 	}
-	info->hardware_id.string[sizeof(info->hardware_id.length) - 1] = '\0';
 
 	if (info->current_status && (info->valid & ACPI_VALID_HID) &&
 			(!strcmp(info->hardware_id.string, IBM_HARDWARE_ID1) ||

[PATCH v3 02/17] ACPI: add debug for device addition

[Bjorn Helgaas]

Add debug output for adding an ACPI device.  Enable this with
"acpi.debug_layer=0x00010000" (ACPI_BUS_COMPONENT).

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 408ebde..75b7c57 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1230,6 +1230,7 @@ acpi_add_single_object(struct acpi_device **child,
 {
 	int result = 0;
 	struct acpi_device *device = NULL;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 
 
 	if (!child)
@@ -1355,9 +1356,16 @@ acpi_add_single_object(struct acpi_device **child,
 	}
 
 end:
-	if (!result)
+	if (!result) {
+		acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+			"Adding %s [%s] parent %s\n", dev_name(&device->dev),
+			 (char *) buffer.pointer,
+			 device->parent ? dev_name(&device->parent->dev) :
+					  "(null)"));
+		kfree(buffer.pointer);
 		*child = device;
-	else
+	} else
 		acpi_device_release(&device->dev);
 
 	return result;

[PATCH v3 03/17] ACPI: remove unused acpi_bus_scan_fixed() argument

[Bjorn Helgaas]

We never use the "root" argument, so just remove it.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 75b7c57..0302dd4 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1580,15 +1580,12 @@ int acpi_bus_trim(struct acpi_device *start, int rmdevice)
 }
 EXPORT_SYMBOL_GPL(acpi_bus_trim);
 
-static int acpi_bus_scan_fixed(struct acpi_device *root)
+static int acpi_bus_scan_fixed(void)
 {
 	int result = 0;
 	struct acpi_device *device = NULL;
 	struct acpi_bus_ops ops;
 
-	if (!root)
-		return -ENODEV;
-
 	memset(&ops, 0, sizeof(ops));
 	ops.acpi_op_add = 1;
 	ops.acpi_op_start = 1;
@@ -1639,7 +1636,7 @@ int __init acpi_scan_init(void)
 	/*
 	 * Enumerate devices in the ACPI namespace.
 	 */
-	result = acpi_bus_scan_fixed(acpi_root);
+	result = acpi_bus_scan_fixed();
 
 	if (!result)
 		result = acpi_bus_scan(acpi_root, &ops);

[PATCH v3 04/17] ACPI: remove redundant "handle" and "parent" arguments

[Bjorn Helgaas]

In several cases, functions take handle and parent device pointers in
addition to acpi_device pointers.  But the acpi_device structure contains
both the handle and the parent pointer, so it's pointless and error-prone
to pass them all.  This patch removes the unnecessary "handle" and "parent"
arguments.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |   27 ++++++++++++---------------
 1 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 0302dd4..ab5a264 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -474,12 +474,12 @@ struct bus_type acpi_bus_type = {
 	.uevent		= acpi_device_uevent,
 };
 
-static int acpi_device_register(struct acpi_device *device,
-				 struct acpi_device *parent)
+static int acpi_device_register(struct acpi_device *device)
 {
 	int result;
 	struct acpi_device_bus_id *acpi_device_bus_id, *new_bus_id;
 	int found = 0;
+
 	/*
 	 * Linkage
 	 * -------
@@ -524,7 +524,7 @@ static int acpi_device_register(struct acpi_device *device,
 	mutex_unlock(&acpi_device_lock);
 
 	if (device->parent)
-		device->dev.parent = &parent->dev;
+		device->dev.parent = &device->parent->dev;
 	device->dev.bus = &acpi_bus_type;
 	device->dev.release = &acpi_device_release;
 	result = device_register(&device->dev);
@@ -918,8 +918,7 @@ static int acpi_bus_get_flags(struct acpi_device *device)
 	return 0;
 }
 
-static void acpi_device_get_busid(struct acpi_device *device,
-				  acpi_handle handle, int type)
+static void acpi_device_get_busid(struct acpi_device *device, int type)
 {
 	char bus_id[5] = { '?', 0 };
 	struct acpi_buffer buffer = { sizeof(bus_id), bus_id };
@@ -942,7 +941,7 @@ static void acpi_device_get_busid(struct acpi_device *device,
 		strcpy(device->pnp.bus_id, "SLPF");
 		break;
 	default:
-		acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer);
+		acpi_get_name(device->handle, ACPI_SINGLE_NAME, &buffer);
 		/* Clean up trailing underscores (if any) */
 		for (i = 3; i > 1; i--) {
 			if (bus_id[i] == '_')
@@ -1058,9 +1057,7 @@ acpi_add_cid(
 	return cid;
 }
 
-static void acpi_device_set_id(struct acpi_device *device,
-			       struct acpi_device *parent, acpi_handle handle,
-			       int type)
+static void acpi_device_set_id(struct acpi_device *device, int type)
 {
 	struct acpi_device_info *info = NULL;
 	char *hid = NULL;
@@ -1071,7 +1068,7 @@ static void acpi_device_set_id(struct acpi_device *device,
 
 	switch (type) {
 	case ACPI_BUS_TYPE_DEVICE:
-		status = acpi_get_object_info(handle, &info);
+		status = acpi_get_object_info(device->handle, &info);
 		if (ACPI_FAILURE(status)) {
 			printk(KERN_ERR PREFIX "%s: Error reading device info\n", __func__);
 			return;
@@ -1126,7 +1123,8 @@ static void acpi_device_set_id(struct acpi_device *device,
 	 * ----
 	 * Fix for the system root bus device -- the only root-level device.
 	 */
-	if (((acpi_handle)parent == ACPI_ROOT_OBJECT) && (type == ACPI_BUS_TYPE_DEVICE)) {
+	if (((acpi_handle)device->parent == ACPI_ROOT_OBJECT) &&
+	     (type == ACPI_BUS_TYPE_DEVICE)) {
 		hid = ACPI_BUS_HID;
 		strcpy(device->pnp.device_name, ACPI_BUS_DEVICE_NAME);
 		strcpy(device->pnp.device_class, ACPI_BUS_CLASS);
@@ -1246,8 +1244,7 @@ acpi_add_single_object(struct acpi_device **child,
 	device->parent = parent;
 	device->bus_ops = *ops; /* workround for not call .start */
 
-
-	acpi_device_get_busid(device, handle, type);
+	acpi_device_get_busid(device, type);
 
 	/*
 	 * Flags
@@ -1310,7 +1307,7 @@ acpi_add_single_object(struct acpi_device **child,
 	 * Hardware ID, Unique ID, & Bus Address
 	 * -------------------------------------
 	 */
-	acpi_device_set_id(device, parent, handle, type);
+	acpi_device_set_id(device, type);
 
 	/*
 	 * Power Management
@@ -1345,7 +1342,7 @@ acpi_add_single_object(struct acpi_device **child,
 	if ((result = acpi_device_set_context(device, type)))
 		goto end;
 
-	result = acpi_device_register(device, parent);
+	result = acpi_device_register(device);
 
 	/*
 	 * Bind _ADR-Based Devices when hot add

[PATCH v3 05/17] ACPI: save device_type in acpi_device

[Bjorn Helgaas]

Most uses of the ACPI bus device_type (ACPI_BUS_TYPE_DEVICE,
ACPI_BUS_TYPE_POWER, etc) are during device initialization, but
we do need it later for notify handler installation, since that
is different for fixed hardware devices vs. namespace devices.

This patch saves the device_type in the acpi_device structure,
so we can check that rather than comparing against the _HID string.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c     |    1 +
 include/acpi/acpi_bus.h |    3 ++-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index ab5a264..c73681b 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1240,6 +1240,7 @@ acpi_add_single_object(struct acpi_device **child,
 		return -ENOMEM;
 	}
 
+	device->device_type = type;
 	device->handle = handle;
 	device->parent = parent;
 	device->bus_ops = *ops; /* workround for not call .start */
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 89bbb2a..8e39b3e 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -262,7 +262,8 @@ struct acpi_device_wakeup {
 /* Device */
 
 struct acpi_device {
-	acpi_handle handle;
+	int device_type;
+	acpi_handle handle;		/* no handle for fixed hardware */
 	struct acpi_device *parent;
 	struct list_head children;
 	struct list_head node;

[PATCH v3 06/17] ACPI: use device_type rather than comparing HID

[Bjorn Helgaas]

Check the acpi_device device_type rather than the HID.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c73681b..c8e867b 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -378,15 +378,13 @@ static acpi_status acpi_device_notify_fixed(void *data)
 static int acpi_device_install_notify_handler(struct acpi_device *device)
 {
 	acpi_status status;
-	char *hid;
 
-	hid = acpi_device_hid(device);
-	if (!strcmp(hid, ACPI_BUTTON_HID_POWERF))
+	if (device->device_type == ACPI_BUS_TYPE_POWER_BUTTON)
 		status =
 		    acpi_install_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
 						     acpi_device_notify_fixed,
 						     device);
-	else if (!strcmp(hid, ACPI_BUTTON_HID_SLEEPF))
+	else if (device->device_type == ACPI_BUS_TYPE_SLEEP_BUTTON)
 		status =
 		    acpi_install_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
 						     acpi_device_notify_fixed,
@@ -404,10 +402,10 @@ static int acpi_device_install_notify_handler(struct acpi_device *device)
 
 static void acpi_device_remove_notify_handler(struct acpi_device *device)
 {
-	if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_POWERF))
+	if (device->device_type == ACPI_BUS_TYPE_POWER_BUTTON)
 		acpi_remove_fixed_event_handler(ACPI_EVENT_POWER_BUTTON,
 						acpi_device_notify_fixed);
-	else if (!strcmp(acpi_device_hid(device), ACPI_BUTTON_HID_SLEEPF))
+	else if (device->device_type == ACPI_BUS_TYPE_SLEEP_BUTTON)
 		acpi_remove_fixed_event_handler(ACPI_EVENT_SLEEP_BUTTON,
 						acpi_device_notify_fixed);
 	else

[PATCH v3 07/17] ACPI: remove acpi_device_set_context() "type" argument

[Bjorn Helgaas]

We only pass the "type" to acpi_device_set_context() so we know whether
the device has a handle to which we can attach the acpi_device pointer.
But it's safer to just check for the handle directly, since it's in the
acpi_device already.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |   32 +++++++++++++++-----------------
 1 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c8e867b..44383fe 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1171,29 +1171,27 @@ static void acpi_device_set_id(struct acpi_device *device, int type)
 	kfree(info);
 }
 
-static int acpi_device_set_context(struct acpi_device *device, int type)
+static int acpi_device_set_context(struct acpi_device *device)
 {
-	acpi_status status = AE_OK;
-	int result = 0;
+	acpi_status status;
+
 	/*
 	 * Context
 	 * -------
 	 * Attach this 'struct acpi_device' to the ACPI object.  This makes
-	 * resolutions from handle->device very efficient.  Note that we need
-	 * to be careful with fixed-feature devices as they all attach to the
-	 * root object.
+	 * resolutions from handle->device very efficient.  Fixed hardware
+	 * devices have no handles, so we skip them.
 	 */
-	if (type != ACPI_BUS_TYPE_POWER_BUTTON &&
-	    type != ACPI_BUS_TYPE_SLEEP_BUTTON) {
-		status = acpi_attach_data(device->handle,
-					  acpi_bus_data_handler, device);
+	if (!device->handle)
+		return 0;
 
-		if (ACPI_FAILURE(status)) {
-			printk(KERN_ERR PREFIX "Error attaching device data\n");
-			result = -ENODEV;
-		}
-	}
-	return result;
+	status = acpi_attach_data(device->handle,
+				  acpi_bus_data_handler, device);
+	if (ACPI_SUCCESS(status))
+		return 0;
+
+	printk(KERN_ERR PREFIX "Error attaching device data\n");
+	return -ENODEV;
 }
 
 static int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
@@ -1338,7 +1336,7 @@ acpi_add_single_object(struct acpi_device **child,
 			goto end;
 	}
 
-	if ((result = acpi_device_set_context(device, type)))
+	if ((result = acpi_device_set_context(device)))
 		goto end;
 
 	result = acpi_device_register(device);

[PATCH v3 08/17] ACPI: remove redundant "type" arguments

[Bjorn Helgaas]

We now save the ACPI bus "device_type" in the acpi_device structure, so
we don't need to pass it around explicitly anymore.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 44383fe..6c83342 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -916,7 +916,7 @@ static int acpi_bus_get_flags(struct acpi_device *device)
 	return 0;
 }
 
-static void acpi_device_get_busid(struct acpi_device *device, int type)
+static void acpi_device_get_busid(struct acpi_device *device)
 {
 	char bus_id[5] = { '?', 0 };
 	struct acpi_buffer buffer = { sizeof(bus_id), bus_id };
@@ -928,7 +928,7 @@ static void acpi_device_get_busid(struct acpi_device *device, int type)
 	 * The device's Bus ID is simply the object name.
 	 * TBD: Shouldn't this value be unique (within the ACPI namespace)?
 	 */
-	switch (type) {
+	switch (device->device_type) {
 	case ACPI_BUS_TYPE_SYSTEM:
 		strcpy(device->pnp.bus_id, "ACPI");
 		break;
@@ -1055,7 +1055,7 @@ acpi_add_cid(
 	return cid;
 }
 
-static void acpi_device_set_id(struct acpi_device *device, int type)
+static void acpi_device_set_id(struct acpi_device *device)
 {
 	struct acpi_device_info *info = NULL;
 	char *hid = NULL;
@@ -1064,7 +1064,7 @@ static void acpi_device_set_id(struct acpi_device *device, int type)
 	char *cid_add = NULL;
 	acpi_status status;
 
-	switch (type) {
+	switch (device->device_type) {
 	case ACPI_BUS_TYPE_DEVICE:
 		status = acpi_get_object_info(device->handle, &info);
 		if (ACPI_FAILURE(status)) {
@@ -1122,7 +1122,7 @@ static void acpi_device_set_id(struct acpi_device *device, int type)
 	 * Fix for the system root bus device -- the only root-level device.
 	 */
 	if (((acpi_handle)device->parent == ACPI_ROOT_OBJECT) &&
-	     (type == ACPI_BUS_TYPE_DEVICE)) {
+	     (device->device_type == ACPI_BUS_TYPE_DEVICE)) {
 		hid = ACPI_BUS_HID;
 		strcpy(device->pnp.device_name, ACPI_BUS_DEVICE_NAME);
 		strcpy(device->pnp.device_class, ACPI_BUS_CLASS);
@@ -1241,7 +1241,7 @@ acpi_add_single_object(struct acpi_device **child,
 	device->parent = parent;
 	device->bus_ops = *ops; /* workround for not call .start */
 
-	acpi_device_get_busid(device, type);
+	acpi_device_get_busid(device);
 
 	/*
 	 * Flags
@@ -1304,7 +1304,7 @@ acpi_add_single_object(struct acpi_device **child,
 	 * Hardware ID, Unique ID, & Bus Address
 	 * -------------------------------------
 	 */
-	acpi_device_set_id(device, type);
+	acpi_device_set_id(device);
 
 	/*
 	 * Power Management

[PATCH v3 09/17] ACPI: remove unnecessary argument checking

[Bjorn Helgaas]

acpi_add_single_object() is static, and all callers supply a valid "child"
argument, so we don't need to check it.  This patch also remove some
unnecessary initializations.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 6c83342..f2e2834 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1222,14 +1222,10 @@ acpi_add_single_object(struct acpi_device **child,
 		       struct acpi_device *parent, acpi_handle handle, int type,
 			struct acpi_bus_ops *ops)
 {
-	int result = 0;
-	struct acpi_device *device = NULL;
+	int result;
+	struct acpi_device *device;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 
-
-	if (!child)
-		return -EINVAL;
-
 	device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
 	if (!device) {
 		printk(KERN_ERR PREFIX "Memory allocation error\n");

[PATCH v3 10/17] ACPI: add acpi_bus_get_parent() and remove "parent" arguments

[Bjorn Helgaas]

This patch adds acpi_bus_get_parent(), which ascends the namespace until
it finds a parent with an acpi_device.

Then we use acpi_bus_get_parent() in acpi_add_single_object(), so callers
don't have to figure out or keep track of the parent acpi_device.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |   50 +++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f2e2834..f205b36 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -662,6 +662,33 @@ EXPORT_SYMBOL(acpi_bus_unregister_driver);
 /* --------------------------------------------------------------------------
                                  Device Enumeration
    -------------------------------------------------------------------------- */
+static struct acpi_device *acpi_bus_get_parent(acpi_handle handle)
+{
+	acpi_status status;
+	int ret;
+	struct acpi_device *device;
+
+	/*
+	 * Fixed hardware devices do not appear in the namespace and do not
+	 * have handles, but we fabricate acpi_devices for them, so we have
+	 * to deal with them specially.
+	 */
+	if (handle == NULL)
+		return acpi_root;
+
+	do {
+		status = acpi_get_parent(handle, &handle);
+		if (status == AE_NULL_ENTRY)
+			return NULL;
+		if (ACPI_FAILURE(status))
+			return acpi_root;
+
+		ret = acpi_bus_get_device(handle, &device);
+		if (ret == 0)
+			return device;
+	} while (1);
+}
+
 acpi_status
 acpi_bus_get_ejd(acpi_handle handle, acpi_handle *ejd)
 {
@@ -1217,10 +1244,9 @@ static int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
 	return 0;
 }
 
-static int
-acpi_add_single_object(struct acpi_device **child,
-		       struct acpi_device *parent, acpi_handle handle, int type,
-			struct acpi_bus_ops *ops)
+static int acpi_add_single_object(struct acpi_device **child,
+				  acpi_handle handle, int type,
+				  struct acpi_bus_ops *ops)
 {
 	int result;
 	struct acpi_device *device;
@@ -1234,7 +1260,7 @@ acpi_add_single_object(struct acpi_device **child,
 
 	device->device_type = type;
 	device->handle = handle;
-	device->parent = parent;
+	device->parent = acpi_bus_get_parent(handle);
 	device->bus_ops = *ops; /* workround for not call .start */
 
 	acpi_device_get_busid(device);
@@ -1434,8 +1460,8 @@ static int acpi_bus_scan(struct acpi_device *start, struct acpi_bus_ops *ops)
 		}
 
 		if (ops->acpi_op_add)
-			status = acpi_add_single_object(&child, parent,
-				chandle, type, ops);
+			status = acpi_add_single_object(&child, chandle, type,
+							ops);
 		else
 			status = acpi_bus_get_device(chandle, &child);
 
@@ -1488,7 +1514,7 @@ acpi_bus_add(struct acpi_device **child,
 	memset(&ops, 0, sizeof(ops));
 	ops.acpi_op_add = 1;
 
-	result = acpi_add_single_object(child, parent, handle, type, &ops);
+	result = acpi_add_single_object(child, handle, type, &ops);
 	if (!result)
 		result = acpi_bus_scan(*child, &ops);
 
@@ -1584,15 +1610,13 @@ static int acpi_bus_scan_fixed(void)
 	 * Enumerate all fixed-feature devices.
 	 */
 	if ((acpi_gbl_FADT.flags & ACPI_FADT_POWER_BUTTON) == 0) {
-		result = acpi_add_single_object(&device, acpi_root,
-						NULL,
+		result = acpi_add_single_object(&device, NULL,
 						ACPI_BUS_TYPE_POWER_BUTTON,
 						&ops);
 	}
 
 	if ((acpi_gbl_FADT.flags & ACPI_FADT_SLEEP_BUTTON) == 0) {
-		result = acpi_add_single_object(&device, acpi_root,
-						NULL,
+		result = acpi_add_single_object(&device, NULL,
 						ACPI_BUS_TYPE_SLEEP_BUTTON,
 						&ops);
 	}
@@ -1618,7 +1642,7 @@ int __init acpi_scan_init(void)
 	/*
 	 * Create the root device in the bus's device tree
 	 */
-	result = acpi_add_single_object(&acpi_root, NULL, ACPI_ROOT_OBJECT,
+	result = acpi_add_single_object(&acpi_root, ACPI_ROOT_OBJECT,
 					ACPI_BUS_TYPE_SYSTEM, &ops);
 	if (result)
 		goto Done;

[PATCH v3 11/17] ACPI: convert acpi_bus_scan() to operate on an acpi_handle

[Bjorn Helgaas]

This patch changes acpi_bus_scan() to take an acpi_handle rather than an
acpi_device pointer.  I plan to use acpi_bus_scan() in the hotplug path,
and I'd rather not assume that notifications only go to nodes that already
have acpi_devices.

This will also help remove the special case for adding the root node.  We
currently add the root by hand before acpi_bus_scan(), but using a handle
here means we can start the acpi_bus_scan() directly with the root even
though it doesn't have an acpi_device yet.

Note that acpi_bus_scan() currently adds and/or starts the *children* of
its device argument.  It doesn't do anything with the device itself.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index f205b36..4fe7359 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1387,7 +1387,7 @@ end:
 	return result;
 }
 
-static int acpi_bus_scan(struct acpi_device *start, struct acpi_bus_ops *ops)
+static int acpi_bus_scan(acpi_handle handle, struct acpi_bus_ops *ops)
 {
 	acpi_status status = AE_OK;
 	struct acpi_device *parent = NULL;
@@ -1396,13 +1396,16 @@ static int acpi_bus_scan(struct acpi_device *start, struct acpi_bus_ops *ops)
 	acpi_handle chandle = NULL;
 	acpi_object_type type = 0;
 	u32 level = 1;
+	int ret;
 
-
-	if (!start)
-		return -EINVAL;
-
-	parent = start;
-	phandle = start->handle;
+	/*
+	 * We must have an acpi_device for the starting node already, and
+	 * we scan its children.
+	 */
+	phandle = handle;
+	ret = acpi_bus_get_device(phandle, &parent);
+	if (ret)
+		return ret;
 
 	/*
 	 * Parse through the ACPI namespace, identify all 'devices', and
@@ -1516,7 +1519,7 @@ acpi_bus_add(struct acpi_device **child,
 
 	result = acpi_add_single_object(child, handle, type, &ops);
 	if (!result)
-		result = acpi_bus_scan(*child, &ops);
+		result = acpi_bus_scan((*child)->handle, &ops);
 
 	return result;
 }
@@ -1527,16 +1530,13 @@ int acpi_bus_start(struct acpi_device *device)
 	int result;
 	struct acpi_bus_ops ops;
 
-
-	if (!device)
-		return -EINVAL;
+	memset(&ops, 0, sizeof(ops));
+	ops.acpi_op_start = 1;
 
 	result = acpi_start_single_object(device);
-	if (!result) {
-		memset(&ops, 0, sizeof(ops));
-		ops.acpi_op_start = 1;
-		result = acpi_bus_scan(device, &ops);
-	}
+	if (!result)
+		result = acpi_bus_scan(device->handle, &ops);
+
 	return result;
 }
 EXPORT_SYMBOL(acpi_bus_start);
@@ -1653,7 +1653,7 @@ int __init acpi_scan_init(void)
 	result = acpi_bus_scan_fixed();
 
 	if (!result)
-		result = acpi_bus_scan(acpi_root, &ops);
+		result = acpi_bus_scan(acpi_root->handle, &ops);
 
 	if (result)
 		acpi_device_unregister(acpi_root, ACPI_BUS_REMOVAL_NORMAL);

[PATCH v3 12/17] ACPI: enumerate namespace before adding functional fixed hardware devices

[Bjorn Helgaas]

This patch changes the order so we enumerate in the "root, namespace,
functional fixed" order instead of the "root, functional fixed, namespace"
order.  When I change acpi_bus_scan() to use acpi_walk_namespace(), it
will use the former order, so this patch isolates the order change for
bisectability.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 4fe7359..27d2dec 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1650,10 +1650,10 @@ int __init acpi_scan_init(void)
 	/*
 	 * Enumerate devices in the ACPI namespace.
 	 */
-	result = acpi_bus_scan_fixed();
+	result = acpi_bus_scan(acpi_root->handle, &ops);
 
 	if (!result)
-		result = acpi_bus_scan(acpi_root->handle, &ops);
+		result = acpi_bus_scan_fixed();
 
 	if (result)
 		acpi_device_unregister(acpi_root, ACPI_BUS_REMOVAL_NORMAL);

[PATCH v3 13/17] ACPI: identify device tree root by null parent pointer, not ACPI_BUS_TYPE

[Bjorn Helgaas]

We can identify the root of the ACPI device tree by the fact that it
has no parent.  This is simpler than passing around ACPI_BUS_TYPE_SYSTEM
and will help remove special treatment of the device tree root.

Currently, we add the root by hand with ACPI_BUS_TYPE_SYSTEM.  If we
traverse the tree treating the root as just another device and use
acpi_get_type(), the root shows up as ACPI_TYPE_DEVICE.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c     |   20 +++++++++++++-------
 include/acpi/acpi_bus.h |    1 -
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 27d2dec..0b5aaf0 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -22,6 +22,8 @@ extern struct acpi_device *acpi_root;
 #define ACPI_BUS_HID			"LNXSYBUS"
 #define ACPI_BUS_DEVICE_NAME		"System Bus"
 
+#define ACPI_IS_ROOT_DEVICE(device)    (!(device)->parent)
+
 static LIST_HEAD(acpi_device_list);
 static LIST_HEAD(acpi_bus_id_list);
 DEFINE_MUTEX(acpi_device_lock);
@@ -955,10 +957,12 @@ static void acpi_device_get_busid(struct acpi_device *device)
 	 * The device's Bus ID is simply the object name.
 	 * TBD: Shouldn't this value be unique (within the ACPI namespace)?
 	 */
-	switch (device->device_type) {
-	case ACPI_BUS_TYPE_SYSTEM:
+	if (ACPI_IS_ROOT_DEVICE(device)) {
 		strcpy(device->pnp.bus_id, "ACPI");
-		break;
+		return;
+	}
+
+	switch (device->device_type) {
 	case ACPI_BUS_TYPE_POWER_BUTTON:
 		strcpy(device->pnp.bus_id, "PWRF");
 		break;
@@ -1093,6 +1097,11 @@ static void acpi_device_set_id(struct acpi_device *device)
 
 	switch (device->device_type) {
 	case ACPI_BUS_TYPE_DEVICE:
+		if (ACPI_IS_ROOT_DEVICE(device)) {
+			hid = ACPI_SYSTEM_HID;
+			break;
+		}
+
 		status = acpi_get_object_info(device->handle, &info);
 		if (ACPI_FAILURE(status)) {
 			printk(KERN_ERR PREFIX "%s: Error reading device info\n", __func__);
@@ -1129,9 +1138,6 @@ static void acpi_device_set_id(struct acpi_device *device)
 	case ACPI_BUS_TYPE_PROCESSOR:
 		hid = ACPI_PROCESSOR_OBJECT_HID;
 		break;
-	case ACPI_BUS_TYPE_SYSTEM:
-		hid = ACPI_SYSTEM_HID;
-		break;
 	case ACPI_BUS_TYPE_THERMAL:
 		hid = ACPI_THERMAL_HID;
 		break;
@@ -1643,7 +1649,7 @@ int __init acpi_scan_init(void)
 	 * Create the root device in the bus's device tree
 	 */
 	result = acpi_add_single_object(&acpi_root, ACPI_ROOT_OBJECT,
-					ACPI_BUS_TYPE_SYSTEM, &ops);
+					ACPI_BUS_TYPE_DEVICE, &ops);
 	if (result)
 		goto Done;
 
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 8e39b3e..ef1cb23 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -70,7 +70,6 @@ enum acpi_bus_device_type {
 	ACPI_BUS_TYPE_POWER,
 	ACPI_BUS_TYPE_PROCESSOR,
 	ACPI_BUS_TYPE_THERMAL,
-	ACPI_BUS_TYPE_SYSTEM,
 	ACPI_BUS_TYPE_POWER_BUTTON,
 	ACPI_BUS_TYPE_SLEEP_BUTTON,
 	ACPI_BUS_DEVICE_TYPE_COUNT

Re: [PATCH v3 13/17] ACPI: identify device tree root by null parent pointer, not ACPI_BUS_TYPE

[ykzhao]

On Tue, 2009-09-22 at 03:29 +0800, Bjorn Helgaas wrote:
> We can identify the root of the ACPI device tree by the fact that it
> has no parent.  This is simpler than passing around ACPI_BUS_TYPE_SYSTEM
> and will help remove special treatment of the device tree root.
> 
> Currently, we add the root by hand with ACPI_BUS_TYPE_SYSTEM.  If we
> traverse the tree treating the root as just another device and use
> acpi_get_type(), the root shows up as ACPI_TYPE_DEVICE.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> ---
>  drivers/acpi/scan.c     |   20 +++++++++++++-------
>  include/acpi/acpi_bus.h |    1 -
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 27d2dec..0b5aaf0 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -22,6 +22,8 @@ extern struct acpi_device *acpi_root;
>  #define ACPI_BUS_HID			"LNXSYBUS"
>  #define ACPI_BUS_DEVICE_NAME		"System Bus"
>  
> +#define ACPI_IS_ROOT_DEVICE(device)    (!(device)->parent)
the following definition will be better
   #define ACPI_IS_ROOT_DEVICE(device)  \
				(device->handle == ACPI_ROOT_OBJECT)

thanks.
> +
>  static LIST_HEAD(acpi_device_list);
>  static LIST_HEAD(acpi_bus_id_list);
>  DEFINE_MUTEX(acpi_device_lock);
> @@ -955,10 +957,12 @@ static void acpi_device_get_busid(struct acpi_device *device)
>  	 * The device's Bus ID is simply the object name.
>  	 * TBD: Shouldn't this value be unique (within the ACPI namespace)?
>  	 */
> -	switch (device->device_type) {
> -	case ACPI_BUS_TYPE_SYSTEM:
> +	if (ACPI_IS_ROOT_DEVICE(device)) {
>  		strcpy(device->pnp.bus_id, "ACPI");
> -		break;
> +		return;
> +	}
> +
> +	switch (device->device_type) {
>  	case ACPI_BUS_TYPE_POWER_BUTTON:
>  		strcpy(device->pnp.bus_id, "PWRF");
>  		break;
> @@ -1093,6 +1097,11 @@ static void acpi_device_set_id(struct acpi_device *device)
>  
>  	switch (device->device_type) {
>  	case ACPI_BUS_TYPE_DEVICE:
> +		if (ACPI_IS_ROOT_DEVICE(device)) {
> +			hid = ACPI_SYSTEM_HID;
> +			break;
> +		}
> +
>  		status = acpi_get_object_info(device->handle, &info);
>  		if (ACPI_FAILURE(status)) {
>  			printk(KERN_ERR PREFIX "%s: Error reading device info\n", __func__);
> @@ -1129,9 +1138,6 @@ static void acpi_device_set_id(struct acpi_device *device)
>  	case ACPI_BUS_TYPE_PROCESSOR:
>  		hid = ACPI_PROCESSOR_OBJECT_HID;
>  		break;
> -	case ACPI_BUS_TYPE_SYSTEM:
> -		hid = ACPI_SYSTEM_HID;
> -		break;
>  	case ACPI_BUS_TYPE_THERMAL:
>  		hid = ACPI_THERMAL_HID;
>  		break;
> @@ -1643,7 +1649,7 @@ int __init acpi_scan_init(void)
>  	 * Create the root device in the bus's device tree
>  	 */
>  	result = acpi_add_single_object(&acpi_root, ACPI_ROOT_OBJECT,
> -					ACPI_BUS_TYPE_SYSTEM, &ops);
> +					ACPI_BUS_TYPE_DEVICE, &ops);
>  	if (result)
>  		goto Done;
>  
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 8e39b3e..ef1cb23 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -70,7 +70,6 @@ enum acpi_bus_device_type {
>  	ACPI_BUS_TYPE_POWER,
>  	ACPI_BUS_TYPE_PROCESSOR,
>  	ACPI_BUS_TYPE_THERMAL,
> -	ACPI_BUS_TYPE_SYSTEM,
>  	ACPI_BUS_TYPE_POWER_BUTTON,
>  	ACPI_BUS_TYPE_SLEEP_BUTTON,
>  	ACPI_BUS_DEVICE_TYPE_COUNT
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 13/17] ACPI: identify device tree root by null parent pointer, not ACPI_BUS_TYPE

[Bjorn Helgaas]

On Tuesday 22 September 2009 09:09:44 pm ykzhao wrote:
> On Tue, 2009-09-22 at 03:29 +0800, Bjorn Helgaas wrote:
> > We can identify the root of the ACPI device tree by the fact that it
> > has no parent.  This is simpler than passing around ACPI_BUS_TYPE_SYSTEM
> > and will help remove special treatment of the device tree root.
> > 
> > Currently, we add the root by hand with ACPI_BUS_TYPE_SYSTEM.  If we
> > traverse the tree treating the root as just another device and use
> > acpi_get_type(), the root shows up as ACPI_TYPE_DEVICE.
> > 
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > ---
> >  drivers/acpi/scan.c     |   20 +++++++++++++-------
> >  include/acpi/acpi_bus.h |    1 -
> >  2 files changed, 13 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > index 27d2dec..0b5aaf0 100644
> > --- a/drivers/acpi/scan.c
> > +++ b/drivers/acpi/scan.c
> > @@ -22,6 +22,8 @@ extern struct acpi_device *acpi_root;
> >  #define ACPI_BUS_HID			"LNXSYBUS"
> >  #define ACPI_BUS_DEVICE_NAME		"System Bus"
> >  
> > +#define ACPI_IS_ROOT_DEVICE(device)    (!(device)->parent)

> the following definition will be better
>    #define ACPI_IS_ROOT_DEVICE(device)  \
> 				(device->handle == ACPI_ROOT_OBJECT)

I wish you'd given me a clue about *why* you think it's better to
check the handle.

I considered checking the handle, but I used the parent pointer
because:

  1) There are synonyms for the root handle: ACPI_ROOT_OBJECT is the
     special-case #define to allow ACPICA users to start traversing
     the namespace before they know any handles, but acpi_gbl_root_node
     is the real root handle.  In fact, if you call acpi_get_parent()
     on a child of the root, it returns acpi_gbl_root_node, not
     ACPI_ROOT_OBJECT.  This is a bit messy, and it seems safest to
     me to just avoid testing against ACPI_ROOT_OBJECT at all.

  2) It's a reminder that the acpi_device tree data structure should
     be a proper fully-connected tree with exactly one root.  If we
     assume a valid tree, there's exactly one node with no parent.
     That's more fundamental than the handle, where you have to
     analyze more code to make sure we don't assign ACPI_ROOT_OBJECT
     to several nodes.

     (There is existing code that allows disconnected trees to be built.
     For example, dock_create_acpi_device() can call acpi_bus_add()
     with a NULL parent pointer.  This "shouldn't happen," but I
     consider the fact that the interface allows this to be a bug.
     That's one reason I'm removing the "parent" argument from
     acpi_add_single_object() and related interfaces.)

  3) We build acpi_devices for things with no handle, e.g., functional
     fixed hardware.  Their handles happen to be NULL but I prefer to
     think of them as undefined and not rely on them at all.

  4) acpi_walk_namespace(..., ACPI_ROOT_OBJECT, ...) doesn't actually
     visit the root node; it starts with the children.  I consider
     this a bug.  If it were ever fixed, we would probably call the
     callback with acpi_gbl_root_node, not ACPI_ROOT_OBJECT, so the
     root acpi_device handle would no longer be ACPI_ROOT_OBJECT.

Bjorn

> > +
> >  static LIST_HEAD(acpi_device_list);
> >  static LIST_HEAD(acpi_bus_id_list);
> >  DEFINE_MUTEX(acpi_device_lock);
> > @@ -955,10 +957,12 @@ static void acpi_device_get_busid(struct acpi_device *device)
> >  	 * The device's Bus ID is simply the object name.
> >  	 * TBD: Shouldn't this value be unique (within the ACPI namespace)?
> >  	 */
> > -	switch (device->device_type) {
> > -	case ACPI_BUS_TYPE_SYSTEM:
> > +	if (ACPI_IS_ROOT_DEVICE(device)) {
> >  		strcpy(device->pnp.bus_id, "ACPI");
> > -		break;
> > +		return;
> > +	}
> > +
> > +	switch (device->device_type) {
> >  	case ACPI_BUS_TYPE_POWER_BUTTON:
> >  		strcpy(device->pnp.bus_id, "PWRF");
> >  		break;
> > @@ -1093,6 +1097,11 @@ static void acpi_device_set_id(struct acpi_device *device)
> >  
> >  	switch (device->device_type) {
> >  	case ACPI_BUS_TYPE_DEVICE:
> > +		if (ACPI_IS_ROOT_DEVICE(device)) {
> > +			hid = ACPI_SYSTEM_HID;
> > +			break;
> > +		}
> > +
> >  		status = acpi_get_object_info(device->handle, &info);
> >  		if (ACPI_FAILURE(status)) {
> >  			printk(KERN_ERR PREFIX "%s: Error reading device info\n", __func__);
> > @@ -1129,9 +1138,6 @@ static void acpi_device_set_id(struct acpi_device *device)
> >  	case ACPI_BUS_TYPE_PROCESSOR:
> >  		hid = ACPI_PROCESSOR_OBJECT_HID;
> >  		break;
> > -	case ACPI_BUS_TYPE_SYSTEM:
> > -		hid = ACPI_SYSTEM_HID;
> > -		break;
> >  	case ACPI_BUS_TYPE_THERMAL:
> >  		hid = ACPI_THERMAL_HID;
> >  		break;
> > @@ -1643,7 +1649,7 @@ int __init acpi_scan_init(void)
> >  	 * Create the root device in the bus's device tree
> >  	 */
> >  	result = acpi_add_single_object(&acpi_root, ACPI_ROOT_OBJECT,
> > -					ACPI_BUS_TYPE_SYSTEM, &ops);
> > +					ACPI_BUS_TYPE_DEVICE, &ops);
> >  	if (result)
> >  		goto Done;
> >  
> > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > index 8e39b3e..ef1cb23 100644
> > --- a/include/acpi/acpi_bus.h
> > +++ b/include/acpi/acpi_bus.h
> > @@ -70,7 +70,6 @@ enum acpi_bus_device_type {
> >  	ACPI_BUS_TYPE_POWER,
> >  	ACPI_BUS_TYPE_PROCESSOR,
> >  	ACPI_BUS_TYPE_THERMAL,
> > -	ACPI_BUS_TYPE_SYSTEM,
> >  	ACPI_BUS_TYPE_POWER_BUTTON,
> >  	ACPI_BUS_TYPE_SLEEP_BUTTON,
> >  	ACPI_BUS_DEVICE_TYPE_COUNT
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH v3 13/17] ACPI: identify device tree root by null parent pointer, not ACPI_BUS_TYPE

[ykzhao]

On Thu, 2009-09-24 at 00:14 +0800, Bjorn Helgaas wrote:
> On Tuesday 22 September 2009 09:09:44 pm ykzhao wrote:
> > On Tue, 2009-09-22 at 03:29 +0800, Bjorn Helgaas wrote:
> > > We can identify the root of the ACPI device tree by the fact that it
> > > has no parent.  This is simpler than passing around ACPI_BUS_TYPE_SYSTEM
> > > and will help remove special treatment of the device tree root.
> > > 
> > > Currently, we add the root by hand with ACPI_BUS_TYPE_SYSTEM.  If we
> > > traverse the tree treating the root as just another device and use
> > > acpi_get_type(), the root shows up as ACPI_TYPE_DEVICE.
> > > 
> > > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > > ---
> > >  drivers/acpi/scan.c     |   20 +++++++++++++-------
> > >  include/acpi/acpi_bus.h |    1 -
> > >  2 files changed, 13 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > index 27d2dec..0b5aaf0 100644
> > > --- a/drivers/acpi/scan.c
> > > +++ b/drivers/acpi/scan.c
> > > @@ -22,6 +22,8 @@ extern struct acpi_device *acpi_root;
> > >  #define ACPI_BUS_HID			"LNXSYBUS"
> > >  #define ACPI_BUS_DEVICE_NAME		"System Bus"
> > >  
> > > +#define ACPI_IS_ROOT_DEVICE(device)    (!(device)->parent)
> 
> > the following definition will be better
> >    #define ACPI_IS_ROOT_DEVICE(device)  \
> > 				(device->handle == ACPI_ROOT_OBJECT)
> 
> I wish you'd given me a clue about *why* you think it's better to
> check the handle.
Every acpi device has its corresponding acpi_handle. We can use this
info to check whether the device is the root device. 

Of course it is also ok to check whether the device is the root device
by using device->parent.
> 
> I considered checking the handle, but I used the parent pointer
> because:
> 
>   1) There are synonyms for the root handle: ACPI_ROOT_OBJECT is the
>      special-case #define to allow ACPICA users to start traversing
>      the namespace before they know any handles, but acpi_gbl_root_node
>      is the real root handle.  In fact, if you call acpi_get_parent()
>      on a child of the root, it returns acpi_gbl_root_node, not
>      ACPI_ROOT_OBJECT.  This is a bit messy, and it seems safest to
>      me to just avoid testing against ACPI_ROOT_OBJECT at all.
> 
>   2) It's a reminder that the acpi_device tree data structure should
>      be a proper fully-connected tree with exactly one root.  If we
>      assume a valid tree, there's exactly one node with no parent.
>      That's more fundamental than the handle, where you have to
>      analyze more code to make sure we don't assign ACPI_ROOT_OBJECT
>      to several nodes.
> 
>      (There is existing code that allows disconnected trees to be built.
>      For example, dock_create_acpi_device() can call acpi_bus_add()
>      with a NULL parent pointer.  This "shouldn't happen," but I
>      consider the fact that the interface allows this to be a bug.
>      That's one reason I'm removing the "parent" argument from
>      acpi_add_single_object() and related interfaces.)
> 
>   3) We build acpi_devices for things with no handle, e.g., functional
>      fixed hardware.  Their handles happen to be NULL but I prefer to
>      think of them as undefined and not rely on them at all.
       Is it possible to build the acpi_device without acpi handle? It
seems that now we won't create a new acpi device for the functional
fixed hardware.For example: the acpi handle is NULL when we add the acpi
device for fixed power/sleep button.  In such case it is still different
with the ACPI
       
> 
>   4) acpi_walk_namespace(..., ACPI_ROOT_OBJECT, ...) doesn't actually
>      visit the root node; it starts with the children.  I consider
>      this a bug.  If it were ever fixed, we would probably call the
>      callback with acpi_gbl_root_node, not ACPI_ROOT_OBJECT, so the
>      root acpi_device handle would no longer be ACPI_ROOT_OBJECT.
       When the ACPI_ROOT_OBJECT is passed to acpi_walk_namespace, then
it will start from acpi_gbl_root_node(this is changed in the function of
acpi_ns_walk_namespace).
> 
> Bjorn
> 
> > > +
> > >  static LIST_HEAD(acpi_device_list);
> > >  static LIST_HEAD(acpi_bus_id_list);
> > >  DEFINE_MUTEX(acpi_device_lock);
> > > @@ -955,10 +957,12 @@ static void acpi_device_get_busid(struct acpi_device *device)
> > >  	 * The device's Bus ID is simply the object name.
> > >  	 * TBD: Shouldn't this value be unique (within the ACPI namespace)?
> > >  	 */
> > > -	switch (device->device_type) {
> > > -	case ACPI_BUS_TYPE_SYSTEM:
> > > +	if (ACPI_IS_ROOT_DEVICE(device)) {
> > >  		strcpy(device->pnp.bus_id, "ACPI");
> > > -		break;
> > > +		return;
> > > +	}
> > > +
> > > +	switch (device->device_type) {
> > >  	case ACPI_BUS_TYPE_POWER_BUTTON:
> > >  		strcpy(device->pnp.bus_id, "PWRF");
> > >  		break;
> > > @@ -1093,6 +1097,11 @@ static void acpi_device_set_id(struct acpi_device *device)
> > >  
> > >  	switch (device->device_type) {
> > >  	case ACPI_BUS_TYPE_DEVICE:
> > > +		if (ACPI_IS_ROOT_DEVICE(device)) {
> > > +			hid = ACPI_SYSTEM_HID;
> > > +			break;
> > > +		}
> > > +
> > >  		status = acpi_get_object_info(device->handle, &info);
> > >  		if (ACPI_FAILURE(status)) {
> > >  			printk(KERN_ERR PREFIX "%s: Error reading device info\n", __func__);
> > > @@ -1129,9 +1138,6 @@ static void acpi_device_set_id(struct acpi_device *device)
> > >  	case ACPI_BUS_TYPE_PROCESSOR:
> > >  		hid = ACPI_PROCESSOR_OBJECT_HID;
> > >  		break;
> > > -	case ACPI_BUS_TYPE_SYSTEM:
> > > -		hid = ACPI_SYSTEM_HID;
> > > -		break;
> > >  	case ACPI_BUS_TYPE_THERMAL:
> > >  		hid = ACPI_THERMAL_HID;
> > >  		break;
> > > @@ -1643,7 +1649,7 @@ int __init acpi_scan_init(void)
> > >  	 * Create the root device in the bus's device tree
> > >  	 */
> > >  	result = acpi_add_single_object(&acpi_root, ACPI_ROOT_OBJECT,
> > > -					ACPI_BUS_TYPE_SYSTEM, &ops);
> > > +					ACPI_BUS_TYPE_DEVICE, &ops);
> > >  	if (result)
> > >  		goto Done;
> > >  
> > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > > index 8e39b3e..ef1cb23 100644
> > > --- a/include/acpi/acpi_bus.h
> > > +++ b/include/acpi/acpi_bus.h
> > > @@ -70,7 +70,6 @@ enum acpi_bus_device_type {
> > >  	ACPI_BUS_TYPE_POWER,
> > >  	ACPI_BUS_TYPE_PROCESSOR,
> > >  	ACPI_BUS_TYPE_THERMAL,
> > > -	ACPI_BUS_TYPE_SYSTEM,
> > >  	ACPI_BUS_TYPE_POWER_BUTTON,
> > >  	ACPI_BUS_TYPE_SLEEP_BUTTON,
> > >  	ACPI_BUS_DEVICE_TYPE_COUNT
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> 
> 

Re: [PATCH v3 13/17] ACPI: identify device tree root by null parent pointer, not ACPI_BUS_TYPE

[Bjorn Helgaas]

On Thu, 2009-09-24 at 10:10 +0800, ykzhao wrote:
> On Thu, 2009-09-24 at 00:14 +0800, Bjorn Helgaas wrote:
> > On Tuesday 22 September 2009 09:09:44 pm ykzhao wrote:
> > > On Tue, 2009-09-22 at 03:29 +0800, Bjorn Helgaas wrote:
> > > > We can identify the root of the ACPI device tree by the fact that it
> > > > has no parent.  This is simpler than passing around ACPI_BUS_TYPE_SYSTEM
> > > > and will help remove special treatment of the device tree root.
> > > > 
> > > > Currently, we add the root by hand with ACPI_BUS_TYPE_SYSTEM.  If we
> > > > traverse the tree treating the root as just another device and use
> > > > acpi_get_type(), the root shows up as ACPI_TYPE_DEVICE.
> > > > 
> > > > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > > > ---
> > > >  drivers/acpi/scan.c     |   20 +++++++++++++-------
> > > >  include/acpi/acpi_bus.h |    1 -
> > > >  2 files changed, 13 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> > > > index 27d2dec..0b5aaf0 100644
> > > > --- a/drivers/acpi/scan.c
> > > > +++ b/drivers/acpi/scan.c
> > > > @@ -22,6 +22,8 @@ extern struct acpi_device *acpi_root;
> > > >  #define ACPI_BUS_HID			"LNXSYBUS"
> > > >  #define ACPI_BUS_DEVICE_NAME		"System Bus"
> > > >  
> > > > +#define ACPI_IS_ROOT_DEVICE(device)    (!(device)->parent)
> > 
> > > the following definition will be better
> > >    #define ACPI_IS_ROOT_DEVICE(device)  \
> > > 				(device->handle == ACPI_ROOT_OBJECT)
> > 
> > I wish you'd given me a clue about *why* you think it's better to
> > check the handle.
> Every acpi device has its corresponding acpi_handle. We can use this
> info to check whether the device is the root device. 
> 
> Of course it is also ok to check whether the device is the root device
> by using device->parent.
> > 
> > I considered checking the handle, but I used the parent pointer
> > because:
> > 
> >   1) There are synonyms for the root handle: ACPI_ROOT_OBJECT is the
> >      special-case #define to allow ACPICA users to start traversing
> >      the namespace before they know any handles, but acpi_gbl_root_node
> >      is the real root handle.  In fact, if you call acpi_get_parent()
> >      on a child of the root, it returns acpi_gbl_root_node, not
> >      ACPI_ROOT_OBJECT.  This is a bit messy, and it seems safest to
> >      me to just avoid testing against ACPI_ROOT_OBJECT at all.
> > 
> >   2) It's a reminder that the acpi_device tree data structure should
> >      be a proper fully-connected tree with exactly one root.  If we
> >      assume a valid tree, there's exactly one node with no parent.
> >      That's more fundamental than the handle, where you have to
> >      analyze more code to make sure we don't assign ACPI_ROOT_OBJECT
> >      to several nodes.
> > 
> >      (There is existing code that allows disconnected trees to be built.
> >      For example, dock_create_acpi_device() can call acpi_bus_add()
> >      with a NULL parent pointer.  This "shouldn't happen," but I
> >      consider the fact that the interface allows this to be a bug.
> >      That's one reason I'm removing the "parent" argument from
> >      acpi_add_single_object() and related interfaces.)
> > 
> >   3) We build acpi_devices for things with no handle, e.g., functional
> >      fixed hardware.  Their handles happen to be NULL but I prefer to
> >      think of them as undefined and not rely on them at all.
>        Is it possible to build the acpi_device without acpi handle? It
> seems that now we won't create a new acpi device for the functional
> fixed hardware.For example: the acpi handle is NULL when we add the acpi
> device for fixed power/sleep button.  In such case it is still different
> with the ACPI

This paragraph looks truncated, so maybe you didn't quite finish it.  In
any case, we do build acpi_devices for functional fixed hardware in
acpi_bus_scan_fixed(), and those devices have a NULL handle.

> >   4) acpi_walk_namespace(..., ACPI_ROOT_OBJECT, ...) doesn't actually
> >      visit the root node; it starts with the children.  I consider
> >      this a bug.  If it were ever fixed, we would probably call the
> >      callback with acpi_gbl_root_node, not ACPI_ROOT_OBJECT, so the
> >      root acpi_device handle would no longer be ACPI_ROOT_OBJECT.
>        When the ACPI_ROOT_OBJECT is passed to acpi_walk_namespace, then
> it will start from acpi_gbl_root_node(this is changed in the function of
> acpi_ns_walk_namespace).

Yes.  But the callback is never called for the root node, only for the
*children* of that node.  That's why, in patch 14/17 of this series,
where I convert acpi_bus_scan() to use acpi_walk_namespace(), I have to
call acpi_bus_check_add() once by hand on the root of the tree, followed
by acpi_walk_namespace() to walk the rest of the tree.

Bjorn

> > > > +
> > > >  static LIST_HEAD(acpi_device_list);
> > > >  static LIST_HEAD(acpi_bus_id_list);
> > > >  DEFINE_MUTEX(acpi_device_lock);
> > > > @@ -955,10 +957,12 @@ static void acpi_device_get_busid(struct acpi_device *device)
> > > >  	 * The device's Bus ID is simply the object name.
> > > >  	 * TBD: Shouldn't this value be unique (within the ACPI namespace)?
> > > >  	 */
> > > > -	switch (device->device_type) {
> > > > -	case ACPI_BUS_TYPE_SYSTEM:
> > > > +	if (ACPI_IS_ROOT_DEVICE(device)) {
> > > >  		strcpy(device->pnp.bus_id, "ACPI");
> > > > -		break;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	switch (device->device_type) {
> > > >  	case ACPI_BUS_TYPE_POWER_BUTTON:
> > > >  		strcpy(device->pnp.bus_id, "PWRF");
> > > >  		break;
> > > > @@ -1093,6 +1097,11 @@ static void acpi_device_set_id(struct acpi_device *device)
> > > >  
> > > >  	switch (device->device_type) {
> > > >  	case ACPI_BUS_TYPE_DEVICE:
> > > > +		if (ACPI_IS_ROOT_DEVICE(device)) {
> > > > +			hid = ACPI_SYSTEM_HID;
> > > > +			break;
> > > > +		}
> > > > +
> > > >  		status = acpi_get_object_info(device->handle, &info);
> > > >  		if (ACPI_FAILURE(status)) {
> > > >  			printk(KERN_ERR PREFIX "%s: Error reading device info\n", __func__);
> > > > @@ -1129,9 +1138,6 @@ static void acpi_device_set_id(struct acpi_device *device)
> > > >  	case ACPI_BUS_TYPE_PROCESSOR:
> > > >  		hid = ACPI_PROCESSOR_OBJECT_HID;
> > > >  		break;
> > > > -	case ACPI_BUS_TYPE_SYSTEM:
> > > > -		hid = ACPI_SYSTEM_HID;
> > > > -		break;
> > > >  	case ACPI_BUS_TYPE_THERMAL:
> > > >  		hid = ACPI_THERMAL_HID;
> > > >  		break;
> > > > @@ -1643,7 +1649,7 @@ int __init acpi_scan_init(void)
> > > >  	 * Create the root device in the bus's device tree
> > > >  	 */
> > > >  	result = acpi_add_single_object(&acpi_root, ACPI_ROOT_OBJECT,
> > > > -					ACPI_BUS_TYPE_SYSTEM, &ops);
> > > > +					ACPI_BUS_TYPE_DEVICE, &ops);
> > > >  	if (result)
> > > >  		goto Done;
> > > >  
> > > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> > > > index 8e39b3e..ef1cb23 100644
> > > > --- a/include/acpi/acpi_bus.h
> > > > +++ b/include/acpi/acpi_bus.h
> > > > @@ -70,7 +70,6 @@ enum acpi_bus_device_type {
> > > >  	ACPI_BUS_TYPE_POWER,
> > > >  	ACPI_BUS_TYPE_PROCESSOR,
> > > >  	ACPI_BUS_TYPE_THERMAL,
> > > > -	ACPI_BUS_TYPE_SYSTEM,
> > > >  	ACPI_BUS_TYPE_POWER_BUTTON,
> > > >  	ACPI_BUS_TYPE_SLEEP_BUTTON,
> > > >  	ACPI_BUS_DEVICE_TYPE_COUNT
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > > 
> > 
> > 
> 

[PATCH v3 14/17] ACPI: use acpi_walk_namespace() to enumerate devices

[Bjorn Helgaas]

acpi_bus_scan() currently walks the namespace manually.  This patch changes
it to use acpi_walk_namespace() instead.

Besides removing some complicated code, this means we take advantage of the
namespace locking done by acpi_walk_namespace().  The locking isn't so
important at boot-time, but I hope to eventually use this same path to
handle hot-addition of devices, when it will be important.

Note that acpi_walk_namespace() does not actually visit the starting node
first, so we need to do that by hand first.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |  196 +++++++++++++++++++--------------------------------
 1 files changed, 74 insertions(+), 122 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 0b5aaf0..ed2b5f9 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1393,123 +1393,92 @@ end:
 	return result;
 }
 
-static int acpi_bus_scan(acpi_handle handle, struct acpi_bus_ops *ops)
+static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
+				      void *context, void **return_value)
 {
 	acpi_status status = AE_OK;
-	struct acpi_device *parent = NULL;
-	struct acpi_device *child = NULL;
-	acpi_handle phandle = NULL;
-	acpi_handle chandle = NULL;
+	struct acpi_device *device = NULL;
 	acpi_object_type type = 0;
-	u32 level = 1;
-	int ret;
+	struct acpi_bus_ops *ops = context;
 
-	/*
-	 * We must have an acpi_device for the starting node already, and
-	 * we scan its children.
-	 */
-	phandle = handle;
-	ret = acpi_bus_get_device(phandle, &parent);
-	if (ret)
-		return ret;
+	status = acpi_get_type(handle, &type);
+	if (ACPI_FAILURE(status))
+		return AE_OK;
 
 	/*
-	 * Parse through the ACPI namespace, identify all 'devices', and
-	 * create a new 'struct acpi_device' for each.
+	 * We're only interested in objects that we consider 'devices'.
 	 */
-	while ((level > 0) && parent) {
+	switch (type) {
+	case ACPI_TYPE_ANY:		/* for ACPI_ROOT_OBJECT */
+	case ACPI_TYPE_DEVICE:
+		type = ACPI_BUS_TYPE_DEVICE;
+		break;
+	case ACPI_TYPE_PROCESSOR:
+		type = ACPI_BUS_TYPE_PROCESSOR;
+		break;
+	case ACPI_TYPE_THERMAL:
+		type = ACPI_BUS_TYPE_THERMAL;
+		break;
+	case ACPI_TYPE_POWER:
+		type = ACPI_BUS_TYPE_POWER;
+		break;
+	default:
+		return AE_OK;
+	}
 
-		status = acpi_get_next_object(ACPI_TYPE_ANY, phandle,
-					      chandle, &chandle);
+	if (ops->acpi_op_add)
+		status = acpi_add_single_object(&device, handle, type, ops);
+	else
+		status = acpi_bus_get_device(handle, &device);
 
-		/*
-		 * If this scope is exhausted then move our way back up.
-		 */
-		if (ACPI_FAILURE(status)) {
-			level--;
-			chandle = phandle;
-			acpi_get_parent(phandle, &phandle);
-			if (parent->parent)
-				parent = parent->parent;
-			continue;
-		}
+	if (ACPI_FAILURE(status))
+		return AE_CTRL_DEPTH;
 
-		status = acpi_get_type(chandle, &type);
+	if (ops->acpi_op_start && !(ops->acpi_op_add)) {
+		status = acpi_start_single_object(device);
 		if (ACPI_FAILURE(status))
-			continue;
-
-		/*
-		 * If this is a scope object then parse it (depth-first).
-		 */
-		if (type == ACPI_TYPE_LOCAL_SCOPE) {
-			level++;
-			phandle = chandle;
-			chandle = NULL;
-			continue;
-		}
+			return AE_CTRL_DEPTH;
+	}
 
-		/*
-		 * We're only interested in objects that we consider 'devices'.
-		 */
-		switch (type) {
-		case ACPI_TYPE_DEVICE:
-			type = ACPI_BUS_TYPE_DEVICE;
-			break;
-		case ACPI_TYPE_PROCESSOR:
-			type = ACPI_BUS_TYPE_PROCESSOR;
-			break;
-		case ACPI_TYPE_THERMAL:
-			type = ACPI_BUS_TYPE_THERMAL;
-			break;
-		case ACPI_TYPE_POWER:
-			type = ACPI_BUS_TYPE_POWER;
-			break;
-		default:
-			continue;
-		}
+	/*
+	 * If the device is present, enabled, and functioning then
+	 * parse its scope (depth-first).  Note that we need to
+	 * represent absent devices to facilitate PnP notifications
+	 * -- but only the subtree head (not all of its children,
+	 * which will be enumerated when the parent is inserted).
+	 *
+	 * TBD: Need notifications and other detection mechanisms
+	 *      in place before we can fully implement this.
+	 *
+	 * When the device is not present but functional, it is also
+	 * necessary to scan the children of this device.
+	 */
+	if (!device->status.present && !device->status.functional)
+		return AE_CTRL_DEPTH;
 
-		if (ops->acpi_op_add)
-			status = acpi_add_single_object(&child, chandle, type,
-							ops);
-		else
-			status = acpi_bus_get_device(chandle, &child);
+	if (!*return_value)
+		*return_value = device;
+	return AE_OK;
+}
 
-		if (ACPI_FAILURE(status))
-			continue;
+static int acpi_bus_scan(acpi_handle handle, struct acpi_bus_ops *ops,
+			 struct acpi_device **child)
+{
+	acpi_status status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	void *device = NULL;
 
-		if (ops->acpi_op_start && !(ops->acpi_op_add)) {
-			status = acpi_start_single_object(child);
-			if (ACPI_FAILURE(status))
-				continue;
-		}
+	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
+	printk(KERN_INFO PREFIX "Enumerating devices from [%s]\n",
+	       (char *) buffer.pointer);
 
-		/*
-		 * If the device is present, enabled, and functioning then
-		 * parse its scope (depth-first).  Note that we need to
-		 * represent absent devices to facilitate PnP notifications
-		 * -- but only the subtree head (not all of its children,
-		 * which will be enumerated when the parent is inserted).
-		 *
-		 * TBD: Need notifications and other detection mechanisms
-		 *      in place before we can fully implement this.
-		 */
-		 /*
-		 * When the device is not present but functional, it is also
-		 * necessary to scan the children of this device.
-		 */
-		if (child->status.present || (!child->status.present &&
-					child->status.functional)) {
-			status = acpi_get_next_object(ACPI_TYPE_ANY, chandle,
-						      NULL, NULL);
-			if (ACPI_SUCCESS(status)) {
-				level++;
-				phandle = chandle;
-				chandle = NULL;
-				parent = child;
-			}
-		}
-	}
+	status = acpi_bus_check_add(handle, 0, ops, &device);
+	if (ACPI_SUCCESS(status))
+		acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX,
+				    acpi_bus_check_add, ops, &device);
 
+	if (child)
+		*child = device;
 	return 0;
 }
 
@@ -1517,33 +1486,25 @@ int
 acpi_bus_add(struct acpi_device **child,
 	     struct acpi_device *parent, acpi_handle handle, int type)
 {
-	int result;
 	struct acpi_bus_ops ops;
 
 	memset(&ops, 0, sizeof(ops));
 	ops.acpi_op_add = 1;
 
-	result = acpi_add_single_object(child, handle, type, &ops);
-	if (!result)
-		result = acpi_bus_scan((*child)->handle, &ops);
-
-	return result;
+	acpi_bus_scan(handle, &ops, child);
+	return 0;
 }
 EXPORT_SYMBOL(acpi_bus_add);
 
 int acpi_bus_start(struct acpi_device *device)
 {
-	int result;
 	struct acpi_bus_ops ops;
 
 	memset(&ops, 0, sizeof(ops));
 	ops.acpi_op_start = 1;
 
-	result = acpi_start_single_object(device);
-	if (!result)
-		result = acpi_bus_scan(device->handle, &ops);
-
-	return result;
+	acpi_bus_scan(device->handle, &ops, NULL);
+	return 0;
 }
 EXPORT_SYMBOL(acpi_bus_start);
 
@@ -1646,17 +1607,9 @@ int __init acpi_scan_init(void)
 	}
 
 	/*
-	 * Create the root device in the bus's device tree
-	 */
-	result = acpi_add_single_object(&acpi_root, ACPI_ROOT_OBJECT,
-					ACPI_BUS_TYPE_DEVICE, &ops);
-	if (result)
-		goto Done;
-
-	/*
 	 * Enumerate devices in the ACPI namespace.
 	 */
-	result = acpi_bus_scan(acpi_root->handle, &ops);
+	result = acpi_bus_scan(ACPI_ROOT_OBJECT, &ops, &acpi_root);
 
 	if (!result)
 		result = acpi_bus_scan_fixed();
@@ -1664,6 +1617,5 @@ int __init acpi_scan_init(void)
 	if (result)
 		acpi_device_unregister(acpi_root, ACPI_BUS_REMOVAL_NORMAL);
 
-Done:
 	return result;
 }

[PATCH v3 15/17] ACPI: add acpi_bus_get_status_handle()

[Bjorn Helgaas]

Add acpi_bus_get_status_handle() so we can get the status of a namespace
object before building a struct acpi_device.

This removes a use of "device->flags.dynamic_status", a cached indicator of
whether _STA exists.  It seems simpler and more reliable to just evaluate
_STA and catch AE_NOT_FOUND errors.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/bus.c      |   49 +++++++++++++++++++++--------------------------
 include/acpi/acpi_bus.h |    2 ++
 2 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 135fbfe..7411915 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -94,36 +94,33 @@ int acpi_bus_get_device(acpi_handle handle, struct acpi_device **device)
 
 EXPORT_SYMBOL(acpi_bus_get_device);
 
-int acpi_bus_get_status(struct acpi_device *device)
+acpi_status acpi_bus_get_status_handle(acpi_handle handle,
+				       unsigned long long *sta)
 {
-	acpi_status status = AE_OK;
-	unsigned long long sta = 0;
-
+	acpi_status status;
 
-	if (!device)
-		return -EINVAL;
+	status = acpi_evaluate_integer(handle, "_STA", NULL, sta);
+	if (ACPI_SUCCESS(status))
+		return AE_OK;
 
-	/*
-	 * Evaluate _STA if present.
-	 */
-	if (device->flags.dynamic_status) {
-		status =
-		    acpi_evaluate_integer(device->handle, "_STA", NULL, &sta);
-		if (ACPI_FAILURE(status))
-			return -ENODEV;
-		STRUCT_TO_INT(device->status) = (int)sta;
+	if (status == AE_NOT_FOUND) {
+		*sta = ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED |
+		       ACPI_STA_DEVICE_UI      | ACPI_STA_DEVICE_FUNCTIONING;
+		return AE_OK;
 	}
+	return status;
+}
 
-	/*
-	 * According to ACPI spec some device can be present and functional
-	 * even if the parent is not present but functional.
-	 * In such conditions the child device should not inherit the status
-	 * from the parent.
-	 */
-	else
-		STRUCT_TO_INT(device->status) =
-		    ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED |
-		    ACPI_STA_DEVICE_UI      | ACPI_STA_DEVICE_FUNCTIONING;
+int acpi_bus_get_status(struct acpi_device *device)
+{
+	acpi_status status;
+	unsigned long long sta;
+
+	status = acpi_bus_get_status_handle(device->handle, &sta);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	STRUCT_TO_INT(device->status) = (int) sta;
 
 	if (device->status.functional && !device->status.present) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] status [%08x]: "
@@ -135,10 +132,8 @@ int acpi_bus_get_status(struct acpi_device *device)
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Device [%s] status [%08x]\n",
 			  device->pnp.bus_id,
 			  (u32) STRUCT_TO_INT(device->status)));
-
 	return 0;
 }
-
 EXPORT_SYMBOL(acpi_bus_get_status);
 
 void acpi_bus_private_data_handler(acpi_handle handle,
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index ef1cb23..13a63a6 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -322,6 +322,8 @@ extern void unregister_acpi_bus_notifier(struct notifier_block *nb);
 
 int acpi_bus_get_device(acpi_handle handle, struct acpi_device **device);
 void acpi_bus_data_handler(acpi_handle handle, void *context);
+acpi_status acpi_bus_get_status_handle(acpi_handle handle,
+				       unsigned long long *sta);
 int acpi_bus_get_status(struct acpi_device *device);
 int acpi_bus_get_power(acpi_handle handle, int *state);
 int acpi_bus_set_power(acpi_handle handle, int state);

[PATCH v3 16/17] ACPI: factor out device type and status checking

[Bjorn Helgaas]

This patch adds acpi_bus_type_and_status(), which determines the type
of the object and whether we want to build an acpi_device for it.  If
it is acpi_device-worthy, it returns the type and the device's current
status.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |  129 +++++++++++++++++++++------------------------------
 1 files changed, 52 insertions(+), 77 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index ed2b5f9..954bd01 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1252,6 +1252,7 @@ static int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
 
 static int acpi_add_single_object(struct acpi_device **child,
 				  acpi_handle handle, int type,
+				  unsigned long long sta,
 				  struct acpi_bus_ops *ops)
 {
 	int result;
@@ -1268,61 +1269,21 @@ static int acpi_add_single_object(struct acpi_device **child,
 	device->handle = handle;
 	device->parent = acpi_bus_get_parent(handle);
 	device->bus_ops = *ops; /* workround for not call .start */
+	STRUCT_TO_INT(device->status) = sta;
 
 	acpi_device_get_busid(device);
 
 	/*
 	 * Flags
 	 * -----
-	 * Get prior to calling acpi_bus_get_status() so we know whether
-	 * or not _STA is present.  Note that we only look for object
-	 * handles -- cannot evaluate objects until we know the device is
-	 * present and properly initialized.
+	 * Note that we only look for object handles -- cannot evaluate objects
+	 * until we know the device is present and properly initialized.
 	 */
 	result = acpi_bus_get_flags(device);
 	if (result)
 		goto end;
 
 	/*
-	 * Status
-	 * ------
-	 * See if the device is present.  We always assume that non-Device
-	 * and non-Processor objects (e.g. thermal zones, power resources,
-	 * etc.) are present, functioning, etc. (at least when parent object
-	 * is present).  Note that _STA has a different meaning for some
-	 * objects (e.g. power resources) so we need to be careful how we use
-	 * it.
-	 */
-	switch (type) {
-	case ACPI_BUS_TYPE_PROCESSOR:
-	case ACPI_BUS_TYPE_DEVICE:
-		result = acpi_bus_get_status(device);
-		if (ACPI_FAILURE(result)) {
-			result = -ENODEV;
-			goto end;
-		}
-		/*
-		 * When the device is neither present nor functional, the
-		 * device should not be added to Linux ACPI device tree.
-		 * When the status of the device is not present but functinal,
-		 * it should be added to Linux ACPI tree. For example : bay
-		 * device , dock device.
-		 * In such conditions it is unncessary to check whether it is
-		 * bay device or dock device.
-		 */
-		if (!device->status.present && !device->status.functional) {
-			result = -ENODEV;
-			goto end;
-		}
-		break;
-	default:
-		STRUCT_TO_INT(device->status) =
-		    ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED |
-		    ACPI_STA_DEVICE_UI      | ACPI_STA_DEVICE_FUNCTIONING;
-		break;
-	}
-
-	/*
 	 * Initialize Device
 	 * -----------------
 	 * TBD: Synch with Core's enumeration/initialization process.
@@ -1393,41 +1354,69 @@ end:
 	return result;
 }
 
-static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
-				      void *context, void **return_value)
+#define ACPI_STA_DEFAULT (ACPI_STA_DEVICE_PRESENT | ACPI_STA_DEVICE_ENABLED | \
+			  ACPI_STA_DEVICE_UI      | ACPI_STA_DEVICE_FUNCTIONING)
+
+static int acpi_bus_type_and_status(acpi_handle handle, int *type,
+				    unsigned long long *sta)
 {
-	acpi_status status = AE_OK;
-	struct acpi_device *device = NULL;
-	acpi_object_type type = 0;
-	struct acpi_bus_ops *ops = context;
+	acpi_status status;
+	acpi_object_type acpi_type;
 
-	status = acpi_get_type(handle, &type);
+	status = acpi_get_type(handle, &acpi_type);
 	if (ACPI_FAILURE(status))
-		return AE_OK;
+		return -ENODEV;
 
-	/*
-	 * We're only interested in objects that we consider 'devices'.
-	 */
-	switch (type) {
+	switch (acpi_type) {
 	case ACPI_TYPE_ANY:		/* for ACPI_ROOT_OBJECT */
 	case ACPI_TYPE_DEVICE:
-		type = ACPI_BUS_TYPE_DEVICE;
+		*type = ACPI_BUS_TYPE_DEVICE;
+		status = acpi_bus_get_status_handle(handle, sta);
+		if (ACPI_FAILURE(status))
+			return -ENODEV;
 		break;
 	case ACPI_TYPE_PROCESSOR:
-		type = ACPI_BUS_TYPE_PROCESSOR;
+		*type = ACPI_BUS_TYPE_PROCESSOR;
+		status = acpi_bus_get_status_handle(handle, sta);
+		if (ACPI_FAILURE(status))
+			return -ENODEV;
 		break;
 	case ACPI_TYPE_THERMAL:
-		type = ACPI_BUS_TYPE_THERMAL;
+		*type = ACPI_BUS_TYPE_THERMAL;
+		*sta = ACPI_STA_DEFAULT;
 		break;
 	case ACPI_TYPE_POWER:
-		type = ACPI_BUS_TYPE_POWER;
+		*type = ACPI_BUS_TYPE_POWER;
+		*sta = ACPI_STA_DEFAULT;
 		break;
 	default:
-		return AE_OK;
+		return -ENODEV;
 	}
 
+	return 0;
+}
+
+static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
+				      void *context, void **return_value)
+{
+	struct acpi_bus_ops *ops = context;
+	struct acpi_device *device = NULL;
+	acpi_status status;
+	int type;
+	unsigned long long sta;
+	int result;
+
+	result = acpi_bus_type_and_status(handle, &type, &sta);
+	if (result)
+		return AE_OK;
+
+	if (!(sta & ACPI_STA_DEVICE_PRESENT) &&
+	    !(sta & ACPI_STA_DEVICE_FUNCTIONING))
+		return AE_CTRL_DEPTH;
+
 	if (ops->acpi_op_add)
-		status = acpi_add_single_object(&device, handle, type, ops);
+		status = acpi_add_single_object(&device, handle, type, sta,
+						ops);
 	else
 		status = acpi_bus_get_device(handle, &device);
 
@@ -1440,22 +1429,6 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
 			return AE_CTRL_DEPTH;
 	}
 
-	/*
-	 * If the device is present, enabled, and functioning then
-	 * parse its scope (depth-first).  Note that we need to
-	 * represent absent devices to facilitate PnP notifications
-	 * -- but only the subtree head (not all of its children,
-	 * which will be enumerated when the parent is inserted).
-	 *
-	 * TBD: Need notifications and other detection mechanisms
-	 *      in place before we can fully implement this.
-	 *
-	 * When the device is not present but functional, it is also
-	 * necessary to scan the children of this device.
-	 */
-	if (!device->status.present && !device->status.functional)
-		return AE_CTRL_DEPTH;
-
 	if (!*return_value)
 		*return_value = device;
 	return AE_OK;
@@ -1579,12 +1552,14 @@ static int acpi_bus_scan_fixed(void)
 	if ((acpi_gbl_FADT.flags & ACPI_FADT_POWER_BUTTON) == 0) {
 		result = acpi_add_single_object(&device, NULL,
 						ACPI_BUS_TYPE_POWER_BUTTON,
+						ACPI_STA_DEFAULT,
 						&ops);
 	}
 
 	if ((acpi_gbl_FADT.flags & ACPI_FADT_SLEEP_BUTTON) == 0) {
 		result = acpi_add_single_object(&device, NULL,
 						ACPI_BUS_TYPE_SLEEP_BUTTON,
+						ACPI_STA_DEFAULT,
 						&ops);
 	}
 

[PATCH v3 17/17] ACPI: handle re-enumeration, when acpi_devices might already exist

[Bjorn Helgaas]

acpi_bus_scan() traverses the namespace to enumerate devices and uses
acpi_add_single_object() to create acpi_devices.  When the platform
notifies us of a hot-plug event, we need to traverse part of the namespace
again to figure out what appeared or disappeared.  (We don't yet call
acpi_bus_scan() during hot-plug, but I plan to do that in the future.)

This patch makes acpi_add_single_object() notice when we already have
an acpi_device, so we don't need to make a new one.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
---
 drivers/acpi/scan.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 954bd01..2c4cac5 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1400,10 +1400,10 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
 				      void *context, void **return_value)
 {
 	struct acpi_bus_ops *ops = context;
-	struct acpi_device *device = NULL;
-	acpi_status status;
 	int type;
 	unsigned long long sta;
+	struct acpi_device *device;
+	acpi_status status;
 	int result;
 
 	result = acpi_bus_type_and_status(handle, &type, &sta);
@@ -1414,13 +1414,16 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
 	    !(sta & ACPI_STA_DEVICE_FUNCTIONING))
 		return AE_CTRL_DEPTH;
 
-	if (ops->acpi_op_add)
-		status = acpi_add_single_object(&device, handle, type, sta,
-						ops);
-	else
-		status = acpi_bus_get_device(handle, &device);
+	/*
+	 * We may already have an acpi_device from a previous enumeration.  If
+	 * so, we needn't add it again, but we may still have to start it.
+	 */
+	device = NULL;
+	acpi_bus_get_device(handle, &device);
+	if (ops->acpi_op_add && !device)
+		acpi_add_single_object(&device, handle, type, sta, ops);
 
-	if (ACPI_FAILURE(status))
+	if (!device)
 		return AE_CTRL_DEPTH;
 
 	if (ops->acpi_op_start && !(ops->acpi_op_add)) {