| Message ID | 2097693.II1PNyeFLY@vostro.rjw.lan |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, Dec 9, 2012 at 3:03 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Notice that one member of struct acpi_bus_ops, acpi_op_add, is not > used anywhere any more and the relationship between its remaining > members, acpi_op_match and acpi_op_start, is such that it doesn't > make sense to set the latter without setting the former at the same > time. Therefore, replace struct acpi_bus_ops with new a enum type, > enum acpi_bus_add_type, with three values, ACPI_BUS_ADD_BASIC, > ACPI_BUS_ADD_MATCH, ACPI_BUS_ADD_START, corresponding to > both acpi_op_match and acpi_op_start unset, acpi_op_match set and > acpi_op_start unset, and both acpi_op_match and acpi_op_start set, > respectively. > Can we expand the BUS_ADD_* concept to other devices instead of just acpi_device? aka we should let struct device has this add_type field. -- 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 Sunday, December 09, 2012 09:34:42 PM Yinghai Lu wrote: > On Sun, Dec 9, 2012 at 3:03 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Notice that one member of struct acpi_bus_ops, acpi_op_add, is not > > used anywhere any more and the relationship between its remaining > > members, acpi_op_match and acpi_op_start, is such that it doesn't > > make sense to set the latter without setting the former at the same > > time. Therefore, replace struct acpi_bus_ops with new a enum type, > > enum acpi_bus_add_type, with three values, ACPI_BUS_ADD_BASIC, > > ACPI_BUS_ADD_MATCH, ACPI_BUS_ADD_START, corresponding to > > both acpi_op_match and acpi_op_start unset, acpi_op_match set and > > acpi_op_start unset, and both acpi_op_match and acpi_op_start set, > > respectively. > > > > Can we expand the BUS_ADD_* concept to other devices instead of just > acpi_device? > > aka we should let struct device has this add_type field. Having done that in ACPI to cover our use case here, we can try to move it into struct device if there are use cases beyond ACPI that can't be covered by using deferred driver probing. Thanks, Rafael
On Mon, Dec 10, 2012 at 6:46 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Sunday, December 09, 2012 09:34:42 PM Yinghai Lu wrote: >> >> Can we expand the BUS_ADD_* concept to other devices instead of just >> acpi_device? >> >> aka we should let struct device has this add_type field. > > Having done that in ACPI to cover our use case here, we can try to move it > into struct device if there are use cases beyond ACPI that can't be covered > by using deferred driver probing. pci device for hotplug have same problem. need to delay driver attach for them too. also BUS_ADD_MATCH and BUS_ADD_START are duplicated. old add are separated to adding all devices to tree and then matching work to load the drivers. so _START is not needed anymore, only user.start in pci_root driver should be removed. code in .start could be moved .add without problem. -- 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 Monday, December 10, 2012 09:07:06 AM Yinghai Lu wrote: > On Mon, Dec 10, 2012 at 6:46 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Sunday, December 09, 2012 09:34:42 PM Yinghai Lu wrote: > >> > >> Can we expand the BUS_ADD_* concept to other devices instead of just > >> acpi_device? > >> > >> aka we should let struct device has this add_type field. > > > > Having done that in ACPI to cover our use case here, we can try to move it > > into struct device if there are use cases beyond ACPI that can't be covered > > by using deferred driver probing. > > pci device for hotplug have same problem. need to delay driver attach > for them too. OK, I'll take a look. Any pointers to speed that up? > also BUS_ADD_MATCH and BUS_ADD_START are duplicated. Not at the moment, they do different things as code goes. > old add are separated to adding all devices to tree and then matching > work to load the drivers. > > so _START is not needed anymore, only user.start in pci_root driver > should be removed. > code in .start could be moved .add without problem. Yes, I'm going to do that as the next step. I didn't want this particular patchset to grow too big. I'll post another one on top of it if people don't have problems with this one. Thanks, Rafael
On Monday, December 10, 2012 11:47:27 PM Rafael J. Wysocki wrote: > On Monday, December 10, 2012 09:07:06 AM Yinghai Lu wrote: > > On Mon, Dec 10, 2012 at 6:46 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > > On Sunday, December 09, 2012 09:34:42 PM Yinghai Lu wrote: > > >> > > >> Can we expand the BUS_ADD_* concept to other devices instead of just > > >> acpi_device? > > >> > > >> aka we should let struct device has this add_type field. > > > > > > Having done that in ACPI to cover our use case here, we can try to move it > > > into struct device if there are use cases beyond ACPI that can't be covered > > > by using deferred driver probing. > > > > pci device for hotplug have same problem. need to delay driver attach > > for them too. > > OK, I'll take a look. Any pointers to speed that up? > > > also BUS_ADD_MATCH and BUS_ADD_START are duplicated. > > Not at the moment, they do different things as code goes. > > > old add are separated to adding all devices to tree and then matching > > work to load the drivers. > > > > so _START is not needed anymore, only user.start in pci_root driver > > should be removed. > > code in .start could be moved .add without problem. > > Yes, I'm going to do that as the next step. I didn't want this particular > patchset to grow too big. I'll post another one on top of it if people > don't have problems with this one. By the way, can you please remind me where you wanted to put the pci_bus_add_devices() and why? Rafael
On Mon, Dec 10, 2012 at 3:09 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Monday, December 10, 2012 11:47:27 PM Rafael J. Wysocki wrote: >> On Monday, December 10, 2012 09:07:06 AM Yinghai Lu wrote: >> > On Mon, Dec 10, 2012 at 6:46 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> > > On Sunday, December 09, 2012 09:34:42 PM Yinghai Lu wrote: >> > >> >> > >> Can we expand the BUS_ADD_* concept to other devices instead of just >> > >> acpi_device? >> > >> >> > >> aka we should let struct device has this add_type field. >> > > >> > > Having done that in ACPI to cover our use case here, we can try to move it >> > > into struct device if there are use cases beyond ACPI that can't be covered >> > > by using deferred driver probing. >> > >> > pci device for hotplug have same problem. need to delay driver attach >> > for them too. >> >> OK, I'll take a look. Any pointers to speed that up? >> >> > also BUS_ADD_MATCH and BUS_ADD_START are duplicated. >> >> Not at the moment, they do different things as code goes. >> >> > old add are separated to adding all devices to tree and then matching >> > work to load the drivers. >> > >> > so _START is not needed anymore, only user.start in pci_root driver >> > should be removed. >> > code in .start could be moved .add without problem. >> >> Yes, I'm going to do that as the next step. I didn't want this particular >> patchset to grow too big. I'll post another one on top of it if people >> don't have problems with this one. > > By the way, can you please remind me where you wanted to put the > pci_bus_add_devices() and why? > please check my for-pci-next branch at http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-next that includes delay loading acpi driver and pci driver. http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=8c031eabbdc83dd4d93933b82d96b55d038bcb64 PCI: prepare to use device drivers_autoprobe to delay attach drivers http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=40a12dc8942a8ed02bfbf75ee1ffbfbdf1511b45 PCI: Use device_add for device and bus early http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=8af9b4c250091c30afedeb2e7f14fca06997c811 ACPI: add drivers_autoprobe in struct acpi_device http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=ae498e157e9dc8794932562b2f885ddc3a1a229a ACPI: use device drivers_autoprobe to delay loading acpi drivers http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=6bee785b563a0b0e311e188321b1160593d5e6ee PCI, ACPI: Remove not used acpi_pci_root_start() http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=f467a1cd18a07a250be8527b94612fd4a654fbd1 ACPI: remove acpi_op_start workaround -- 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 Mon, Dec 10, 2012 at 2:47 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Monday, December 10, 2012 09:07:06 AM Yinghai Lu wrote: >> On Mon, Dec 10, 2012 at 6:46 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> > On Sunday, December 09, 2012 09:34:42 PM Yinghai Lu wrote: >> >> >> >> Can we expand the BUS_ADD_* concept to other devices instead of just >> >> acpi_device? >> >> >> >> aka we should let struct device has this add_type field. >> > >> > Having done that in ACPI to cover our use case here, we can try to move it >> > into struct device if there are use cases beyond ACPI that can't be covered >> > by using deferred driver probing. >> >> pci device for hotplug have same problem. need to delay driver attach >> for them too. > > OK, I'll take a look. Any pointers to speed that up? > >> also BUS_ADD_MATCH and BUS_ADD_START are duplicated. > > Not at the moment, they do different things as code goes. I do think that is same problem which is in the driver/base core. it should support delay loading driver for the hotplug case at the first point. that is reason that .start is introduced for acpi driver..to workround the problem in driver core. > >> old add are separated to adding all devices to tree and then matching >> work to load the drivers. >> >> so _START is not needed anymore, only user.start in pci_root driver >> should be removed. >> code in .start could be moved .add without problem. > > Yes, I'm going to do that as the next step. I didn't want this particular after you remove that .start, that will the same as my two patches. except that you patches toggle that per-device add_type for every device. and my patches is only toggle that per device drivers_autoprobe for hot-add devices. > patchset to grow too big. I'll post another one on top of it if people > don't have problems with this one. yesterday, I replace my two acpi driver delay loading patches with your patches. during pci root bus hot remove/add test, got the panic... did not have time to look at it yet. 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 Monday, December 10, 2012 03:22:48 PM Yinghai Lu wrote: > On Mon, Dec 10, 2012 at 2:47 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Monday, December 10, 2012 09:07:06 AM Yinghai Lu wrote: > >> On Mon, Dec 10, 2012 at 6:46 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >> > On Sunday, December 09, 2012 09:34:42 PM Yinghai Lu wrote: > >> >> > >> >> Can we expand the BUS_ADD_* concept to other devices instead of just > >> >> acpi_device? > >> >> > >> >> aka we should let struct device has this add_type field. > >> > > >> > Having done that in ACPI to cover our use case here, we can try to move it > >> > into struct device if there are use cases beyond ACPI that can't be covered > >> > by using deferred driver probing. > >> > >> pci device for hotplug have same problem. need to delay driver attach > >> for them too. > > > > OK, I'll take a look. Any pointers to speed that up? > > > >> also BUS_ADD_MATCH and BUS_ADD_START are duplicated. > > > > Not at the moment, they do different things as code goes. > > I do think that is same problem which is in the driver/base core. > > it should support delay loading driver for the hotplug case at the first point. > that is reason that .start is introduced for acpi driver..to workround > the problem > in driver core. OK, but let's eliminate .start() first and then go make changes to the driver core. That is, let's take one step at a time. :-) > >> old add are separated to adding all devices to tree and then matching > >> work to load the drivers. > >> > >> so _START is not needed anymore, only user.start in pci_root driver > >> should be removed. > >> code in .start could be moved .add without problem. > > > > Yes, I'm going to do that as the next step. I didn't want this particular > > after you remove that .start, that will the same as my two patches. > except that you patches toggle that per-device add_type for every device. > > and my patches is only toggle that per device drivers_autoprobe for > hot-add devices. I'm not 100% sure of that, but quite likely that's the case. I'd like to avoid special-casing hotplug in any way, if possible. Also, I have other reasons to do it for all devices (like the resources management for one example). > > patchset to grow too big. I'll post another one on top of it if people > > don't have problems with this one. > > yesterday, I replace my two acpi driver delay loading patches with your patches. > > during pci root bus hot remove/add test, got the panic... I don't see what the reason may be at the moment, it would be good to get more info (like a screenshot of the panic). > did not have time to look at it yet. OK Thanks, Rafael
On Monday, December 10, 2012 03:14:32 PM Yinghai Lu wrote: > On Mon, Dec 10, 2012 at 3:09 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Monday, December 10, 2012 11:47:27 PM Rafael J. Wysocki wrote: > >> On Monday, December 10, 2012 09:07:06 AM Yinghai Lu wrote: > >> > On Mon, Dec 10, 2012 at 6:46 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >> > > On Sunday, December 09, 2012 09:34:42 PM Yinghai Lu wrote: > >> > >> > >> > >> Can we expand the BUS_ADD_* concept to other devices instead of just > >> > >> acpi_device? > >> > >> > >> > >> aka we should let struct device has this add_type field. > >> > > > >> > > Having done that in ACPI to cover our use case here, we can try to move it > >> > > into struct device if there are use cases beyond ACPI that can't be covered > >> > > by using deferred driver probing. > >> > > >> > pci device for hotplug have same problem. need to delay driver attach > >> > for them too. > >> > >> OK, I'll take a look. Any pointers to speed that up? > >> > >> > also BUS_ADD_MATCH and BUS_ADD_START are duplicated. > >> > >> Not at the moment, they do different things as code goes. > >> > >> > old add are separated to adding all devices to tree and then matching > >> > work to load the drivers. > >> > > >> > so _START is not needed anymore, only user.start in pci_root driver > >> > should be removed. > >> > code in .start could be moved .add without problem. > >> > >> Yes, I'm going to do that as the next step. I didn't want this particular > >> patchset to grow too big. I'll post another one on top of it if people > >> don't have problems with this one. > > > > By the way, can you please remind me where you wanted to put the > > pci_bus_add_devices() and why? > > > > please check my for-pci-next branch at > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-next > > that includes delay loading acpi driver and pci driver. > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=8c031eabbdc83dd4d93933b82d96b55d038bcb64 > PCI: prepare to use device drivers_autoprobe to delay attach drivers > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=40a12dc8942a8ed02bfbf75ee1ffbfbdf1511b45 > PCI: Use device_add for device and bus early > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=8af9b4c250091c30afedeb2e7f14fca06997c811 > ACPI: add drivers_autoprobe in struct acpi_device > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=ae498e157e9dc8794932562b2f885ddc3a1a229a > ACPI: use device drivers_autoprobe to delay loading acpi drivers > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=6bee785b563a0b0e311e188321b1160593d5e6ee > PCI, ACPI: Remove not used acpi_pci_root_start() > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=f467a1cd18a07a250be8527b94612fd4a654fbd1 > ACPI: remove acpi_op_start workaround OK, thanks for the pointers. I actually see more differences between our patchsets. For one example, you seem to have left the parent->ops.bind() stuff in acpi_add_single_object() which calls it even drivers_autoprobe is set. Is that the case, or am I missing anything? Rafael
On Tuesday, December 11, 2012 02:02:14 AM Rafael J. Wysocki wrote: > On Monday, December 10, 2012 03:14:32 PM Yinghai Lu wrote: > > On Mon, Dec 10, 2012 at 3:09 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > > On Monday, December 10, 2012 11:47:27 PM Rafael J. Wysocki wrote: > > >> On Monday, December 10, 2012 09:07:06 AM Yinghai Lu wrote: > > >> > On Mon, Dec 10, 2012 at 6:46 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > >> > > On Sunday, December 09, 2012 09:34:42 PM Yinghai Lu wrote: > > >> > >> > > >> > >> Can we expand the BUS_ADD_* concept to other devices instead of just > > >> > >> acpi_device? > > >> > >> > > >> > >> aka we should let struct device has this add_type field. > > >> > > > > >> > > Having done that in ACPI to cover our use case here, we can try to move it > > >> > > into struct device if there are use cases beyond ACPI that can't be covered > > >> > > by using deferred driver probing. > > >> > > > >> > pci device for hotplug have same problem. need to delay driver attach > > >> > for them too. > > >> > > >> OK, I'll take a look. Any pointers to speed that up? > > >> > > >> > also BUS_ADD_MATCH and BUS_ADD_START are duplicated. > > >> > > >> Not at the moment, they do different things as code goes. > > >> > > >> > old add are separated to adding all devices to tree and then matching > > >> > work to load the drivers. > > >> > > > >> > so _START is not needed anymore, only user.start in pci_root driver > > >> > should be removed. > > >> > code in .start could be moved .add without problem. > > >> > > >> Yes, I'm going to do that as the next step. I didn't want this particular > > >> patchset to grow too big. I'll post another one on top of it if people > > >> don't have problems with this one. > > > > > > By the way, can you please remind me where you wanted to put the > > > pci_bus_add_devices() and why? > > > > > > > please check my for-pci-next branch at > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=shortlog;h=refs/heads/for-pci-next > > > > that includes delay loading acpi driver and pci driver. > > > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=8c031eabbdc83dd4d93933b82d96b55d038bcb64 > > PCI: prepare to use device drivers_autoprobe to delay attach drivers > > > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=40a12dc8942a8ed02bfbf75ee1ffbfbdf1511b45 > > PCI: Use device_add for device and bus early > > > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=8af9b4c250091c30afedeb2e7f14fca06997c811 > > ACPI: add drivers_autoprobe in struct acpi_device > > > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=ae498e157e9dc8794932562b2f885ddc3a1a229a > > ACPI: use device drivers_autoprobe to delay loading acpi drivers > > > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=6bee785b563a0b0e311e188321b1160593d5e6ee > > PCI, ACPI: Remove not used acpi_pci_root_start() > > > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=f467a1cd18a07a250be8527b94612fd4a654fbd1 > > ACPI: remove acpi_op_start workaround > > OK, thanks for the pointers. I actually see more differences between our > patchsets. For one example, you seem to have left the parent->ops.bind() > stuff in acpi_add_single_object() which calls it even drivers_autoprobe is > set. Sorry, that should have been "which calls it even when drivers_autoprobe is not set". I need to be more careful. > Is that the case, or am I missing anything? Thanks, Rafael
On Mon, Dec 10, 2012 at 5:28 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> >> OK, thanks for the pointers. I actually see more differences between our >> patchsets. For one example, you seem to have left the parent->ops.bind() >> stuff in acpi_add_single_object() which calls it even drivers_autoprobe is >> set. > > Sorry, that should have been "which calls it even when drivers_autoprobe is > not set". I need to be more careful. > oh, Jiang Liu had one patch to remove that workaround. http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=b40dba80c2b8395570d8357e6b3f417c27c84504 ACPI/pci-bind: remove bind/unbind callbacks from acpi_device_ops Maybe you can review that patches in my for-pci-next2... those are ACPI related anyway. those patches have been there for a while, and Bjorn did not have time to digest them. or you prefer I resend updated version as huge whole patchset? 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
On Monday, December 10, 2012 06:26:08 PM Yinghai Lu wrote: > On Mon, Dec 10, 2012 at 5:28 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >> > >> OK, thanks for the pointers. I actually see more differences between our > >> patchsets. For one example, you seem to have left the parent->ops.bind() > >> stuff in acpi_add_single_object() which calls it even drivers_autoprobe is > >> set. > > > > Sorry, that should have been "which calls it even when drivers_autoprobe is > > not set". I need to be more careful. > > > > oh, Jiang Liu had one patch to remove that workaround. > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=b40dba80c2b8395570d8357e6b3f417c27c84504 > > ACPI/pci-bind: remove bind/unbind callbacks from acpi_device_ops OK, so I'm looking at the current code, which for me is the master branch of the linux-pm.git tree (the linux-next branch is the same ATM), and I'm not seeing acpi_pci_unbind_cb() in there. So surely this patch applies to something different, right? In which case I wonder what reason there is for me to look at it at all? Besides, I think it may be done differently and in a more straightforward way. Namely, on top of my current patchset, it is guaranteed that not only struct pci_dev objects will always be registered after the companion struct acpi_device ones, but also they always will be *created* after those companion objects have been registered. So in principle we can populate a new struct pci_dev's ACPI handle as soon as in pci_scan_device(), next to pci_set_of_node(). Then, we can do something like acpi_pci_bind(), although without the whole acpi_get_pci_dev() nonsense, in pci_setup_device(), in which case we won't need to do it anywhere else. As an added benefit, acpi_platform_notify() would then see a populated ACPI handle in that struct pci_dev when finally registering the PCI device, so it wouldn't need to do the whole acpi_find_bridge_device() and type->find_device() dance. > Maybe you can review that patches in my for-pci-next2... > those are ACPI related anyway. I can, provided that (1) they are based on top of my tree or v3.7 and (2) they don't conflict with patches we're currently discussing. > those patches have been there for a while, and Bjorn did not have time > to digest them. Well, Bjorn's review bandwidth is limited and we need to take that into account. > or you prefer I resend updated version as huge whole patchset? No, no huge patchsets, please. Let's take one step at a time, so that everyone involved/interested can understand what's going on, OK? My review capacity also is not unlimited, mind you. I can't promise I'll have the time to review more than a few patches a day (where "a few" is rather less than "several"). Thanks, Rafael
Hi Rafael, I have worked out a patch set to clean up ACPI/PCI related notifications, please refer to http://www.spinics.net/lists/linux-pci/msg17822.html The patchset doesn't apply cleanly to Bjorn's latest pci-next tree. I will help to rebase it if needed. Regards! Gerry On 12/11/2012 10:26 AM, Yinghai Lu wrote: > On Mon, Dec 10, 2012 at 5:28 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: >>> >>> OK, thanks for the pointers. I actually see more differences between our >>> patchsets. For one example, you seem to have left the parent->ops.bind() >>> stuff in acpi_add_single_object() which calls it even drivers_autoprobe is >>> set. >> >> Sorry, that should have been "which calls it even when drivers_autoprobe is >> not set". I need to be more careful. >> > > oh, Jiang Liu had one patch to remove that workaround. > > http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=b40dba80c2b8395570d8357e6b3f417c27c84504 > > ACPI/pci-bind: remove bind/unbind callbacks from acpi_device_ops > > Maybe you can review that patches in my for-pci-next2... > those are ACPI related anyway. > > those patches have been there for a while, and Bjorn did not have time > to digest them. > > or you prefer I resend updated version as huge whole patchset? > > Thanks > > Yinghai > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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 Gerry, On Tuesday, December 11, 2012 11:09:06 PM Jiang Liu wrote: > Hi Rafael, > I have worked out a patch set to clean up ACPI/PCI related notifications, > please refer to > http://www.spinics.net/lists/linux-pci/msg17822.html > The patchset doesn't apply cleanly to Bjorn's latest pci-next tree. I will > help to rebase it if needed. I have reviewed the patches and I think they make sense overall. However, the statement that acpi_pci_bind()/acpi_pci_unbind() are used to maintain PCI-ACPI binding relationships is quite inaccurate, because all what these functions do is to (1) add/remove ACPI PM notifiers to/from PCI devices and (2) retrieve the _PRT information for bridges from ACPI tables. In fact, the *binding* itself is managed by the code in drivers/acpi/glue.c. Also, please have a look at my suggestion in the last reply to Yinghai: http://marc.info/?l=linux-pci&m=135522965707752&w=2 In fact, I think we can go even further than that. Namely, if we populate the ACPI handle of the device in pci_scan_device(), then we can just move the PM notifier and wakeup setup to platform_pci_wakeup_init(), where it should be (we'll also need to add a corresponding _exit() function, then, but that'll be much cleaner anyway). Then, the remaining thing would be to ensure that _PRT entries are parsed as appropriate somewhere around pci_init_capabilities(). Also, I wonder if you can help test the $subject patchset on a system with hardware PCI hotplug (preferably on top of the linux-pm.git/master branch)? Rafael
于 2012-12-12 2:30, Rafael J. Wysocki 写道: > Hi Gerry, > > On Tuesday, December 11, 2012 11:09:06 PM Jiang Liu wrote: >> Hi Rafael, >> I have worked out a patch set to clean up ACPI/PCI related notifications, >> please refer to >> http://www.spinics.net/lists/linux-pci/msg17822.html >> The patchset doesn't apply cleanly to Bjorn's latest pci-next tree. I will >> help to rebase it if needed. > > I have reviewed the patches and I think they make sense overall. However, > the statement that acpi_pci_bind()/acpi_pci_unbind() are used to maintain > PCI-ACPI binding relationships is quite inaccurate, because all what these > functions do is to (1) add/remove ACPI PM notifiers to/from PCI devices and > (2) retrieve the _PRT information for bridges from ACPI tables. In fact, > the *binding* itself is managed by the code in drivers/acpi/glue.c. > > Also, please have a look at my suggestion in the last reply to Yinghai: > > http://marc.info/?l=linux-pci&m=135522965707752&w=2 > > In fact, I think we can go even further than that. Namely, if we populate the > ACPI handle of the device in pci_scan_device(), then we can just move the PM > notifier and wakeup setup to platform_pci_wakeup_init(), where it should be > (we'll also need to add a corresponding _exit() function, then, but that'll be > much cleaner anyway). > > Then, the remaining thing would be to ensure that _PRT entries are parsed > as appropriate somewhere around pci_init_capabilities(). > > Also, I wonder if you can help test the $subject patchset on a system with > hardware PCI hotplug (preferably on top of the linux-pm.git/master branch)? > Hi Rafael, We are doing test for this series patches, I will send out the test result as soon. Thanks! Yijing -- 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 12/12/2012 10:34 PM, Yijing Wang wrote: > 于 2012-12-12 2:30, Rafael J. Wysocki 写道: >> Hi Gerry, >> >> On Tuesday, December 11, 2012 11:09:06 PM Jiang Liu wrote: >>> Hi Rafael, >>> I have worked out a patch set to clean up ACPI/PCI related notifications, >>> please refer to >>> http://www.spinics.net/lists/linux-pci/msg17822.html >>> The patchset doesn't apply cleanly to Bjorn's latest pci-next tree. I will >>> help to rebase it if needed. >> >> I have reviewed the patches and I think they make sense overall. However, >> the statement that acpi_pci_bind()/acpi_pci_unbind() are used to maintain >> PCI-ACPI binding relationships is quite inaccurate, because all what these >> functions do is to (1) add/remove ACPI PM notifiers to/from PCI devices and >> (2) retrieve the _PRT information for bridges from ACPI tables. In fact, >> the *binding* itself is managed by the code in drivers/acpi/glue.c. >> >> Also, please have a look at my suggestion in the last reply to Yinghai: >> >> http://marc.info/?l=linux-pci&m=135522965707752&w=2 >> >> In fact, I think we can go even further than that. Namely, if we populate the >> ACPI handle of the device in pci_scan_device(), then we can just move the PM >> notifier and wakeup setup to platform_pci_wakeup_init(), where it should be >> (we'll also need to add a corresponding _exit() function, then, but that'll be >> much cleaner anyway). >> >> Then, the remaining thing would be to ensure that _PRT entries are parsed >> as appropriate somewhere around pci_init_capabilities(). >> >> Also, I wonder if you can help test the $subject patchset on a system with >> hardware PCI hotplug (preferably on top of the linux-pm.git/master branch)? >> > Hi Rafael, > We are doing test for this series patches, I will send out the test result as soon. Hi Rafael, We have tried to merge your patchset with IOH hotplug patchsets from Yinghai, and obviously it's not a ease task and we have run into panics. We will try to find some ways to test your patchset only next step. Thanks! > > Thanks! > Yijing > > > -- 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 Wednesday, December 12, 2012 11:05:15 PM Jiang Liu wrote: > On 12/12/2012 10:34 PM, Yijing Wang wrote: > > 于 2012-12-12 2:30, Rafael J. Wysocki 写道: > >> Hi Gerry, > >> > >> On Tuesday, December 11, 2012 11:09:06 PM Jiang Liu wrote: > >>> Hi Rafael, > >>> I have worked out a patch set to clean up ACPI/PCI related notifications, > >>> please refer to > >>> http://www.spinics.net/lists/linux-pci/msg17822.html > >>> The patchset doesn't apply cleanly to Bjorn's latest pci-next tree. I will > >>> help to rebase it if needed. > >> > >> I have reviewed the patches and I think they make sense overall. However, > >> the statement that acpi_pci_bind()/acpi_pci_unbind() are used to maintain > >> PCI-ACPI binding relationships is quite inaccurate, because all what these > >> functions do is to (1) add/remove ACPI PM notifiers to/from PCI devices and > >> (2) retrieve the _PRT information for bridges from ACPI tables. In fact, > >> the *binding* itself is managed by the code in drivers/acpi/glue.c. > >> > >> Also, please have a look at my suggestion in the last reply to Yinghai: > >> > >> http://marc.info/?l=linux-pci&m=135522965707752&w=2 > >> > >> In fact, I think we can go even further than that. Namely, if we populate the > >> ACPI handle of the device in pci_scan_device(), then we can just move the PM > >> notifier and wakeup setup to platform_pci_wakeup_init(), where it should be > >> (we'll also need to add a corresponding _exit() function, then, but that'll be > >> much cleaner anyway). > >> > >> Then, the remaining thing would be to ensure that _PRT entries are parsed > >> as appropriate somewhere around pci_init_capabilities(). > >> > >> Also, I wonder if you can help test the $subject patchset on a system with > >> hardware PCI hotplug (preferably on top of the linux-pm.git/master branch)? > >> > > Hi Rafael, > > We are doing test for this series patches, I will send out the test result as soon. > Hi Rafael, > We have tried to merge your patchset with IOH hotplug patchsets from Yinghai, > and obviously it's not a ease task and we have run into panics. We will try to find > some ways to test your patchset only next step. Thanks a lot for doing this, really appreciated! Rafael
Index: linux/drivers/acpi/scan.c =================================================================== --- linux.orig/drivers/acpi/scan.c +++ linux/drivers/acpi/scan.c @@ -494,7 +494,7 @@ 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_dev->bus_ops.acpi_op_match + return acpi_dev->add_type >= ACPI_BUS_ADD_MATCH && !acpi_match_device_ids(acpi_dev, acpi_drv->ids); } @@ -580,7 +580,7 @@ static int acpi_device_probe(struct devi ret = acpi_bus_driver_init(acpi_dev, acpi_drv); if (!ret) { - if (acpi_dev->bus_ops.acpi_op_start) + if (acpi_dev->add_type == ACPI_BUS_ADD_START) acpi_start_single_object(acpi_dev); if (acpi_drv->ops.notify) { @@ -1433,7 +1433,7 @@ static void acpi_hot_add_bind(struct acp static int acpi_add_single_object(struct acpi_device **child, acpi_handle handle, int type, unsigned long long sta, - struct acpi_bus_ops *ops) + enum acpi_bus_add_type add_type) { int result; struct acpi_device *device; @@ -1449,7 +1449,7 @@ static int acpi_add_single_object(struct device->device_type = type; device->handle = handle; device->parent = acpi_bus_get_parent(handle); - device->bus_ops = *ops; /* workround for not call .start */ + device->add_type = add_type; STRUCT_TO_INT(device->status) = sta; acpi_device_get_busid(device); @@ -1502,7 +1502,7 @@ static int acpi_add_single_object(struct result = acpi_device_register(device); - if (device->bus_ops.acpi_op_match) + if (device->add_type >= ACPI_BUS_ADD_MATCH) acpi_hot_add_bind(device); end: @@ -1526,16 +1526,12 @@ end: static void acpi_bus_add_power_resource(acpi_handle handle) { - struct acpi_bus_ops ops = { - .acpi_op_start = 1, - .acpi_op_match = 1, - }; struct acpi_device *device = NULL; acpi_bus_get_device(handle, &device); if (!device) acpi_add_single_object(&device, handle, ACPI_BUS_TYPE_POWER, - ACPI_STA_DEFAULT, &ops); + ACPI_STA_DEFAULT, ACPI_BUS_ADD_START); } static int acpi_bus_type_and_status(acpi_handle handle, int *type, @@ -1608,16 +1604,13 @@ static acpi_status acpi_bus_check_add(ac */ acpi_bus_get_device(handle, &device); if (!device) { - struct acpi_bus_ops ops = { - .acpi_op_start = !!context, - .acpi_op_match = 0, - }; - - acpi_add_single_object(&device, handle, type, sta, &ops); + acpi_add_single_object(&device, handle, type, sta, + ACPI_BUS_ADD_BASIC); if (!device) return AE_CTRL_DEPTH; - device->bus_ops.acpi_op_match = 1; + device->add_type = context ? + ACPI_BUS_ADD_START : ACPI_BUS_ADD_MATCH; } if (!*return_value) @@ -1779,10 +1772,6 @@ static int acpi_bus_scan_fixed(void) { int result = 0; struct acpi_device *device = NULL; - struct acpi_bus_ops ops = { - .acpi_op_start = 1, - .acpi_op_match = 1, - }; /* * Enumerate all fixed-feature devices. @@ -1791,7 +1780,7 @@ static int acpi_bus_scan_fixed(void) result = acpi_add_single_object(&device, NULL, ACPI_BUS_TYPE_POWER_BUTTON, ACPI_STA_DEFAULT, - &ops); + ACPI_BUS_ADD_START); device_init_wakeup(&device->dev, true); } @@ -1799,7 +1788,7 @@ static int acpi_bus_scan_fixed(void) result = acpi_add_single_object(&device, NULL, ACPI_BUS_TYPE_SLEEP_BUTTON, ACPI_STA_DEFAULT, - &ops); + ACPI_BUS_ADD_START); } return result; Index: linux/include/acpi/acpi_bus.h =================================================================== --- linux.orig/include/acpi/acpi_bus.h +++ linux/include/acpi/acpi_bus.h @@ -63,6 +63,13 @@ acpi_get_physical_device_location(acpi_h #define ACPI_BUS_FILE_ROOT "acpi" extern struct proc_dir_entry *acpi_root_dir; +enum acpi_bus_add_type { + ACPI_BUS_ADD_BASIC = 0, + ACPI_BUS_ADD_MATCH, + ACPI_BUS_ADD_START, + ACPI_BUS_ADD_TYPE_COUNT +}; + enum acpi_bus_removal_type { ACPI_BUS_REMOVAL_NORMAL = 0, ACPI_BUS_REMOVAL_EJECT, @@ -95,12 +102,6 @@ typedef int (*acpi_op_bind) (struct acpi typedef int (*acpi_op_unbind) (struct acpi_device * device); typedef void (*acpi_op_notify) (struct acpi_device * device, u32 event); -struct acpi_bus_ops { - u32 acpi_op_add:1; - u32 acpi_op_start:1; - u32 acpi_op_match:1; -}; - struct acpi_device_ops { acpi_op_add add; acpi_op_remove remove; @@ -284,7 +285,7 @@ struct acpi_device { struct acpi_driver *driver; void *driver_data; struct device dev; - struct acpi_bus_ops bus_ops; /* workaround for different code path for hotplug */ + enum acpi_bus_add_type add_type; /* how to handle adding */ enum acpi_bus_removal_type removal_type; /* indicate for different removal type */ u8 physical_node_count; struct list_head physical_node_list;