[PATCH 0/4] ACPI / scan: Clean up the handling of the PRP0001 device ID

[PATCH 0/4] ACPI / scan: Clean up the handling of the PRP0001 device ID

[Rafael J. Wysocki]

Hi,

This series of patches reworks the handling of the PRP0001 device ID with
the following goals:

 (a) Make PRP0001 work as a _CID too.
 (b) Prevent PRP0001 from overriding the other ACPI/PNP IDs entirely.
 (c) Change ACPI modalias to cover ACPI/PNP/compatible at the same time.

[1/4] Generalize the of_compatible matching routine
[2/4] Simplify acpi_match_device()
[3/4] Take PRP0001 in _CID lists into account too.
[4/4] Rework modalias creation if the "compatible" property is present.

Comments, remarks, testing welcome!

Kind regards,
Rafael

[PATCH 1/4] ACPI / scan: Generalize of_compatible matching

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Redefine the function used for matching the device's "compatible"
property against a given list of "compatible" strings to take
a pointer to that list instead of a driver object pointer to
make it more general.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -953,9 +953,17 @@ int acpi_match_device_ids(struct acpi_de
 }
 EXPORT_SYMBOL(acpi_match_device_ids);
 
-/* Performs match against special "PRP0001" shoehorn ACPI ID */
-static bool acpi_of_driver_match_device(struct device *dev,
-					const struct device_driver *drv)
+/**
+ * acpi_of_match_device - Match device using the "compatible" property.
+ * @dev: Device to match.
+ * @of_match_table: List of device IDs to match against.
+ *
+ * If @dev has an ACPI companion which has the special PRP0001 device ID in its
+ * list of identifiers and a _DSD object with the "compatible" property, use
+ * that property to match against the given list of identifiers.
+ */
+static bool acpi_of_match_device(struct device *dev,
+				 const struct of_device_id *of_match_table)
 {
 	const union acpi_object *of_compatible, *obj;
 	struct acpi_device *adev;
@@ -966,7 +974,7 @@ static bool acpi_of_driver_match_device(
 		return false;
 
 	of_compatible = adev->data.of_compatible;
-	if (!drv->of_match_table || !of_compatible)
+	if (!of_match_table || !of_compatible)
 		return false;
 
 	if (of_compatible->type == ACPI_TYPE_PACKAGE) {
@@ -980,7 +988,7 @@ static bool acpi_of_driver_match_device(
 	for (i = 0; i < nval; i++, obj++) {
 		const struct of_device_id *id;
 
-		for (id = drv->of_match_table; id->compatible[0]; id++)
+		for (id = of_match_table; id->compatible[0]; id++)
 			if (!strcasecmp(obj->string.pointer, id->compatible))
 				return true;
 	}
@@ -992,7 +1000,7 @@ bool acpi_driver_match_device(struct dev
 			      const struct device_driver *drv)
 {
 	if (!drv->acpi_match_table)
-		return acpi_of_driver_match_device(dev, drv);
+		return acpi_of_match_device(dev, drv->of_match_table);
 
 	return !!acpi_match_device(drv->acpi_match_table, dev);
 }

[PATCH 2/4] ACPI / scan: Simplify acpi_match_device()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Redefine acpi_companion_match() to return an ACPI device object
pointer instead of a bool and use it to remove some redundant code
from acpi_match_device().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |   30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -194,7 +194,8 @@ static int create_modalias(struct acpi_d
  *
  * Check if the given device has an ACPI companion and if that companion has
  * a valid list of PNP IDs, and if the device is the first (primary) physical
- * device associated with it.
+ * device associated with it.  Return the companion pointer if that's the case
+ * or NULL otherwise.
  *
  * If multiple physical devices are attached to a single ACPI companion, we need
  * to be careful.  The usage scenario for this kind of relationship is that all
@@ -208,31 +209,31 @@ static int create_modalias(struct acpi_d
  * resources available from it but they will be matched normally using functions
  * provided by their bus types (and analogously for their modalias).
  */
-static bool acpi_companion_match(const struct device *dev)
+static struct acpi_device *acpi_companion_match(const struct device *dev)
 {
 	struct acpi_device *adev;
-	bool ret;
 
 	adev = ACPI_COMPANION(dev);
 	if (!adev)
-		return false;
+		return NULL;
 
 	if (list_empty(&adev->pnp.ids))
-		return false;
+		return NULL;
 
 	mutex_lock(&adev->physical_node_lock);
 	if (list_empty(&adev->physical_node_list)) {
-		ret = false;
+		adev = NULL;
 	} else {
 		const struct acpi_device_physical_node *node;
 
 		node = list_first_entry(&adev->physical_node_list,
 					struct acpi_device_physical_node, node);
-		ret = node->dev == dev;
+		if (node->dev != dev)
+			adev = NULL;
 	}
 	mutex_unlock(&adev->physical_node_lock);
 
-	return ret;
+	return adev;
 }
 
 /*
@@ -908,7 +909,7 @@ static const struct acpi_device_id *__ac
 	 * If the device is not present, it is unnecessary to load device
 	 * driver for it.
 	 */
-	if (!device->status.present)
+	if (!device || !device->status.present)
 		return NULL;
 
 	for (id = ids; id->id[0]; id++)
@@ -933,16 +934,7 @@ static const struct acpi_device_id *__ac
 const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 					       const struct device *dev)
 {
-	struct acpi_device *adev;
-	acpi_handle handle = ACPI_HANDLE(dev);
-
-	if (!ids || !handle || acpi_bus_get_device(handle, &adev))
-		return NULL;
-
-	if (!acpi_companion_match(dev))
-		return NULL;
-
-	return __acpi_match_device(adev, ids);
+	return __acpi_match_device(acpi_companion_match(dev), ids);
 }
 EXPORT_SYMBOL_GPL(acpi_match_device);
 

[PATCH 3/4] ACPI / scan: Take PRP0001 in _CID lists into account too

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If the special PRP0001 device ID is present in a device's _CID list,
it should be treated the same way as for the _HID case.  That is,
if none of the IDs preceding it in the device's PNP/ACPI IDs list
matches the IDs recognized by the driver, the driver's list of
"compatible" IDs should be matched against the device's "compatible"
property, if present.

To make that happen, rework acpi_driver_match_device() to do the
"compatible" property check even if acpi_match_table is present
for the given driver.

That will also cover cases in which drivers provide both
acpi_match_table and of_match_table (which is perfectly valid) at the
same time.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |  115 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 65 insertions(+), 50 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -899,8 +899,51 @@ static void acpi_device_remove_files(str
 			ACPI Bus operations
    -------------------------------------------------------------------------- */
 
+/**
+ * acpi_of_match_device - Match device object using the "compatible" property.
+ * @adev: ACPI device object to match.
+ * @of_match_table: List of device IDs to match against.
+ *
+ * If @dev has an ACPI companion which has the special PRP0001 device ID in its
+ * list of identifiers and a _DSD object with the "compatible" property, use
+ * that property to match against the given list of identifiers.
+ */
+static bool acpi_of_match_device(struct acpi_device *adev,
+				 const struct of_device_id *of_match_table)
+{
+	const union acpi_object *of_compatible, *obj;
+	int i, nval;
+
+	if (!adev)
+		return false;
+
+	of_compatible = adev->data.of_compatible;
+	if (!of_match_table || !of_compatible)
+		return false;
+
+	if (of_compatible->type == ACPI_TYPE_PACKAGE) {
+		nval = of_compatible->package.count;
+		obj = of_compatible->package.elements;
+	} else { /* Must be ACPI_TYPE_STRING. */
+		nval = 1;
+		obj = of_compatible;
+	}
+	/* Now we can look for the driver DT compatible strings */
+	for (i = 0; i < nval; i++, obj++) {
+		const struct of_device_id *id;
+
+		for (id = of_match_table; id->compatible[0]; id++)
+			if (!strcasecmp(obj->string.pointer, id->compatible))
+				return true;
+	}
+
+	return false;
+}
+
 static const struct acpi_device_id *__acpi_match_device(
-	struct acpi_device *device, const struct acpi_device_id *ids)
+	struct acpi_device *device,
+	const struct acpi_device_id *ids,
+	const struct of_device_id *of_ids)
 {
 	const struct acpi_device_id *id;
 	struct acpi_hardware_id *hwid;
@@ -912,11 +955,24 @@ static const struct acpi_device_id *__ac
 	if (!device || !device->status.present)
 		return NULL;
 
-	for (id = ids; id->id[0]; id++)
-		list_for_each_entry(hwid, &device->pnp.ids, list)
+	list_for_each_entry(hwid, &device->pnp.ids, list) {
+		/* First, check the ACPI/PNP IDs provided by the caller. */
+		for (id = ids; id->id[0]; id++)
 			if (!strcmp((char *) id->id, hwid->id))
 				return id;
 
+		/*
+		 * Next, check the special "PRP0001" ID and try to match the
+		 * "compatible" property if found.
+		 *
+		 * The id returned by the below is not valid, but the only
+		 * caller passing non-NULL of_ids here is only interested in
+		 * whether or not the return value is NULL.
+		 */
+		if (!strcmp("PRP0001", hwid->id)
+		    && acpi_of_match_device(device, of_ids))
+			return id;
+	}
 	return NULL;
 }
 
@@ -934,67 +990,26 @@ static const struct acpi_device_id *__ac
 const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 					       const struct device *dev)
 {
-	return __acpi_match_device(acpi_companion_match(dev), ids);
+	return __acpi_match_device(acpi_companion_match(dev), ids, NULL);
 }
 EXPORT_SYMBOL_GPL(acpi_match_device);
 
 int acpi_match_device_ids(struct acpi_device *device,
 			  const struct acpi_device_id *ids)
 {
-	return __acpi_match_device(device, ids) ? 0 : -ENOENT;
+	return __acpi_match_device(device, ids, NULL) ? 0 : -ENOENT;
 }
 EXPORT_SYMBOL(acpi_match_device_ids);
 
-/**
- * acpi_of_match_device - Match device using the "compatible" property.
- * @dev: Device to match.
- * @of_match_table: List of device IDs to match against.
- *
- * If @dev has an ACPI companion which has the special PRP0001 device ID in its
- * list of identifiers and a _DSD object with the "compatible" property, use
- * that property to match against the given list of identifiers.
- */
-static bool acpi_of_match_device(struct device *dev,
-				 const struct of_device_id *of_match_table)
-{
-	const union acpi_object *of_compatible, *obj;
-	struct acpi_device *adev;
-	int i, nval;
-
-	adev = ACPI_COMPANION(dev);
-	if (!adev)
-		return false;
-
-	of_compatible = adev->data.of_compatible;
-	if (!of_match_table || !of_compatible)
-		return false;
-
-	if (of_compatible->type == ACPI_TYPE_PACKAGE) {
-		nval = of_compatible->package.count;
-		obj = of_compatible->package.elements;
-	} else { /* Must be ACPI_TYPE_STRING. */
-		nval = 1;
-		obj = of_compatible;
-	}
-	/* Now we can look for the driver DT compatible strings */
-	for (i = 0; i < nval; i++, obj++) {
-		const struct of_device_id *id;
-
-		for (id = of_match_table; id->compatible[0]; id++)
-			if (!strcasecmp(obj->string.pointer, id->compatible))
-				return true;
-	}
-
-	return false;
-}
-
 bool acpi_driver_match_device(struct device *dev,
 			      const struct device_driver *drv)
 {
 	if (!drv->acpi_match_table)
-		return acpi_of_match_device(dev, drv->of_match_table);
+		return acpi_of_match_device(ACPI_COMPANION(dev),
+					    drv->of_match_table);
 
-	return !!acpi_match_device(drv->acpi_match_table, dev);
+	return !!__acpi_match_device(acpi_companion_match(dev),
+				     drv->acpi_match_table, drv->of_match_table);
 }
 EXPORT_SYMBOL_GPL(acpi_driver_match_device);
 

[PATCH 4/4] ACPI / scan: Rework modalias creation when "compatible" is present

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Currently, the ACPI modalias creation covers two mutually exclusive
cases: If the PRP0001 device ID is present in the device's list of
ACPI/PNP IDs and the "compatible" property is present in _DSD, the
created modalias will follow the OF rules of modalias creation.
Otherwise, ACPI rules are used.

However, that is not really desirable, because the presence of PRP0001
in the list of device IDs generally does not preclude using other
ACPI/PNP IDs with that device and those other IDs may be of higher
priority.  In those cases, the other IDs should take preference over
PRP0001 and therefore they also should be present in the modalias.

For this reason, rework the modalias creation for ACPI so that it
shows both the ACPI-style and OF-style modalias strings if the
device has a non-empty list of ACPI/PNP IDs (other than PNP0001)
and a valid "compatible" property at the same time.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |  204 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 120 insertions(+), 84 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -122,8 +122,8 @@ int acpi_scan_add_handler_with_hotplug(s
  *         -EINVAL: output error
  *         -ENOMEM: output is truncated
 */
-static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
-			   int size)
+static int create_pnp_modalias(struct acpi_device *acpi_dev, char *modalias,
+			       int size)
 {
 	int len;
 	int count;
@@ -132,58 +132,74 @@ static int create_modalias(struct acpi_d
 	if (list_empty(&acpi_dev->pnp.ids))
 		return 0;
 
+	len = snprintf(modalias, size, "acpi:");
+	size -= len;
+
+	list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
+		if (!strcmp(id->id, "PRP0001"))
+			continue;
+
+		count = snprintf(&modalias[len], size, "%s:", id->id);
+		if (count < 0)
+			return -EINVAL;
+
+		if (count >= size)
+			return -ENOMEM;
+
+		len += count;
+		size -= count;
+	}
+	modalias[len] = '\0';
+	return len;
+}
+
+static int create_of_modalias(struct acpi_device *acpi_dev, char *modalias,
+			      int size)
+{
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
+	const union acpi_object *of_compatible, *obj;
+	int len, count;
+	int i, nval;
+	char *c;
+
 	/*
-	 * If the device has PRP0001 we expose DT compatible modalias
-	 * instead in form of of:NnameTCcompatible.
+	 * If the device has PRP0001 we expose DT compatible modalias as
+	 * of:NnameTCcompatible.
 	 */
-	if (acpi_dev->data.of_compatible) {
-		struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
-		const union acpi_object *of_compatible, *obj;
-		int i, nval;
-		char *c;
-
-		acpi_get_name(acpi_dev->handle, ACPI_SINGLE_NAME, &buf);
-		/* DT strings are all in lower case */
-		for (c = buf.pointer; *c != '\0'; c++)
-			*c = tolower(*c);
-
-		len = snprintf(modalias, size, "of:N%sT", (char *)buf.pointer);
-		ACPI_FREE(buf.pointer);
-
-		of_compatible = acpi_dev->data.of_compatible;
-		if (of_compatible->type == ACPI_TYPE_PACKAGE) {
-			nval = of_compatible->package.count;
-			obj = of_compatible->package.elements;
-		} else { /* Must be ACPI_TYPE_STRING. */
-			nval = 1;
-			obj = of_compatible;
-		}
-		for (i = 0; i < nval; i++, obj++) {
-			count = snprintf(&modalias[len], size, "C%s",
-					 obj->string.pointer);
-			if (count < 0)
-				return -EINVAL;
-			if (count >= size)
-				return -ENOMEM;
+	if (!acpi_dev->data.of_compatible)
+		return 0;
 
-			len += count;
-			size -= count;
-		}
-	} else {
-		len = snprintf(modalias, size, "acpi:");
-		size -= len;
+	acpi_get_name(acpi_dev->handle, ACPI_SINGLE_NAME, &buf);
+	/* DT strings are all in lower case */
+	for (c = buf.pointer; *c != '\0'; c++)
+		*c = tolower(*c);
 
-		list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
-			count = snprintf(&modalias[len], size, "%s:", id->id);
-			if (count < 0)
-				return -EINVAL;
-			if (count >= size)
-				return -ENOMEM;
-			len += count;
-			size -= count;
-		}
+	len = snprintf(modalias, size, "of:N%sT", (char *)buf.pointer);
+	ACPI_FREE(buf.pointer);
+
+	if (len <= 0)
+		return len;
+
+	of_compatible = acpi_dev->data.of_compatible;
+	if (of_compatible->type == ACPI_TYPE_PACKAGE) {
+		nval = of_compatible->package.count;
+		obj = of_compatible->package.elements;
+	} else { /* Must be ACPI_TYPE_STRING. */
+		nval = 1;
+		obj = of_compatible;
 	}
+	for (i = 0; i < nval; i++, obj++) {
+		count = snprintf(&modalias[len], size, "C%s",
+				 obj->string.pointer);
+		if (count < 0)
+			return -EINVAL;
+
+		if (count >= size)
+			return -ENOMEM;
 
+		len += count;
+		size -= count;
+	}
 	modalias[len] = '\0';
 	return len;
 }
@@ -236,61 +252,89 @@ static struct acpi_device *acpi_companio
 	return adev;
 }
 
-/*
- * Creates uevent modalias field for ACPI enumerated devices.
- * Because the other buses does not support ACPI HIDs & CIDs.
- * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
- * "acpi:IBM0001:ACPI0001"
- */
-int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
+int __acpi_device_uevent_modalias(struct acpi_device *adev,
+				  struct kobj_uevent_env *env)
 {
 	int len;
 
-	if (!acpi_companion_match(dev))
+	if (!adev)
 		return -ENODEV;
 
 	if (add_uevent_var(env, "MODALIAS="))
 		return -ENOMEM;
-	len = create_modalias(ACPI_COMPANION(dev), &env->buf[env->buflen - 1],
-				sizeof(env->buf) - env->buflen);
-	if (len <= 0)
+
+	len = create_pnp_modalias(adev, &env->buf[env->buflen - 1],
+				  sizeof(env->buf) - env->buflen);
+	if (len < 0)
+		return len;
+
+	env->buflen += len;
+
+	if (add_uevent_var(env, "MODALIAS="))
+		return -ENOMEM;
+
+	len = create_of_modalias(adev, &env->buf[env->buflen - 1],
+				 sizeof(env->buf) - env->buflen);
+	if (len < 0)
 		return len;
+
 	env->buflen += len;
+
 	return 0;
 }
-EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
 
 /*
- * Creates modalias sysfs attribute for ACPI enumerated devices.
+ * Creates uevent modalias field for ACPI enumerated devices.
  * Because the other buses does not support ACPI HIDs & CIDs.
  * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
  * "acpi:IBM0001:ACPI0001"
  */
-int acpi_device_modalias(struct device *dev, char *buf, int size)
+int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
 {
-	int len;
+	return __acpi_device_uevent_modalias(acpi_companion_match(dev), env);
+}
+EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
+
+static int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size)
+{
+	int len, count;
 
-	if (!acpi_companion_match(dev))
+	if (!adev)
 		return -ENODEV;
 
-	len = create_modalias(ACPI_COMPANION(dev), buf, size -1);
-	if (len <= 0)
+	len = create_pnp_modalias(adev, buf, size - 1);
+	if (len < 0) {
 		return len;
-	buf[len++] = '\n';
+	} else if (len > 0) {
+		buf[len++] = '\n';
+		size -= len;
+	}
+	count = create_of_modalias(adev, buf, size - 1);
+	if (count < 0) {
+		return count;
+	} else if (count > 0) {
+		len += count;
+		buf[len++] = '\n';
+	}
+
 	return len;
 }
+
+/*
+ * Creates modalias sysfs attribute for ACPI enumerated devices.
+ * Because the other buses does not support ACPI HIDs & CIDs.
+ * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
+ * "acpi:IBM0001:ACPI0001"
+ */
+int acpi_device_modalias(struct device *dev, char *buf, int size)
+{
+	return __acpi_device_modalias(acpi_companion_match(dev), buf, size);
+}
 EXPORT_SYMBOL_GPL(acpi_device_modalias);
 
 static ssize_t
 acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, char *buf) {
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	int len;
-
-	len = create_modalias(acpi_dev, buf, 1024);
-	if (len <= 0)
-		return len;
-	buf[len++] = '\n';
-	return len;
+	return __acpi_device_modalias(to_acpi_device(dev), buf, 1024);
 }
 static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
 
@@ -1051,19 +1095,11 @@ static int acpi_bus_match(struct device
 static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	int len;
 
 	if (list_empty(&acpi_dev->pnp.ids))
 		return 0;
 
-	if (add_uevent_var(env, "MODALIAS="))
-		return -ENOMEM;
-	len = create_modalias(acpi_dev, &env->buf[env->buflen - 1],
-			      sizeof(env->buf) - env->buflen);
-	if (len <= 0)
-		return len;
-	env->buflen += len;
-	return 0;
+	return __acpi_device_uevent_modalias(acpi_dev, env);
 }
 
 static void acpi_device_notify(acpi_handle handle, u32 event, void *data)

[Update][PATCH 3/4] ACPI / scan: Take the PRP0001 position in the list of IDs into account

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If the special PRP0001 device ID is present in a device's _CID list,
it should not prevent any ACPI/PNP IDs preceding it in the device's
list of identifiers from being matched first.  That is, only if none
of the IDs preceding PRP0001 in the device's PNP/ACPI IDs list
matches the IDs recognized by the driver, the driver's list of
"compatible" IDs should be matched against the device's "compatible"
property, if present.

In addition to that, drivers can provide both acpi_match_table and
of_match_table at the same time and the of_compatible matching
should be used in that case too if PRP0001 is present in the
device's list of identifiers.

To make that happen, rework acpi_driver_match_device() to do the
"compatible" property check in addition to matching the driver's
list of ACPI IDs against the device's one.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

The subject and changelog of the previous version didn't make sense.

Sorry about that.

---
 drivers/acpi/scan.c |  115 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 65 insertions(+), 50 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -899,8 +899,51 @@ static void acpi_device_remove_files(str
 			ACPI Bus operations
    -------------------------------------------------------------------------- */
 
+/**
+ * acpi_of_match_device - Match device object using the "compatible" property.
+ * @adev: ACPI device object to match.
+ * @of_match_table: List of device IDs to match against.
+ *
+ * If @dev has an ACPI companion which has the special PRP0001 device ID in its
+ * list of identifiers and a _DSD object with the "compatible" property, use
+ * that property to match against the given list of identifiers.
+ */
+static bool acpi_of_match_device(struct acpi_device *adev,
+				 const struct of_device_id *of_match_table)
+{
+	const union acpi_object *of_compatible, *obj;
+	int i, nval;
+
+	if (!adev)
+		return false;
+
+	of_compatible = adev->data.of_compatible;
+	if (!of_match_table || !of_compatible)
+		return false;
+
+	if (of_compatible->type == ACPI_TYPE_PACKAGE) {
+		nval = of_compatible->package.count;
+		obj = of_compatible->package.elements;
+	} else { /* Must be ACPI_TYPE_STRING. */
+		nval = 1;
+		obj = of_compatible;
+	}
+	/* Now we can look for the driver DT compatible strings */
+	for (i = 0; i < nval; i++, obj++) {
+		const struct of_device_id *id;
+
+		for (id = of_match_table; id->compatible[0]; id++)
+			if (!strcasecmp(obj->string.pointer, id->compatible))
+				return true;
+	}
+
+	return false;
+}
+
 static const struct acpi_device_id *__acpi_match_device(
-	struct acpi_device *device, const struct acpi_device_id *ids)
+	struct acpi_device *device,
+	const struct acpi_device_id *ids,
+	const struct of_device_id *of_ids)
 {
 	const struct acpi_device_id *id;
 	struct acpi_hardware_id *hwid;
@@ -912,11 +955,24 @@ static const struct acpi_device_id *__ac
 	if (!device || !device->status.present)
 		return NULL;
 
-	for (id = ids; id->id[0]; id++)
-		list_for_each_entry(hwid, &device->pnp.ids, list)
+	list_for_each_entry(hwid, &device->pnp.ids, list) {
+		/* First, check the ACPI/PNP IDs provided by the caller. */
+		for (id = ids; id->id[0]; id++)
 			if (!strcmp((char *) id->id, hwid->id))
 				return id;
 
+		/*
+		 * Next, check the special "PRP0001" ID and try to match the
+		 * "compatible" property if found.
+		 *
+		 * The id returned by the below is not valid, but the only
+		 * caller passing non-NULL of_ids here is only interested in
+		 * whether or not the return value is NULL.
+		 */
+		if (!strcmp("PRP0001", hwid->id)
+		    && acpi_of_match_device(device, of_ids))
+			return id;
+	}
 	return NULL;
 }
 
@@ -934,67 +990,26 @@ static const struct acpi_device_id *__ac
 const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 					       const struct device *dev)
 {
-	return __acpi_match_device(acpi_companion_match(dev), ids);
+	return __acpi_match_device(acpi_companion_match(dev), ids, NULL);
 }
 EXPORT_SYMBOL_GPL(acpi_match_device);
 
 int acpi_match_device_ids(struct acpi_device *device,
 			  const struct acpi_device_id *ids)
 {
-	return __acpi_match_device(device, ids) ? 0 : -ENOENT;
+	return __acpi_match_device(device, ids, NULL) ? 0 : -ENOENT;
 }
 EXPORT_SYMBOL(acpi_match_device_ids);
 
-/**
- * acpi_of_match_device - Match device using the "compatible" property.
- * @dev: Device to match.
- * @of_match_table: List of device IDs to match against.
- *
- * If @dev has an ACPI companion which has the special PRP0001 device ID in its
- * list of identifiers and a _DSD object with the "compatible" property, use
- * that property to match against the given list of identifiers.
- */
-static bool acpi_of_match_device(struct device *dev,
-				 const struct of_device_id *of_match_table)
-{
-	const union acpi_object *of_compatible, *obj;
-	struct acpi_device *adev;
-	int i, nval;
-
-	adev = ACPI_COMPANION(dev);
-	if (!adev)
-		return false;
-
-	of_compatible = adev->data.of_compatible;
-	if (!of_match_table || !of_compatible)
-		return false;
-
-	if (of_compatible->type == ACPI_TYPE_PACKAGE) {
-		nval = of_compatible->package.count;
-		obj = of_compatible->package.elements;
-	} else { /* Must be ACPI_TYPE_STRING. */
-		nval = 1;
-		obj = of_compatible;
-	}
-	/* Now we can look for the driver DT compatible strings */
-	for (i = 0; i < nval; i++, obj++) {
-		const struct of_device_id *id;
-
-		for (id = of_match_table; id->compatible[0]; id++)
-			if (!strcasecmp(obj->string.pointer, id->compatible))
-				return true;
-	}
-
-	return false;
-}
-
 bool acpi_driver_match_device(struct device *dev,
 			      const struct device_driver *drv)
 {
 	if (!drv->acpi_match_table)
-		return acpi_of_match_device(dev, drv->of_match_table);
+		return acpi_of_match_device(ACPI_COMPANION(dev),
+					    drv->of_match_table);
 
-	return !!acpi_match_device(drv->acpi_match_table, dev);
+	return !!__acpi_match_device(acpi_companion_match(dev),
+				     drv->acpi_match_table, drv->of_match_table);
 }
 EXPORT_SYMBOL_GPL(acpi_driver_match_device);
 

