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@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
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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
