| Message ID | 5604758.tWMNMRG2En@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Sun, Nov 18, 2012 at 10:12:52PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Currently, the ACPI handles of devices are initialized from within > device_add(), by acpi_bind_one() called from acpi_platform_notify() > which first uses the .find_device() routine provided by the device's > bus type to find the matching device node in the ACPI namespace. > This is a source of some computational overhead and, moreover, the > correctness of the result depends on the implementation of > .find_device() which is known to fail occasionally for some bus types > (e.g. PCI). In some cases, however, the corresponding ACPI device > node is known already before calling device_add() for the given > struct device object and the whole .find_device() dance in > acpi_platform_notify() is then simply unnecessary. > > For this reason, make it possible to initialize the ACPI handles of > devices before calling device_add() for them. Modify > acpi_platform_notify() to call acpi_bind_one() in advance to check > the device's existing ACPI handle and skip the .find_device() > search if that is successful. Change acpi_bind_one() accordingly. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/acpi/glue.c | 42 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 33 insertions(+), 9 deletions(-) > > Index: linux/drivers/acpi/glue.c > =================================================================== > --- linux.orig/drivers/acpi/glue.c > +++ linux/drivers/acpi/glue.c > @@ -135,41 +135,54 @@ static int acpi_bind_one(struct device * > int retval = -EINVAL; > > if (dev->acpi_handle) { > - dev_warn(dev, "Drivers changed 'acpi_handle'\n"); > - return -EINVAL; > + if (handle) { > + dev_warn(dev, "ACPI handle is already set\n"); > + return -EINVAL; > + } else { > + handle = dev->acpi_handle; > + } > } > + if (!handle) > + return -EINVAL; > > get_device(dev); > status = acpi_bus_get_device(handle, &acpi_dev); > if (ACPI_FAILURE(status)) > goto err; > > - physical_node = kzalloc(sizeof(struct acpi_device_physical_node), > - GFP_KERNEL); > + physical_node = kzalloc(sizeof(*physical_node), GFP_KERNEL); Here we allocate memory for the physical node... > if (!physical_node) { > retval = -ENOMEM; > goto err; > } > > mutex_lock(&acpi_dev->physical_node_lock); > + > + /* Sanity check. */ > + list_for_each_entry(physical_node, &acpi_dev->physical_node_list, node) .. and overwrite it here ;-) Maybe using a different variable for the sanity check? I've changed the SPI/I2C patches to use this as well and they got a lot smaller as we don't have to do the .find_device() magic. Once you have fixed the above, you can add my Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> to these two patches, if you like. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux/drivers/acpi/glue.c =================================================================== --- linux.orig/drivers/acpi/glue.c +++ linux/drivers/acpi/glue.c @@ -135,41 +135,54 @@ static int acpi_bind_one(struct device * int retval = -EINVAL; if (dev->acpi_handle) { - dev_warn(dev, "Drivers changed 'acpi_handle'\n"); - return -EINVAL; + if (handle) { + dev_warn(dev, "ACPI handle is already set\n"); + return -EINVAL; + } else { + handle = dev->acpi_handle; + } } + if (!handle) + return -EINVAL; get_device(dev); status = acpi_bus_get_device(handle, &acpi_dev); if (ACPI_FAILURE(status)) goto err; - physical_node = kzalloc(sizeof(struct acpi_device_physical_node), - GFP_KERNEL); + physical_node = kzalloc(sizeof(*physical_node), GFP_KERNEL); if (!physical_node) { retval = -ENOMEM; goto err; } mutex_lock(&acpi_dev->physical_node_lock); + + /* Sanity check. */ + list_for_each_entry(physical_node, &acpi_dev->physical_node_list, node) + if (physical_node->dev == dev) { + dev_warn(dev, "Already associated with ACPI node\n"); + goto err_free; + } + /* allocate physical node id according to physical_node_id_bitmap */ physical_node->node_id = find_first_zero_bit(acpi_dev->physical_node_id_bitmap, ACPI_MAX_PHYSICAL_NODE); if (physical_node->node_id >= ACPI_MAX_PHYSICAL_NODE) { retval = -ENOSPC; - mutex_unlock(&acpi_dev->physical_node_lock); - kfree(physical_node); - goto err; + goto err_free; } set_bit(physical_node->node_id, acpi_dev->physical_node_id_bitmap); physical_node->dev = dev; list_add_tail(&physical_node->node, &acpi_dev->physical_node_list); acpi_dev->physical_node_count++; + mutex_unlock(&acpi_dev->physical_node_lock); - dev->acpi_handle = handle; + if (!dev->acpi_handle) + dev->acpi_handle = handle; if (!physical_node->node_id) strcpy(physical_node_name, PHYSICAL_NODE_STRING); @@ -187,8 +200,14 @@ static int acpi_bind_one(struct device * return 0; err: + dev->acpi_handle = NULL; put_device(dev); return retval; + + err_free: + mutex_unlock(&acpi_dev->physical_node_lock); + kfree(physical_node); + goto err; } static int acpi_unbind_one(struct device *dev) @@ -247,6 +266,10 @@ static int acpi_platform_notify(struct d acpi_handle handle; int ret = -EINVAL; + ret = acpi_bind_one(dev, NULL); + if (!ret) + goto out; + if (!dev->bus || !dev->parent) { /* bridge devices genernally haven't bus or parent */ ret = acpi_find_bridge_device(dev, &handle); @@ -260,10 +283,11 @@ static int acpi_platform_notify(struct d } if ((ret = type->find_device(dev, &handle)) != 0) DBG("Can't get handler for %s\n", dev_name(dev)); - end: + end: if (!ret) acpi_bind_one(dev, handle); + out: #if ACPI_GLUE_DEBUG if (!ret) { struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };