On Sunday, January 27, 2013 05:00:38 PM Yinghai Lu wrote: > On Sun, Jan 27, 2013 at 2:32 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote: > > On Sunday, January 27, 2013 11:23:31 AM Yinghai Lu wrote: > >> only device that does not have bus_type, will go to that path... > > > > While I agree that the comment doesn't make sense any more, I also think that > > changing the comment alone is not sufficient, because what USB does with > > .find_bridge() is really disgusting to me and USB is the only real user of it > > (SATA just pretends to be one). > > agree. that is really like hack for usb. > > > > > So, instead of this change, I'd prefer to apply the appended patch some time > > in the second part of the 3.9 merge window, after integrating all of the > > ACPI and PCI changes already queued up. > > Good, I will drop patch 4 but will keep patch3 about libata part with > ack from Jeff Garzik. OK > > --- > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > Subject: ACPI / glue: Drop .find_bridge() callback from struct acpi_bus_type > > > > After PCI has stopped using the .find_bridge() callback in > > struct acpi_bus_type, the only remaining users of it are SATA and > > USB. However, SATA only pretends to be a user, because it points > > that callback to a stub always returning -ENODEV, and USB uses it > > incorrectly, because as a result of the way it is used by USB every > > device in the system that doesn't have a bus type or parent is > > passed to usb_acpi_find_device() for inspection. > > > > What USB actually needs, though, is to call usb_acpi_find_device() > > for USB ports that don't have a bus type defined, but have > > usb_port_device_type as their device type. > > > > To fix that add a device type field to struct acpi_bus_type and > > make acpi_get_bus_type() compare it with the device type fields of > > the device objects it inspects in addition to checking their bus > > types. Next, make USB set the new type field in usb_acpi_bus to > > point to usb_port_device_type and stop abusing the .find_bridge() > > callback. > > > > In addition to that drop the SATA's dummy .find_bridge() callback, > > and remove .find_bridge(), which is not used any more, from struct > > acpi_bus_type entirely. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > --- > > drivers/acpi/glue.c | 39 ++++++--------------------------------- > > drivers/ata/libata-acpi.c | 6 ------ > > drivers/usb/core/usb-acpi.c | 2 +- > > include/acpi/acpi_bus.h | 4 +--- > > 4 files changed, 8 insertions(+), 43 deletions(-) > > > > Index: linux-pm/drivers/acpi/glue.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/glue.c > > +++ linux-pm/drivers/acpi/glue.c > > @@ -64,16 +64,17 @@ int unregister_acpi_bus_type(struct acpi > > } > > EXPORT_SYMBOL_GPL(unregister_acpi_bus_type); > > > > -static struct acpi_bus_type *acpi_get_bus_type(struct bus_type *type) > > +static struct acpi_bus_type *acpi_get_bus_type(struct device *dev) > > { > > struct acpi_bus_type *tmp, *ret = NULL; > > > > - if (!type) > > + if (!dev->bus && !dev->type) > > return NULL; > > > > down_read(&bus_type_sem); > > list_for_each_entry(tmp, &bus_type_list, list) { > > - if (tmp->bus == type) { > > + if ((tmp->bus && tmp->bus == dev->bus) > > + || (tmp->type && tmp->type == dev->type)) { > > ret = tmp; > > break; > > } > > @@ -82,22 +83,6 @@ static struct acpi_bus_type *acpi_get_bu > > return ret; > > } > > > > -static int acpi_find_bridge_device(struct device *dev, acpi_handle * handle) > > -{ > > - struct acpi_bus_type *tmp; > > - int ret = -ENODEV; > > - > > - down_read(&bus_type_sem); > > - list_for_each_entry(tmp, &bus_type_list, list) { > > - if (tmp->find_bridge && !tmp->find_bridge(dev, handle)) { > > - ret = 0; > > - break; > > - } > > - } > > - up_read(&bus_type_sem); > > - return ret; > > -} > > - > > static acpi_status do_acpi_find_child(acpi_handle handle, u32 lvl_not_used, > > void *addr_p, void **ret_p) > > { > > @@ -261,22 +246,11 @@ err: > > > > static int acpi_platform_notify(struct device *dev) > > { > > - struct acpi_bus_type *type; > > + struct acpi_bus_type *type = acpi_get_bus_type(dev); > > acpi_handle handle; > > int ret; > > > > ret = acpi_bind_one(dev, NULL); > > - if (ret && (!dev->bus || !dev->parent)) { > > - /* bridge devices genernally haven't bus or parent */ > > - ret = acpi_find_bridge_device(dev, &handle); > > - if (!ret) { > > - ret = acpi_bind_one(dev, handle); > > - if (ret) > > - goto out; > > - } > > - } > > - > > - type = acpi_get_bus_type(dev->bus); > > if (ret) { > > if (!type || !type->find_device) { > > DBG("No ACPI bus support for %s\n", dev_name(dev)); > > @@ -293,7 +267,6 @@ static int acpi_platform_notify(struct d > > if (ret) > > goto out; > > } > > - > > if (type && type->setup) > > type->setup(dev); > > > > @@ -316,7 +289,7 @@ static int acpi_platform_notify_remove(s > > { > > struct acpi_bus_type *type; > > > > - type = acpi_get_bus_type(dev->bus); > > + type = acpi_get_bus_type(dev); > > if (type && type->cleanup) > > type->cleanup(dev); > > > > Index: linux-pm/include/acpi/acpi_bus.h > > =================================================================== > > --- linux-pm.orig/include/acpi/acpi_bus.h > > +++ linux-pm/include/acpi/acpi_bus.h > > @@ -412,10 +412,8 @@ void acpi_remove_dir(struct acpi_device > > struct acpi_bus_type { > > struct list_head list; > > struct bus_type *bus; > > - /* For general devices under the bus */ > > + struct device_type *type; > > int (*find_device) (struct device *, acpi_handle *); > > - /* For bridges, such as PCI root bridge, IDE controller */ > > - int (*find_bridge) (struct device *, acpi_handle *); > > void (*setup)(struct device *); > > void (*cleanup)(struct device *); > > }; > > Index: linux-pm/drivers/ata/libata-acpi.c > > =================================================================== > > --- linux-pm.orig/drivers/ata/libata-acpi.c > > +++ linux-pm/drivers/ata/libata-acpi.c > > @@ -1167,13 +1167,7 @@ static int ata_acpi_find_device(struct d > > return -ENODEV; > > } > > > > -static int ata_acpi_find_dummy(struct device *dev, acpi_handle *handle) > > -{ > > - return -ENODEV; > > -} > > - > > static struct acpi_bus_type ata_acpi_bus = { > > - .find_bridge = ata_acpi_find_dummy, > > .find_device = ata_acpi_find_device, > > }; > > > > Index: linux-pm/drivers/usb/core/usb-acpi.c > > =================================================================== > > --- linux-pm.orig/drivers/usb/core/usb-acpi.c > > +++ linux-pm/drivers/usb/core/usb-acpi.c > > @@ -212,7 +212,7 @@ static int usb_acpi_find_device(struct d > > > > static struct acpi_bus_type usb_acpi_bus = { > > .bus = &usb_bus_type, > > - .find_bridge = usb_acpi_find_device, > > + .type = &usb_port_device_type, > > .find_device = usb_acpi_find_device, > > }; > > Can we add one acpi_bus_type for port directly? We could, but then we'd trade one extra check in usb_acpi_find_device() for one extra item in bus_type_list that would be checked for every invocation of acpi_platform_notify() and acpi_platform_notify(). I'm not sure if that's a good deal. :-) Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- 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