diff mbox

[01/10] device: add drivers_autoprobe in struct device

Message ID 1349159588-15029-2-git-send-email-yinghai@kernel.org (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Yinghai Lu Oct. 2, 2012, 6:32 a.m. UTC
To use to control the delay attach driver for specific device.

Will use bus notifier to toggle this bits when needed.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/bus.c     |    2 +-
 drivers/base/core.c    |    1 +
 include/linux/device.h |    1 +
 3 files changed, 3 insertions(+), 1 deletions(-)

Comments

Greg KH Oct. 2, 2012, 1:33 p.m. UTC | #1
On Mon, Oct 01, 2012 at 11:32:59PM -0700, Yinghai Lu wrote:
> To use to control the delay attach driver for specific device.
> 
> Will use bus notifier to toggle this bits when needed.
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Without a whole lot more context as to why you want this change, no,
sorry, I'll not accept it.

greg k-h
--
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 Oct. 2, 2012, 5:39 p.m. UTC | #2
On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 01, 2012 at 11:32:59PM -0700, Yinghai Lu wrote:
>> To use to control the delay attach driver for specific device.
>>
>> Will use bus notifier to toggle this bits when needed.
>>
>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>
> Without a whole lot more context as to why you want this change, no,
> sorry, I'll not accept it.

Sorry, I should put you on the to list for the whole patchset.

Those patches address Bjorn's request:
1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
2. register pci device to device as soon as possible, so for_each_pci_device
   could be used early before pci_bus_add_devices.

please check change log patch2-4.

https://patchwork.kernel.org/patch/1535731/
https://patchwork.kernel.org/patch/1535821/
https://patchwork.kernel.org/patch/1535851/

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
Greg KH Oct. 2, 2012, 5:47 p.m. UTC | #3
On Tue, Oct 02, 2012 at 10:39:25AM -0700, Yinghai Lu wrote:
> On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Oct 01, 2012 at 11:32:59PM -0700, Yinghai Lu wrote:
> >> To use to control the delay attach driver for specific device.
> >>
> >> Will use bus notifier to toggle this bits when needed.
> >>
> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > Without a whole lot more context as to why you want this change, no,
> > sorry, I'll not accept it.
> 
> Sorry, I should put you on the to list for the whole patchset.
> 
> Those patches address Bjorn's request:
> 1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
> 2. register pci device to device as soon as possible, so for_each_pci_device
>    could be used early before pci_bus_add_devices.
> 
> please check change log patch2-4.

Sorry, I still don't see what is so special about PCI that they have to
do something different here.  What am I missing?

greg k-h
--
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
Bjorn Helgaas Oct. 2, 2012, 6 p.m. UTC | #4
On Tue, Oct 2, 2012 at 11:47 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 02, 2012 at 10:39:25AM -0700, Yinghai Lu wrote:
>> On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Mon, Oct 01, 2012 at 11:32:59PM -0700, Yinghai Lu wrote:
>> >> To use to control the delay attach driver for specific device.
>> >>
>> >> Will use bus notifier to toggle this bits when needed.
>> >>
>> >> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >
>> > Without a whole lot more context as to why you want this change, no,
>> > sorry, I'll not accept it.
>>
>> Sorry, I should put you on the to list for the whole patchset.
>>
>> Those patches address Bjorn's request:
>> 1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
>> 2. register pci device to device as soon as possible, so for_each_pci_device
>>    could be used early before pci_bus_add_devices.

Just to be clear, I'm in favor of eliminating acpi_pci_root_start()
eventually, but I didn't suggest this approach, and I don't like it
much either.  I don't have much constructive feedback yet, but this
feels like a special case that's not cleanly integrated into the
existing infrastructure.

>> please check change log patch2-4.
>
> Sorry, I still don't see what is so special about PCI that they have to
> do something different here.  What am I missing?
--
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 Oct. 2, 2012, 8:20 p.m. UTC | #5
On Tue, Oct 2, 2012 at 10:47 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 02, 2012 at 10:39:25AM -0700, Yinghai Lu wrote:
>> On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
>> Those patches address Bjorn's request:
>> 1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
>> 2. register pci device to device as soon as possible, so for_each_pci_device
>>    could be used early before pci_bus_add_devices.
>>
>> please check change log patch2-4.
>
> Sorry, I still don't see what is so special about PCI that they have to
> do something different here.  What am I missing?

That is something that core related with hotplug:

for booting and hotplug, we have different code path.
1. for booting, all device are enumerated and registered, and later
driver get registered, driver is attached to the devices.
2. for hotplug, one device on the bus is found, and then registered
and driver is attached, after that
                     another device on the bus is found, and then
registered and driver is attached...

The reason for the difference, device driver is not registered later
during booting path.

We should make the two path have same sequence. the solution that I
suggest is adding per device drivers_autoprobe.

and thanking for bus_type notifier that we could toggle that bit
during device_add, so could make all device on same
bus get probed and registered but driver get skipped. and after that
call device_attach for all device at one batch.

That will remove those hotplug related hacks like acpi_pci_root_start
that try to delay registering of pci devices.
and make the code lesser and readable.

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
Greg KH Oct. 2, 2012, 8:45 p.m. UTC | #6
On Tue, Oct 02, 2012 at 01:20:58PM -0700, Yinghai Lu wrote:
> On Tue, Oct 2, 2012 at 10:47 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Tue, Oct 02, 2012 at 10:39:25AM -0700, Yinghai Lu wrote:
> >> On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
> >> Those patches address Bjorn's request:
> >> 1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
> >> 2. register pci device to device as soon as possible, so for_each_pci_device
> >>    could be used early before pci_bus_add_devices.
> >>
> >> please check change log patch2-4.
> >
> > Sorry, I still don't see what is so special about PCI that they have to
> > do something different here.  What am I missing?
> 
> That is something that core related with hotplug:
> 
> for booting and hotplug, we have different code path.
> 1. for booting, all device are enumerated and registered, and later
> driver get registered, driver is attached to the devices.
> 2. for hotplug, one device on the bus is found, and then registered
> and driver is attached, after that
>                      another device on the bus is found, and then
> registered and driver is attached...
> 
> The reason for the difference, device driver is not registered later
> during booting path.

But on each case, the code path in the driver core is the same, and you
don't know if for 1) a driver is not already registered in the system
(think drivers built into the kernel).

> We should make the two path have same sequence. the solution that I
> suggest is adding per device drivers_autoprobe.

No, now you have 2 code paths in the driver core, do not do that.

> and thanking for bus_type notifier that we could toggle that bit
> during device_add, so could make all device on same
> bus get probed and registered but driver get skipped. and after that
> call device_attach for all device at one batch.
> 
> That will remove those hotplug related hacks like acpi_pci_root_start
> that try to delay registering of pci devices.
> and make the code lesser and readable.

I don't know what you are doing in the acpi code, but I do not think it
requires a driver core change like this.

Again, why is PCI different from any other bus type?  (hint, it
shouldn't be...)

greg k-h
--
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 Oct. 2, 2012, 9:47 p.m. UTC | #7
On Tue, Oct 2, 2012 at 1:45 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 02, 2012 at 01:20:58PM -0700, Yinghai Lu wrote:
>> On Tue, Oct 2, 2012 at 10:47 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Tue, Oct 02, 2012 at 10:39:25AM -0700, Yinghai Lu wrote:
>> >> On Tue, Oct 2, 2012 at 6:33 AM, Greg Kroah-Hartman
>> >> Those patches address Bjorn's request:
>> >> 1. kill acpi_pci_root_start in drivers/acpi/pci_root.c
>> >> 2. register pci device to device as soon as possible, so for_each_pci_device
>> >>    could be used early before pci_bus_add_devices.
>> >>
>> >> please check change log patch2-4.
>> >
>> > Sorry, I still don't see what is so special about PCI that they have to
>> > do something different here.  What am I missing?
>>
>> That is something that core related with hotplug:
>>
>> for booting and hotplug, we have different code path.
>> 1. for booting, all device are enumerated and registered, and later
>> driver get registered, driver is attached to the devices.
>> 2. for hotplug, one device on the bus is found, and then registered
>> and driver is attached, after that
>>                      another device on the bus is found, and then
>> registered and driver is attached...
>>
>> The reason for the difference, device driver is not registered later
>> during booting path.
>
> But on each case, the code path in the driver core is the same, and you
> don't know if for 1) a driver is not already registered in the system
> (think drivers built into the kernel).

different level initcall or link sequence make sure that in order. aka
bus_type come first before driver of that bus type.

>
>> We should make the two path have same sequence. the solution that I
>> suggest is adding per device drivers_autoprobe.
>
> No, now you have 2 code paths in the driver core, do not do that.

no, reduce 2 to 1.

>
>> and thanking for bus_type notifier that we could toggle that bit
>> during device_add, so could make all device on same
>> bus get probed and registered but driver get skipped. and after that
>> call device_attach for all device at one batch.
>>
>> That will remove those hotplug related hacks like acpi_pci_root_start
>> that try to delay registering of pci devices.
>> and make the code lesser and readable.
>
> I don't know what you are doing in the acpi code, but I do not think it
> requires a driver core change like this.
>
> Again, why is PCI different from any other bus type?  (hint, it
> shouldn't be...)

it is not pci susbsystem problem.

the problem come from device-driver core for hotplug handling, and
acpi system is
forcing pci system to split the scan and registering.
aka: to scan all pci device under one pci_root from acpi_pci_root_add.
then do pci_bus_add_devices for that bus in acpi_pci_root_start.

on hotplug path:
when acpi enumerating acpi devices, it will create acpi_device for pci_root,
then probe driver for that device : acpi_pci_root_driver.
<at that time no acpi_device under that pci_root acpi device is created>
device_add for root acpi_device will call bus_probe_device and ... and execute
acpi_pci_root_driver .add aka acpi_pci_root_add.
acpi_pci_root_add will call pci_acpi_scan_root. that will scan pci
root bus to find all
pci devices but does not add those pci device into devices.
If at the time device_add for pci device get called, platform_notify
will try to bind pci_dev
with acpi devices, but corresponding acpi device is not created yet.
that bind will fail.
So acpi_driver introduce .start in ops to add pci device later.

The patches will hold that acpi_driver for root to be probed for root
to early. aka
make sure all acpi devices come first, then probe driver for acpi device one by.
In that case we don' t need subsystem to introduce .start to hold add
device for next level.

Now HOTPLUG is enabled forcely, and bus_type add notifier help to
control that device
drivers_autoprobe during device hot add.

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
Bjorn Helgaas Oct. 2, 2012, 10:38 p.m. UTC | #8
On Tue, Oct 2, 2012 at 3:47 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Oct 2, 2012 at 1:45 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:

>> I don't know what you are doing in the acpi code, but I do not think it
>> requires a driver core change like this.
>>
>> Again, why is PCI different from any other bus type?  (hint, it
>> shouldn't be...)
>
> it is not pci susbsystem problem.
>
> the problem come from device-driver core for hotplug handling, and
> acpi system is
> forcing pci system to split the scan and registering.
> aka: to scan all pci device under one pci_root from acpi_pci_root_add.
> then do pci_bus_add_devices for that bus in acpi_pci_root_start.

I don't believe there's anything in ACPI that forces this split
between scanning and registering.

Certainly we have Linux *implementation* issues that currently lead us
to split them, but I don't think those are forced by the hardware or
the spec.  I'd rather address those implementation issues than add
band-aids.

> on hotplug path:
> when acpi enumerating acpi devices, it will create acpi_device for pci_root,
> then probe driver for that device : acpi_pci_root_driver.
> <at that time no acpi_device under that pci_root acpi device is created>
> device_add for root acpi_device will call bus_probe_device and ... and execute
> acpi_pci_root_driver .add aka acpi_pci_root_add.
> acpi_pci_root_add will call pci_acpi_scan_root. that will scan pci
> root bus to find all
> pci devices but does not add those pci device into devices.
> If at the time device_add for pci device get called, platform_notify
> will try to bind pci_dev
> with acpi devices, but corresponding acpi device is not created yet.
> that bind will fail.
> So acpi_driver introduce .start in ops to add pci device later.

We enumerate ACPI devices by doing a depth-first traversal of the
namespace.  We should be able to bind a driver to a device as soon as
we discover it.  For PNP0A03/08 host bridges, that will be the
pci_root.c driver.  That driver's .add() method enumerates all the PCI
devices behind the host bridge, which is another depth-first
traversal, this time of the PCI hierarchy.  Similarly, we ought to be
able to bind drivers to the PCI devices as we discover them.

But the PCI/ACPI binding is an issue here, because the ACPI
enumeration hasn't descended below the host bridge yet, so we have the
pci_dev structs but the acpi_device structs don't exist yet.  I think
this is part of the reason for the .add()/.start() split.  But I don't
think the split is the correct solution.  I think we need to think
about the binding in a different way.  Maybe we don't bind at all and
instead search the namespace every time we need the association.  Or
maybe we do the binding but base it on the acpi_handle (which exists
before the ACPI subtree has been enumerated) rather than the
acpi_device (which doesn't exist until we enumerate the subtree).
It's not even clear to me that we should build acpi_device structs for
things that already have pci_dev structs.

I don't know what the right answer is, but I do think it's better to
resolve it inside PCI and ACPI rather than playing games with driver
binding times in the driver core.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu Oct. 2, 2012, 11:20 p.m. UTC | #9
On Tue, Oct 2, 2012 at 3:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Tue, Oct 2, 2012 at 3:47 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Tue, Oct 2, 2012 at 1:45 PM, Greg Kroah-Hartman
>
> We enumerate ACPI devices by doing a depth-first traversal of the
> namespace.  We should be able to bind a driver to a device as soon as
> we discover it.  For PNP0A03/08 host bridges, that will be the
> pci_root.c driver.  That driver's .add() method enumerates all the PCI
> devices behind the host bridge, which is another depth-first
> traversal, this time of the PCI hierarchy.  Similarly, we ought to be
> able to bind drivers to the PCI devices as we discover them.

before introducing this per device .drivers_autoprobe, i did one acpi
to pci bind version
as attached patch.

that mean that will have two version: for booting path: use pci to acpi binding.
and hot plug path will use acpi to pci binding.
Also that make code pretty ugly to consider platform_notifier at the same time.

and acp_get_pci_dev in that acpi_pci_hp_notifier is not very efficient.

-Yinghai
Yinghai Lu Oct. 3, 2012, midnight UTC | #10
On Tue, Oct 2, 2012 at 3:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> I don't know what the right answer is, but I do think it's better to
> resolve it inside PCI and ACPI rather than playing games with driver
> binding times in the driver core.

is that ok to save that drivers_autoprobe bit in device.archdata for
ia64 and x86 that using acpi?
--
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 Oct. 3, 2012, 2:07 a.m. UTC | #11
On Tue, Oct 2, 2012 at 5:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Oct 2, 2012 at 3:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> I don't know what the right answer is, but I do think it's better to
>> resolve it inside PCI and ACPI rather than playing games with driver
>> binding times in the driver core.
>
> is that ok to save that drivers_autoprobe bit in device.archdata for
> ia64 and x86 that using acpi?

updated patch to put the bit in pci_dev and acpi_device instead.

please check attached patches.

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
Yinghai Lu Oct. 3, 2012, 2:08 a.m. UTC | #12
On Tue, Oct 2, 2012 at 7:07 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Oct 2, 2012 at 5:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Tue, Oct 2, 2012 at 3:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>>> I don't know what the right answer is, but I do think it's better to
>>> resolve it inside PCI and ACPI rather than playing games with driver
>>> binding times in the driver core.
>>
>> is that ok to save that drivers_autoprobe bit in device.archdata for
>> ia64 and x86 that using acpi?
>
> updated patch to put the bit in pci_dev and acpi_device instead.
>
> please check attached patches.

attached.
Bjorn Helgaas Oct. 3, 2012, 7:48 p.m. UTC | #13
On Tue, Oct 2, 2012 at 6:00 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Oct 2, 2012 at 3:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
>> I don't know what the right answer is, but I do think it's better to
>> resolve it inside PCI and ACPI rather than playing games with driver
>> binding times in the driver core.
>
> is that ok to save that drivers_autoprobe bit in device.archdata for
> ia64 and x86 that using acpi?

You'll have to ask Len (cc'd) what he thinks about the ACPI piece, but
I don't like the PCI piece.  It's the same idea that Greg already said
he wouldn't put in the driver core.  Why would it be better to add
this complexity in the ACPI and PCI cores?

I still think you need to work on the ACPI/PCI binding issue.  Yes,
it's hard, but I'm not in favor of avoiding that hard work by adding
kludges everywhere.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu Oct. 3, 2012, 8:50 p.m. UTC | #14
On Wed, Oct 3, 2012 at 12:48 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> You'll have to ask Len (cc'd) what he thinks about the ACPI piece, but
> I don't like the PCI piece.  It's the same idea that Greg already said
> he wouldn't put in the driver core.  Why would it be better to add
> this complexity in the ACPI and PCI cores?

It should be simplify acpi code a little.

I will reorder acpi parts as separate patcheset for review.

> I still think you need to work on the ACPI/PCI binding issue.  Yes,
> it's hard, but I'm not in favor of avoiding that hard work by adding
> kludges everywhere.

acpi pci binding should be solved if len could accept acpi changes.

for pci separate driver attach, that is for registering pci dev to
tree early instead of
waiting till pci_bus_add_devices.
Let me know if you still want us to pursue that.

Otherwise I'd like to move root bus and host bridge registering into
pci_bus_add_devices().

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
Yinghai Lu Oct. 3, 2012, 11 p.m. UTC | #15
Now acpi_pci_root_driver has two ops: .add and .start, aka acpi_pci_root_add
and acpi_pci_root_start.

That is for hotplug handling: .add need to return early to make sure all
acpi device could be created and added early. So .start could device_add
pci device that are found in acpi_pci_root_add/pci_acpi_scan_root().

That is holding pci devics to be out of devices for while.

We could use bus notifier to handle hotplug case.
	CONFIG_HOTPLUG is enabled always now.
Need to add drivers_autoprobe bit in acpi_device to hold attaching drivers
for acpi_devices, so could make sure all acpi_devices get created at first.
Then acpi_bus_attach() that is called from acpi_bus_add will attach driver
for all acpi_devices that are just created.

That make the logic more simple: hotplug path handling just like booting path
that drivers are attached after all acpi device get created.

At last we could remove all acpi_bus_start workaround.

Thanks

Yinghai

Yinghai Lu (4):
  ACPI: add drivers_autoprobe in struct acpi_device
  ACPI: use device drivers_autoprobe to delay loading acpi drivers
  PCI, ACPI: Remove not used acpi_pci_root_start()
  ACPI: remove acpi_op_start workaround

 drivers/acpi/pci_root.c |   27 +++------
 drivers/acpi/scan.c     |  145 ++++++++++++++++++++++-------------------------
 include/acpi/acpi_bus.h |    9 +---
 3 files changed, 77 insertions(+), 104 deletions(-)
Bjorn Helgaas Oct. 4, 2012, 5:47 p.m. UTC | #16
On Wed, Oct 03, 2012 at 04:00:10PM -0700, Yinghai Lu wrote:
> Now acpi_pci_root_driver has two ops: .add and .start, aka acpi_pci_root_add
> and acpi_pci_root_start.
> 
> That is for hotplug handling: .add need to return early to make sure all
> acpi device could be created and added early. So .start could device_add
> pci device that are found in acpi_pci_root_add/pci_acpi_scan_root().
> 
> That is holding pci devics to be out of devices for while.
> 
> We could use bus notifier to handle hotplug case.
> 	CONFIG_HOTPLUG is enabled always now.
> Need to add drivers_autoprobe bit in acpi_device to hold attaching drivers
> for acpi_devices, so could make sure all acpi_devices get created at first.
> Then acpi_bus_attach() that is called from acpi_bus_add will attach driver
> for all acpi_devices that are just created.
> 
> That make the logic more simple: hotplug path handling just like booting path
> that drivers are attached after all acpi device get created.
> 
> At last we could remove all acpi_bus_start workaround.

I think we should get rid of ACPI .start() methods, but I'm opposed to this
approach of fiddling with driver binding order.

At hot-plug time, the pci_root.c driver is already loaded, then we enumerate
the new host bridge.  Since pci_root.c is already loaded, we bind the driver
before descending the ACPI tree below the host bridge.  We need to make that
same ordering work at boot-time.

At boot-time, we currently enumerate ALL ACPI devices before registering
the pci_root.c driver:

    acpi_init				# subsys_initcall
	acpi_scan_init
	    acpi_bus_scan		# enumerate ACPI devices

    acpi_pci_root_init			# subsys_initcall for pci_root.c
	acpi_bus_register_driver
	    ...
		acpi_pci_root_add	# pci_root.c add method

This is a fundamental difference: at boot-time, all the ACPI devices below the
host bridge already exist before the pci_root.c driver claims the bridge,
while at hot-add time, pci_root.c claims the bridge *before* those ACPI
devices exist.

I think this is wrong.  The hot-plug case (where the driver is already
loaded and binds to the device as soon as it's discovered, before the
ACPI hierarchy below it is enumerated) seems like the obviously correct
order.  I think we should change the boot-time order to match that, i.e.,
we should register pci_root.c *before* enumerating ACPI devices.

I realize that this will require changes in the way we bind PCI and ACPI
devices.  I pointed that out in a previous response, appended below for
convenience:

On Tue, Oct 2, 2012 at 4:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

>> We enumerate ACPI devices by doing a depth-first traversal of the
>> namespace.  We should be able to bind a driver to a device as soon as
>> we discover it.  For PNP0A03/08 host bridges, that will be the
>> pci_root.c driver.  That driver's .add() method enumerates all the PCI
>> devices behind the host bridge, which is another depth-first
>> traversal, this time of the PCI hierarchy.  Similarly, we ought to be
>> able to bind drivers to the PCI devices as we discover them.
>
>> But the PCI/ACPI binding is an issue here, because the ACPI
>> enumeration hasn't descended below the host bridge yet, so we have the
>> pci_dev structs but the acpi_device structs don't exist yet.  I think
>> this is part of the reason for the .add()/.start() split.  But I don't
>> think the split is the correct solution.  I think we need to think
>> about the binding in a different way.  Maybe we don't bind at all and
>> instead search the namespace every time we need the association.  Or
>> maybe we do the binding but base it on the acpi_handle (which exists
>> before the ACPI subtree has been enumerated) rather than the
>> acpi_device (which doesn't exist until we enumerate the subtree).
>> It's not even clear to me that we should build acpi_device structs for
>> things that already have pci_dev structs.
--
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 Oct. 4, 2012, 6:36 p.m. UTC | #17
On Thu, Oct 4, 2012 at 10:47 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Oct 03, 2012 at 04:00:10PM -0700, Yinghai Lu wrote:
> This is a fundamental difference: at boot-time, all the ACPI devices below the
> host bridge already exist before the pci_root.c driver claims the bridge,
> while at hot-add time, pci_root.c claims the bridge *before* those ACPI
> devices exist.
>
> I think this is wrong.  The hot-plug case (where the driver is already
> loaded and binds to the device as soon as it's discovered, before the
> ACPI hierarchy below it is enumerated) seems like the obviously correct
> order.  I think we should change the boot-time order to match that, i.e.,
> we should register pci_root.c *before* enumerating ACPI devices.

in booting path, all device get probed at first, and then register driver...
do you want to register all pci driver before probing pci devices?

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Oct. 4, 2012, 7:44 p.m. UTC | #18
On Thu, Oct 4, 2012 at 12:36 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Oct 4, 2012 at 10:47 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> On Wed, Oct 03, 2012 at 04:00:10PM -0700, Yinghai Lu wrote:
>> This is a fundamental difference: at boot-time, all the ACPI devices below the
>> host bridge already exist before the pci_root.c driver claims the bridge,
>> while at hot-add time, pci_root.c claims the bridge *before* those ACPI
>> devices exist.
>>
>> I think this is wrong.  The hot-plug case (where the driver is already
>> loaded and binds to the device as soon as it's discovered, before the
>> ACPI hierarchy below it is enumerated) seems like the obviously correct
>> order.  I think we should change the boot-time order to match that, i.e.,
>> we should register pci_root.c *before* enumerating ACPI devices.
>
> in booting path, all device get probed at first, and then register driver...
> do you want to register all pci driver before probing pci devices?

I don't think we should have dependencies either way.  It shouldn't
matter whether we enumerate devices first or we register the driver
first.  That's why I think the current PCI/ACPI binding is broken --
it assumes that we fully enumerate ACPI before the driver is
registered.

To answer your specific question, yes, I do think drivers that are
statically built in probably should be registered before devices are
enumerated.  That way, the boot-time case is more similar to the
hot-add case.

Obviously, for drivers that can be modules, the reverse must work as
well (enumerate devices, then load and register the driver).  And then
the other order (register driver, then enumerate device) must also
work so future hot-adds of the same device type work.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael Wysocki Oct. 4, 2012, 7:53 p.m. UTC | #19
On Thursday 04 of October 2012 11:47:46 Bjorn Helgaas wrote:
> On Wed, Oct 03, 2012 at 04:00:10PM -0700, Yinghai Lu wrote:
> > Now acpi_pci_root_driver has two ops: .add and .start, aka acpi_pci_root_add
> > and acpi_pci_root_start.
> > 
> > That is for hotplug handling: .add need to return early to make sure all
> > acpi device could be created and added early. So .start could device_add
> > pci device that are found in acpi_pci_root_add/pci_acpi_scan_root().
> > 
> > That is holding pci devics to be out of devices for while.
> > 
> > We could use bus notifier to handle hotplug case.
> > 	CONFIG_HOTPLUG is enabled always now.
> > Need to add drivers_autoprobe bit in acpi_device to hold attaching drivers
> > for acpi_devices, so could make sure all acpi_devices get created at first.
> > Then acpi_bus_attach() that is called from acpi_bus_add will attach driver
> > for all acpi_devices that are just created.
> > 
> > That make the logic more simple: hotplug path handling just like booting path
> > that drivers are attached after all acpi device get created.
> > 
> > At last we could remove all acpi_bus_start workaround.
> 
> I think we should get rid of ACPI .start() methods, but I'm opposed to this
> approach of fiddling with driver binding order.
> 
> At hot-plug time, the pci_root.c driver is already loaded, then we enumerate
> the new host bridge.  Since pci_root.c is already loaded, we bind the driver
> before descending the ACPI tree below the host bridge.  We need to make that
> same ordering work at boot-time.
> 
> At boot-time, we currently enumerate ALL ACPI devices before registering
> the pci_root.c driver:
> 
>     acpi_init				# subsys_initcall
> 	acpi_scan_init
> 	    acpi_bus_scan		# enumerate ACPI devices
> 
>     acpi_pci_root_init			# subsys_initcall for pci_root.c
> 	acpi_bus_register_driver
> 	    ...
> 		acpi_pci_root_add	# pci_root.c add method
> 
> This is a fundamental difference: at boot-time, all the ACPI devices below the
> host bridge already exist before the pci_root.c driver claims the bridge,
> while at hot-add time, pci_root.c claims the bridge *before* those ACPI
> devices exist.
> 
> I think this is wrong.  The hot-plug case (where the driver is already
> loaded and binds to the device as soon as it's discovered, before the
> ACPI hierarchy below it is enumerated) seems like the obviously correct
> order.  I think we should change the boot-time order to match that, i.e.,
> we should register pci_root.c *before* enumerating ACPI devices.
> 
> I realize that this will require changes in the way we bind PCI and ACPI
> devices.  I pointed that out in a previous response, appended below for
> convenience:
> 
> On Tue, Oct 2, 2012 at 4:38 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 
> >> We enumerate ACPI devices by doing a depth-first traversal of the
> >> namespace.  We should be able to bind a driver to a device as soon as
> >> we discover it.  For PNP0A03/08 host bridges, that will be the
> >> pci_root.c driver.  That driver's .add() method enumerates all the PCI
> >> devices behind the host bridge, which is another depth-first
> >> traversal, this time of the PCI hierarchy.  Similarly, we ought to be
> >> able to bind drivers to the PCI devices as we discover them.
> >
> >> But the PCI/ACPI binding is an issue here, because the ACPI
> >> enumeration hasn't descended below the host bridge yet, so we have the
> >> pci_dev structs but the acpi_device structs don't exist yet.  I think
> >> this is part of the reason for the .add()/.start() split.  But I don't
> >> think the split is the correct solution.  I think we need to think
> >> about the binding in a different way.  Maybe we don't bind at all and
> >> instead search the namespace every time we need the association.  Or
> >> maybe we do the binding but base it on the acpi_handle (which exists
> >> before the ACPI subtree has been enumerated) rather than the
> >> acpi_device (which doesn't exist until we enumerate the subtree).
> >> It's not even clear to me that we should build acpi_device structs for
> >> things that already have pci_dev structs.

No, we shouldn't.

We generally have problems in this area, not only with PCI.  For
example, some platform x86 drivers cannot bind to the devices they
should handle, because there's a "generic" ACPI driver that binds to
the device in question earlier.

My personal opinion is that so called "ACPI devices" (i.e. things
represented by struct acpi_device) are generally partially redundant, because
either they already have "sibling" struct device objects representing "real"
devices, like PCI, SATA or PNP, or there should be struct platform_device
"sibling" objects corresponding to them.  Moreover, all ACPI drivers can be
replaced with platform drivers and the only problem is the .notify() callback
that is not present in struct platform_driver. So perhaps we can create
a "real device" for every "ACPI device" we discover (if there isn't one
already) and stop using ACPI drivers entirely, eventually.

That also may help to address the problem of hypothetical future systems
with ACPI tables describing the same hardware that we already have device
tree representation for. In those cases drivers should work regardless of
whether the information on devices is read from ACPI tables or from device
trees and quite frankly I don't see how we can achieve this with the
existing ACPI drivers.

Perhaps we don't even need the ACPI bus type and struct acpi_device
objects can be treated simply as storage of information used by the generic
ACPI power management code etc. I'm not sure how to get there yet in the
least painful way, but I think it would make things generally more
straightforward.

Thanks,
Rafael
Rafael Wysocki Oct. 4, 2012, 7:54 p.m. UTC | #20
On Thursday 04 of October 2012 13:44:42 Bjorn Helgaas wrote:
> On Thu, Oct 4, 2012 at 12:36 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Thu, Oct 4, 2012 at 10:47 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> On Wed, Oct 03, 2012 at 04:00:10PM -0700, Yinghai Lu wrote:
> >> This is a fundamental difference: at boot-time, all the ACPI devices below the
> >> host bridge already exist before the pci_root.c driver claims the bridge,
> >> while at hot-add time, pci_root.c claims the bridge *before* those ACPI
> >> devices exist.
> >>
> >> I think this is wrong.  The hot-plug case (where the driver is already
> >> loaded and binds to the device as soon as it's discovered, before the
> >> ACPI hierarchy below it is enumerated) seems like the obviously correct
> >> order.  I think we should change the boot-time order to match that, i.e.,
> >> we should register pci_root.c *before* enumerating ACPI devices.
> >
> > in booting path, all device get probed at first, and then register driver...
> > do you want to register all pci driver before probing pci devices?
> 
> I don't think we should have dependencies either way.  It shouldn't
> matter whether we enumerate devices first or we register the driver
> first.  That's why I think the current PCI/ACPI binding is broken --
> it assumes that we fully enumerate ACPI before the driver is
> registered.
> 
> To answer your specific question, yes, I do think drivers that are
> statically built in probably should be registered before devices are
> enumerated.  That way, the boot-time case is more similar to the
> hot-add case.
> 
> Obviously, for drivers that can be modules, the reverse must work as
> well (enumerate devices, then load and register the driver).  And then
> the other order (register driver, then enumerate device) must also
> work so future hot-adds of the same device type work.

Agreed.

Thanks,
Rafael
Yinghai Lu Oct. 4, 2012, 8:14 p.m. UTC | #21
On Thu, Oct 4, 2012 at 12:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> To answer your specific question, yes, I do think drivers that are
> statically built in probably should be registered before devices are
> enumerated.  That way, the boot-time case is more similar to the
> hot-add case.
>
> Obviously, for drivers that can be modules, the reverse must work as
> well (enumerate devices, then load and register the driver).  And then
> the other order (register driver, then enumerate device) must also
> work so future hot-adds of the same device type work.

so you will have to handle two paths instead one.

current booting path sequence are tested more than hot add path.
--
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
Bjorn Helgaas Oct. 4, 2012, 8:47 p.m. UTC | #22
On Thu, Oct 4, 2012 at 2:14 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Thu, Oct 4, 2012 at 12:44 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> To answer your specific question, yes, I do think drivers that are
>> statically built in probably should be registered before devices are
>> enumerated.  That way, the boot-time case is more similar to the
>> hot-add case.
>>
>> Obviously, for drivers that can be modules, the reverse must work as
>> well (enumerate devices, then load and register the driver).  And then
>> the other order (register driver, then enumerate device) must also
>> work so future hot-adds of the same device type work.
>
> so you will have to handle two paths instead one.

I'm not proposing any additional requirements; I'm just describing the
way Linux driver modules work.  Any module *already* must support both
orders (enumerate devices then register driver, as well as register
driver then enumerate and bind to a hot-added device).  And this
should not be two paths, it should be one and the same path for both
orders.

> current booting path sequence are tested more than hot add path.

True.  You're proposing fiddling with driver binding order so we can
use the current boot path ordering at hot-add time.  I'm suggesting
that the "register driver then enumerate device" order is something we
have to support for hot-add anyway, and that we should use the same
ordering at boot-time.  That's more change for the boot-time path, but
I think it's cleaner and more maintainable in the long term.
--
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 Wysocki Oct. 4, 2012, 9:23 p.m. UTC | #23
On Wednesday 03 of October 2012 16:00:10 Yinghai Lu wrote:
> Now acpi_pci_root_driver has two ops: .add and .start, aka acpi_pci_root_add
> and acpi_pci_root_start.
> 
> That is for hotplug handling: .add need to return early to make sure all
> acpi device could be created and added early. So .start could device_add
> pci device that are found in acpi_pci_root_add/pci_acpi_scan_root().
> 
> That is holding pci devics to be out of devices for while.
> 
> We could use bus notifier to handle hotplug case.
> 	CONFIG_HOTPLUG is enabled always now.
> Need to add drivers_autoprobe bit in acpi_device to hold attaching drivers
> for acpi_devices, so could make sure all acpi_devices get created at first.
> Then acpi_bus_attach() that is called from acpi_bus_add will attach driver
> for all acpi_devices that are just created.
> 
> That make the logic more simple: hotplug path handling just like booting path
> that drivers are attached after all acpi device get created.
> 
> At last we could remove all acpi_bus_start workaround.

Do I understand correctly that you just want to prevent acpi_pci_root_driver
from binding to the host bridge's struct acpi_device created while we're
walking the ACPI namespace?

Rafael
Yinghai Lu Oct. 4, 2012, 9:31 p.m. UTC | #24
On Thu, Oct 4, 2012 at 2:23 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>
>> At last we could remove all acpi_bus_start workaround.
>
> Do I understand correctly that you just want to prevent acpi_pci_root_driver
> from binding to the host bridge's struct acpi_device created while we're
> walking the ACPI namespace?

yes, during hot add path.

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 Wysocki Oct. 4, 2012, 9:53 p.m. UTC | #25
On Thursday 04 of October 2012 14:31:46 Yinghai Lu wrote:
> On Thu, Oct 4, 2012 at 2:23 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >>
> >> At last we could remove all acpi_bus_start workaround.
> >
> > Do I understand correctly that you just want to prevent acpi_pci_root_driver
> > from binding to the host bridge's struct acpi_device created while we're
> > walking the ACPI namespace?
> 
> yes, during hot add path.

Why exactly do you want to prevent that from happening?

Rafael
Yinghai Lu Oct. 4, 2012, 10:01 p.m. UTC | #26
On Thu, Oct 4, 2012 at 2:53 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday 04 of October 2012 14:31:46 Yinghai Lu wrote:
>> On Thu, Oct 4, 2012 at 2:23 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >>
>> >> At last we could remove all acpi_bus_start workaround.
>> >
>> > Do I understand correctly that you just want to prevent acpi_pci_root_driver
>> > from binding to the host bridge's struct acpi_device created while we're
>> > walking the ACPI namespace?
>>
>> yes, during hot add path.
>
> Why exactly do you want to prevent that from happening?
>

during adding pci root bus hotplug, Bjorn found some unsafe searching
that caused by pci_bus_add_devices.
pci devices are created during pci scan root, but until very late
acpi_pci_root_start call pci_bus_add_devices.

To fill the gap, we need to move pci_bus_add_devices to acpi_pci_root_add
at first.

but after we move that there, pci device will be added to device tree, and it
will try to bind with acpi devices that should be under acpi pci root,
but are not
created yet. because device_add for acpi_device for acpi pci root is done yet.
it still calling the .add in the acpi_driver aka acpi_pci_root_add.

So I want to hold the driver attach for pci root acpi devices, and
later attach it
until pci devices created.

booting path, all acpi devices get created, and attach driver for them
one by one.
--
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 Wysocki Oct. 4, 2012, 10:36 p.m. UTC | #27
On Thursday 04 of October 2012 15:01:21 Yinghai Lu wrote:
> On Thu, Oct 4, 2012 at 2:53 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday 04 of October 2012 14:31:46 Yinghai Lu wrote:
> >> On Thu, Oct 4, 2012 at 2:23 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >>
> >> >> At last we could remove all acpi_bus_start workaround.
> >> >
> >> > Do I understand correctly that you just want to prevent acpi_pci_root_driver
> >> > from binding to the host bridge's struct acpi_device created while we're
> >> > walking the ACPI namespace?
> >>
> >> yes, during hot add path.
> >
> > Why exactly do you want to prevent that from happening?
> >
> 
> during adding pci root bus hotplug, Bjorn found some unsafe searching
> that caused by pci_bus_add_devices.

Do you have a link to a description of that problem?

> pci devices are created during pci scan root, but until very late
> acpi_pci_root_start call pci_bus_add_devices.

So you mean that pci_bus_add_devices() is called too late, right?

> To fill the gap, we need to move pci_bus_add_devices to acpi_pci_root_add
> at first.
> 
> but after we move that there, pci device will be added to device tree, and it
> will try to bind with acpi devices that should be under acpi pci root,
> but are not
> created yet. because device_add for acpi_device for acpi pci root is done yet.
> it still calling the .add in the acpi_driver aka acpi_pci_root_add.

Quite obviously, we haven't walked the ACPI namespace below the host bridge
object yet at that point.

> So I want to hold the driver attach for pci root acpi devices, and
> later attach it
> until pci devices created.
> 
> booting path, all acpi devices get created, and attach driver for them
> one by one.

I see.

Your patches seem to affect all devices in the ACPI namespace added after
boot, though, not only host bridges.

And the problem seems to be that the scanning of the ACPI namespace and
configuring the host bridge are kind of independent operations now.  What
we should do, actually, seems to be something like this:

(1) Configure the host bridge when discovered (i.e. do what the current
    acpi_pci_root_add() does.
(2) Parse the ACPI namespace under the host bridge (without binding ACPI
    drivers to the struct acpi_device objects created in the process,
    because they are known to correspond to PCI devices).
(3) Run pci_bus_add_devices() for the bridge.

in one routine.

What do you think?

Rafael
Yinghai Lu Oct. 4, 2012, 10:46 p.m. UTC | #28
On Thu, Oct 4, 2012 at 3:36 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday 04 of October 2012 15:01:21 Yinghai Lu wrote:
>> during adding pci root bus hotplug, Bjorn found some unsafe searching
>> that caused by pci_bus_add_devices.
>
> Do you have a link to a description of that problem?

Maybe bjorn could expand it more.

>
>> pci devices are created during pci scan root, but until very late
>> acpi_pci_root_start call pci_bus_add_devices.
>
> So you mean that pci_bus_add_devices() is called too late, right?

yes.

>
>> To fill the gap, we need to move pci_bus_add_devices to acpi_pci_root_add
>> at first.
>>
>> but after we move that there, pci device will be added to device tree, and it
>> will try to bind with acpi devices that should be under acpi pci root,
>> but are not
>> created yet. because device_add for acpi_device for acpi pci root is done yet.
>> it still calling the .add in the acpi_driver aka acpi_pci_root_add.
>
> Quite obviously, we haven't walked the ACPI namespace below the host bridge
> object yet at that point.

yes.

>
>> So I want to hold the driver attach for pci root acpi devices, and
>> later attach it
>> until pci devices created.
>>
>> booting path, all acpi devices get created, and attach driver for them
>> one by one.
>
> I see.
>
> Your patches seem to affect all devices in the ACPI namespace added after
> boot, though, not only host bridges.

yes, but it still should be safe.

>
> And the problem seems to be that the scanning of the ACPI namespace and
> configuring the host bridge are kind of independent operations now.  What
> we should do, actually, seems to be something like this:
>
> (1) Configure the host bridge when discovered (i.e. do what the current
>     acpi_pci_root_add() does.
> (2) Parse the ACPI namespace under the host bridge (without binding ACPI
>     drivers to the struct acpi_device objects created in the process,
>     because they are known to correspond to PCI devices).
> (3) Run pci_bus_add_devices() for the bridge.
>
> in one routine.

problem is still there. if 1 still has acpi_pci_root_add and pci_acpi_scan_root
that scan pci devices. what is need is we need to bind 1 and 3 together.

or we could move pci_scan_root_scan from acpi_pci_root_add to
acpi_pci_root_start?
--
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 Wysocki Oct. 5, 2012, 11:01 p.m. UTC | #29
On Thursday 04 of October 2012 15:46:39 Yinghai Lu wrote:
> On Thu, Oct 4, 2012 at 3:36 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday 04 of October 2012 15:01:21 Yinghai Lu wrote:
> >> during adding pci root bus hotplug, Bjorn found some unsafe searching
> >> that caused by pci_bus_add_devices.
> >
> > Do you have a link to a description of that problem?
> 
> Maybe bjorn could expand it more.
> 
> >
> >> pci devices are created during pci scan root, but until very late
> >> acpi_pci_root_start call pci_bus_add_devices.
> >
> > So you mean that pci_bus_add_devices() is called too late, right?
> 
> yes.
> 
> >
> >> To fill the gap, we need to move pci_bus_add_devices to acpi_pci_root_add
> >> at first.
> >>
> >> but after we move that there, pci device will be added to device tree, and it
> >> will try to bind with acpi devices that should be under acpi pci root,
> >> but are not
> >> created yet. because device_add for acpi_device for acpi pci root is done yet.
> >> it still calling the .add in the acpi_driver aka acpi_pci_root_add.
> >
> > Quite obviously, we haven't walked the ACPI namespace below the host bridge
> > object yet at that point.
> 
> yes.
> 
> >
> >> So I want to hold the driver attach for pci root acpi devices, and
> >> later attach it
> >> until pci devices created.
> >>
> >> booting path, all acpi devices get created, and attach driver for them
> >> one by one.
> >
> > I see.
> >
> > Your patches seem to affect all devices in the ACPI namespace added after
> > boot, though, not only host bridges.
> 
> yes, but it still should be safe.

I'm not really sure of that (what about undock/dock, for exmaple?) and it's
damn ugly.

> > And the problem seems to be that the scanning of the ACPI namespace and
> > configuring the host bridge are kind of independent operations now.  What
> > we should do, actually, seems to be something like this:
> >
> > (1) Configure the host bridge when discovered (i.e. do what the current
> >     acpi_pci_root_add() does.
> > (2) Parse the ACPI namespace under the host bridge (without binding ACPI
> >     drivers to the struct acpi_device objects created in the process,
> >     because they are known to correspond to PCI devices).
> > (3) Run pci_bus_add_devices() for the bridge.
> >
> > in one routine.
> 
> problem is still there. if 1 still has acpi_pci_root_add and pci_acpi_scan_root

OK, so why don't we do (2) in acpi_pci_root_add(), before pci_acpi_scan_root()
is called?

> that scan pci devices. what is need is we need to bind 1 and 3 together.

I don't understand now.  You said previously that we need the ACPI namespace
below the bridge to be scanned before (3), so why do you want to do (3) before
(2) now?

> or we could move pci_scan_root_scan from acpi_pci_root_add to
> acpi_pci_root_start?

No, I don't think so, because we call acpi_pci_bind_root() after that.

Thanks,
Rafael
Yinghai Lu Oct. 5, 2012, 11:10 p.m. UTC | #30
On Fri, Oct 5, 2012 at 4:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday 04 of October 2012 15:46:39 Yinghai Lu wrote:
>> > Your patches seem to affect all devices in the ACPI namespace added after
>> > boot, though, not only host bridges.
>>
>> yes, but it still should be safe.
>
> I'm not really sure of that (what about undock/dock, for exmaple?) and it's
> damn ugly.

only one acpi_driver has .start , that is acpi_pci_root_driver.

should be clean than with .add/start pair.

>
>> > And the problem seems to be that the scanning of the ACPI namespace and
>> > configuring the host bridge are kind of independent operations now.  What
>> > we should do, actually, seems to be something like this:
>> >
>> > (1) Configure the host bridge when discovered (i.e. do what the current
>> >     acpi_pci_root_add() does.
>> > (2) Parse the ACPI namespace under the host bridge (without binding ACPI
>> >     drivers to the struct acpi_device objects created in the process,
>> >     because they are known to correspond to PCI devices).
>> > (3) Run pci_bus_add_devices() for the bridge.
>> >
>> > in one routine.
>>
>> problem is still there. if 1 still has acpi_pci_root_add and pci_acpi_scan_root
>
> OK, so why don't we do (2) in acpi_pci_root_add(), before pci_acpi_scan_root()
> is called?
>
>> that scan pci devices. what is need is we need to bind 1 and 3 together.

some one already walk the acpi space, and during that it create
acpi_device for pci_root
and then attach driver for that, aka acpi_pci_root_add is executing.

Now you want to walking the acpi acpi space to create children devices
before device_add really done for that pci root
acpi device. ?

is that some kind of nesting?

>
> I don't understand now.  You said previously that we need the ACPI namespace
> below the bridge to be scanned before (3), so why do you want to do (3) before
> (2) now?

purpose is calling pci_bus_add_devices in pci_acpi_scan_root.

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 Wysocki Oct. 8, 2012, 8:12 p.m. UTC | #31
On Friday 05 of October 2012 16:10:43 Yinghai Lu wrote:
> On Fri, Oct 5, 2012 at 4:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday 04 of October 2012 15:46:39 Yinghai Lu wrote:
> >> > Your patches seem to affect all devices in the ACPI namespace added after
> >> > boot, though, not only host bridges.
> >>
> >> yes, but it still should be safe.
> >
> > I'm not really sure of that (what about undock/dock, for exmaple?) and it's
> > damn ugly.
> 
> only one acpi_driver has .start , that is acpi_pci_root_driver.
> 
> should be clean than with .add/start pair.
> 
> >
> >> > And the problem seems to be that the scanning of the ACPI namespace and
> >> > configuring the host bridge are kind of independent operations now.  What
> >> > we should do, actually, seems to be something like this:
> >> >
> >> > (1) Configure the host bridge when discovered (i.e. do what the current
> >> >     acpi_pci_root_add() does.
> >> > (2) Parse the ACPI namespace under the host bridge (without binding ACPI
> >> >     drivers to the struct acpi_device objects created in the process,
> >> >     because they are known to correspond to PCI devices).
> >> > (3) Run pci_bus_add_devices() for the bridge.
> >> >
> >> > in one routine.
> >>
> >> problem is still there. if 1 still has acpi_pci_root_add and pci_acpi_scan_root
> >
> > OK, so why don't we do (2) in acpi_pci_root_add(), before pci_acpi_scan_root()
> > is called?
> >
> >> that scan pci devices. what is need is we need to bind 1 and 3 together.
> 
> some one already walk the acpi space, and during that it create
> acpi_device for pci_root
> and then attach driver for that, aka acpi_pci_root_add is executing.
> 
> Now you want to walking the acpi acpi space to create children devices
> before device_add really done for that pci root
> acpi device. ?
> 
> is that some kind of nesting?

Yes, basically.  The idea is to do the scan of the host bridge's children in
the ACPI tree synchronously within acpi_pci_root_add() instead of trying to
delay the execution of it until the children have been scanned (and using
notifiers to kind of trigger the driver binding, i.e. the execution of .add()).

> > I don't understand now.  You said previously that we need the ACPI namespace
> > below the bridge to be scanned before (3), so why do you want to do (3) before
> > (2) now?
> 
> purpose is calling pci_bus_add_devices in pci_acpi_scan_root.

OK, but what's the reason?

Rafael
diff mbox

Patch

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 181ed26..22b826a 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -538,7 +538,7 @@  void bus_probe_device(struct device *dev)
 	if (!bus)
 		return;
 
-	if (bus->p->drivers_autoprobe) {
+	if (bus->p->drivers_autoprobe && dev->drivers_autoprobe) {
 		ret = device_attach(dev);
 		WARN_ON(ret < 0);
 	}
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5e6e00b..8a71f88 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -656,6 +656,7 @@  void device_initialize(struct device *dev)
 	INIT_LIST_HEAD(&dev->devres_head);
 	device_pm_init(dev);
 	set_dev_node(dev, -1);
+	dev->drivers_autoprobe = true;
 }
 
 static struct kobject *virtual_device_parent(struct device *dev)
diff --git a/include/linux/device.h b/include/linux/device.h
index 52a5f15..e08db26 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -647,6 +647,7 @@  struct device {
 	struct bus_type	*bus;		/* type of bus device is on */
 	struct device_driver *driver;	/* which driver has allocated this
 					   device */
+	bool	drivers_autoprobe;
 	void		*platform_data;	/* Platform specific data, device
 					   core doesn't touch it */
 	struct dev_pm_info	power;