Message ID | 2612891.I2roH54cPk@vostro.rjw.lan |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
On Wed, Feb 27, 2013 at 11:06:52PM +0100, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > 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. Ick, that's not good. Can you have the original creator of that code (someone else from Intel, I can't remember at the moment), fix that up properly and send me patches? > Please let me know if there are any objections. No objection from me: Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 27, 2013 at 2:20 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Wed, Feb 27, 2013 at 11:06:52PM +0100, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> >> 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. > > Ick, that's not good. Can you have the original creator of that code > (someone else from Intel, I can't remember at the moment), fix that up > properly and send me patches? [Add To: Lan Tianyu <tianyu.lan@intel.com>] > >> Please let me know if there are any objections. I still prefer to ask USB to add bus_type instead at first. Thanks Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013年02月28日 07:31, Yinghai Lu wrote: > On Wed, Feb 27, 2013 at 2:20 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: >> On Wed, Feb 27, 2013 at 11:06:52PM +0100, Rafael J. Wysocki wrote: >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>> >>> 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. >> >> Ick, that's not good. Can you have the original creator of that code >> (someone else from Intel, I can't remember at the moment), fix that up >> properly and send me patches? > > [Add To: Lan Tianyu <tianyu.lan@intel.com>] Ok. I will fix it later. > >> >>> Please let me know if there are any objections. > > I still prefer to ask USB to add bus_type instead at first. > > Thanks > > Yinghai >
On Wednesday, February 27, 2013 02:20:32 PM Greg Kroah-Hartman wrote: > On Wed, Feb 27, 2013 at 11:06:52PM +0100, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > 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. > > Ick, that's not good. Can you have the original creator of that code > (someone else from Intel, I can't remember at the moment), fix that up > properly and send me patches? That won't be necessary afer this patch. Or do you want to fix up USB separately first? Rafael
On Wednesday, February 27, 2013 03:31:08 PM Yinghai Lu wrote: > On Wed, Feb 27, 2013 at 2:20 PM, Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > On Wed, Feb 27, 2013 at 11:06:52PM +0100, Rafael J. Wysocki wrote: > >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > >> 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. > > > > Ick, that's not good. Can you have the original creator of that code > > (someone else from Intel, I can't remember at the moment), fix that up > > properly and send me patches? > > [Add To: Lan Tianyu <tianyu.lan@intel.com>] > > > > >> Please let me know if there are any objections. > > I still prefer to ask USB to add bus_type instead at first. IIRC, You want USB to register an *additional* ACPI bus type for usb_port_device_type, which quite frankly will be very confusing and I don't see any actual benefits from doing that. Thanks, Rafael
On Thu, Feb 28, 2013 at 02:11:58AM +0100, Rafael J. Wysocki wrote: > On Wednesday, February 27, 2013 02:20:32 PM Greg Kroah-Hartman wrote: > > On Wed, Feb 27, 2013 at 11:06:52PM +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > 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. > > > > Ick, that's not good. Can you have the original creator of that code > > (someone else from Intel, I can't remember at the moment), fix that up > > properly and send me patches? > > That won't be necessary afer this patch. Or do you want to fix up USB > separately first? No, sorry, my misunderstanding, I was assuming we still needed to do other USB work after this. If not, that's an even better reason to accept this patch :) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, February 27, 2013 06:33:13 PM Greg Kroah-Hartman wrote: > On Thu, Feb 28, 2013 at 02:11:58AM +0100, Rafael J. Wysocki wrote: > > On Wednesday, February 27, 2013 02:20:32 PM Greg Kroah-Hartman wrote: > > > On Wed, Feb 27, 2013 at 11:06:52PM +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > 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. > > > > > > Ick, that's not good. Can you have the original creator of that code > > > (someone else from Intel, I can't remember at the moment), fix that up > > > properly and send me patches? > > > > That won't be necessary afer this patch. Or do you want to fix up USB > > separately first? > > No, sorry, my misunderstanding, I was assuming we still needed to do > other USB work after this. If not, that's an even better reason to > accept this patch :) Heh, thanks! Still, it seems that we can do a bit better that this. The two patches that will follow should be almost functionally equivalent to the $subject one, but the resulting code looks somewhat cleaner to me. [1/2] Add .macth() callback to struct acpi_bus_type (instead of the bus pointer). [2/2] Drop .find_bridge() from struct acpi_bus_type 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 @@ -438,10 +438,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 @@ -1144,13 +1144,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, };