Patchwork [5/6] ACPI: Replace struct acpi_bus_ops with enum type

login
register
mail settings
Submitter Rafael J. Wysocki
Date Dec. 9, 2012, 11:03 p.m.
Message ID <2097693.II1PNyeFLY@vostro.rjw.lan>
Download mbox | patch
Permalink /patch/204795/
State Superseded
Headers show

Comments

Rafael J. Wysocki - Dec. 9, 2012, 11:03 p.m.
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.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/scan.c     |   35 ++++++++++++-----------------------
 include/acpi/acpi_bus.h |   15 ++++++++-------
 2 files changed, 20 insertions(+), 30 deletions(-)


--
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
Yinghai Lu - Dec. 10, 2012, 5:34 a.m.
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
Rafael J. Wysocki - Dec. 10, 2012, 2:46 p.m.
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
Yinghai Lu - Dec. 10, 2012, 5:07 p.m.
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
Rafael J. Wysocki - Dec. 10, 2012, 10:47 p.m.
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
Rafael J. Wysocki - Dec. 10, 2012, 11:09 p.m.
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
Yinghai Lu - Dec. 10, 2012, 11:14 p.m.
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
Yinghai Lu - Dec. 10, 2012, 11:22 p.m.
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
Rafael J. Wysocki - Dec. 11, 2012, 12:48 a.m.
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
Rafael J. Wysocki - Dec. 11, 2012, 1:02 a.m.
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
Rafael J. Wysocki - Dec. 11, 2012, 1:28 a.m.
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
Yinghai Lu - Dec. 11, 2012, 2:26 a.m.
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
Rafael J. Wysocki - Dec. 11, 2012, 12:45 p.m.
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
Jiang Liu - Dec. 11, 2012, 3:09 p.m.
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
Rafael J. Wysocki - Dec. 11, 2012, 6:30 p.m.
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
Yijing Wang - Dec. 12, 2012, 2:34 p.m.
于 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
Jiang Liu - Dec. 12, 2012, 3:05 p.m.
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
Rafael J. Wysocki - Dec. 12, 2012, 10:39 p.m.
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

Patch

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;