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@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> ---
>  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) {
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/