PNPACPI: cope with invalid device IDs

If primary ID (HID) is invalid try locating first valid ID on compatible ID list before giving up. This helps, for example, to recognize i8042 AUX port on Sony Vaio VPCZ1 which uses SNYSYN0003 as HID.

Tested-by Jan-Hendrik Zab <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/pnp/pnpacpi/core.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

drivers/pnp/pnpacpi/core.c +23 -4

index f7ff628..1bf1677 100644
@@ -28,7 +28,7 @@
 #include "../base.h"
 #include "pnpacpi.h"
 
-static int num = 0;
+static int num;
 
 /* We need only to blacklist devices that have already an acpi driver that
  * can't use pnp layer. We don't need to blacklist device that are directly

@@ -157,11 +157,24 @@ struct pnp_protocol pnpacpi_protocol = {
 };
 EXPORT_SYMBOL(pnpacpi_protocol);
 
+static char *pnpacpi_get_id(struct acpi_device *device)
+{
+	struct acpi_hardware_id *id;
+
+	list_for_each_entry(id, &device->pnp.ids, list) {
+		if (ispnpidacpi(id->id))
+			return id->id;
+	}
+
+	return NULL;
+}
+
 static int __init pnpacpi_add_device(struct acpi_device *device)
 {
 	acpi_handle temp = NULL;
 	acpi_status status;
 	struct pnp_dev *dev;
+	char *pnpid;
 	struct acpi_hardware_id *id;
 
 	/*

@@ -169,11 +182,17 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	 * driver should not be loaded.
 	 */
 	status = acpi_get_handle(device->handle, "_CRS", &temp);
-	if (ACPI_FAILURE(status) || !ispnpidacpi(acpi_device_hid(device)) ||
-	    is_exclusive_device(device) || (!device->status.present))
+	if (ACPI_FAILURE(status))
+		return 0;
+
+	pnpid = pnpacpi_get_id(device);
+	if (!pnpid)
+		return 0;
+
+	if (!is_exclusive_device(device) || !device->status.present)
 		return 0;
 
-	dev = pnp_alloc_dev(&pnpacpi_protocol, num, acpi_device_hid(device));
+	dev = pnp_alloc_dev(&pnpacpi_protocol, num, pnpid);
 	if (!dev)
 		return -ENOMEM;
 

If primary ID (HID) is invalid try locating first valid ID on compatible ID list before giving up. This helps, for example, to recognize i8042 AUX port on Sony Vaio VPCZ1 which uses SNYSYN0003 as HID.

Tested-by Jan-Hendrik Zab <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/pnp/pnpacpi/core.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

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

index f7ff628..1bf1677 100644

