From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756013AbbDIROl (ORCPT ); Thu, 9 Apr 2015 13:14:41 -0400 Received: from v094114.home.net.pl ([79.96.170.134]:56807 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1753965AbbDIROh (ORCPT ); Thu, 9 Apr 2015 13:14:37 -0400 From: "Rafael J. Wysocki" To: Mika Westerberg Cc: Darren Hart , ACPI Devel Maling List , Linux Kernel Mailing List Subject: Re: [PATCH 4/4] ACPI / scan: Rework modalias creation when "compatible" is present Date: Thu, 09 Apr 2015 19:39:02 +0200 Message-ID: <7377982.P2rKGcn6lO@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/3.19.0+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20150409122756.GJ1734@lahna.fi.intel.com> References: <4378053.gHTT51U7FJ@vostro.rjw.lan> <1630931.9AX0JeoSOf@vostro.rjw.lan> <20150409122756.GJ1734@lahna.fi.intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > > 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