Re: [PATCH 4/4] ACPI / scan: Rework modalias creation when "compatible" is present

[Mika Westerberg]

On Thu, Apr 09, 2015 at 02:22:10AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Currently, the ACPI modalias creation covers two mutually exclusive
> cases: If the PRP0001 device ID is present in the device's list of
> ACPI/PNP IDs and the "compatible" property is present in _DSD, the
> created modalias will follow the OF rules of modalias creation.
> Otherwise, ACPI rules are used.
> 
> However, that is not really desirable, because the presence of PRP0001
> in the list of device IDs generally does not preclude using other
> ACPI/PNP IDs with that device and those other IDs may be of higher
> priority.  In those cases, the other IDs should take preference over
> PRP0001 and therefore they also should be present in the modalias.
> 
> For this reason, rework the modalias creation for ACPI so that it
> shows both the ACPI-style and OF-style modalias strings if the
> device has a non-empty list of ACPI/PNP IDs (other than PNP0001)
                                                          ^^^^^^^
Typo


> and a valid "compatible" property at the same time.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/scan.c |  204 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 120 insertions(+), 84 deletions(-)
> 
> Index: linux-pm/drivers/acpi/scan.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -122,8 +122,8 @@ int acpi_scan_add_handler_with_hotplug(s
>   *         -EINVAL: output error
>   *         -ENOMEM: output is truncated
>  */
> -static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
> -			   int size)
> +static int create_pnp_modalias(struct acpi_device *acpi_dev, char *modalias,
> +			       int size)
>  {
>  	int len;
>  	int count;
> @@ -132,58 +132,74 @@ static int create_modalias(struct acpi_d
>  	if (list_empty(&acpi_dev->pnp.ids))
>  		return 0;
>  
> +	len = snprintf(modalias, size, "acpi:");
> +	size -= len;
> +
> +	list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
> +		if (!strcmp(id->id, "PRP0001"))
> +			continue;
> +
> +		count = snprintf(&modalias[len], size, "%s:", id->id);
> +		if (count < 0)
> +			return -EINVAL;
> +
> +		if (count >= size)
> +			return -ENOMEM;
> +
> +		len += count;
> +		size -= count;
> +	}
> +	modalias[len] = '\0';
> +	return len;
> +}
> +
> +static int create_of_modalias(struct acpi_device *acpi_dev, char *modalias,
> +			      int size)
> +{
> +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
> +	const union acpi_object *of_compatible, *obj;
> +	int len, count;
> +	int i, nval;
> +	char *c;
> +
>  	/*
> -	 * If the device has PRP0001 we expose DT compatible modalias
> -	 * instead in form of of:NnameTCcompatible.
> +	 * If the device has PRP0001 we expose DT compatible modalias as
> +	 * of:NnameTCcompatible.
>  	 */
> -	if (acpi_dev->data.of_compatible) {
> -		struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
> -		const union acpi_object *of_compatible, *obj;
> -		int i, nval;
> -		char *c;
> -
> -		acpi_get_name(acpi_dev->handle, ACPI_SINGLE_NAME, &buf);
> -		/* DT strings are all in lower case */
> -		for (c = buf.pointer; *c != '\0'; c++)
> -			*c = tolower(*c);
> -
> -		len = snprintf(modalias, size, "of:N%sT", (char *)buf.pointer);
> -		ACPI_FREE(buf.pointer);
> -
> -		of_compatible = acpi_dev->data.of_compatible;
> -		if (of_compatible->type == ACPI_TYPE_PACKAGE) {
> -			nval = of_compatible->package.count;
> -			obj = of_compatible->package.elements;
> -		} else { /* Must be ACPI_TYPE_STRING. */
> -			nval = 1;
> -			obj = of_compatible;
> -		}
> -		for (i = 0; i < nval; i++, obj++) {
> -			count = snprintf(&modalias[len], size, "C%s",
> -					 obj->string.pointer);
> -			if (count < 0)
> -				return -EINVAL;
> -			if (count >= size)
> -				return -ENOMEM;
> +	if (!acpi_dev->data.of_compatible)
> +		return 0;
>  
> -			len += count;
> -			size -= count;
> -		}
> -	} else {
> -		len = snprintf(modalias, size, "acpi:");
> -		size -= len;
> +	acpi_get_name(acpi_dev->handle, ACPI_SINGLE_NAME, &buf);
> +	/* DT strings are all in lower case */
> +	for (c = buf.pointer; *c != '\0'; c++)
> +		*c = tolower(*c);
>  
> -		list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
> -			count = snprintf(&modalias[len], size, "%s:", id->id);
> -			if (count < 0)
> -				return -EINVAL;
> -			if (count >= size)
> -				return -ENOMEM;
> -			len += count;
> -			size -= count;
> -		}
> +	len = snprintf(modalias, size, "of:N%sT", (char *)buf.pointer);
> +	ACPI_FREE(buf.pointer);
> +
> +	if (len <= 0)
> +		return len;
> +
> +	of_compatible = acpi_dev->data.of_compatible;
> +	if (of_compatible->type == ACPI_TYPE_PACKAGE) {
> +		nval = of_compatible->package.count;
> +		obj = of_compatible->package.elements;
> +	} else { /* Must be ACPI_TYPE_STRING. */
> +		nval = 1;
> +		obj = of_compatible;
>  	}
> +	for (i = 0; i < nval; i++, obj++) {
> +		count = snprintf(&modalias[len], size, "C%s",
> +				 obj->string.pointer);
> +		if (count < 0)
> +			return -EINVAL;
> +
> +		if (count >= size)
> +			return -ENOMEM;
>  
> +		len += count;
> +		size -= count;
> +	}
>  	modalias[len] = '\0';
>  	return len;
>  }
> @@ -236,61 +252,89 @@ static struct acpi_device *acpi_companio
>  	return adev;
>  }
>  
> -/*
> - * Creates uevent modalias field for ACPI enumerated devices.
> - * Because the other buses does not support ACPI HIDs & CIDs.
> - * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
> - * "acpi:IBM0001:ACPI0001"
> - */
> -int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
> +int __acpi_device_uevent_modalias(struct acpi_device *adev,
> +				  struct kobj_uevent_env *env)
>  {
>  	int len;
>  
> -	if (!acpi_companion_match(dev))
> +	if (!adev)
>  		return -ENODEV;
>  
>  	if (add_uevent_var(env, "MODALIAS="))
>  		return -ENOMEM;
> -	len = create_modalias(ACPI_COMPANION(dev), &env->buf[env->buflen - 1],
> -				sizeof(env->buf) - env->buflen);
> -	if (len <= 0)
> +
> +	len = create_pnp_modalias(adev, &env->buf[env->buflen - 1],
> +				  sizeof(env->buf) - env->buflen);
> +	if (len < 0)
> +		return len;

If there is only PRP0001 in the list this will create:

MODALIAS=acpi:

which I think is not correct. Better not to create the entry at all in
that case.

> +
> +	env->buflen += len;
> +
> +	if (add_uevent_var(env, "MODALIAS="))
> +		return -ENOMEM;
> +
> +	len = create_of_modalias(adev, &env->buf[env->buflen - 1],
> +				 sizeof(env->buf) - env->buflen);
> +	if (len < 0)
>  		return len;
> +
>  	env->buflen += len;
> +
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
>  
>  /*
> - * Creates modalias sysfs attribute for ACPI enumerated devices.
> + * Creates uevent modalias field for ACPI enumerated devices.
>   * Because the other buses does not support ACPI HIDs & CIDs.
>   * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
>   * "acpi:IBM0001:ACPI0001"
>   */
> -int acpi_device_modalias(struct device *dev, char *buf, int size)
> +int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>  {
> -	int len;
> +	return __acpi_device_uevent_modalias(acpi_companion_match(dev), env);
> +}
> +EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
> +
> +static int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size)
> +{
> +	int len, count;
>  
> -	if (!acpi_companion_match(dev))
> +	if (!adev)
>  		return -ENODEV;
>  
> -	len = create_modalias(ACPI_COMPANION(dev), buf, size -1);
> -	if (len <= 0)
> +	len = create_pnp_modalias(adev, buf, size - 1);
> +	if (len < 0) {
>  		return len;
> -	buf[len++] = '\n';
> +	} else if (len > 0) {
> +		buf[len++] = '\n';
> +		size -= len;
> +	}
> +	count = create_of_modalias(adev, buf, size - 1);

This will overwrite the ACPI modalias as you did not increase buf
itself.

> +	if (count < 0) {
> +		return count;
> +	} else if (count > 0) {
> +		len += count;
> +		buf[len++] = '\n';
> +	}
> +
>  	return len;
>  }

