Re: [PATCH v3 04/22] PCI, ACPI: Update comments for find_bridge in acpi_bus_type

["Rafael J. Wysocki" <rjw@sisk.pl>]

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


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.