make every acpi_device have a HID

PATCH Superseded Bjorn Helgaas 1 participant 11 messages Aug 24, 2009

Subsystems (3)

ACPI ACPI component architecture (acpica) Pnp support

lore

Share & tools

lore

Bjorn Helgaas

author

2009-08-24 16:38 UTC #0 lore raw

cover make every acpi_device have a HID

This series was prompted by an issue we saw recently in the mm tree:
http://lkml.org/lkml/2009/7/20/422

Synopsis:

Linux/ACPI discovered a video device. The device had no _HID or _CID methods, but based on acpi_is_video_device(), we added a sythetic CID ("LNXVIDEO"), so the device ended up with a CID but no HID.

We bound the acpi_video_bus (drivers/acpi/video.c) driver to the device based on the CID. This driver has a .notify() method, so we tried to install a notify handler on the device. This uses strcmp() on the device HID to determine whether it's a fixed hardware device, and this oopsed because this device had no HID.

This used to work because the HID was stored in an array that was initialized to zeroes, but a recent change replaced this with a pointer that is NULL until a HID is set.

This thread: http://marc.info/?l=linux-acpi&m=124959955813409&w=2
has patches that work around this by basically returning a pointer to an empty string rather than a NULL pointer in this case.

However, the HID is a pretty central part of an acpi_device, and I think it complicates the driver model too much to deal with HIDs that might be empty strings. I think it's better for Linux/ACPI to just make sure that *every* acpi_device has at least one ID (either a HID, a CID, or a synthetic ID).

The main place this default "device" ID appears is with ACPI devices that just have _ADR methods, e.g., PCI slots.

This series:
- Applies on top of this previous series:
http://marc.info/?l=linux-acpi&m=124907619701194&w=2
(Len said these were applied to acpi-test, but I haven't actually seen that yet.)
- Fixes a memory leak introduced by the previous series. Could be folded into:
http://marc.info/?l=linux-acpi&m=124907622001229&w=2
if git tree management allows.
- Fixes the synthetic HID for \_SB_. This has been broken for a long time, so we've been seeing "device:00" instead of "LNXSYBUS" in sysfs.
- Makes sure every acpi_device has a default "device" HID if nothing else. (We already used "device" in sysfs if we didn't have a HID or CID.)
- Adds a single list containing the HID and any CIDs. No users make a distinction between them, so maintaining them separately just made things complicated.
- Removes the _UID stuff, which nobody uses.

---

Bjorn Helgaas (10):
ACPI: fix leak in debug code
ACPI: add ACPI_IS_ROOT_DEVICE()
ACPI: fix synthetic HID for \_SB_
ACPI: use acpi_device_hid() when possible
ACPI: make sure every acpi_device has an ID
ACPI: maintain a single list of _HID and _CID IDs
ACPI: remove acpi_device.flags.compatible_ids
ACPI: remove acpi_device.flags.hardware_id
ACPI: remove acpi_device_uid() and related stuff
ACPI: simplify building device HID/CID list

 drivers/acpi/scan.c        |  260 +++++++++++++++++---------------------------
 drivers/pnp/pnpacpi/core.c |   21 ++--
 include/acpi/acpi_bus.h    |   19 +--
 3 files changed, 117 insertions(+), 183 deletions(-)

Signature · 1 line

Bjorn

Bjorn Helgaas

author

2009-08-24 16:38 UTC #1 lore raw

PATCH 1/10 ACPI: fix leak in debug code

Free the buffer after getting the namespace pathname.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/scan.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

drivers/acpi/scan.c +1 -0

index 87d3df4..cbeb4c0 100644
@@ -1299,6 +1299,7 @@ end:
 			 (char *) buffer.pointer,
 			 device->parent ? dev_name(&device->parent->dev) :
 					  "(null)"));
+		kfree(buffer.pointer);
 		*child = device;
 	} else {
 		kfree(device->pnp.cid_list);

Bjorn Helgaas

author

2009-08-24 16:38 UTC #2 lore raw

PATCH 2/10 ACPI: add ACPI_IS_ROOT_DEVICE()

In reply to Bjorn Helgaas

This adds a macro to identify the root of the acpi_device tree.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/scan.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

drivers/acpi/scan.c +4 -2

index cbeb4c0..a253373 100644
@@ -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);

@@ -950,7 +952,7 @@ 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)?
 	 */