I used patch below on top of this and tried on Minnowboard MAX with two
devices: one with sole PRP0001 entry and one with _HID and _CIDs where
one _CID is PRP0001.

First is the SPI eeprom with only PRP0001 in its _HID:

	# cat /sys/bus/spi/devices/spi-PRP0001\:00/modalias 
	of:Nat25TCatmel,at25
	# cat /sys/bus/spi/devices/spi-PRP0001\:00/uevent 
	DRIVER=at25
	MODALIAS=of:Nat25TCatmel,at25

Looks good to me.

Next is an I2C eeprom with following IDs in DSDT:

	Device (AT24)
	{
	    Name (_HID, "EEP0024")
	    Name (_CID, Package () {"PRP0001","ATML2402"})
	...

This gives me:

	# cat /sys/bus/i2c/devices/i2c-EEP0024\:00/modalias 
	acpi:EEP0024:ATML2402:
	of:Nat24TCatmel,24c02
	# cat /sys/bus/i2c/devices/i2c-EEP0024\:00/uevent 
	DRIVER=at24
	MODALIAS=acpi:EEP0024:ATML2402:
	MODALIAS=of:Nat24TCatmel,24c02

Also looks okay.

For both devices modprobe/udev is able to load the drivers
automatically:

	# lsmod
	...
	at24                    6602  0 
	at25                    5741  0 

------8<-------8<--------

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 75697e06aad5..f509ddc29c0c 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -125,13 +125,25 @@ int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
 static int create_pnp_modalias(struct acpi_device *acpi_dev, char *modalias,
 			       int size)
 {
-	int len;
+	int len, nids = 0;
 	int count;
 	struct acpi_hardware_id *id;
 
 	if (list_empty(&acpi_dev->pnp.ids))
 		return 0;
 
+	/*
+	 * First check for a sole PRP0001 and in that case do not create
+	 * empty ACPI modalias for the device.
+	 */
+	nids = 0;
+	list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
+		if (strcmp(id->id, "PRP0001"))
+			nids++;
+	}
+	if (!nids)
+		return 0;
+
 	len = snprintf(modalias, size, "acpi:");
 	size -= len;
 
@@ -268,10 +280,12 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev,
 	if (len < 0)
 		return len;
 
-	env->buflen += len;
+	if (len > 0) {
+		env->buflen += len;
 
-	if (add_uevent_var(env, "MODALIAS="))
-		return -ENOMEM;
+		if (add_uevent_var(env, "MODALIAS="))
+			return -ENOMEM;
+	}
 
 	len = create_of_modalias(adev, &env->buf[env->buflen - 1],
 				 sizeof(env->buf) - env->buflen);
@@ -309,7 +323,7 @@ static int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size)
 		buf[len++] = '\n';
 		size -= len;
 	}
