| Message ID | 2327246.IIyGppobQo@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Sun, Jan 27, 2013 at 2:32 PM, Rafael J. Wysocki <rjw@sisk.pl> 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. > > Thanks, > Rafael > > > --- > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > 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@intel.com> > --- > 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? Thanks Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@sisk.pl> 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@intel.com> > > 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@intel.com> > > --- > > 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
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, };