| Message ID | 4082842.31rfDviYHK@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote: > On Monday, December 17, 2012 05:08:17 PM Toshi Kani wrote: > > On Thu, 2012-12-13 at 23:17 +0100, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > (snip) > > > > > struct acpi_device_ops { > > > Index: linux/drivers/acpi/scan.c > > > =================================================================== > > > --- linux.orig/drivers/acpi/scan.c > > > +++ linux/drivers/acpi/scan.c > > > @@ -494,7 +494,8 @@ static int acpi_bus_match(struct device > > > struct acpi_device *acpi_dev = to_acpi_device(dev); > > > struct acpi_driver *acpi_drv = to_acpi_driver(drv); > > > > > > - return !acpi_match_device_ids(acpi_dev, acpi_drv->ids); > > > + return acpi_dev->bus_ops.acpi_op_match > > > + && !acpi_match_device_ids(acpi_dev, acpi_drv->ids); > > > } > > > > > > static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env) > > > @@ -1418,6 +1419,17 @@ static int acpi_bus_remove(struct acpi_d > > > return 0; > > > } > > > > > > +/* > > > + * acpi_hot_add_bind - Bind _ADR-based devices on hot-add. > > > + * @device: ACPI device node to bind. > > > + */ > > > +static void acpi_hot_add_bind(struct acpi_device *device) > > > +{ > > > + if (device->flags.bus_address > > > + && device->parent && device->parent->ops.bind) > > > + device->parent->ops.bind(device); > > > +} > > > + > > > static int acpi_add_single_object(struct acpi_device **child, > > > acpi_handle handle, int type, > > > unsigned long long sta, > > > @@ -1490,13 +1502,8 @@ static int acpi_add_single_object(struct > > > > > > result = acpi_device_register(device); > > > > > > - /* > > > - * Bind _ADR-Based Devices when hot add > > > - */ > > > - if (device->flags.bus_address) { > > > - if (device->parent && device->parent->ops.bind) > > > - device->parent->ops.bind(device); > > > - } > > > > I think the original code above is hot-add only because ops.bind is not > > set at boot since the acpi_pci driver has not been registered yet. It > > seems that acpi_pci_bridge_scan() called from acpi_pci_root_add() takes > > care of the binding. > > Ah, I see the problem. During boot the PCI root bridge driver is not present > yet when all struct acpi_device "devices" are registered, so their parents' > .bind() callbacks are all empty, so the code above has no effect. > > But say we're doing a PCI root bridge hotplug, in which case the driver is > present, so acpi_pci_bind() will be executed both from acpi_pci_bridge_scan() > and from here, won't it? Right. > OK, this needs to be addressed. > > > This brings me a question for acpi_bus_probe_start() below... > > > > > > > + if (device->bus_ops.acpi_op_match) > > > + acpi_hot_add_bind(device); > > > > > > end: > > > if (!result) { > > > @@ -1522,6 +1529,7 @@ static void acpi_bus_add_power_resource( > > > struct acpi_bus_ops ops = { > > > .acpi_op_add = 1, > > > .acpi_op_start = 1, > > > + .acpi_op_match = 1, > > > }; > > > struct acpi_device *device = NULL; > > > > > > @@ -1574,9 +1582,9 @@ static acpi_status acpi_bus_check_add(ac > > > void *context, void **return_value) > > > { > > > struct acpi_bus_ops *ops = context; > > > + struct acpi_device *device = NULL; > > > int type; > > > unsigned long long sta; > > > - struct acpi_device *device; > > > acpi_status status; > > > int result; > > > > > > @@ -1596,52 +1604,86 @@ static acpi_status acpi_bus_check_add(ac > > > return AE_CTRL_DEPTH; > > > } > > > > > > - /* > > > - * We may already have an acpi_device from a previous enumeration. If > > > - * so, we needn't add it again, but we may still have to start it. > > > - */ > > > - device = NULL; > > > acpi_bus_get_device(handle, &device); > > > if (ops->acpi_op_add && !device) { > > > - acpi_add_single_object(&device, handle, type, sta, ops); > > > - /* Is the device a known good platform device? */ > > > - if (device > > > - && !acpi_match_device_ids(device, acpi_platform_device_ids)) > > > - acpi_create_platform_device(device); > > > - } > > > - > > > - if (!device) > > > - return AE_CTRL_DEPTH; > > > + struct acpi_bus_ops add_ops = *ops; > > > > > > - if (ops->acpi_op_start && !(ops->acpi_op_add)) { > > > - status = acpi_start_single_object(device); > > > - if (ACPI_FAILURE(status)) > > > + add_ops.acpi_op_match = 0; > > > + acpi_add_single_object(&device, handle, type, sta, &add_ops); > > > + if (!device) > > > return AE_CTRL_DEPTH; > > > + > > > + device->bus_ops.acpi_op_match = 1; > > > } > > > > > > if (!*return_value) > > > *return_value = device; > > > + > > > return AE_OK; > > > } > > > > > > +static acpi_status acpi_bus_probe_start(acpi_handle handle, u32 lvl, > > > + void *context, void **not_used) > > > +{ > > > + struct acpi_bus_ops *ops = context; > > > + acpi_status status = AE_OK; > > > + struct acpi_device *device; > > > + unsigned long long sta_not_used; > > > + int type_not_used; > > > + > > > + /* > > > + * Ignore errors ignored by acpi_bus_check_add() to avoid terminating > > > > "ignore" seems duplicated. > > It is not. This is supposed to mean that the errors previously ignored by > acpi_bus_check_add() should be ignored here as well. Oh, I see. Thanks for the clarification. > > > + * namespace walks prematurely. > > > + */ > > > + if (acpi_bus_type_and_status(handle, &type_not_used, &sta_not_used)) > > > + return AE_OK; > > > + > > > + if (acpi_bus_get_device(handle, &device)) > > > + return AE_CTRL_DEPTH; > > > + > > > + if (ops->acpi_op_add) { > > > + if (!acpi_match_device_ids(device, acpi_platform_device_ids)) { > > > + /* This is a known good platform device. */ > > > + acpi_create_platform_device(device); > > > + } else { > > > + int ret = device_attach(&device->dev); > > > + acpi_hot_add_bind(device); > > > > Since acpi_pci_root_add() is called by device_attach(), I think this > > acpi_hot_add_bind() calls .bind() of a device at boot since its .bind() > > may be set. Is that correct? If so, how does it coordinate with the > > bind procedure in acpi_pci_bridge_scan()? > > It actually doesn't. > > However, the $subject patch doesn't change this particular aspect of the > original behavior, because with it applied the PCI root bridge driver is still > not present when the device_attach() above is executed for all objects in the > given namespace scope, so the .bind() callbacks should all be empty. In other > words, it doesn't change the boot case. > > It also reproduces the original behavior in the hotplug case which may not be > correct. Patch [2/6], however, kind of changes the boot case into the hotplug > case and things start to get ugly. Yes, I was concerned with the behavior when patch [2/6] applied. It is actually a good thing that this hotplug issue is now exposed in the boot path. This means that boot and hot-add paths are consistent. So, we just need to fix it. > Well, what about calling acpi_hot_add_bind() from acpi_bus_check_add(), > right after doing the acpi_add_single_object()? It would avoid calling > acpi_pci_bind() twice for the same device during root bridge hotplug too, > because in that case acpi_pci_root_add() will be called after all of these > acpi_hot_add_bind() calls. At the same time if a single device is > hot-added and its parent happens to have .bind() set, it will be run > from acpi_bus_check_add(). I may be missing something here, but in case of root bridge hot-add, I think acpi_pci_root_add() still calls .bind() after acpi_bus_check_add() called it. So, it's called twice, isn't it? We need to decide which module is responsible for calling .bind(). I think it should be the ACPI scan module, not the ACPI PCI root bridge driver, because: - bind() needs to be called when _ADR device is added. The ACPI scan module can scan any devices, while the PCI root driver can only scan when it is added. - acpi_bus_remove() calls unbind() at hot-remove. The same module should be responsible for both bind() and unbind() handling. - It is cleaner to keep struct acpi_device_ops interface to be called by the ACPI core. So, I would propose the following changes. - Move the acpi_hot_add_bind() call back to the original place after the device_attach() call. - Rename the name of acpi_hot_add_bind() to something like acpi_bind_adr_device() since it is no longer hot-add only (and is specific to _ADR devices). - Create its pair function, acpi_unbind_adr_device(), which is called from acpi_bus_remove(). When a constructor interface is introduced, its destructor should be introduced as well. - Remove the binding procedure from acpi_pci_root_add(). This should be done in patch [2/6]. Thanks, -Toshi -- 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 Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani <toshi.kani@hp.com> wrote: > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote: > > So, I would propose the following changes. > > - Move the acpi_hot_add_bind() call back to the original place after > the device_attach() call. > - Rename the name of acpi_hot_add_bind() to something like > acpi_bind_adr_device() since it is no longer hot-add only (and is > specific to _ADR devices). > - Create its pair function, acpi_unbind_adr_device(), which is called > from acpi_bus_remove(). When a constructor interface is introduced, its > destructor should be introduced as well. > - Remove the binding procedure from acpi_pci_root_add(). This should > be done in patch [2/6]. i think we should put jiang four patches before Rafael's patches. http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-jiang-hotplug -- 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 Tue, 2012-12-18 at 10:59 -0800, Yinghai Lu wrote: > On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani <toshi.kani@hp.com> wrote: > > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote: > > > > So, I would propose the following changes. > > > > - Move the acpi_hot_add_bind() call back to the original place after > > the device_attach() call. > > - Rename the name of acpi_hot_add_bind() to something like > > acpi_bind_adr_device() since it is no longer hot-add only (and is > > specific to _ADR devices). > > - Create its pair function, acpi_unbind_adr_device(), which is called > > from acpi_bus_remove(). When a constructor interface is introduced, its > > destructor should be introduced as well. > > - Remove the binding procedure from acpi_pci_root_add(). This should > > be done in patch [2/6]. > > i think we should put jiang four patches before Rafael's patches. > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-jiang-hotplug Thanks for the pointer! Oh, I did not know that pci_dev gets removed before acpi_bus_trim() is called... I looked at Jiang's last two patches for the bind update, and they look good to me. Thanks, -Toshi -- 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 Tue, Dec 18, 2012 at 12:48 PM, Toshi Kani <toshi.kani@hp.com> wrote: > On Tue, 2012-12-18 at 10:59 -0800, Yinghai Lu wrote: >> On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani <toshi.kani@hp.com> wrote: >> > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote: >> > >> > So, I would propose the following changes. >> > >> > - Move the acpi_hot_add_bind() call back to the original place after >> > the device_attach() call. >> > - Rename the name of acpi_hot_add_bind() to something like >> > acpi_bind_adr_device() since it is no longer hot-add only (and is >> > specific to _ADR devices). >> > - Create its pair function, acpi_unbind_adr_device(), which is called >> > from acpi_bus_remove(). When a constructor interface is introduced, its >> > destructor should be introduced as well. >> > - Remove the binding procedure from acpi_pci_root_add(). This should >> > be done in patch [2/6]. >> >> i think we should put jiang four patches before Rafael's patches. >> >> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-jiang-hotplug > > Thanks for the pointer! Oh, I did not know that pci_dev gets removed > before acpi_bus_trim() is called... yes. that is http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=8b4b836d8c56c290bc80ffa0b08b91fb3fe38867 > I looked at Jiang's last two > patches for the bind update, and they look good to me. > > Thanks, > -Toshi > -- 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 Tuesday, December 18, 2012 09:10:41 AM Toshi Kani wrote: > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote: > > On Monday, December 17, 2012 05:08:17 PM Toshi Kani wrote: > > > On Thu, 2012-12-13 at 23:17 +0100, Rafael J. Wysocki wrote: > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > > > (snip) > > > > > > > struct acpi_device_ops { > > > > Index: linux/drivers/acpi/scan.c > > > > =================================================================== > > > > --- linux.orig/drivers/acpi/scan.c > > > > +++ linux/drivers/acpi/scan.c > > > > @@ -494,7 +494,8 @@ static int acpi_bus_match(struct device > > > > struct acpi_device *acpi_dev = to_acpi_device(dev); > > > > struct acpi_driver *acpi_drv = to_acpi_driver(drv); > > > > > > > > - return !acpi_match_device_ids(acpi_dev, acpi_drv->ids); > > > > + return acpi_dev->bus_ops.acpi_op_match > > > > + && !acpi_match_device_ids(acpi_dev, acpi_drv->ids); > > > > } > > > > > > > > static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env) > > > > @@ -1418,6 +1419,17 @@ static int acpi_bus_remove(struct acpi_d > > > > return 0; > > > > } > > > > > > > > +/* > > > > + * acpi_hot_add_bind - Bind _ADR-based devices on hot-add. > > > > + * @device: ACPI device node to bind. > > > > + */ > > > > +static void acpi_hot_add_bind(struct acpi_device *device) > > > > +{ > > > > + if (device->flags.bus_address > > > > + && device->parent && device->parent->ops.bind) > > > > + device->parent->ops.bind(device); > > > > +} > > > > + > > > > static int acpi_add_single_object(struct acpi_device **child, > > > > acpi_handle handle, int type, > > > > unsigned long long sta, > > > > @@ -1490,13 +1502,8 @@ static int acpi_add_single_object(struct > > > > > > > > result = acpi_device_register(device); > > > > > > > > - /* > > > > - * Bind _ADR-Based Devices when hot add > > > > - */ > > > > - if (device->flags.bus_address) { > > > > - if (device->parent && device->parent->ops.bind) > > > > - device->parent->ops.bind(device); > > > > - } > > > > > > I think the original code above is hot-add only because ops.bind is not > > > set at boot since the acpi_pci driver has not been registered yet. It > > > seems that acpi_pci_bridge_scan() called from acpi_pci_root_add() takes > > > care of the binding. > > > > Ah, I see the problem. During boot the PCI root bridge driver is not present > > yet when all struct acpi_device "devices" are registered, so their parents' > > .bind() callbacks are all empty, so the code above has no effect. > > > > But say we're doing a PCI root bridge hotplug, in which case the driver is > > present, so acpi_pci_bind() will be executed both from acpi_pci_bridge_scan() > > and from here, won't it? > > Right. > > > > OK, this needs to be addressed. > > > > > This brings me a question for acpi_bus_probe_start() below... > > > > > > > > > > + if (device->bus_ops.acpi_op_match) > > > > + acpi_hot_add_bind(device); > > > > > > > > end: > > > > if (!result) { > > > > @@ -1522,6 +1529,7 @@ static void acpi_bus_add_power_resource( > > > > struct acpi_bus_ops ops = { > > > > .acpi_op_add = 1, > > > > .acpi_op_start = 1, > > > > + .acpi_op_match = 1, > > > > }; > > > > struct acpi_device *device = NULL; > > > > > > > > @@ -1574,9 +1582,9 @@ static acpi_status acpi_bus_check_add(ac > > > > void *context, void **return_value) > > > > { > > > > struct acpi_bus_ops *ops = context; > > > > + struct acpi_device *device = NULL; > > > > int type; > > > > unsigned long long sta; > > > > - struct acpi_device *device; > > > > acpi_status status; > > > > int result; > > > > > > > > @@ -1596,52 +1604,86 @@ static acpi_status acpi_bus_check_add(ac > > > > return AE_CTRL_DEPTH; > > > > } > > > > > > > > - /* > > > > - * We may already have an acpi_device from a previous enumeration. If > > > > - * so, we needn't add it again, but we may still have to start it. > > > > - */ > > > > - device = NULL; > > > > acpi_bus_get_device(handle, &device); > > > > if (ops->acpi_op_add && !device) { > > > > - acpi_add_single_object(&device, handle, type, sta, ops); > > > > - /* Is the device a known good platform device? */ > > > > - if (device > > > > - && !acpi_match_device_ids(device, acpi_platform_device_ids)) > > > > - acpi_create_platform_device(device); > > > > - } > > > > - > > > > - if (!device) > > > > - return AE_CTRL_DEPTH; > > > > + struct acpi_bus_ops add_ops = *ops; > > > > > > > > - if (ops->acpi_op_start && !(ops->acpi_op_add)) { > > > > - status = acpi_start_single_object(device); > > > > - if (ACPI_FAILURE(status)) > > > > + add_ops.acpi_op_match = 0; > > > > + acpi_add_single_object(&device, handle, type, sta, &add_ops); > > > > + if (!device) > > > > return AE_CTRL_DEPTH; > > > > + > > > > + device->bus_ops.acpi_op_match = 1; > > > > } > > > > > > > > if (!*return_value) > > > > *return_value = device; > > > > + > > > > return AE_OK; > > > > } > > > > > > > > +static acpi_status acpi_bus_probe_start(acpi_handle handle, u32 lvl, > > > > + void *context, void **not_used) > > > > +{ > > > > + struct acpi_bus_ops *ops = context; > > > > + acpi_status status = AE_OK; > > > > + struct acpi_device *device; > > > > + unsigned long long sta_not_used; > > > > + int type_not_used; > > > > + > > > > + /* > > > > + * Ignore errors ignored by acpi_bus_check_add() to avoid terminating > > > > > > "ignore" seems duplicated. > > > > It is not. This is supposed to mean that the errors previously ignored by > > acpi_bus_check_add() should be ignored here as well. > > Oh, I see. Thanks for the clarification. > > > > > > + * namespace walks prematurely. > > > > + */ > > > > + if (acpi_bus_type_and_status(handle, &type_not_used, &sta_not_used)) > > > > + return AE_OK; > > > > + > > > > + if (acpi_bus_get_device(handle, &device)) > > > > + return AE_CTRL_DEPTH; > > > > + > > > > + if (ops->acpi_op_add) { > > > > + if (!acpi_match_device_ids(device, acpi_platform_device_ids)) { > > > > + /* This is a known good platform device. */ > > > > + acpi_create_platform_device(device); > > > > + } else { > > > > + int ret = device_attach(&device->dev); > > > > + acpi_hot_add_bind(device); > > > > > > Since acpi_pci_root_add() is called by device_attach(), I think this > > > acpi_hot_add_bind() calls .bind() of a device at boot since its .bind() > > > may be set. Is that correct? If so, how does it coordinate with the > > > bind procedure in acpi_pci_bridge_scan()? > > > > It actually doesn't. > > > > However, the $subject patch doesn't change this particular aspect of the > > original behavior, because with it applied the PCI root bridge driver is still > > not present when the device_attach() above is executed for all objects in the > > given namespace scope, so the .bind() callbacks should all be empty. In other > > words, it doesn't change the boot case. > > > > It also reproduces the original behavior in the hotplug case which may not be > > correct. Patch [2/6], however, kind of changes the boot case into the hotplug > > case and things start to get ugly. > > Yes, I was concerned with the behavior when patch [2/6] applied. It is > actually a good thing that this hotplug issue is now exposed in the boot > path. This means that boot and hot-add paths are consistent. So, we > just need to fix it. > > > > Well, what about calling acpi_hot_add_bind() from acpi_bus_check_add(), > > right after doing the acpi_add_single_object()? It would avoid calling > > acpi_pci_bind() twice for the same device during root bridge hotplug too, > > because in that case acpi_pci_root_add() will be called after all of these > > acpi_hot_add_bind() calls. At the same time if a single device is > > hot-added and its parent happens to have .bind() set, it will be run > > from acpi_bus_check_add(). > > I may be missing something here, but in case of root bridge hot-add, I > think acpi_pci_root_add() still calls .bind() after acpi_bus_check_add() > called it. So, it's called twice, isn't it? No, it isn't. The reason is that the .bind pointers of all devices below the root bridge are populated from within acpi_pci_root_add() and are NULL before it is called. Therefore they are NULL when acpi_bus_check_add() checks them. Of course, I'm referring to this patch: https://patchwork.kernel.org/patch/1889821/ After this and [2/6] (https://patchwork.kernel.org/patch/1884701/) have been applied, it is quite easy to verify that acpi_pci_bind() is called exactly once for each PCI device during boot (just add a debug printk to that function) and the same should happen during root bridge hotplug. > We need to decide which module is responsible for calling .bind(). I > think it should be the ACPI scan module, not the ACPI PCI root bridge > driver, because: > - bind() needs to be called when _ADR device is added. The ACPI scan > module can scan any devices, while the PCI root driver can only scan > when it is added. > - acpi_bus_remove() calls unbind() at hot-remove. The same module > should be responsible for both bind() and unbind() handling. > - It is cleaner to keep struct acpi_device_ops interface to be called > by the ACPI core. I agree with that. :-) Moreover, I don't think we need acpi_pci_bind() and acpi_pci_unbind() at all. > So, I would propose the following changes. > > - Move the acpi_hot_add_bind() call back to the original place after > the device_attach() call. > - Rename the name of acpi_hot_add_bind() to something like > acpi_bind_adr_device() since it is no longer hot-add only (and is > specific to _ADR devices). > - Create its pair function, acpi_unbind_adr_device(), which is called > from acpi_bus_remove(). When a constructor interface is introduced, its > destructor should be introduced as well. > - Remove the binding procedure from acpi_pci_root_add(). This should > be done in patch [2/6]. Well, what about moving the code from acpi_pci_bind()/acpi_pci_unbind() somewhere else and removing those things altogether? Rafael
On Tuesday, December 18, 2012 10:59:46 AM Yinghai Lu wrote: > On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani <toshi.kani@hp.com> wrote: > > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote: > > > > So, I would propose the following changes. > > > > - Move the acpi_hot_add_bind() call back to the original place after > > the device_attach() call. > > - Rename the name of acpi_hot_add_bind() to something like > > acpi_bind_adr_device() since it is no longer hot-add only (and is > > specific to _ADR devices). > > - Create its pair function, acpi_unbind_adr_device(), which is called > > from acpi_bus_remove(). When a constructor interface is introduced, its > > destructor should be introduced as well. > > - Remove the binding procedure from acpi_pci_root_add(). This should > > be done in patch [2/6]. > > i think we should put jiang four patches before Rafael's patches. > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-jiang-hotplug Actually, I have something more radical than that in mind. :-) Namely, we don't need to call the wakeup-related stuff from acpi_pci_bind() and acpi_pci_unbind(). It's only there, because I did't find a better place for it when I added it. If we can set the ACPI handles of PCI devices in pci_scan_device(), which isn't too difficult to do (I actually have a patch for that and it's not too invasive), we can easily move the wakeup enabling stuff to pci_pm_init() and wakeup disabling somewhere near pci_release_capabilities(). Then we are only left with the _PRT setup in there, but that could be done as soon as in pci_scan_device() too, if I'm not mistaken. Thanks, Rafael
On Tue, 2012-12-18 at 22:57 +0100, Rafael J. Wysocki wrote: > On Tuesday, December 18, 2012 09:10:41 AM Toshi Kani wrote: > > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote: : > > We need to decide which module is responsible for calling .bind(). I > > think it should be the ACPI scan module, not the ACPI PCI root bridge > > driver, because: > > - bind() needs to be called when _ADR device is added. The ACPI scan > > module can scan any devices, while the PCI root driver can only scan > > when it is added. > > - acpi_bus_remove() calls unbind() at hot-remove. The same module > > should be responsible for both bind() and unbind() handling. > > - It is cleaner to keep struct acpi_device_ops interface to be called > > by the ACPI core. > > I agree with that. :-) > > Moreover, I don't think we need acpi_pci_bind() and acpi_pci_unbind() at all. > > > So, I would propose the following changes. > > > > - Move the acpi_hot_add_bind() call back to the original place after > > the device_attach() call. > > - Rename the name of acpi_hot_add_bind() to something like > > acpi_bind_adr_device() since it is no longer hot-add only (and is > > specific to _ADR devices). > > - Create its pair function, acpi_unbind_adr_device(), which is called > > from acpi_bus_remove(). When a constructor interface is introduced, its > > destructor should be introduced as well. > > - Remove the binding procedure from acpi_pci_root_add(). This should > > be done in patch [2/6]. > > Well, what about moving the code from acpi_pci_bind()/acpi_pci_unbind() > somewhere else and removing those things altogether? Sounds nice. It will be bonus point if you can do that. :-) Thanks, -Toshi -- 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 Tuesday, December 18, 2012 03:15:12 PM Toshi Kani wrote: > On Tue, 2012-12-18 at 22:57 +0100, Rafael J. Wysocki wrote: > > On Tuesday, December 18, 2012 09:10:41 AM Toshi Kani wrote: > > > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote: > : > > > We need to decide which module is responsible for calling .bind(). I > > > think it should be the ACPI scan module, not the ACPI PCI root bridge > > > driver, because: > > > - bind() needs to be called when _ADR device is added. The ACPI scan > > > module can scan any devices, while the PCI root driver can only scan > > > when it is added. > > > - acpi_bus_remove() calls unbind() at hot-remove. The same module > > > should be responsible for both bind() and unbind() handling. > > > - It is cleaner to keep struct acpi_device_ops interface to be called > > > by the ACPI core. > > > > I agree with that. :-) > > > > Moreover, I don't think we need acpi_pci_bind() and acpi_pci_unbind() at all. > > > > > So, I would propose the following changes. > > > > > > - Move the acpi_hot_add_bind() call back to the original place after > > > the device_attach() call. > > > - Rename the name of acpi_hot_add_bind() to something like > > > acpi_bind_adr_device() since it is no longer hot-add only (and is > > > specific to _ADR devices). > > > - Create its pair function, acpi_unbind_adr_device(), which is called > > > from acpi_bus_remove(). When a constructor interface is introduced, its > > > destructor should be introduced as well. > > > - Remove the binding procedure from acpi_pci_root_add(). This should > > > be done in patch [2/6]. > > > > Well, what about moving the code from acpi_pci_bind()/acpi_pci_unbind() > > somewhere else and removing those things altogether? > > Sounds nice. It will be bonus point if you can do that. :-) I think I can, but I need a few more patches on top of what I've already posted to do that. I think that https://patchwork.kernel.org/patch/1889821/ and https://patchwork.kernel.org/patch/1884701/ can stay as they are, since there's some material on top of them already and I'll cut the new patches on top of all that. I'll repost the whole series some time later this week, stay tuned. :-) Thanks, Rafael
On Tue, Dec 18, 2012 at 4:00 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Tuesday, December 18, 2012 03:15:12 PM Toshi Kani wrote: >> On Tue, 2012-12-18 at 22:57 +0100, Rafael J. Wysocki wrote: >> > On Tuesday, December 18, 2012 09:10:41 AM Toshi Kani wrote: >> > > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote: >> : >> > > We need to decide which module is responsible for calling .bind(). I >> > > think it should be the ACPI scan module, not the ACPI PCI root bridge >> > > driver, because: >> > > - bind() needs to be called when _ADR device is added. The ACPI scan >> > > module can scan any devices, while the PCI root driver can only scan >> > > when it is added. >> > > - acpi_bus_remove() calls unbind() at hot-remove. The same module >> > > should be responsible for both bind() and unbind() handling. >> > > - It is cleaner to keep struct acpi_device_ops interface to be called >> > > by the ACPI core. >> > >> > I agree with that. :-) >> > >> > Moreover, I don't think we need acpi_pci_bind() and acpi_pci_unbind() at all. >> > >> > > So, I would propose the following changes. >> > > >> > > - Move the acpi_hot_add_bind() call back to the original place after >> > > the device_attach() call. >> > > - Rename the name of acpi_hot_add_bind() to something like >> > > acpi_bind_adr_device() since it is no longer hot-add only (and is >> > > specific to _ADR devices). >> > > - Create its pair function, acpi_unbind_adr_device(), which is called >> > > from acpi_bus_remove(). When a constructor interface is introduced, its >> > > destructor should be introduced as well. >> > > - Remove the binding procedure from acpi_pci_root_add(). This should >> > > be done in patch [2/6]. >> > >> > Well, what about moving the code from acpi_pci_bind()/acpi_pci_unbind() >> > somewhere else and removing those things altogether? >> >> Sounds nice. It will be bonus point if you can do that. :-) > > I think I can, but I need a few more patches on top of what I've already posted > to do that. > > I think that https://patchwork.kernel.org/patch/1889821/ and > https://patchwork.kernel.org/patch/1884701/ can stay as they are, since there's > some material on top of them already and I'll cut the new patches on top of all > that. I'll repost the whole series some time later this week, stay tuned. :-) I haven't follow this closely enough to give useful feedback, but I trust that what you're doing is going in the right direction. The only question I have right now is what I mentioned earlier on IRC, namely, the idea of "binding" an ACPI handle or device to a pci_dev, and whether there's a way to guarantee that the binding doesn't become stale. For example, if we bind pci_dev A to acpi_device B, I think we essentially capture the pointer to B and store that pointer in A. Obviously we want to know that the captured pointer in A remains valid as long as A exists, but I don't know what assures us of that. I don't think this is a new question; I have the same question about the current code before your changes. But it seems like you're simplifying this area in a way that might make it easier to answer the question. Bjorn -- 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 Tue, Dec 18, 2012 at 2:05 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> >> i think we should put jiang four patches before Rafael's patches. >> >> http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-jiang-hotplug > > Actually, I have something more radical than that in mind. :-) > > Namely, we don't need to call the wakeup-related stuff from acpi_pci_bind() and > acpi_pci_unbind(). It's only there, because I did't find a better place for it > when I added it. > > If we can set the ACPI handles of PCI devices in pci_scan_device(), which isn't > too difficult to do (I actually have a patch for that and it's not too invasive), > we can easily move the wakeup enabling stuff to pci_pm_init() and wakeup > disabling somewhere near pci_release_capabilities(). good, let's see how acpi handles could be be passed to pci devices. 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 Tuesday, December 18, 2012 04:19:55 PM Bjorn Helgaas wrote: > On Tue, Dec 18, 2012 at 4:00 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Tuesday, December 18, 2012 03:15:12 PM Toshi Kani wrote: > >> On Tue, 2012-12-18 at 22:57 +0100, Rafael J. Wysocki wrote: > >> > On Tuesday, December 18, 2012 09:10:41 AM Toshi Kani wrote: > >> > > On Tue, 2012-12-18 at 02:48 +0100, Rafael J. Wysocki wrote: > >> : > >> > > We need to decide which module is responsible for calling .bind(). I > >> > > think it should be the ACPI scan module, not the ACPI PCI root bridge > >> > > driver, because: > >> > > - bind() needs to be called when _ADR device is added. The ACPI scan > >> > > module can scan any devices, while the PCI root driver can only scan > >> > > when it is added. > >> > > - acpi_bus_remove() calls unbind() at hot-remove. The same module > >> > > should be responsible for both bind() and unbind() handling. > >> > > - It is cleaner to keep struct acpi_device_ops interface to be called > >> > > by the ACPI core. > >> > > >> > I agree with that. :-) > >> > > >> > Moreover, I don't think we need acpi_pci_bind() and acpi_pci_unbind() at all. > >> > > >> > > So, I would propose the following changes. > >> > > > >> > > - Move the acpi_hot_add_bind() call back to the original place after > >> > > the device_attach() call. > >> > > - Rename the name of acpi_hot_add_bind() to something like > >> > > acpi_bind_adr_device() since it is no longer hot-add only (and is > >> > > specific to _ADR devices). > >> > > - Create its pair function, acpi_unbind_adr_device(), which is called > >> > > from acpi_bus_remove(). When a constructor interface is introduced, its > >> > > destructor should be introduced as well. > >> > > - Remove the binding procedure from acpi_pci_root_add(). This should > >> > > be done in patch [2/6]. > >> > > >> > Well, what about moving the code from acpi_pci_bind()/acpi_pci_unbind() > >> > somewhere else and removing those things altogether? > >> > >> Sounds nice. It will be bonus point if you can do that. :-) > > > > I think I can, but I need a few more patches on top of what I've already posted > > to do that. > > > > I think that https://patchwork.kernel.org/patch/1889821/ and > > https://patchwork.kernel.org/patch/1884701/ can stay as they are, since there's > > some material on top of them already and I'll cut the new patches on top of all > > that. I'll repost the whole series some time later this week, stay tuned. :-) > > I haven't follow this closely enough to give useful feedback, but I > trust that what you're doing is going in the right direction. Cool. :-) > The only question I have right now is what I mentioned earlier on IRC, > namely, the idea of "binding" an ACPI handle or device to a pci_dev, > and whether there's a way to guarantee that the binding doesn't become > stale. For example, if we bind pci_dev A to acpi_device B, I think we > essentially capture the pointer to B and store that pointer in A. > Obviously we want to know that the captured pointer in A remains valid > as long as A exists, but I don't know what assures us of that. This really is a bit more complicated. First, what we store in A is not a pointer to B, but rather the corresponding ACPICA handle, which is guaranteed to live longer that B itself. Second, we not only store the B's handle in A, but also a pointer to A in B (in the physical_node_list list). Moreover, we create the "firmware_node" sysfs file under A and a "physical_node" sysfs file under B. All of that becomes invalid when either A or B is removed without notice. For the removal of A we have acpi_platform_notify() that calls acpi_unbind_one() which kills all of that stuff, so as long as A is removed earlier than B, we have no problems. For the removal of B, however, we seem to assume that if the device is ejectable, there will be an ACPI driver bound to B (ie. the struct acpi_device) that will take care of the removal of the physical nodes associated with it before B itself is removed. At least that's my understanding of the current code. Moreover, I'm not sure if this assumption is universally satisfied. > I don't think this is a new question; I have the same question about > the current code before your changes. But it seems like you're > simplifying this area in a way that might make it easier to answer the > question. Well, my patches are not likely to make things worse in this area. :-) Anyway, I think we should make acpi_bus_scan(), or even acpi_bus_add() after my changes, mutually exclusive with acpi_bus_trim(), because that will ensure that we won't be removing ACPI device objects while setting them up (or the other way around). That would require us to redesign some ACPI drivers first, though, because they call acpi_bus_trim() in their initialization code paths (I don't really think they should be doing that). Moreover, I think we should make the ACPI core trigger the removal of all physical nodes (As) associated with the given ACPI node (B) after calling device_release_driver() in acpi_bus_remove(). That will ensure that all physical nodes are gone along with all the binding-related stuff (thanks to acpi_platform_notify()) before the ACPI node is removed. Thanks, Rafael
Hi all, I've finally cut the patches removing acpi_pci_bind() and acpi_pci_unbind(), so I'm kind of ready to post the entire series reworking the ACPI namespace scanning code. Patches [1-6/16] are essentially these ones: https://patchwork.kernel.org/patch/1889821/ https://patchwork.kernel.org/patch/1876481/ https://patchwork.kernel.org/patch/1876531/ https://patchwork.kernel.org/patch/1876571/ https://patchwork.kernel.org/patch/1876511/ https://patchwork.kernel.org/patch/1876401/ but [2-6/16] have been rebased on top of the first one. Patches [7-12/16] are essentially these: https://patchwork.kernel.org/patch/1884721/ https://patchwork.kernel.org/patch/1884701/ https://patchwork.kernel.org/patch/1884761/ https://patchwork.kernel.org/patch/1884731/ https://patchwork.kernel.org/patch/1884751/ https://patchwork.kernel.org/patch/1884661/ but they have been rebased on top of https://patchwork.kernel.org/patch/1889821/. I added Yinghai's ACKs to them tentatively, although they are a bit different from the previous versions. The difference is not too important, however, because the following patches finally remove the acpi_pci_bind()/acpi_pci_unbind() stuff: [13/16] Add .setup() and .cleanup() callbacks to struct acpi_bus_type. [14/16] Rework the setup and cleanup of ACPI/PCI device wakeup. [15/16] Move the _PRT setup and cleanup code to pci-acpi.c. [16/16] Drop ACPI device .bind() and .unbind() callbacks. This is done a bit differently than I thought it would be, mostly because the _PRT-related operations require the "subordinate" pointers of bridges to be populated. I think it may be possible to simplify this further if that requirement can be removed (I haven't looked into that). In fact, patches [13-15/16] do not essentially depend on [1-12/16], only the last one does. The patches are on top of my master branch and I'm going to rebase them when v3.8-rc1 is out. There is a git tree you can pull them from at: http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new It's v3.7 with my master branch merged and the new patches on top. Thanks, Rafael
On Wed, Dec 19, 2012 at 5:45 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: great!, the .bind and .unbind go away. > > The patches are on top of my master branch and I'm going to rebase them when > v3.8-rc1 is out. > > There is a git tree you can pull them from at: > > http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new > > It's v3.7 with my master branch merged and the new patches on top. can you make it base to today's Linus tree ? otherwise I have to do some manual merge... Bjorn change the _PRT for v.3.8... 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
Hi Rafael, The changes look very good. It's much cleaner and consistent. :-) For the series: Acked-by: Toshi Kani <toshi.kani@hp.com> Thanks, -Toshi On Thu, 2012-12-20 at 02:45 +0100, Rafael J. Wysocki wrote: > Hi all, > > I've finally cut the patches removing acpi_pci_bind() and acpi_pci_unbind(), > so I'm kind of ready to post the entire series reworking the ACPI namespace > scanning code. > > Patches [1-6/16] are essentially these ones: > > https://patchwork.kernel.org/patch/1889821/ > https://patchwork.kernel.org/patch/1876481/ > https://patchwork.kernel.org/patch/1876531/ > https://patchwork.kernel.org/patch/1876571/ > https://patchwork.kernel.org/patch/1876511/ > https://patchwork.kernel.org/patch/1876401/ > > but [2-6/16] have been rebased on top of the first one. > > Patches [7-12/16] are essentially these: > > https://patchwork.kernel.org/patch/1884721/ > https://patchwork.kernel.org/patch/1884701/ > https://patchwork.kernel.org/patch/1884761/ > https://patchwork.kernel.org/patch/1884731/ > https://patchwork.kernel.org/patch/1884751/ > https://patchwork.kernel.org/patch/1884661/ > > but they have been rebased on top of https://patchwork.kernel.org/patch/1889821/. > I added Yinghai's ACKs to them tentatively, although they are a bit different > from the previous versions. The difference is not too important, however, > because the following patches finally remove the acpi_pci_bind()/acpi_pci_unbind() > stuff: > > [13/16] Add .setup() and .cleanup() callbacks to struct acpi_bus_type. > [14/16] Rework the setup and cleanup of ACPI/PCI device wakeup. > [15/16] Move the _PRT setup and cleanup code to pci-acpi.c. > [16/16] Drop ACPI device .bind() and .unbind() callbacks. > > This is done a bit differently than I thought it would be, mostly because the > _PRT-related operations require the "subordinate" pointers of bridges to be > populated. I think it may be possible to simplify this further if that > requirement can be removed (I haven't looked into that). > > In fact, patches [13-15/16] do not essentially depend on [1-12/16], only the > last one does. > > The patches are on top of my master branch and I'm going to rebase them when > v3.8-rc1 is out. > > There is a git tree you can pull them from at: > > http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new > > It's v3.7 with my master branch merged and the new patches on top. > > > Thanks, > Rafael > > -- 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 Thursday, December 20, 2012 02:07:27 PM Toshi Kani wrote: > Hi Rafael, > > The changes look very good. It's much cleaner and consistent. :-) For > the series: > > Acked-by: Toshi Kani <toshi.kani@hp.com> Thanks!
On Wednesday, December 19, 2012 06:06:57 PM Yinghai Lu wrote: > On Wed, Dec 19, 2012 at 5:45 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > great!, the .bind and .unbind go away. > > > > > The patches are on top of my master branch and I'm going to rebase them when > > v3.8-rc1 is out. > > > > > There is a git tree you can pull them from at: > > > > http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new > > > > It's v3.7 with my master branch merged and the new patches on top. > > > can you make it base to today's Linus tree ? Sure. Rebased and pushed out: git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git acpi-scan-next Untested, though, so caveat emptor. :-) > otherwise I have to do some manual merge... > > Bjorn change the _PRT for v.3.8... Well, not only that has changed. Thanks, Rafael
On Thu, Dec 20, 2012 at 4:09 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Wednesday, December 19, 2012 06:06:57 PM Yinghai Lu wrote: >> On Wed, Dec 19, 2012 at 5:45 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> >> great!, the .bind and .unbind go away. >> >> > >> > The patches are on top of my master branch and I'm going to rebase them when >> > v3.8-rc1 is out. >> >> > >> > There is a git tree you can pull them from at: >> > >> > http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new >> > >> > It's v3.7 with my master branch merged and the new patches on top. >> >> >> can you make it base to today's Linus tree ? > > Sure. Rebased and pushed out: > > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git acpi-scan-next > Thanks. I rebased pci-root-bus-hotplug patchset on top it, and it works well. So Acked-by: Yinghai Lu <yinghai@kernel.org> for the four new ones in your acpi_scan_next branch. i put the updated patches in git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-root-bus-3.9 -- 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 Thursday, December 20, 2012 07:59:15 PM Yinghai Lu wrote: > On Thu, Dec 20, 2012 at 4:09 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Wednesday, December 19, 2012 06:06:57 PM Yinghai Lu wrote: > >> On Wed, Dec 19, 2012 at 5:45 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >> > >> great!, the .bind and .unbind go away. > >> > >> > > >> > The patches are on top of my master branch and I'm going to rebase them when > >> > v3.8-rc1 is out. > >> > >> > > >> > There is a git tree you can pull them from at: > >> > > >> > http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=summary acpi-scan-temp-new > >> > > >> > It's v3.7 with my master branch merged and the new patches on top. > >> > >> > >> can you make it base to today's Linus tree ? > > > > Sure. Rebased and pushed out: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git acpi-scan-next > > > > Thanks. > > I rebased pci-root-bus-hotplug patchset on top it, and it works well. Great, thanks for testing! > So > Acked-by: Yinghai Lu <yinghai@kernel.org> > for the four new ones in your acpi_scan_next branch. Thanks! I'll rebase the whole series on top of v3.8-rc1 when it's out. > i put the updated patches in > > git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git > for-pci-root-bus-3.9 OK, thanks. I'll have a look at that series next week. Thanks, Rafael
Index: linux/include/acpi/acpi_bus.h =================================================================== --- linux.orig/include/acpi/acpi_bus.h +++ linux/include/acpi/acpi_bus.h @@ -98,6 +98,7 @@ typedef void (*acpi_op_notify) (struct a struct acpi_bus_ops { u32 acpi_op_add:1; u32 acpi_op_start:1; + u32 acpi_op_match:1; }; struct acpi_device_ops { Index: linux/drivers/acpi/scan.c =================================================================== --- linux.orig/drivers/acpi/scan.c +++ linux/drivers/acpi/scan.c @@ -494,7 +494,8 @@ static int acpi_bus_match(struct device struct acpi_device *acpi_dev = to_acpi_device(dev); struct acpi_driver *acpi_drv = to_acpi_driver(drv); - return !acpi_match_device_ids(acpi_dev, acpi_drv->ids); + return acpi_dev->bus_ops.acpi_op_match + && !acpi_match_device_ids(acpi_dev, acpi_drv->ids); } static int acpi_device_uevent(struct device *dev, struct kobj_uevent_env *env) @@ -1418,6 +1419,17 @@ static int acpi_bus_remove(struct acpi_d return 0; } +/* + * acpi_hot_add_bind - Bind _ADR-based devices on hot-add. + * @device: ACPI device node to bind. + */ +static void acpi_hot_add_bind(struct acpi_device *device) +{ + if (device->flags.bus_address + && device->parent && device->parent->ops.bind) + device->parent->ops.bind(device); +} + static int acpi_add_single_object(struct acpi_device **child, acpi_handle handle, int type, unsigned long long sta, @@ -1490,13 +1502,8 @@ static int acpi_add_single_object(struct result = acpi_device_register(device); - /* - * Bind _ADR-Based Devices when hot add - */ - if (device->flags.bus_address) { - if (device->parent && device->parent->ops.bind) - device->parent->ops.bind(device); - } + if (device->bus_ops.acpi_op_match) + acpi_hot_add_bind(device); end: if (!result) { @@ -1522,6 +1529,7 @@ static void acpi_bus_add_power_resource( struct acpi_bus_ops ops = { .acpi_op_add = 1, .acpi_op_start = 1, + .acpi_op_match = 1, }; struct acpi_device *device = NULL; @@ -1574,9 +1582,9 @@ static acpi_status acpi_bus_check_add(ac void *context, void **return_value) { struct acpi_bus_ops *ops = context; + struct acpi_device *device = NULL; int type; unsigned long long sta; - struct acpi_device *device; acpi_status status; int result; @@ -1596,52 +1604,84 @@ static acpi_status acpi_bus_check_add(ac return AE_CTRL_DEPTH; } - /* - * We may already have an acpi_device from a previous enumeration. If - * so, we needn't add it again, but we may still have to start it. - */ - device = NULL; acpi_bus_get_device(handle, &device); if (ops->acpi_op_add && !device) { - acpi_add_single_object(&device, handle, type, sta, ops); - /* Is the device a known good platform device? */ - if (device - && !acpi_match_device_ids(device, acpi_platform_device_ids)) - acpi_create_platform_device(device); - } - - if (!device) - return AE_CTRL_DEPTH; + struct acpi_bus_ops add_ops = *ops; - if (ops->acpi_op_start && !(ops->acpi_op_add)) { - status = acpi_start_single_object(device); - if (ACPI_FAILURE(status)) + add_ops.acpi_op_match = 0; + acpi_add_single_object(&device, handle, type, sta, &add_ops); + if (!device) return AE_CTRL_DEPTH; + + device->bus_ops.acpi_op_match = 1; + acpi_hot_add_bind(device); } if (!*return_value) *return_value = device; + return AE_OK; } +static acpi_status acpi_bus_probe_start(acpi_handle handle, u32 lvl, + void *context, void **not_used) +{ + struct acpi_bus_ops *ops = context; + acpi_status status = AE_OK; + struct acpi_device *device; + unsigned long long sta_not_used; + int type_not_used; + + /* + * Ignore errors ignored by acpi_bus_check_add() to avoid terminating + * namespace walks prematurely. + */ + if (acpi_bus_type_and_status(handle, &type_not_used, &sta_not_used)) + return AE_OK; + + if (acpi_bus_get_device(handle, &device)) + return AE_CTRL_DEPTH; + + if (ops->acpi_op_add) { + if (!acpi_match_device_ids(device, acpi_platform_device_ids)) { + /* This is a known good platform device. */ + acpi_create_platform_device(device); + } else if (device_attach(&device->dev)) { + status = AE_CTRL_DEPTH; + } + } else if (ops->acpi_op_start) { + if (ACPI_FAILURE(acpi_start_single_object(device))) + status = AE_CTRL_DEPTH; + } + return status; +} + static int acpi_bus_scan(acpi_handle handle, struct acpi_bus_ops *ops, struct acpi_device **child) { - acpi_status status; void *device = NULL; + acpi_status status; + int ret = -ENODEV; status = acpi_bus_check_add(handle, 0, ops, &device); if (ACPI_SUCCESS(status)) acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, acpi_bus_check_add, NULL, ops, &device); + if (!device) + goto out; + + ret = 0; + status = acpi_bus_probe_start(handle, 0, ops, NULL); + if (ACPI_SUCCESS(status)) + acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, + acpi_bus_probe_start, NULL, ops, NULL); + + out: if (child) *child = device; - if (device) - return 0; - else - return -ENODEV; + return ret; } /* @@ -1752,6 +1792,7 @@ static int acpi_bus_scan_fixed(void) memset(&ops, 0, sizeof(ops)); ops.acpi_op_add = 1; ops.acpi_op_start = 1; + ops.acpi_op_match = 1; /* * Enumerate all fixed-feature devices.