-	count = create_of_modalias(adev, buf, size - 1);
+	count = create_of_modalias(adev, buf + len, size - 1);
 	if (count < 0) {
 		return count;
 	} else if (count > 0) {

Re: [PATCH 4/4] ACPI / scan: Rework modalias creation when "compatible" is present

[Rafael J. Wysocki]

On Thursday, April 09, 2015 03:27:56 PM Mika Westerberg wrote:
> On Thu, Apr 09, 2015 at 02:22:10AM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > Currently, the ACPI modalias creation covers two mutually exclusive
> > cases: If the PRP0001 device ID is present in the device's list of
> > ACPI/PNP IDs and the "compatible" property is present in _DSD, the
> > created modalias will follow the OF rules of modalias creation.
> > Otherwise, ACPI rules are used.
> > 
> > However, that is not really desirable, because the presence of PRP0001
> > in the list of device IDs generally does not preclude using other
> > ACPI/PNP IDs with that device and those other IDs may be of higher
> > priority.  In those cases, the other IDs should take preference over
> > PRP0001 and therefore they also should be present in the modalias.
> > 
> > For this reason, rework the modalias creation for ACPI so that it
> > shows both the ACPI-style and OF-style modalias strings if the
> > device has a non-empty list of ACPI/PNP IDs (other than PNP0001)
>                                                           ^^^^^^^
> Typo

Right, thanks!

[cut]

> > @@ -236,61 +252,89 @@ static struct acpi_device *acpi_companio
> >  	return adev;
> >  }
> >  
> > -/*
> > - * Creates uevent modalias field for ACPI enumerated devices.
> > - * Because the other buses does not support ACPI HIDs & CIDs.
> > - * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
> > - * "acpi:IBM0001:ACPI0001"
> > - */
> > -int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
> > +int __acpi_device_uevent_modalias(struct acpi_device *adev,
> > +				  struct kobj_uevent_env *env)
> >  {
> >  	int len;
> >  
> > -	if (!acpi_companion_match(dev))
> > +	if (!adev)
> >  		return -ENODEV;
> >  
> >  	if (add_uevent_var(env, "MODALIAS="))
> >  		return -ENOMEM;
> > -	len = create_modalias(ACPI_COMPANION(dev), &env->buf[env->buflen - 1],
> > -				sizeof(env->buf) - env->buflen);
> > -	if (len <= 0)
> > +
> > +	len = create_pnp_modalias(adev, &env->buf[env->buflen - 1],
> > +				  sizeof(env->buf) - env->buflen);
> > +	if (len < 0)
> > +		return len;
> 
> If there is only PRP0001 in the list this will create:
> 
> MODALIAS=acpi:
> 
> which I think is not correct. Better not to create the entry at all in
> that case.

Right.  I even fixed that locally, but then forgot to refresh the patch.
Oh well.

> > +
> > +	env->buflen += len;
> > +
> > +	if (add_uevent_var(env, "MODALIAS="))
> > +		return -ENOMEM;
> > +
> > +	len = create_of_modalias(adev, &env->buf[env->buflen - 1],
> > +				 sizeof(env->buf) - env->buflen);
> > +	if (len < 0)
> >  		return len;
> > +
> >  	env->buflen += len;
> > +
> >  	return 0;
> >  }
> > -EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
> >  
> >  /*
> > - * Creates modalias sysfs attribute for ACPI enumerated devices.
> > + * Creates uevent modalias field for ACPI enumerated devices.
> >   * Because the other buses does not support ACPI HIDs & CIDs.
> >   * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
> >   * "acpi:IBM0001:ACPI0001"
> >   */
> > -int acpi_device_modalias(struct device *dev, char *buf, int size)
> > +int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
> >  {
> > -	int len;
> > +	return __acpi_device_uevent_modalias(acpi_companion_match(dev), env);
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
> > +
> > +static int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size)
> > +{
> > +	int len, count;
> >  
> > -	if (!acpi_companion_match(dev))
> > +	if (!adev)
> >  		return -ENODEV;
> >  
> > -	len = create_modalias(ACPI_COMPANION(dev), buf, size -1);
> > -	if (len <= 0)
> > +	len = create_pnp_modalias(adev, buf, size - 1);
> > +	if (len < 0) {
> >  		return len;
> > -	buf[len++] = '\n';
> > +	} else if (len > 0) {
> > +		buf[len++] = '\n';
> > +		size -= len;
> > +	}
> > +	count = create_of_modalias(adev, buf, size - 1);
> 
> This will overwrite the ACPI modalias as you did not increase buf
> itself.

Good catch, thanks!

> > +	if (count < 0) {
> > +		return count;
> > +	} else if (count > 0) {
> > +		len += count;
> > +		buf[len++] = '\n';
> > +	}
> > +
> >  	return len;
> >  }
> 
> I used patch below on top of this and tried on Minnowboard MAX with two
> devices: one with sole PRP0001 entry and one with _HID and _CIDs where
> one _CID is PRP0001.
> 
> First is the SPI eeprom with only PRP0001 in its _HID:
> 
> 	# cat /sys/bus/spi/devices/spi-PRP0001\:00/modalias 
> 	of:Nat25TCatmel,at25
> 	# cat /sys/bus/spi/devices/spi-PRP0001\:00/uevent 
> 	DRIVER=at25
> 	MODALIAS=of:Nat25TCatmel,at25
> 
> Looks good to me.
> 
> Next is an I2C eeprom with following IDs in DSDT:
> 
> 	Device (AT24)
> 	{
> 	    Name (_HID, "EEP0024")
> 	    Name (_CID, Package () {"PRP0001","ATML2402"})
> 	...
> 
> This gives me:
> 
> 	# cat /sys/bus/i2c/devices/i2c-EEP0024\:00/modalias 
> 	acpi:EEP0024:ATML2402:
> 	of:Nat24TCatmel,24c02
> 	# cat /sys/bus/i2c/devices/i2c-EEP0024\:00/uevent 
> 	DRIVER=at24
> 	MODALIAS=acpi:EEP0024:ATML2402:
> 	MODALIAS=of:Nat24TCatmel,24c02
> 
> Also looks okay.
> 
> For both devices modprobe/udev is able to load the drivers
> automatically:
> 
> 	# lsmod
> 	...
> 	at24                    6602  0 
> 	at25                    5741  0 
> 
> ------8<-------8<--------

Awesome, thanks for verification!

> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 75697e06aad5..f509ddc29c0c 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -125,13 +125,25 @@ int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
>  static int create_pnp_modalias(struct acpi_device *acpi_dev, char *modalias,
>  			       int size)
>  {
> -	int len;
> +	int len, nids = 0;
>  	int count;
>  	struct acpi_hardware_id *id;
>  
>  	if (list_empty(&acpi_dev->pnp.ids))
>  		return 0;
>  
> +	/*
> +	 * First check for a sole PRP0001 and in that case do not create
> +	 * empty ACPI modalias for the device.
> +	 */
> +	nids = 0;
> +	list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
> +		if (strcmp(id->id, "PRP0001"))
> +			nids++;
> +	}
> +	if (!nids)
> +		return 0;
> +
>  	len = snprintf(modalias, size, "acpi:");
>  	size -= len;
>  
> @@ -268,10 +280,12 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev,
>  	if (len < 0)
>  		return len;
>  
> -	env->buflen += len;
> +	if (len > 0) {
> +		env->buflen += len;
>  
> -	if (add_uevent_var(env, "MODALIAS="))
> -		return -ENOMEM;
> +		if (add_uevent_var(env, "MODALIAS="))
> +			return -ENOMEM;
> +	}
>  
>  	len = create_of_modalias(adev, &env->buf[env->buflen - 1],
>  				 sizeof(env->buf) - env->buflen);
> @@ -309,7 +323,7 @@ static int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size)
>  		buf[len++] = '\n';
>  		size -= len;
>  	}
> -	count = create_of_modalias(adev, buf, size - 1);
> +	count = create_of_modalias(adev, buf + len, size - 1);
>  	if (count < 0) {
>  		return count;
>  	} else if (count > 0) {

Thanks for the patch, I'll send an update of the $subject one later today.

Rafael

[update][PATCH 4/4] ACPI / scan: Rework modalias creation when "compatible" is present

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Currently, the ACPI modalias creation covers two mutually exclusive
cases: If the PRP0001 device ID is present in the device's list of
ACPI/PNP IDs and the "compatible" property is present in _DSD, the
created modalias will follow the OF rules of modalias creation.
Otherwise, ACPI rules are used.

However, that is not really desirable, because the presence of PRP0001
in the list of device IDs generally does not preclude using other
ACPI/PNP IDs with that device and those other IDs may be of higher
priority.  In those cases, the other IDs should take preference over
PRP0001 and therefore they also should be present in the modalias.

For this reason, rework the modalias creation for ACPI so that it
shows both the ACPI-style and OF-style modalias strings if the
device has a non-empty list of ACPI/PNP IDs (other than PRP0001)
and a valid "compatible" property at the same time.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

This should have the problems spotted by Mika in the previous version fixed.

---
 drivers/acpi/scan.c |  216 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 132 insertions(+), 84 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -122,8 +122,8 @@ int acpi_scan_add_handler_with_hotplug(s
  *         -EINVAL: output error
  *         -ENOMEM: output is truncated
 */
-static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
-			   int size)
+static int create_pnp_modalias(struct acpi_device *acpi_dev, char *modalias,
+			       int size)
 {
 	int len;
 	int count;
@@ -133,57 +133,85 @@ static int create_modalias(struct acpi_d
 		return 0;
 
 	/*
-	 * If the device has PRP0001 we expose DT compatible modalias
-	 * instead in form of of:NnameTCcompatible.
+	 * Since we skip PRP0001 from the modalias below, 0 should be returned
+	 * if PRP0001 is the only ACPI/PNP ID in the device's list.
 	 */
-	if (acpi_dev->data.of_compatible) {
-		struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
-		const union acpi_object *of_compatible, *obj;
-		int i, nval;
-		char *c;
-
-		acpi_get_name(acpi_dev->handle, ACPI_SINGLE_NAME, &buf);
-		/* DT strings are all in lower case */
-		for (c = buf.pointer; *c != '\0'; c++)
-			*c = tolower(*c);
-
-		len = snprintf(modalias, size, "of:N%sT", (char *)buf.pointer);
-		ACPI_FREE(buf.pointer);
-
-		of_compatible = acpi_dev->data.of_compatible;
-		if (of_compatible->type == ACPI_TYPE_PACKAGE) {
-			nval = of_compatible->package.count;
-			obj = of_compatible->package.elements;
-		} else { /* Must be ACPI_TYPE_STRING. */
-			nval = 1;
-			obj = of_compatible;
-		}
-		for (i = 0; i < nval; i++, obj++) {
-			count = snprintf(&modalias[len], size, "C%s",
-					 obj->string.pointer);
-			if (count < 0)
-				return -EINVAL;
-			if (count >= size)
-				return -ENOMEM;
+	count = 0;
+	list_for_each_entry(id, &acpi_dev->pnp.ids, list)
+		if (strcmp(id->id, "PRP0001"))
+			count++;
 
-			len += count;
-			size -= count;
-		}
-	} else {
-		len = snprintf(modalias, size, "acpi:");
-		size -= len;
+	if (!count)
+		return 0;
 
-		list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
-			count = snprintf(&modalias[len], size, "%s:", id->id);
-			if (count < 0)
-				return -EINVAL;
-			if (count >= size)
-				return -ENOMEM;
-			len += count;
-			size -= count;
-		}
+	len = snprintf(modalias, size, "acpi:");
+	size -= len;
+
+	list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
+		if (!strcmp(id->id, "PRP0001"))
+			continue;
+
+		count = snprintf(&modalias[len], size, "%s:", id->id);
+		if (count < 0)
+			return -EINVAL;
+
+		if (count >= size)
+			return -ENOMEM;
+
+		len += count;
+		size -= count;
 	}
+	modalias[len] = '\0';
+	return len;
+}
+
+static int create_of_modalias(struct acpi_device *acpi_dev, char *modalias,
+			      int size)
+{
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
+	const union acpi_object *of_compatible, *obj;
+	int len, count;
+	int i, nval;
+	char *c;
+
+	/*
+	 * If the device has PRP0001 we expose DT compatible modalias as
+	 * of:NnameTCcompatible.
+	 */
+	if (!acpi_dev->data.of_compatible)
+		return 0;
+
+	acpi_get_name(acpi_dev->handle, ACPI_SINGLE_NAME, &buf);
+	/* DT strings are all in lower case */
+	for (c = buf.pointer; *c != '\0'; c++)
+		*c = tolower(*c);
+
+	len = snprintf(modalias, size, "of:N%sT", (char *)buf.pointer);
+	ACPI_FREE(buf.pointer);
 
+	if (len <= 0)
+		return len;
+
+	of_compatible = acpi_dev->data.of_compatible;
+	if (of_compatible->type == ACPI_TYPE_PACKAGE) {
+		nval = of_compatible->package.count;
+		obj = of_compatible->package.elements;
+	} else { /* Must be ACPI_TYPE_STRING. */
+		nval = 1;
+		obj = of_compatible;
+	}
+	for (i = 0; i < nval; i++, obj++) {
+		count = snprintf(&modalias[len], size, "C%s",
+				 obj->string.pointer);
+		if (count < 0)
+			return -EINVAL;
+
+		if (count >= size)
+			return -ENOMEM;
+
+		len += count;
+		size -= count;
+	}
 	modalias[len] = '\0';
 	return len;
 }
@@ -236,61 +264,89 @@ static struct acpi_device *acpi_companio
 	return adev;
 }
 
