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-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