-	if (!device->parent) {
+	if (ACPI_IS_ROOT_DEVICE(device)) {
 		strcpy(device->pnp.bus_id, "ACPI");
 		return;
 	}

@@ -1033,7 +1035,7 @@ static void acpi_device_set_id(struct acpi_device *device)
 
 	switch (device->device_type) {
 	case ACPI_BUS_TYPE_DEVICE:
-		if (!device->parent) {
+		if (ACPI_IS_ROOT_DEVICE(device)) {
 			hid = ACPI_SYSTEM_HID;
 			break;
 		}

Bjorn Helgaas

author

2009-08-24 16:38 UTC #3 lore raw

PATCH 3/10 ACPI: fix synthetic HID for \_SB_

In reply to Bjorn Helgaas

This makes \_SB_ show up as /sys/devices/LNXSYSTM:00/LNXSYBUS:00 rather than "device:00". This has been broken for a loooong time (at least since 2.6.13) because device->parent is an acpi_device pointer, not a handle.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/scan.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

drivers/acpi/scan.c +6 ~~-12~~

index a253373..6de6b11 100644
@@ -1038,6 +1038,12 @@ static void acpi_device_set_id(struct acpi_device *device)
 		if (ACPI_IS_ROOT_DEVICE(device)) {
 			hid = ACPI_SYSTEM_HID;
 			break;
+		} else if (ACPI_IS_ROOT_DEVICE(device->parent)) {
+			/* \_SB_, the only root-level namespace device */
+			hid = ACPI_BUS_HID;
+			strcpy(device->pnp.device_name, ACPI_BUS_DEVICE_NAME);
+			strcpy(device->pnp.device_class, ACPI_BUS_CLASS);
+			break;
 		}
 
 		status = acpi_get_object_info(device->handle, &buffer);

@@ -1088,18 +1094,6 @@ static void acpi_device_set_id(struct acpi_device *device)
 		break;
 	}
 
-	/*
-	 * \_SB
-	 * ----
-	 * Fix for the system root bus device -- the only root-level device.
-	 */
-	if (((acpi_handle)device->parent == ACPI_ROOT_OBJECT) &&
-	     (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);
-	}
-
 	if (hid) {
 		strcpy(device->pnp.hardware_id, hid);
 		device->flags.hardware_id = 1;

Bjorn Helgaas

author

2009-08-24 16:38 UTC #4 lore raw

PATCH 4/10 ACPI: use acpi_device_hid() when possible

In reply to Bjorn Helgaas

Use acpi_device_hid() rather than accessing acpi_device.pnp.hardware_id directly.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/scan.c        |    6 +++---
 drivers/pnp/pnpacpi/core.c |    2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

drivers/acpi/scan.c +3 -3

index 6de6b11..8a7616f 100644
@@ -185,7 +185,7 @@ static ssize_t
 acpi_device_hid_show(struct device *dev, struct device_attribute *attr, char *buf) {
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
 
-	return sprintf(buf, "%s\n", acpi_dev->pnp.hardware_id);
+	return sprintf(buf, "%s\n", acpi_device_hid(acpi_dev));
 }
 static DEVICE_ATTR(hid, 0444, acpi_device_hid_show, NULL);

@@ -497,7 +497,7 @@ static int acpi_device_register(struct acpi_device *device)
 	 * If failed, create one and link it into acpi_bus_id_list
 	 */
 	list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) {
-		if(!strcmp(acpi_device_bus_id->bus_id, device->flags.hardware_id? device->pnp.hardware_id : "device")) {
+		if (!strcmp(acpi_device_bus_id->bus_id, device->flags.hardware_id ? acpi_device_hid(device) : "device")) {
 			acpi_device_bus_id->instance_no ++;
 			found = 1;
 			kfree(new_bus_id);

@@ -506,7 +506,7 @@ static int acpi_device_register(struct acpi_device *device)
 	}
 	if (!found) {
 		acpi_device_bus_id = new_bus_id;
-		strcpy(acpi_device_bus_id->bus_id, device->flags.hardware_id ? device->pnp.hardware_id : "device");
+		strcpy(acpi_device_bus_id->bus_id, device->flags.hardware_id ? acpi_device_hid(device) : "device");
 		acpi_device_bus_id->instance_no = 0;
 		list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
 	}

drivers/pnp/pnpacpi/core.c +1 -1

index 9496494..1f5e2c1 100644
@@ -234,7 +234,7 @@ static int __init acpi_pnp_match(struct device *dev, void *_pnp)
 	/* true means it matched */
 	return acpi->flags.hardware_id
 	    && !acpi_get_physical_device(acpi->handle)
-	    && compare_pnp_id(pnp->id, acpi->pnp.hardware_id);
+	    && compare_pnp_id(pnp->id, acpi_device_hid(acpi));
 }
 
 static int __init acpi_pnp_find_device(struct device *dev, acpi_handle * handle)

Bjorn Helgaas

author

2009-08-24 16:38 UTC #5 lore raw

PATCH 5/10 ACPI: make sure every acpi_device has an ID

In reply to Bjorn Helgaas

This makes sure every acpi_device has at least one ID. If we build an acpi_device for a namespace node with no _HID or _CID, we sometimes synthesize an ID like "LNXCPU" or "LNXVIDEO". If we don't even have that, give it a default "device" ID.

Note that this means things like:

    /sys/devices/LNXSYSTM:00/LNXSYBUS:00/HWP0001:00/HWP0002:04/device:00

(a PCI slot SxFy device) will have "hid" and "modprobe" entries, where they didn't before. These aren't very useful (a HID of "device" doesn't tell you what *kind* of device it is, so it doesn't help find a driver), but I don't think they're harmful.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/scan.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

drivers/acpi/scan.c +10 -0

index 8a7616f..88f39e1 100644
@@ -1094,6 +1094,16 @@ static void acpi_device_set_id(struct acpi_device *device)
 		break;
 	}
 
+	/*
+	 * We build acpi_devices for some objects that don't have _HID or _CID,
+	 * e.g., PCI bridges and slots.  Drivers can't bind to these objects,
+	 * but we do use them indirectly by traversing the acpi_device tree.
+	 * This generic ID isn't useful for driver binding, but it provides
+	 * the useful property that "every acpi_device has an ID."
+	 */
+	if (!hid && !cid_list && !cid_add)
+		hid = "device";
+
 	if (hid) {
 		strcpy(device->pnp.hardware_id, hid);
 		device->flags.hardware_id = 1;

Bjorn Helgaas

author

2009-08-24 16:38 UTC #6 lore raw

PATCH 6/10 ACPI: maintain a single list of _HID and _CID IDs

In reply to Bjorn Helgaas

There's no need to treat _HID and _CID differently. Keeping them in a single list makes code that uses the IDs a little simpler because it can just traverse the list rather than checking "do we have a HID?", "do we have any CIDs?"

Signed-off-by Bjorn Helgaas <[email protected]>

Reviewed-by Alex Chiang <[email protected]>

---
 drivers/acpi/scan.c        |  136 +++++++++++++++++---------------------------
 drivers/pnp/pnpacpi/core.c |   16 ++---
 include/acpi/acpi_bus.h    |   11 ++--
 3 files changed, 68 insertions(+), 95 deletions(-)

drivers/acpi/scan.c +54 ~~-82~~

index 88f39e1..76ba200 100644
@@ -45,6 +45,7 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
 {
 	int len;
 	int count;
+	struct acpi_hardware_id *id;
 
 	if (!acpi_dev->flags.hardware_id && !acpi_dev->flags.compatible_ids)
 		return -ENODEV;

@@ -52,33 +53,14 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
 	len = snprintf(modalias, size, "acpi:");
 	size -= len;
 
-	if (acpi_dev->flags.hardware_id) {
-		count = snprintf(&modalias[len], size, "%s:",
-				 acpi_dev->pnp.hardware_id);
+	list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
+		count = snprintf(&modalias[len], size, "%s:", id->id);
 		if (count < 0 || count >= size)
 			return -EINVAL;
 		len += count;
 		size -= count;
 	}
 
-	if (acpi_dev->flags.compatible_ids) {
-		struct acpi_compatible_id_list *cid_list;
-		int i;
-
-		cid_list = acpi_dev->pnp.cid_list;
-		for (i = 0; i < cid_list->count; i++) {
-			count = snprintf(&modalias[len], size, "%s:",
-					 cid_list->id[i].value);
-			if (count < 0 || count >= size) {
-				printk(KERN_ERR PREFIX "%s cid[%i] exceeds event buffer size",
-				       acpi_dev->pnp.device_name, i);
-				break;
-			}
-			len += count;
-			size -= count;
-		}
-	}
-
 	modalias[len] = '\0';
 	return len;
 }

@@ -273,6 +255,7 @@ int acpi_match_device_ids(struct acpi_device *device,
 			  const struct acpi_device_id *ids)
 {
 	const struct acpi_device_id *id;
+	struct acpi_hardware_id *hwid;
 
 	/*
 	 * If the device is not present, it is unnecessary to load device

@@ -281,36 +264,30 @@ int acpi_match_device_ids(struct acpi_device *device,
 	if (!device->status.present)
 		return -ENODEV;
 
-	if (device->flags.hardware_id) {
-		for (id = ids; id->id[0]; id++) {
-			if (!strcmp((char*)id->id, device->pnp.hardware_id))
+	for (id = ids; id->id[0]; id++)
+		list_for_each_entry(hwid, &device->pnp.ids, list)
+			if (!strcmp((char *) id->id, hwid->id))
 				return 0;
-		}
-	}
-
-	if (device->flags.compatible_ids) {
-		struct acpi_compatible_id_list *cid_list = device->pnp.cid_list;
-		int i;
-
-		for (id = ids; id->id[0]; id++) {
-			/* compare multiple _CID entries against driver ids */
-			for (i = 0; i < cid_list->count; i++) {
-				if (!strcmp((char*)id->id,
-					    cid_list->id[i].value))
-					return 0;
-			}
-		}
-	}
 
 	return -ENOENT;
 }
 EXPORT_SYMBOL(acpi_match_device_ids);
 
+static void acpi_free_ids(struct acpi_device *device)
+{
+	struct acpi_hardware_id *id, *tmp;
+
+	list_for_each_entry_safe(id, tmp, &device->pnp.ids, list) {
+		kfree(id->id);
+		kfree(id);
+	}
+}
+
 static void acpi_device_release(struct device *dev)
 {
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
 
-	kfree(acpi_dev->pnp.cid_list);
+	acpi_free_ids(acpi_dev);
 	kfree(acpi_dev);
 }

@@ -1023,6 +1000,32 @@ static int acpi_dock_match(struct acpi_device *device)
 	return acpi_get_handle(device->handle, "_DCK", &tmp);
 }
 