-/*
- * Creates uevent modalias field for ACPI enumerated devices.
- * Because the other buses does not support ACPI HIDs & CIDs.
- * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
- * "acpi:IBM0001:ACPI0001"
- */
-int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
+int __acpi_device_uevent_modalias(struct acpi_device *adev,
+				  struct kobj_uevent_env *env)
 {
 	int len;
 
-	if (!acpi_companion_match(dev))
+	if (!adev)
 		return -ENODEV;
 
 	if (add_uevent_var(env, "MODALIAS="))
 		return -ENOMEM;
-	len = create_modalias(ACPI_COMPANION(dev), &env->buf[env->buflen - 1],
-				sizeof(env->buf) - env->buflen);
-	if (len <= 0)
+
+	len = create_pnp_modalias(adev, &env->buf[env->buflen - 1],
+				  sizeof(env->buf) - env->buflen);
+	if (len < 0) {
+		return len;
+	} else if (len > 0) {
+		env->buflen += len;
+
+		if (add_uevent_var(env, "MODALIAS="))
+			return -ENOMEM;
+	}
+	len = create_of_modalias(adev, &env->buf[env->buflen - 1],
+				 sizeof(env->buf) - env->buflen);
+	if (len < 0)
 		return len;
+
 	env->buflen += len;
+
 	return 0;
 }
-EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
 
 /*
- * Creates modalias sysfs attribute for ACPI enumerated devices.
+ * Creates uevent modalias field for ACPI enumerated devices.
  * Because the other buses does not support ACPI HIDs & CIDs.
  * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
  * "acpi:IBM0001:ACPI0001"
  */
-int acpi_device_modalias(struct device *dev, char *buf, int size)
+int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
 {
-	int len;
+	return __acpi_device_uevent_modalias(acpi_companion_match(dev), env);
+}
+EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
+
+static int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size)
+{
+	int len, count;
 
-	if (!acpi_companion_match(dev))
+	if (!adev)
 		return -ENODEV;
 
-	len = create_modalias(ACPI_COMPANION(dev), buf, size -1);
-	if (len <= 0)
+	len = create_pnp_modalias(adev, buf, size - 1);
+	if (len < 0) {
 		return len;
-	buf[len++] = '\n';
+	} else if (len > 0) {
+		buf[len++] = '\n';
+		size -= len;
+	}
+	count = create_of_modalias(adev, buf + len, size - 1);
+	if (count < 0) {
+		return count;
+	} else if (count > 0) {
+		len += count;
+		buf[len++] = '\n';
+	}
+
 	return len;
 }
+
+/*
+ * Creates modalias sysfs attribute for ACPI enumerated devices.
+ * Because the other buses does not support ACPI HIDs & CIDs.
+ * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
+ * "acpi:IBM0001:ACPI0001"
+ */
+int acpi_device_modalias(struct device *dev, char *buf, int size)
+{
+	return __acpi_device_modalias(acpi_companion_match(dev), buf, size);
+}
 EXPORT_SYMBOL_GPL(acpi_device_modalias);
 
 static ssize_t
 acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, char *buf) {
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	int len;
-
-	len = create_modalias(acpi_dev, buf, 1024);
-	if (len <= 0)
-		return len;
-	buf[len++] = '\n';
-	return len;
+	return __acpi_device_modalias(to_acpi_device(dev), buf, 1024);
 }
 static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
 
@@ -1051,19 +1107,11 @@ static int acpi_bus_match(struct device
 static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	int len;
 
 	if (list_empty(&acpi_dev->pnp.ids))
 		return 0;
 
-	if (add_uevent_var(env, "MODALIAS="))
-		return -ENOMEM;
-	len = create_modalias(acpi_dev, &env->buf[env->buflen - 1],
-			      sizeof(env->buf) - env->buflen);
-	if (len <= 0)
-		return len;
-	env->buflen += len;
-	return 0;
+	return __acpi_device_uevent_modalias(acpi_dev, env);
 }
 
 static void acpi_device_notify(acpi_handle handle, u32 event, void *data)

Re: [update][PATCH 4/4] ACPI / scan: Rework modalias creation when "compatible" is present

[Mika Westerberg]