drivers/pnp/pnpacpi/core.c +13 -0

 /* We need only to blacklist devices that have already an acpi driver that
  * can't use pnp layer. We don't need to blacklist device that are directly

@@ -157,11 +157,24 @@ struct pnp_protocol pnpacpi_protocol = {
 };
 EXPORT_SYMBOL(pnpacpi_protocol);
 
+static char *pnpacpi_get_id(struct acpi_device *device)
+{
+	struct acpi_hardware_id *id;
+
+	list_for_each_entry(id, &device->pnp.ids, list) {
+		if (ispnpidacpi(id->id))
+			return id->id;
+	}
+
+	return NULL;
+}
+
 static int __init pnpacpi_add_device(struct acpi_device *device)
 {
 	acpi_handle temp = NULL;
 	acpi_status status;
 	struct pnp_dev *dev;
+	char *pnpid;
 	struct acpi_hardware_id *id;
 
 	/*

@@ -169,11 +182,17 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	 * driver should not be loaded.

Quoted 3 lines · Dmitry Torokhov

If primary ID (HID) is invalid try locating first valid ID on compatible ID list before giving up. This helps, for example, to recognize i8042 AUX port on Sony Vaio VPCZ1 which uses SNYSYN0003 as HID.

Is there a bugzilla report or mailing list discussion you could point to here (in the changelog)?

Quoted 12 lines

Tested-by Jan-Hendrik Zab <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

 drivers/pnp/pnpacpi/core.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

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

index f7ff628..1bf1677 100644

drivers/pnp/pnpacpi/core.c line 31

Quoted 5 lines

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

@@ -28,7 +28,7 @@
 #include "../base.h"
 #include "pnpacpi.h"
 
-static int num = 0;
+static int num;

Unrelated change, but OK by me :-)

Quoted 29 lines

drivers/pnp/pnpacpi/core.c +13 -0

 /* We need only to blacklist devices that have already an acpi driver that
  * can't use pnp layer. We don't need to blacklist device that are directly

@@ -157,11 +157,24 @@ struct pnp_protocol pnpacpi_protocol = {
 };
 EXPORT_SYMBOL(pnpacpi_protocol);
 
+static char *pnpacpi_get_id(struct acpi_device *device)
+{
+	struct acpi_hardware_id *id;
+
+	list_for_each_entry(id, &device->pnp.ids, list) {
+		if (ispnpidacpi(id->id))
+			return id->id;
+	}
+
+	return NULL;
+}
+
 static int __init pnpacpi_add_device(struct acpi_device *device)
 {
 	acpi_handle temp = NULL;
 	acpi_status status;
 	struct pnp_dev *dev;
+	char *pnpid;
 	struct acpi_hardware_id *id;
 
 	/*

@@ -169,11 +182,17 @@ static int __init pnpacpi_add_device(struct acpi_device *device)
 	 * driver should not be loaded.

drivers/pnp/pnpacpi/core.c static int __init pnpacpi_add_device(struct acpi_device *device) · line 190

Quoted 10 lines

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

 	 */
 	status = acpi_get_handle(device->handle, "_CRS", &temp);
-	if (ACPI_FAILURE(status) || !ispnpidacpi(acpi_device_hid(device)) ||
-	    is_exclusive_device(device) || (!device->status.present))
+	if (ACPI_FAILURE(status))
+		return 0;
+
+	pnpid = pnpacpi_get_id(device);
+	if (!pnpid)
+		return 0;

This part (run ispnpidacpi() on all _HIDs & _CIDs, not just on _HID, so we'll now build a PNPACPI device for things with an invalid _HID but a valid _CID) makes sense to me and is probably required for the i8042 PNP driver to claim the device.

drivers/pnp/pnpacpi/core.c static int __init pnpacpi_add_device(struct acpi_device *device) · line 195

Quoted 6 lines

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

+
+	if (!is_exclusive_device(device) || !device->status.present)
 		return 0;
 
-	dev = pnp_alloc_dev(&pnpacpi_protocol, num, acpi_device_hid(device));
+	dev = pnp_alloc_dev(&pnpacpi_protocol, num, pnpid);

I'm curious about this part. Does it fix something?

Let's say this device has _HID=SNYSYN0003, _CID=PNP0F13.

Previously, we didn't make a PNP device at all because SNYSYN0003 is invalid. Now, we'll make a device, exclude SNYSYN0003 from the PNP ID list, and it looks like the loop farther down will add PNP0f13 again, so we'll end up with "PNP0f13 PNP0f13" (I think I mentioned this when reviewing an earlier version of the patch :-)).

With the original pnp_alloc_dev(acpi_device_hid()) call, we'll probably end up with "SNYsyn0 PNP0f13". That's clearly wrong, too.

For now, I think the best fix is to keep this pnp_alloc_dev() call change and adjust the loop so it doesn't add "pnpid" again, so we end up with just "PNP0f13".

In the long term, I wonder if it'd be better to quit checking the ID for validity and make pnp_id.id a pointer rather than an array, so we could have "SNYSYN0003 PNP0f13" as PNP IDs for this device. I bet that's what Windows does.