+char *acpi_device_hid(struct acpi_device *device)
+{
+	struct acpi_hardware_id *hid;
+
+	hid = list_first_entry(&device->pnp.ids, struct acpi_hardware_id, list);
+	return hid->id;
+}
+
+static void acpi_add_id(struct acpi_device *device, const char *dev_id)
+{
+	struct acpi_hardware_id *id;
+
+	id = kmalloc(sizeof(*id), GFP_KERNEL);
+	if (!id)
+		return;
+
+	id->id = kmalloc(strlen(dev_id) + 1, GFP_KERNEL);
+	if (!id->id) {
+		kfree(id);
+		return;
+	}
+
+	strcpy(id->id, dev_id);
+	list_add_tail(&id->list, &device->pnp.ids);
+}
+
 static void acpi_device_set_id(struct acpi_device *device)
 {
 	struct acpi_device_info *info;

@@ -1032,6 +1035,7 @@ static void acpi_device_set_id(struct acpi_device *device)
 	struct acpi_compatible_id_list *cid_list = NULL;
 	const char *cid_add = NULL;
 	acpi_status status;
+	int i;
 
 	switch (device->device_type) {
 	case ACPI_BUS_TYPE_DEVICE:

@@ -1105,52 +1109,19 @@ static void acpi_device_set_id(struct acpi_device *device)
 		hid = "device";
 
 	if (hid) {
-		strcpy(device->pnp.hardware_id, hid);
+		acpi_add_id(device, hid);
 		device->flags.hardware_id = 1;
 	}
 	if (uid) {
 		strcpy(device->pnp.unique_id, uid);
 		device->flags.unique_id = 1;
 	}
-	if (cid_list || cid_add) {
-		struct  acpi_compatible_id_list *list;
-		int size = 0;
-		int count = 0;
-
-		if (cid_list) {
-			size = cid_list->size;
-		} else if (cid_add) {
-			size = sizeof(struct acpi_compatible_id_list);
-			cid_list = ACPI_ALLOCATE_ZEROED((acpi_size) size);
-			if (!cid_list) {
-				printk(KERN_ERR "Memory allocation error\n");
-				kfree(buffer.pointer);
-				return;
-			} else {
-				cid_list->count = 0;
-				cid_list->size = size;
-			}
-		}
-		if (cid_add)
-			size += sizeof(struct acpi_compatible_id);
-		list = kmalloc(size, GFP_KERNEL);
-
-		if (list) {
-			if (cid_list) {
-				memcpy(list, cid_list, cid_list->size);
-				count = cid_list->count;
-			}
-			if (cid_add) {
-				strncpy(list->id[count].value, cid_add,
-					ACPI_MAX_CID_LENGTH);
-				count++;
-				device->flags.compatible_ids = 1;
-			}
-			list->size = size;
-			list->count = count;
-			device->pnp.cid_list = list;
-		} else
-			printk(KERN_ERR PREFIX "Memory allocation error\n");
+	if (cid_list)
+		for (i = 0; i < cid_list->count; i++)
+			acpi_add_id(device, cid_list->id[i].value);
+	if (cid_add) {
+		acpi_add_id(device, cid_add);
+		device->flags.compatible_ids = 1;
 	}
 
 	kfree(buffer.pointer);

@@ -1217,6 +1188,7 @@ static int acpi_add_single_object(struct acpi_device **child,
 		return -ENOMEM;
 	}
 
+	INIT_LIST_HEAD(&device->pnp.ids);
 	device->device_type = type;
 	device->handle = handle;
 	device->parent = acpi_bus_get_parent(handle);

@@ -1308,7 +1280,7 @@ end:
 		kfree(buffer.pointer);
 		*child = device;
 	} else {
-		kfree(device->pnp.cid_list);
+		acpi_free_ids(device);
 		kfree(device);
 	}

drivers/pnp/pnpacpi/core.c +7 -9

index 1f5e2c1..3a4478f 100644
@@ -153,6 +153,7 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	acpi_handle temp = NULL;
 	acpi_status status;
 	struct pnp_dev *dev;
+	struct acpi_hardware_id *id;
 
 	/*
 	 * If a PnPacpi device is not present , the device

@@ -193,15 +194,12 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	if (dev->capabilities & PNP_CONFIGURABLE)
 		pnpacpi_parse_resource_option_data(dev);
 
-	if (device->flags.compatible_ids) {
-		struct acpi_compatible_id_list *cid_list = device->pnp.cid_list;
-		int i;
-
-		for (i = 0; i < cid_list->count; i++) {
-			if (!ispnpidacpi(cid_list->id[i].value))
-				continue;
-			pnp_add_id(dev, cid_list->id[i].value);
-		}
+	list_for_each_entry(id, &device->pnp.ids, list) {
+		if (!strcmp(id->id, acpi_device_hid(device)))
+			continue;
+		if (!ispnpidacpi(id->id))
+			continue;
+		pnp_add_id(dev, id->id);
 	}
 
 	/* clear out the damaged flags */

include/acpi/acpi_bus.h +7 -4

index 26be825..3b3fef5 100644
@@ -170,16 +170,19 @@ struct acpi_device_dir {
 
 typedef char acpi_bus_id[8];
 typedef unsigned long acpi_bus_address;
-typedef char acpi_hardware_id[15];
 typedef char acpi_unique_id[9];
 typedef char acpi_device_name[40];
 typedef char acpi_device_class[20];
 
+struct acpi_hardware_id {
+	struct list_head list;
+	char *id;
+};
+
 struct acpi_device_pnp {
 	acpi_bus_id bus_id;	/* Object name */
 	acpi_bus_address bus_address;	/* _ADR */
-	acpi_hardware_id hardware_id;	/* _HID */
-	struct acpi_compatible_id_list *cid_list;	/* _CIDs */
+	struct list_head ids;		/* _HID and _CIDs */
 	acpi_unique_id unique_id;	/* _UID */
 	acpi_device_name device_name;	/* Driver-determined */
 	acpi_device_class device_class;	/*        "          */

@@ -187,7 +190,7 @@ struct acpi_device_pnp {
 
 #define acpi_device_bid(d)	((d)->pnp.bus_id)
 #define acpi_device_adr(d)	((d)->pnp.bus_address)
-#define acpi_device_hid(d)	((d)->pnp.hardware_id)
+char *acpi_device_hid(struct acpi_device *device);
 #define acpi_device_uid(d)	((d)->pnp.unique_id)
 #define acpi_device_name(d)	((d)->pnp.device_name)
 #define acpi_device_class(d)	((d)->pnp.device_class)

Bjorn Helgaas

author

2009-08-24 16:39 UTC #7 lore raw

PATCH 7/10 ACPI: remove acpi_device.flags.compatible_ids

In reply to Bjorn Helgaas

We now keep a single list of IDs that includes both the _HID and any _CIDs. We no longer need to keep track of whether the device has a _CID.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/scan.c     |   15 ++++-----------
 include/acpi/acpi_bus.h |    3 +--
 2 files changed, 5 insertions(+), 13 deletions(-)

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

index 76ba200..f389510 100644
@@ -47,7 +47,7 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
 	int count;
 	struct acpi_hardware_id *id;
 
-	if (!acpi_dev->flags.hardware_id && !acpi_dev->flags.compatible_ids)
+	if (!acpi_dev->flags.hardware_id)
 		return -ENODEV;
 
 	len = snprintf(modalias, size, "acpi:");

@@ -209,7 +209,7 @@ static int acpi_device_setup_files(struct acpi_device *dev)
 			goto end;
 	}
 
-	if (dev->flags.hardware_id || dev->flags.compatible_ids) {
+	if (dev->flags.hardware_id) {
 		result = device_create_file(&dev->dev, &dev_attr_modalias);
 		if (result)
 			goto end;

@@ -239,7 +239,7 @@ static void acpi_device_remove_files(struct acpi_device *dev)
 	if (ACPI_SUCCESS(status))
 		device_remove_file(&dev->dev, &dev_attr_eject);
 
-	if (dev->flags.hardware_id || dev->flags.compatible_ids)
+	if (dev->flags.hardware_id)
 		device_remove_file(&dev->dev, &dev_attr_modalias);
 
 	if (dev->flags.hardware_id)

@@ -875,11 +875,6 @@ static int acpi_bus_get_flags(struct acpi_device *device)
 	if (ACPI_SUCCESS(status))
 		device->flags.dynamic_status = 1;
 
-	/* Presence of _CID indicates 'compatible_ids' */
-	status = acpi_get_handle(device->handle, "_CID", &temp);
-	if (ACPI_SUCCESS(status))
-		device->flags.compatible_ids = 1;
-
 	/* Presence of _RMV indicates 'removable' */
 	status = acpi_get_handle(device->handle, "_RMV", &temp);
 	if (ACPI_SUCCESS(status))

@@ -1119,10 +1114,8 @@ static void acpi_device_set_id(struct acpi_device *device)
 	if (cid_list)
 		for (i = 0; i < cid_list->count; i++)
 			acpi_add_id(device, cid_list->id[i].value);
-	if (cid_add) {
+	if (cid_add)
 		acpi_add_id(device, cid_add);
-		device->flags.compatible_ids = 1;
-	}
 
 	kfree(buffer.pointer);
 }

include/acpi/acpi_bus.h +1 -2

index 3b3fef5..c7702a5 100644
@@ -144,7 +144,6 @@ struct acpi_device_status {
 struct acpi_device_flags {
 	u32 dynamic_status:1;
 	u32 hardware_id:1;
-	u32 compatible_ids:1;
 	u32 bus_address:1;
 	u32 unique_id:1;
 	u32 removable:1;

@@ -155,7 +154,7 @@ struct acpi_device_flags {
 	u32 performance_manageable:1;
 	u32 wake_capable:1;	/* Wakeup(_PRW) supported? */
 	u32 force_power_state:1;
-	u32 reserved:19;
+	u32 reserved:20;
 };
 
 /* File System */

Bjorn Helgaas

author

2009-08-24 16:39 UTC #8 lore raw

PATCH 8/10 ACPI: remove acpi_device.flags.hardware_id

In reply to Bjorn Helgaas

Every acpi_device has at least one ID (if there's no _HID or _CID, we give it a synthetic or default ID). So there's no longer a need to check whether an ID exists; we can just use it.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/scan.c        |   37 +++++++++++++------------------------
 drivers/pnp/pnpacpi/core.c |    3 +--
 include/acpi/acpi_bus.h    |    3 +--
 3 files changed, 15 insertions(+), 28 deletions(-)

drivers/acpi/scan.c +13 ~~-24~~

index f389510..c181728 100644
@@ -47,9 +47,6 @@ static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
 	int count;
 	struct acpi_hardware_id *id;
 
-	if (!acpi_dev->flags.hardware_id)
-		return -ENODEV;
-
 	len = snprintf(modalias, size, "acpi:");
 	size -= len;

@@ -203,17 +200,13 @@ static int acpi_device_setup_files(struct acpi_device *dev)
 			goto end;
 	}
 
-	if (dev->flags.hardware_id) {
-		result = device_create_file(&dev->dev, &dev_attr_hid);
-		if (result)
-			goto end;
-	}
+	result = device_create_file(&dev->dev, &dev_attr_hid);
+	if (result)
+		goto end;
 
-	if (dev->flags.hardware_id) {
-		result = device_create_file(&dev->dev, &dev_attr_modalias);
-		if (result)
-			goto end;
-	}
+	result = device_create_file(&dev->dev, &dev_attr_modalias);
+	if (result)
+		goto end;
 
         /*
          * If device has _EJ0, 'eject' file is created that is used to trigger

@@ -239,11 +232,8 @@ static void acpi_device_remove_files(struct acpi_device *dev)
 	if (ACPI_SUCCESS(status))
 		device_remove_file(&dev->dev, &dev_attr_eject);
 
-	if (dev->flags.hardware_id)
-		device_remove_file(&dev->dev, &dev_attr_modalias);
-
-	if (dev->flags.hardware_id)
-		device_remove_file(&dev->dev, &dev_attr_hid);
+	device_remove_file(&dev->dev, &dev_attr_modalias);
+	device_remove_file(&dev->dev, &dev_attr_hid);
 	if (dev->handle)
 		device_remove_file(&dev->dev, &dev_attr_path);
 }

@@ -474,8 +464,9 @@ static int acpi_device_register(struct acpi_device *device)
 	 * If failed, create one and link it into acpi_bus_id_list
 	 */
 	list_for_each_entry(acpi_device_bus_id, &acpi_bus_id_list, node) {
-		if (!strcmp(acpi_device_bus_id->bus_id, device->flags.hardware_id ? acpi_device_hid(device) : "device")) {
-			acpi_device_bus_id->instance_no ++;
+		if (!strcmp(acpi_device_bus_id->bus_id,
+			    acpi_device_hid(device))) {
+			acpi_device_bus_id->instance_no++;
 			found = 1;
 			kfree(new_bus_id);
 			break;

@@ -483,7 +474,7 @@ static int acpi_device_register(struct acpi_device *device)
 	}
 	if (!found) {
 		acpi_device_bus_id = new_bus_id;
-		strcpy(acpi_device_bus_id->bus_id, device->flags.hardware_id ? acpi_device_hid(device) : "device");
+		strcpy(acpi_device_bus_id->bus_id, acpi_device_hid(device));
 		acpi_device_bus_id->instance_no = 0;
 		list_add_tail(&acpi_device_bus_id->node, &acpi_bus_id_list);
 	}

@@ -1103,10 +1094,8 @@ static void acpi_device_set_id(struct acpi_device *device)
 	if (!hid && !cid_list && !cid_add)
 		hid = "device";
 
-	if (hid) {
+	if (hid)
 		acpi_add_id(device, hid);
-		device->flags.hardware_id = 1;
-	}
 	if (uid) {
 		strcpy(device->pnp.unique_id, uid);
 		device->flags.unique_id = 1;

drivers/pnp/pnpacpi/core.c +1 -2

index 3a4478f..83b8b5a 100644
@@ -230,8 +230,7 @@ static int __init acpi_pnp_match(struct device *dev, void *_pnp)
 	struct pnp_dev *pnp = _pnp;
 
 	/* true means it matched */
-	return acpi->flags.hardware_id
-	    && !acpi_get_physical_device(acpi->handle)
+	return !acpi_get_physical_device(acpi->handle)
 	    && compare_pnp_id(pnp->id, acpi_device_hid(acpi));
 }

include/acpi/acpi_bus.h +1 -2

index c7702a5..7b353b7 100644
@@ -143,7 +143,6 @@ struct acpi_device_status {
 
 struct acpi_device_flags {
 	u32 dynamic_status:1;
-	u32 hardware_id:1;
 	u32 bus_address:1;
 	u32 unique_id:1;
 	u32 removable:1;

@@ -154,7 +153,7 @@ struct acpi_device_flags {
 	u32 performance_manageable:1;
 	u32 wake_capable:1;	/* Wakeup(_PRW) supported? */
 	u32 force_power_state:1;
-	u32 reserved:20;
+	u32 reserved:21;
 };
 
 /* File System */

Bjorn Helgaas

author

2009-08-24 16:39 UTC #9 lore raw

PATCH 9/10 ACPI: remove acpi_device_uid() and related stuff

In reply to Bjorn Helgaas

Nobody uses acpi_device_uid(), so this patch removes it.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/scan.c     |   12 ------------
 include/acpi/acpi_bus.h |    6 +-----
 2 files changed, 1 insertions(+), 17 deletions(-)

drivers/acpi/scan.c +0 ~~-12~~

index c181728..a776018 100644
@@ -1017,7 +1017,6 @@ static void acpi_device_set_id(struct acpi_device *device)
 	struct acpi_device_info *info;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	char *hid = NULL;
-	char *uid = NULL;
 	struct acpi_compatible_id_list *cid_list = NULL;
 	const char *cid_add = NULL;
 	acpi_status status;

@@ -1045,8 +1044,6 @@ static void acpi_device_set_id(struct acpi_device *device)
 		info = buffer.pointer;
 		if (info->valid & ACPI_VALID_HID)
 			hid = info->hardware_id.value;
-		if (info->valid & ACPI_VALID_UID)
-			uid = info->unique_id.value;
 		if (info->valid & ACPI_VALID_CID)
 			cid_list = &info->compatibility_id;
 		if (info->valid & ACPI_VALID_ADR) {

@@ -1096,10 +1093,6 @@ static void acpi_device_set_id(struct acpi_device *device)
 
 	if (hid)
 		acpi_add_id(device, hid);
-	if (uid) {
-		strcpy(device->pnp.unique_id, uid);
-		device->flags.unique_id = 1;
-	}
 	if (cid_list)
 		for (i = 0; i < cid_list->count; i++)
 			acpi_add_id(device, cid_list->id[i].value);

@@ -1194,11 +1187,6 @@ static int acpi_add_single_object(struct acpi_device **child,
 	 * -----------------
 	 * TBD: Synch with Core's enumeration/initialization process.
 	 */
-
-	/*
-	 * Hardware ID, Unique ID, & Bus Address
-	 * -------------------------------------
-	 */
 	acpi_device_set_id(device);
 
 	/*

include/acpi/acpi_bus.h +1 -5

index 7b353b7..d8b93a2 100644
@@ -144,7 +144,6 @@ struct acpi_device_status {
 struct acpi_device_flags {
 	u32 dynamic_status:1;
 	u32 bus_address:1;
-	u32 unique_id:1;
 	u32 removable:1;
 	u32 ejectable:1;
 	u32 lockable:1;

@@ -153,7 +152,7 @@ struct acpi_device_flags {
 	u32 performance_manageable:1;
 	u32 wake_capable:1;	/* Wakeup(_PRW) supported? */
 	u32 force_power_state:1;
-	u32 reserved:21;
+	u32 reserved:22;
 };
 
 /* File System */

@@ -168,7 +167,6 @@ struct acpi_device_dir {
 
 typedef char acpi_bus_id[8];
 typedef unsigned long acpi_bus_address;
-typedef char acpi_unique_id[9];
 typedef char acpi_device_name[40];
 typedef char acpi_device_class[20];

@@ -181,7 +179,6 @@ struct acpi_device_pnp {
 	acpi_bus_id bus_id;	/* Object name */
 	acpi_bus_address bus_address;	/* _ADR */
 	struct list_head ids;		/* _HID and _CIDs */
-	acpi_unique_id unique_id;	/* _UID */
 	acpi_device_name device_name;	/* Driver-determined */
 	acpi_device_class device_class;	/*        "          */
 };

@@ -189,7 +186,6 @@ struct acpi_device_pnp {
 #define acpi_device_bid(d)	((d)->pnp.bus_id)
 #define acpi_device_adr(d)	((d)->pnp.bus_address)
 char *acpi_device_hid(struct acpi_device *device);
-#define acpi_device_uid(d)	((d)->pnp.unique_id)
 #define acpi_device_name(d)	((d)->pnp.device_name)
 #define acpi_device_class(d)	((d)->pnp.device_class)

Bjorn Helgaas

author

2009-08-24 16:39 UTC #10 lore raw

PATCH 10/10 ACPI: simplify building device HID/CID list

In reply to Bjorn Helgaas

Minor code cleanup, no functional change. Instead of remembering what HIDs & CIDs to add later, just add them immediately.

Signed-off-by Bjorn Helgaas <[email protected]>

---
 drivers/acpi/scan.c |   59 ++++++++++++++++++++++-----------------------------
 1 files changed, 25 insertions(+), 34 deletions(-)

drivers/acpi/scan.c +25 ~~-34~~

index a776018..44c333f 100644
@@ -1014,22 +1014,20 @@ static void acpi_add_id(struct acpi_device *device, const char *dev_id)
 
 static void acpi_device_set_id(struct acpi_device *device)
 {
-	struct acpi_device_info *info;
-	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	char *hid = NULL;
-	struct acpi_compatible_id_list *cid_list = NULL;
-	const char *cid_add = NULL;
 	acpi_status status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct acpi_device_info *info;
+	struct acpi_compatible_id_list *cid_list;
 	int i;
 
 	switch (device->device_type) {
 	case ACPI_BUS_TYPE_DEVICE:
 		if (ACPI_IS_ROOT_DEVICE(device)) {
-			hid = ACPI_SYSTEM_HID;
+			acpi_add_id(device, ACPI_SYSTEM_HID);
 			break;
 		} else if (ACPI_IS_ROOT_DEVICE(device->parent)) {
 			/* \_SB_, the only root-level namespace device */
-			hid = ACPI_BUS_HID;
+			acpi_add_id(device, ACPI_BUS_HID);
 			strcpy(device->pnp.device_name, ACPI_BUS_DEVICE_NAME);
 			strcpy(device->pnp.device_class, ACPI_BUS_CLASS);
 			break;

@@ -1043,41 +1041,44 @@ static void acpi_device_set_id(struct acpi_device *device)
 
 		info = buffer.pointer;
 		if (info->valid & ACPI_VALID_HID)
-			hid = info->hardware_id.value;
-		if (info->valid & ACPI_VALID_CID)
+			acpi_add_id(device, info->hardware_id.value);
+		if (info->valid & ACPI_VALID_CID) {
 			cid_list = &info->compatibility_id;
+			for (i = 0; i < cid_list->count; i++)
+				acpi_add_id(device, cid_list->id[i].value);
+		}
 		if (info->valid & ACPI_VALID_ADR) {
 			device->pnp.bus_address = info->address;
 			device->flags.bus_address = 1;
 		}
+		kfree(buffer.pointer);
 
-		/* If we have a video/bay/dock device, add our selfdefined
-		   HID to the CID list. Like that the video/bay/dock drivers
-		   will get autoloaded and the device might still match
-		   against another driver.
-		*/
+		/*
+		 * Some devices don't reliably have _HIDs & _CIDs, so add
+		 * synthetic HIDs to make sure drivers can find them.
+		 */
 		if (acpi_is_video_device(device))
-			cid_add = ACPI_VIDEO_HID;
+			acpi_add_id(device, ACPI_VIDEO_HID);
 		else if (ACPI_SUCCESS(acpi_bay_match(device)))
-			cid_add = ACPI_BAY_HID;
+			acpi_add_id(device, ACPI_BAY_HID);
 		else if (ACPI_SUCCESS(acpi_dock_match(device)))
-			cid_add = ACPI_DOCK_HID;
+			acpi_add_id(device, ACPI_DOCK_HID);
 
 		break;
 	case ACPI_BUS_TYPE_POWER:
-		hid = ACPI_POWER_HID;
+		acpi_add_id(device, ACPI_POWER_HID);
 		break;
 	case ACPI_BUS_TYPE_PROCESSOR:
-		hid = ACPI_PROCESSOR_OBJECT_HID;
+		acpi_add_id(device, ACPI_PROCESSOR_OBJECT_HID);
 		break;
 	case ACPI_BUS_TYPE_THERMAL:
-		hid = ACPI_THERMAL_HID;
+		acpi_add_id(device, ACPI_THERMAL_HID);
 		break;
 	case ACPI_BUS_TYPE_POWER_BUTTON:
-		hid = ACPI_BUTTON_HID_POWERF;
+		acpi_add_id(device, ACPI_BUTTON_HID_POWERF);
 		break;
 	case ACPI_BUS_TYPE_SLEEP_BUTTON:
-		hid = ACPI_BUTTON_HID_SLEEPF;
+		acpi_add_id(device, ACPI_BUTTON_HID_SLEEPF);
 		break;
 	}

@@ -1088,18 +1089,8 @@ static void acpi_device_set_id(struct acpi_device *device)
 	 * This generic ID isn't useful for driver binding, but it provides
 	 * the useful property that "every acpi_device has an ID."
 	 */
-	if (!hid && !cid_list && !cid_add)
-		hid = "device";
-
-	if (hid)
-		acpi_add_id(device, hid);
-	if (cid_list)
-		for (i = 0; i < cid_list->count; i++)
-			acpi_add_id(device, cid_list->id[i].value);
-	if (cid_add)
-		acpi_add_id(device, cid_add);
-
-	kfree(buffer.pointer);
+	if (list_empty(&device->pnp.ids))
+		acpi_add_id(device, "device");
 }
 
 static int acpi_device_set_context(struct acpi_device *device)