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.wyso...@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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to