On Thu, Apr 09, 2015 at 11:13:56PM +0200, Rafael J. Wysocki wrote:
> @@ -236,61 +264,89 @@ static struct acpi_device *acpi_companio
>  	return adev;
>  }
>  
> -/*
> - * Creates uevent modalias field for ACPI enumerated devices.
> - * Because the other buses does not support ACPI HIDs & CIDs.
> - * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
> - * "acpi:IBM0001:ACPI0001"
> - */
> -int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
> +int __acpi_device_uevent_modalias(struct acpi_device *adev,
> +				  struct kobj_uevent_env *env)
>  {
>  	int len;
>  
> -	if (!acpi_companion_match(dev))
> +	if (!adev)
>  		return -ENODEV;
>  
>  	if (add_uevent_var(env, "MODALIAS="))
>  		return -ENOMEM;
> -	len = create_modalias(ACPI_COMPANION(dev), &env->buf[env->buflen - 1],
> -				sizeof(env->buf) - env->buflen);
> -	if (len <= 0)
> +
> +	len = create_pnp_modalias(adev, &env->buf[env->buflen - 1],
> +				  sizeof(env->buf) - env->buflen);
> +	if (len < 0) {
> +		return len;
> +	} else if (len > 0) {
> +		env->buflen += len;
> +
> +		if (add_uevent_var(env, "MODALIAS="))
> +			return -ENOMEM;

If the device does not have compatible property the below will result
an empty MODALIAS= line in the uevent file:

	# cat /sys/bus/i2c/devices/i2c-MSFT0002\:00/uevent
	MODALIAS=acpi:MSFT0002:PNP0C50:
	MODALIAS=

	# cat /sys/bus/platform/devices/80860F0E\:00/uevent
	DRIVER=pxa2xx-spi
	MODALIAS=acpi:80860F0E:80860F0E:
	MODALIAS=

> +	}
> +	len = create_of_modalias(adev, &env->buf[env->buflen - 1],
> +				 sizeof(env->buf) - env->buflen);
> +	if (len < 0)
>  		return len;
> +
>  	env->buflen += len;
> +
>  	return 0;
>  }

Following patch got rid of those. Not sure if that's the best place.
Perhaps we could move this to create_*_modalias() functions instead.

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 40b0a1b7f6f9..e0eb8161149c 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -282,6 +282,9 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev,
 	} else if (len > 0) {
 		env->buflen += len;
 
+		if (!adev->data.of_compatible)
+			return 0;
+
 		if (add_uevent_var(env, "MODALIAS="))
 			return -ENOMEM;
 	}

Re: [update][PATCH 4/4] ACPI / scan: Rework modalias creation when "compatible" is present

[Mika Westerberg]

On Fri, Apr 10, 2015 at 02:34:44PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: ACPI / scan: Rework modalias creation when "compatible" is present
> 
> Currently, the ACPI modalias creation covers two mutually exclusive
> cases: If the PRP0001 device ID is present in the device's list of
> ACPI/PNP IDs and the "compatible" property is present in _DSD, the
> created modalias will follow the OF rules of modalias creation.
> Otherwise, ACPI rules are used.
> 
> However, that is not really desirable, because the presence of PRP0001
> in the list of device IDs generally does not preclude using other
> ACPI/PNP IDs with that device and those other IDs may be of higher
> priority.  In those cases, the other IDs should take preference over
> PRP0001 and therefore they also should be present in the modalias.
> 
> For this reason, rework the modalias creation for ACPI so that it
> shows both the ACPI-style and OF-style modalias strings if the
> device has a non-empty list of ACPI/PNP IDs (other than PRP0001)
> and a valid "compatible" property at the same time.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Works fine now, thanks!

For the whole series,

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [update][PATCH 4/4] ACPI / scan: Rework modalias creation when "compatible" is present

[Rafael J. Wysocki]

On Friday, April 10, 2015 11:12:24 AM Mika Westerberg wrote:
> On Thu, Apr 09, 2015 at 11:13:56PM +0200, Rafael J. Wysocki wrote:
> > @@ -236,61 +264,89 @@ static struct acpi_device *acpi_companio
> >  	return adev;
> >  }
> >  
> > -/*
> > - * Creates uevent modalias field for ACPI enumerated devices.
> > - * Because the other buses does not support ACPI HIDs & CIDs.
> > - * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
> > - * "acpi:IBM0001:ACPI0001"
> > - */
> > -int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
> > +int __acpi_device_uevent_modalias(struct acpi_device *adev,
> > +				  struct kobj_uevent_env *env)
> >  {
> >  	int len;
> >  
> > -	if (!acpi_companion_match(dev))
> > +	if (!adev)
> >  		return -ENODEV;
> >  
> >  	if (add_uevent_var(env, "MODALIAS="))
> >  		return -ENOMEM;
> > -	len = create_modalias(ACPI_COMPANION(dev), &env->buf[env->buflen - 1],
> > -				sizeof(env->buf) - env->buflen);
> > -	if (len <= 0)
> > +
> > +	len = create_pnp_modalias(adev, &env->buf[env->buflen - 1],
> > +				  sizeof(env->buf) - env->buflen);
> > +	if (len < 0) {
> > +		return len;
> > +	} else if (len > 0) {
> > +		env->buflen += len;
> > +
> > +		if (add_uevent_var(env, "MODALIAS="))
> > +			return -ENOMEM;
> 
> If the device does not have compatible property the below will result
> an empty MODALIAS= line in the uevent file:
> 
> 	# cat /sys/bus/i2c/devices/i2c-MSFT0002\:00/uevent
> 	MODALIAS=acpi:MSFT0002:PNP0C50:
> 	MODALIAS=
> 
> 	# cat /sys/bus/platform/devices/80860F0E\:00/uevent
> 	DRIVER=pxa2xx-spi
> 	MODALIAS=acpi:80860F0E:80860F0E:
> 	MODALIAS=

Ouch.

> > +	}
> > +	len = create_of_modalias(adev, &env->buf[env->buflen - 1],
> > +				 sizeof(env->buf) - env->buflen);
> > +	if (len < 0)
> >  		return len;
> > +
> >  	env->buflen += len;
> > +
> >  	return 0;
> >  }
> 
> Following patch got rid of those. Not sure if that's the best place.
> Perhaps we could move this to create_*_modalias() functions instead.

Well, we don't pass env to those and we don't need to in one case.

I've rearranged the code a bit and moved the checks around to avoid
doing the same ones for multiple times in the same code paths.  Below is
what I've got.

> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 40b0a1b7f6f9..e0eb8161149c 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -282,6 +282,9 @@ int __acpi_device_uevent_modalias(struct acpi_device *adev,
>  	} else if (len > 0) {
>  		env->buflen += len;
>  
> +		if (!adev->data.of_compatible)
> +			return 0;
> +
>  		if (add_uevent_var(env, "MODALIAS="))
>  			return -ENOMEM;
>  	}

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI / scan: Rework modalias creation when "compatible" is present

Currently, the ACPI modalias creation covers two mutually exclusive
cases: If the PRP0001 device ID is present in the device's list of
ACPI/PNP IDs and the "compatible" property is present in _DSD, the
created modalias will follow the OF rules of modalias creation.
Otherwise, ACPI rules are used.

However, that is not really desirable, because the presence of PRP0001
in the list of device IDs generally does not preclude using other
ACPI/PNP IDs with that device and those other IDs may be of higher
priority.  In those cases, the other IDs should take preference over
PRP0001 and therefore they also should be present in the modalias.

For this reason, rework the modalias creation for ACPI so that it
shows both the ACPI-style and OF-style modalias strings if the
device has a non-empty list of ACPI/PNP IDs (other than PRP0001)
and a valid "compatible" property at the same time.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c |  247 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 154 insertions(+), 93 deletions(-)

Index: linux-pm/drivers/acpi/scan.c
===================================================================
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -114,7 +114,12 @@ int acpi_scan_add_handler_with_hotplug(s
 	return 0;
 }
 
-/*
+/**
+ * create_pnp_modalias - Create hid/cid(s) string for modalias and uevent
+ * @acpi_dev: ACPI device object.
+ * @modalias: Buffer to print into.
+ * @size: Size of the buffer.
+ *
  * Creates hid/cid(s) string needed for modalias and uevent
  * e.g. on a device with hid:IBM0001 and cid:ACPI0001 you get:
  * char *modalias: "acpi:IBM0001:ACPI0001"
@@ -122,68 +127,98 @@ int acpi_scan_add_handler_with_hotplug(s
  *         -EINVAL: output error
  *         -ENOMEM: output is truncated
 */
