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

login
register
mail settings
Submitter Rafael J. Wysocki
Date Jan. 27, 2013, 10:32 p.m.
Message ID <2327246.IIyGppobQo@vostro.rjw.lan>
Download mbox | patch
Permalink /patch/216072/
State Superseded
Headers show

Comments

Rafael J. Wysocki - Jan. 27, 2013, 10:32 p.m.
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).

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.

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(-)
Yinghai Lu - Jan. 28, 2013, 1 a.m.
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
Rafael J. Wysocki - Jan. 28, 2013, 12:37 p.m.
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

Patch

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,
 };