Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tuesday, December 18, 2012 04:19:55 PM Bjorn Helgaas wrote: > On Tue, Dec 18, 2012 at 4:00 PM, Rafael J. Wysocki 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
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tuesday, December 18, 2012 04:19:55 PM Bjorn Helgaas wrote: On Tue, Dec 18, 2012 at 4:00 PM, Rafael J. Wysocki r...@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
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tue, Dec 18, 2012 at 2:05 PM, Rafael J. Wysocki 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tue, Dec 18, 2012 at 4:00 PM, Rafael J. Wysocki 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
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 -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tuesday, December 18, 2012 10:59:46 AM Yinghai Lu wrote: > On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani 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 -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
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
> > > >
> > >
> > > (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, );
> > > > if (ops->acpi_op_add && !device) {
> > > > - acpi_add_single_object(, 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)
> > > > -
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tue, Dec 18, 2012 at 12:48 PM, Toshi Kani wrote: > On Tue, 2012-12-18 at 10:59 -0800, Yinghai Lu wrote: >> On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tue, 2012-12-18 at 10:59 -0800, Yinghai Lu wrote: > On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani 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-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
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
> > >
> >
> > (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, );
> > > if (ops->acpi_op_add && !device) {
> > > - acpi_add_single_object(, 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(, handle, type, sta, _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
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
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.wyso...@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
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani toshi.k...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tue, 2012-12-18 at 10:59 -0800, Yinghai Lu wrote: On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani toshi.k...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tue, Dec 18, 2012 at 12:48 PM, Toshi Kani toshi.k...@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.k...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
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.wyso...@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)
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tuesday, December 18, 2012 10:59:46 AM Yinghai Lu wrote: On Tue, Dec 18, 2012 at 8:10 AM, Toshi Kani toshi.k...@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 -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
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 -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tue, Dec 18, 2012 at 4:00 PM, Rafael J. Wysocki r...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Tue, Dec 18, 2012 at 2:05 PM, Rafael J. Wysocki r...@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-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
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
> >
>
> (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?
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, );
> > if (ops->acpi_op_add && !device) {
> > - acpi_add_single_object(, 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(, handle, type, sta, _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
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Thu, 2012-12-13 at 23:17 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki
>
(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.
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, );
> if (ops->acpi_op_add && !device) {
> - acpi_add_single_object(, 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(, handle, type, sta, _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.
> + * namespace walks prematurely.
> + */
> + if (acpi_bus_type_and_status(handle, _not_used, _not_used))
> + return AE_OK;
> +
> + if (acpi_bus_get_device(handle, ))
> + return AE_CTRL_DEPTH;
> +
> + if (ops->acpi_op_add) {
> + if (!acpi_match_device_ids(device, acpi_platform_device_ids)) {
> + /* This is a known
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
On Thu, 2012-12-13 at 23:17 +0100, Rafael J. Wysocki wrote:
From: Rafael J. Wysocki rafael.j.wyso...@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.
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.
+ * 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 {
+
Re: [PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
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.wyso...@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?
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
[PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
From: Rafael J. Wysocki
Split the ACPI namespace scanning for devices into two passes, such
that struct acpi_device objects are registerd in the first pass
without probing ACPI drivers and the drivers are probed against them
directly in the second pass.
There are two main reasons for doing that.
First, the ACPI PCI root bridge driver's .add() routine,
acpi_pci_root_add(), causes struct pci_dev objects to be created for
all PCI devices under the given root bridge. Usually, there are
corresponding ACPI device nodes in the ACPI namespace for some of
those devices and therefore there should be "companion" struct
acpi_device objects to attach those struct pci_dev objects to. These
struct acpi_device objects should exist when the corresponding
struct pci_dev objects are created, but that is only guaranteed
during boot and not during hotplug. This leads to a number of
functional differences between the boot and the hotplug cases which
are not strictly necessary and make the code more complicated.
For example, this forces the ACPI PCI root bridge driver to defer the
registration of the just created struct pci_dev objects and to use a
special .start() callback routine, acpi_pci_root_start(), to make
sure that all of the "companion" struct acpi_device objects will be
present at PCI devices registration time during hotplug.
If those differences can be eliminated, we will be able to
consolidate the boot and hotplug code paths for the enumeration and
registration of PCI devices and to reduce the complexity of that
code quite a bit.
The second reason is that, in general, it should be possible to
resolve conflicts of resources assigned by the BIOS to different
devices represented by ACPI namespace nodes before any drivers bind
to them and before they are attached to "companion" objects
representing physical devices (such as struct pci_dev). However, for
this purpose we first need to enumerate all ACPI device nodes in the
given namespace scope.
Signed-off-by: Rafael J. Wysocki
---
drivers/acpi/scan.c | 105 +---
include/acpi/acpi_bus.h |1
2 files changed, 75 insertions(+), 31 deletions(-)
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,86 @@ static acpi_status acpi_bus_check_add(ac
return AE_CTRL_DEPTH;
}
- /*
-
[PATCH rev.2 1/6] ACPI: Separate adding ACPI device objects from probing ACPI drivers
From: Rafael J. Wysocki rafael.j.wyso...@intel.com
Split the ACPI namespace scanning for devices into two passes, such
that struct acpi_device objects are registerd in the first pass
without probing ACPI drivers and the drivers are probed against them
directly in the second pass.
There are two main reasons for doing that.
First, the ACPI PCI root bridge driver's .add() routine,
acpi_pci_root_add(), causes struct pci_dev objects to be created for
all PCI devices under the given root bridge. Usually, there are
corresponding ACPI device nodes in the ACPI namespace for some of
those devices and therefore there should be companion struct
acpi_device objects to attach those struct pci_dev objects to. These
struct acpi_device objects should exist when the corresponding
struct pci_dev objects are created, but that is only guaranteed
during boot and not during hotplug. This leads to a number of
functional differences between the boot and the hotplug cases which
are not strictly necessary and make the code more complicated.
For example, this forces the ACPI PCI root bridge driver to defer the
registration of the just created struct pci_dev objects and to use a
special .start() callback routine, acpi_pci_root_start(), to make
sure that all of the companion struct acpi_device objects will be
present at PCI devices registration time during hotplug.
If those differences can be eliminated, we will be able to
consolidate the boot and hotplug code paths for the enumeration and
registration of PCI devices and to reduce the complexity of that
code quite a bit.
The second reason is that, in general, it should be possible to
resolve conflicts of resources assigned by the BIOS to different
devices represented by ACPI namespace nodes before any drivers bind
to them and before they are attached to companion objects
representing physical devices (such as struct pci_dev). However, for
this purpose we first need to enumerate all ACPI device nodes in the
given namespace scope.
Signed-off-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
---
drivers/acpi/scan.c | 105 +---
include/acpi/acpi_bus.h |1
2 files changed, 75 insertions(+), 31 deletions(-)
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,86 @@ static acpi_status acpi_bus_check_add(ac
return AE_CTRL_DEPTH;
}