-static int create_modalias(struct acpi_device *acpi_dev, char *modalias,
-			   int size)
+static int create_pnp_modalias(struct acpi_device *acpi_dev, char *modalias,
+			       int size)
 {
 	int len;
 	int count;
 	struct acpi_hardware_id *id;
 
-	if (list_empty(&acpi_dev->pnp.ids))
-		return 0;
-
 	/*
-	 * If the device has PRP0001 we expose DT compatible modalias
-	 * instead in form of of:NnameTCcompatible.
+	 * Since we skip PRP0001 from the modalias below, 0 should be returned
+	 * if PRP0001 is the only ACPI/PNP ID in the device's list.
 	 */
-	if (acpi_dev->data.of_compatible) {
-		struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
-		const union acpi_object *of_compatible, *obj;
-		int i, nval;
-		char *c;
-
-		acpi_get_name(acpi_dev->handle, ACPI_SINGLE_NAME, &buf);
-		/* DT strings are all in lower case */
-		for (c = buf.pointer; *c != '\0'; c++)
-			*c = tolower(*c);
-
-		len = snprintf(modalias, size, "of:N%sT", (char *)buf.pointer);
-		ACPI_FREE(buf.pointer);
-
-		of_compatible = acpi_dev->data.of_compatible;
-		if (of_compatible->type == ACPI_TYPE_PACKAGE) {
-			nval = of_compatible->package.count;
-			obj = of_compatible->package.elements;
-		} else { /* Must be ACPI_TYPE_STRING. */
-			nval = 1;
-			obj = of_compatible;
-		}
-		for (i = 0; i < nval; i++, obj++) {
-			count = snprintf(&modalias[len], size, "C%s",
-					 obj->string.pointer);
-			if (count < 0)
-				return -EINVAL;
-			if (count >= size)
-				return -ENOMEM;
+	count = 0;
+	list_for_each_entry(id, &acpi_dev->pnp.ids, list)
+		if (strcmp(id->id, "PRP0001"))
+			count++;
 
-			len += count;
-			size -= count;
-		}
-	} else {
-		len = snprintf(modalias, size, "acpi:");
-		size -= len;
+	if (!count)
+		return 0;
 
-		list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
-			count = snprintf(&modalias[len], size, "%s:", id->id);
-			if (count < 0)
-				return -EINVAL;
-			if (count >= size)
-				return -ENOMEM;
-			len += count;
-			size -= count;
-		}
+	len = snprintf(modalias, size, "acpi:");
+	if (len <= 0)
+		return len;
+
+	size -= len;
+
+	list_for_each_entry(id, &acpi_dev->pnp.ids, list) {
+		if (!strcmp(id->id, "PRP0001"))
+			continue;
+
+		count = snprintf(&modalias[len], size, "%s:", id->id);
+		if (count < 0)
+			return -EINVAL;
+
+		if (count >= size)
+			return -ENOMEM;
+
+		len += count;
+		size -= count;
+	}
+	modalias[len] = '\0';
+	return len;
+}
+
+/**
+ * create_of_modalias - Creates DT compatible string for modalias and uevent
+ * @acpi_dev: ACPI device object.
+ * @modalias: Buffer to print into.
+ * @size: Size of the buffer.
+ *
+ * Expose DT compatible modalias as of:NnameTCcompatible.  This function should
+ * only be called for devices having PRP0001 in their list of ACPI/PNP IDs.
+ */
+static int create_of_modalias(struct acpi_device *acpi_dev, char *modalias,
+			      int size)
+{
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
+	const union acpi_object *of_compatible, *obj;
+	int len, count;
+	int i, nval;
+	char *c;
+
+	acpi_get_name(acpi_dev->handle, ACPI_SINGLE_NAME, &buf);
+	/* DT strings are all in lower case */
+	for (c = buf.pointer; *c != '\0'; c++)
+		*c = tolower(*c);
+
+	len = snprintf(modalias, size, "of:N%sT", (char *)buf.pointer);
+	ACPI_FREE(buf.pointer);
+
+	if (len <= 0)
+		return len;
+
+	of_compatible = acpi_dev->data.of_compatible;
+	if (of_compatible->type == ACPI_TYPE_PACKAGE) {
+		nval = of_compatible->package.count;
+		obj = of_compatible->package.elements;
+	} else { /* Must be ACPI_TYPE_STRING. */
+		nval = 1;
+		obj = of_compatible;
 	}
+	for (i = 0; i < nval; i++, obj++) {
+		count = snprintf(&modalias[len], size, "C%s",
+				 obj->string.pointer);
+		if (count < 0)
+			return -EINVAL;
+
+		if (count >= size)
+			return -ENOMEM;
 
+		len += count;
+		size -= count;
+	}
 	modalias[len] = '\0';
 	return len;
 }
@@ -236,61 +271,100 @@ static struct acpi_device *acpi_companio
 	return adev;
 }
 
-/*
- * Creates uevent modalias field for ACPI enumerated devices.
- * Because the other buses does not support ACPI HIDs & CIDs.
- * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
- * "acpi:IBM0001:ACPI0001"
- */
-int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
+int __acpi_device_uevent_modalias(struct acpi_device *adev,
+				  struct kobj_uevent_env *env)
 {
 	int len;
 
-	if (!acpi_companion_match(dev))
+	if (!adev)
 		return -ENODEV;
 
+	if (list_empty(&adev->pnp.ids))
+		return 0;
+
 	if (add_uevent_var(env, "MODALIAS="))
 		return -ENOMEM;
-	len = create_modalias(ACPI_COMPANION(dev), &env->buf[env->buflen - 1],
-				sizeof(env->buf) - env->buflen);
-	if (len <= 0)
+
+	len = create_pnp_modalias(adev, &env->buf[env->buflen - 1],
+				  sizeof(env->buf) - env->buflen);
+	if (len < 0)
 		return len;
+
+	env->buflen += len;
+	if (!adev->data.of_compatible)
+		return 0;
+
+	if (len > 0 && add_uevent_var(env, "MODALIAS="))
+		return -ENOMEM;
+
+	len = create_of_modalias(adev, &env->buf[env->buflen - 1],
+				 sizeof(env->buf) - env->buflen);
+	if (len < 0)
+		return len;
+
 	env->buflen += len;
+
 	return 0;
 }
-EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
 
 /*
- * Creates modalias sysfs attribute for ACPI enumerated devices.
+ * Creates uevent modalias field for ACPI enumerated devices.
  * Because the other buses does not support ACPI HIDs & CIDs.
  * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
  * "acpi:IBM0001:ACPI0001"
  */
-int acpi_device_modalias(struct device *dev, char *buf, int size)
+int acpi_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
 {
-	int len;
+	return __acpi_device_uevent_modalias(acpi_companion_match(dev), env);
+}
+EXPORT_SYMBOL_GPL(acpi_device_uevent_modalias);
+
+static int __acpi_device_modalias(struct acpi_device *adev, char *buf, int size)
+{
+	int len, count;
 
-	if (!acpi_companion_match(dev))
+	if (!adev)
 		return -ENODEV;
 
-	len = create_modalias(ACPI_COMPANION(dev), buf, size -1);
-	if (len <= 0)
+	if (list_empty(&adev->pnp.ids))
+		return 0;
+
+	len = create_pnp_modalias(adev, buf, size - 1);
+	if (len < 0) {
 		return len;
-	buf[len++] = '\n';
+	} else if (len > 0) {
+		buf[len++] = '\n';
+		size -= len;
+	}
+	if (!adev->data.of_compatible)
+		return len;
+
+	count = create_of_modalias(adev, buf + len, size - 1);
+	if (count < 0) {
+		return count;
+	} else if (count > 0) {
+		len += count;
+		buf[len++] = '\n';
+	}
+
 	return len;
 }
+
+/*
+ * Creates modalias sysfs attribute for ACPI enumerated devices.
+ * Because the other buses does not support ACPI HIDs & CIDs.
+ * e.g. for a device with hid:IBM0001 and cid:ACPI0001 you get:
+ * "acpi:IBM0001:ACPI0001"
+ */
+int acpi_device_modalias(struct device *dev, char *buf, int size)
+{
+	return __acpi_device_modalias(acpi_companion_match(dev), buf, size);
+}
 EXPORT_SYMBOL_GPL(acpi_device_modalias);
 
 static ssize_t
 acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, char *buf) {
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	int len;
-
-	len = create_modalias(acpi_dev, buf, 1024);
-	if (len <= 0)
-		return len;
-	buf[len++] = '\n';
-	return len;
+	return __acpi_device_modalias(to_acpi_device(dev), buf, 1024);
 }
 static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
 
@@ -1050,20 +1124,7 @@ static int acpi_bus_match(struct device
 
 static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	struct acpi_device *acpi_dev = to_acpi_device(dev);
-	int len;
-
-	if (list_empty(&acpi_dev->pnp.ids))
-		return 0;
-
-	if (add_uevent_var(env, "MODALIAS="))
-		return -ENOMEM;
-	len = create_modalias(acpi_dev, &env->buf[env->buflen - 1],
-			      sizeof(env->buf) - env->buflen);
-	if (len <= 0)
-		return len;
-	env->buflen += len;
-	return 0;
+	return __acpi_device_uevent_modalias(to_acpi_device(dev), env);
 }
 
 static void acpi_device_notify(acpi_handle handle, u32 event, void *data)

Re: [update][PATCH 4/4] ACPI / scan: Rework modalias creation when "compatible" is present

[Rafael J. Wysocki]

On Friday, April 10, 2015 03:29:02 PM Mika Westerberg wrote:
> On Fri, Apr 10, 2015 at 02:34:44PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: ACPI / scan: Rework modalias creation when "compatible" is present
> > 
> > Currently, the ACPI modalias creation covers two mutually exclusive
> > cases: If the PRP0001 device ID is present in the device's list of
> > ACPI/PNP IDs and the "compatible" property is present in _DSD, the
> > created modalias will follow the OF rules of modalias creation.
> > Otherwise, ACPI rules are used.
> > 
> > However, that is not really desirable, because the presence of PRP0001
> > in the list of device IDs generally does not preclude using other
> > ACPI/PNP IDs with that device and those other IDs may be of higher
> > priority.  In those cases, the other IDs should take preference over
> > PRP0001 and therefore they also should be present in the modalias.
> > 
> > For this reason, rework the modalias creation for ACPI so that it
> > shows both the ACPI-style and OF-style modalias strings if the
> > device has a non-empty list of ACPI/PNP IDs (other than PRP0001)
> > and a valid "compatible" property at the same time.
> > 
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Works fine now, thanks!
> 
> For the whole series,
> 
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Great, thanks for your help!  Much appreciated.

Rafael