Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Yinghai Lu]

[Trim down CC list, otherwise lkml will drop it, Thanks for David
Miller to bring attention to me ]

On Sat, Feb 2, 2013 at 1:50 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Sun, Jan 27, 2013 at 12:23 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> Now we have pci_root_buses list, and there is lots of iteration with
>> list_of_each of it, that is not safe after we add pci root bus hotplug
>> support after booting stage.
>>
>> Add pci_get_next_host_bridge and use bus_find_device in driver core to
>> iterate host bridge and the same time get root bus.
>>
>> We replace searching root bus with searching host_bridge,
>> as host_bridge->bus is the root bus.
>> After those replacing, we even could kill pci_root_buses list.
>
> These are the problems I think you're fixing:
>
> 1) pci_find_next_bus() is not safe because even though it holds
> pci_bus_sem while walking the pci_root_buses list, it doesn't hold a
> reference on the bus it returns.  The bus could be removed while the
> caller is using it.
>
> 2) "list_for_each_entry(bus, &pci_root_buses, node)" is not safe
> because hotplug might modify the pci_root_buses list.  Replacing that
> with for_each_pci_host_bridge() solves that problem by using
> bus_find_device(), which is built on klists, which are designed for
> that problem.
>
> 3) pci_find_next_bus() claims to iterate through all known PCI buses,
> but in fact only iterates through root buses.
>
> So far, so good.  Those are problems we need to fix.
>
> Your solution is to introduce for_each_pci_host_bridge() as an
> iterator through the known host bridges.  There are two scenarios
> where we use something like this:
>
> 1) We want to perform an operation on every known host bridge.
>
> 2) We want to initialize something for every host bridge.
>
> In my opinion, the only instance of scenario 1) is bus_rescan_store(),
> where we want to rescan all PCI host bridges.
>
> In every other case, we're doing some kind of initialization of all
> the host bridges.  For these cases, for_each_pci_host_bridge() is the
> wrong solution because it doesn't work for hot-added bridges.  I think
> these cases should be changed to use pcibios_root_bridge_prepare() or
> something something else called in the host bridge add path.

Yes, will check if those for_pci_host_bridge could be converted to
adding calling in pcibios_root_bridge_prepare one by one.

Thanks

Yinghai

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Yinghai Lu]

On Sat, Feb 2, 2013 at 9:05 PM, Yinghai Lu <yinghai@kernel.org> wrote:

>> Your solution is to introduce for_each_pci_host_bridge() as an
>> iterator through the known host bridges.  There are two scenarios
>> where we use something like this:
>>
>> 1) We want to perform an operation on every known host bridge.
>>
>> 2) We want to initialize something for every host bridge.
>>
>> In my opinion, the only instance of scenario 1) is bus_rescan_store(),
>> where we want to rescan all PCI host bridges.
>>
>> In every other case, we're doing some kind of initialization of all
>> the host bridges.  For these cases, for_each_pci_host_bridge() is the
>> wrong solution because it doesn't work for hot-added bridges.  I think
>> these cases should be changed to use pcibios_root_bridge_prepare() or
>> something something else called in the host bridge add path.
>
> Yes, will check if those for_pci_host_bridge could be converted to
> adding calling in pcibios_root_bridge_prepare one by one.

I went through those patches again, here are findings:
1. run-time iteration includes at least 3 occurrence are addressed.
a. in pci-sysfs::bus_rescan_store
b. powerpc pci_64.c::sys_pciconfig_iobase system call
c. probe.c::pci_create_root_bus/pci_find_bus

2. edac: i7core_edac.c::i7core_pci_lastbus()
this one is not support host bridge hotplug, and in i7core_probe it will
bail-out if probed before overall.
To make edac with i7 to support host-bridge hotadd, will change the structure of
that i7core_probe.
But I'm not worried about that yet, because Nehalem-EX does not support EDAC,
and assume we will NOT have use case with new -EX intel cpu with
hotplug support to use EDAC.

3. pcibios_resource_survey/pcibios_assign_unassigned_resource in __init path.
it is with x86/frv/microblaze/mn10300.
for x86/acpi, we have per root bus calling in pci_root.c::acpi_pci_root_add().
other three does not support pci_root_bridge hotplug yet.

4. looks like all other changes are __init path, and those arch does not support
pci_root_bridge hotplug yet.

So looks like this patch set is ok now.

Also i updated
	git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git
for-pci-for-each-host-bridge

with new ordering of patches.

BTW, I would address unifying pcibios_reserource_survey between
x86/frv/microblaze/mn10300 later.

Thanks

Yinghai

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Bjorn Helgaas]

On Tue, Feb 5, 2013 at 4:55 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Sat, Feb 2, 2013 at 9:05 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>
>>> Your solution is to introduce for_each_pci_host_bridge() as an
>>> iterator through the known host bridges.  There are two scenarios
>>> where we use something like this:
>>>
>>> 1) We want to perform an operation on every known host bridge.
>>>
>>> 2) We want to initialize something for every host bridge.
>>>
>>> In my opinion, the only instance of scenario 1) is bus_rescan_store(),
>>> where we want to rescan all PCI host bridges.
>>>
>>> In every other case, we're doing some kind of initialization of all
>>> the host bridges.  For these cases, for_each_pci_host_bridge() is the
>>> wrong solution because it doesn't work for hot-added bridges.  I think
>>> these cases should be changed to use pcibios_root_bridge_prepare() or
>>> something something else called in the host bridge add path.
>>
>> Yes, will check if those for_pci_host_bridge could be converted to
>> adding calling in pcibios_root_bridge_prepare one by one.
>
> I went through those patches again, here are findings:
> 1. run-time iteration includes at least 3 occurrence are addressed.
> a. in pci-sysfs::bus_rescan_store
> b. powerpc pci_64.c::sys_pciconfig_iobase system call
> c. probe.c::pci_create_root_bus/pci_find_bus
>
> 2. edac: i7core_edac.c::i7core_pci_lastbus()
> this one is not support host bridge hotplug, and in i7core_probe it will
> bail-out if probed before overall.
> To make edac with i7 to support host-bridge hotadd, will change the structure of
> that i7core_probe.
> But I'm not worried about that yet, because Nehalem-EX does not support EDAC,
> and assume we will NOT have use case with new -EX intel cpu with
> hotplug support to use EDAC.
>
> 3. pcibios_resource_survey/pcibios_assign_unassigned_resource in __init path.
> it is with x86/frv/microblaze/mn10300.
> for x86/acpi, we have per root bus calling in pci_root.c::acpi_pci_root_add().
> other three does not support pci_root_bridge hotplug yet.
>
> 4. looks like all other changes are __init path, and those arch does not support
> pci_root_bridge hotplug yet.
>
> So looks like this patch set is ok now.

Maybe.  I'd rather not introduce for_each_pci_host_bridge() at all, if
we can avoid it.  Every place it's used is a place we have to audit to
make sure it's safe.  I think your audit above is correct and
complete, but it relies on way too much architecture knowledge.  It's
better if we can deduce correctness without knowing which arches
support hotplug and which CPUs support EDAC.

As soon as for_each_pci_host_bridge() is in the tree, those uses can
be copied to even more places.  It's a macro, so it's usable by any
module, even out-of-tree ones that we'll never see and can't fix.  So
we won't really have a good way to deprecate and remove it.

Bjorn

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Yinghai Lu]

On Tue, Feb 5, 2013 at 4:19 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> Maybe.  I'd rather not introduce for_each_pci_host_bridge() at all, if
> we can avoid it.  Every place it's used is a place we have to audit to
> make sure it's safe.  I think your audit above is correct and
> complete, but it relies on way too much architecture knowledge.  It's
> better if we can deduce correctness without knowing which arches
> support hotplug and which CPUs support EDAC.
>
> As soon as for_each_pci_host_bridge() is in the tree, those uses can
> be copied to even more places.  It's a macro, so it's usable by any
> module, even out-of-tree ones that we'll never see and can't fix.  So
> we won't really have a good way to deprecate and remove it.

Now we only have two references in modules.

drivers/edac/i7core_edac.c:     for_each_pci_host_bridge(host_bridge) {
drivers/pci/hotplug/sgi_hotplug.c:      for_each_pci_host_bridge(host_bridge) {

for the sgi_hotplug.c, it should be same problem that have for acpiphp
and pciehp.
need to make it support pci host bridge hotplug anyway.

for edac, we need to check Mauro about their plan.

After those two are addressed, we can drop that EXPORT_SYMBOL_GPL for
pci_get_next_host_bridge.

We do have pci_get_domain_bus_and_slot() as export symbol.
So we export pci_get_next_host_bridge should be ok now.
and it would be better than export root buses list.

Thanks

Yinghai

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Mauro Carvalho Chehab]

Em Tue, 5 Feb 2013 16:47:10 -0800
Yinghai Lu <yinghai@kernel.org> escreveu:

> On Tue, Feb 5, 2013 at 4:19 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >
> > Maybe.  I'd rather not introduce for_each_pci_host_bridge() at all, if
> > we can avoid it.  Every place it's used is a place we have to audit to
> > make sure it's safe.  I think your audit above is correct and
> > complete, but it relies on way too much architecture knowledge.  It's
> > better if we can deduce correctness without knowing which arches
> > support hotplug and which CPUs support EDAC.
> >
> > As soon as for_each_pci_host_bridge() is in the tree, those uses can
> > be copied to even more places.  It's a macro, so it's usable by any
> > module, even out-of-tree ones that we'll never see and can't fix.  So
> > we won't really have a good way to deprecate and remove it.
> 
> Now we only have two references in modules.
> 
> drivers/edac/i7core_edac.c:     for_each_pci_host_bridge(host_bridge) {
> drivers/pci/hotplug/sgi_hotplug.c:      for_each_pci_host_bridge(host_bridge) {
> 
> for the sgi_hotplug.c, it should be same problem that have for acpiphp
> and pciehp.
> need to make it support pci host bridge hotplug anyway.
> 
> for edac, we need to check Mauro about their plan.

The i7core_pci_lastbus() code at i7core_edac is there to make it work
with some Nehalem/Nehalem-EP machines that hide the memory controller's
PCI ID by using an artificially low last bus. There's no plan to
support Nehalem-EX, as the information I got is that there's no way
to access the memory controller registers on it without interfering with
BIOS.

It should be noticed, however, that I got a report of a Sandy Bridge
server that is also hiding the memory controller's PCI address.
So, maybe we'll need to add later some logic at sb_edac to unhide the
memory controller, just like the one in i7core_edac.

> 
> After those two are addressed, we can drop that EXPORT_SYMBOL_GPL for
> pci_get_next_host_bridge.
> 
> We do have pci_get_domain_bus_and_slot() as export symbol.
> So we export pci_get_next_host_bridge should be ok now.
> and it would be better than export root buses list.
> 
> Thanks
> 
> Yinghai


-- 

Cheers,
Mauro

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Bjorn Helgaas]

On Wed, Feb 6, 2013 at 1:53 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em Tue, 5 Feb 2013 16:47:10 -0800
> Yinghai Lu <yinghai@kernel.org> escreveu:
>
>> On Tue, Feb 5, 2013 at 4:19 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> >
>> > Maybe.  I'd rather not introduce for_each_pci_host_bridge() at all, if
>> > we can avoid it.  Every place it's used is a place we have to audit to
>> > make sure it's safe.  I think your audit above is correct and
>> > complete, but it relies on way too much architecture knowledge.  It's
>> > better if we can deduce correctness without knowing which arches
>> > support hotplug and which CPUs support EDAC.
>> >
>> > As soon as for_each_pci_host_bridge() is in the tree, those uses can
>> > be copied to even more places.  It's a macro, so it's usable by any
>> > module, even out-of-tree ones that we'll never see and can't fix.  So
>> > we won't really have a good way to deprecate and remove it.
>>
>> Now we only have two references in modules.
>>
>> drivers/edac/i7core_edac.c:     for_each_pci_host_bridge(host_bridge) {
>> drivers/pci/hotplug/sgi_hotplug.c:      for_each_pci_host_bridge(host_bridge) {
>>
>> for the sgi_hotplug.c, it should be same problem that have for acpiphp
>> and pciehp.
>> need to make it support pci host bridge hotplug anyway.
>>
>> for edac, we need to check Mauro about their plan.
>
> The i7core_pci_lastbus() code at i7core_edac is there to make it work
> with some Nehalem/Nehalem-EP machines that hide the memory controller's
> PCI ID by using an artificially low last bus.

I don't really understand how this helps.  An example would probably
make it clearer.

i7core_edac.c has some very creative use of PCI infrastructure.
Normally a driver has a pci_device_id table that identifies the
vendor/device IDs of the devices it cares about, and the driver's
.probe() method is called for every matching device.

But i7core_edac only has two entries in its id_table.  When we find a
device that matches one of those two entries, we call i7core_probe(),
which then gropes around for all the *other* devices related to that
original one.  This is a bit messy.

I'd like it a lot better if the device IDs in
pci_dev_descr_i7core_nehalem[], pci_dev_descr_lynnfield[], etc., were
just in the pci_device_id table directly.  Then i7core_probe() would
be called directly for every device you care about, and you could sort
them out there.  That should work without any need for
pci_get_device(), i7core_pci_lastbus(), etc.

Bjorn

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Mauro Carvalho Chehab]

Em Wed, 6 Feb 2013 10:45:16 -0700
Bjorn Helgaas <bhelgaas@google.com> escreveu:

> On Wed, Feb 6, 2013 at 1:53 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
> > Em Tue, 5 Feb 2013 16:47:10 -0800
> > Yinghai Lu <yinghai@kernel.org> escreveu:
> >
> >> On Tue, Feb 5, 2013 at 4:19 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> >
> >> > Maybe.  I'd rather not introduce for_each_pci_host_bridge() at all, if
> >> > we can avoid it.  Every place it's used is a place we have to audit to
> >> > make sure it's safe.  I think your audit above is correct and
> >> > complete, but it relies on way too much architecture knowledge.  It's
> >> > better if we can deduce correctness without knowing which arches
> >> > support hotplug and which CPUs support EDAC.
> >> >
> >> > As soon as for_each_pci_host_bridge() is in the tree, those uses can
> >> > be copied to even more places.  It's a macro, so it's usable by any
> >> > module, even out-of-tree ones that we'll never see and can't fix.  So
> >> > we won't really have a good way to deprecate and remove it.
> >>
> >> Now we only have two references in modules.
> >>
> >> drivers/edac/i7core_edac.c:     for_each_pci_host_bridge(host_bridge) {
> >> drivers/pci/hotplug/sgi_hotplug.c:      for_each_pci_host_bridge(host_bridge) {
> >>
> >> for the sgi_hotplug.c, it should be same problem that have for acpiphp
> >> and pciehp.
> >> need to make it support pci host bridge hotplug anyway.
> >>
> >> for edac, we need to check Mauro about their plan.
> >
> > The i7core_pci_lastbus() code at i7core_edac is there to make it work
> > with some Nehalem/Nehalem-EP machines that hide the memory controller's
> > PCI ID by using an artificially low last bus.
> 
> I don't really understand how this helps.  An example would probably
> make it clearer.
> 
> i7core_edac.c has some very creative use of PCI infrastructure.
> Normally a driver has a pci_device_id table that identifies the
> vendor/device IDs of the devices it cares about, and the driver's
> .probe() method is called for every matching device.
> 
> But i7core_edac only has two entries in its id_table.  When we find a
> device that matches one of those two entries, we call i7core_probe(),
> which then gropes around for all the *other* devices related to that
> original one.  This is a bit messy.
> 
> I'd like it a lot better if the device IDs in
> pci_dev_descr_i7core_nehalem[], pci_dev_descr_lynnfield[], etc., were
> just in the pci_device_id table directly.  Then i7core_probe() would
> be called directly for every device you care about, and you could sort
> them out there.  That should work without any need for
> pci_get_device(), i7core_pci_lastbus(), etc.


Bjorn,

On almost all Intel memory controllers since 2002, the memory controller
handling function were split into several different PCI devices and
PCI functions. So, for example, even if you look on old driver like 
i5000_edac.c, you'll see 5 different PCI IDs that are required to
control a single device:

#ifndef PCI_DEVICE_ID_INTEL_FBD_0
#define PCI_DEVICE_ID_INTEL_FBD_0	0x25F5
#endif
#ifndef PCI_DEVICE_ID_INTEL_FBD_1
#define PCI_DEVICE_ID_INTEL_FBD_1	0x25F6
#endif

/* Device 16,
 * Function 0: System Address
 * Function 1: Memory Branch Map, Control, Errors Register
 * Function 2: FSB Error Registers
 *
 * All 3 functions of Device 16 (0,1,2) share the SAME DID
 */
#define	PCI_DEVICE_ID_INTEL_I5000_DEV16	0x25F0

/* OFFSETS for Function 1 */

(a long list of registers there)

/*
 * Device 21,
 * Function 0: Memory Map Branch 0
 *
 * Device 22,
 * Function 0: Memory Map Branch 1
 */
#define PCI_DEVICE_ID_I5000_BRANCH_0	0x25F5
#define PCI_DEVICE_ID_I5000_BRANCH_1	0x25F6

(another long list or registers there)

I've no idea why the hardware engineers there decided on that way, 
but the number of different PCI devices required for the functionality 
to work has been increased on their newer chipsets. At the Sandy
Bridge driver (sb_edac.c), all those PCI devices need to be opened
at the same time, in order to allow controlling a single memory
controller (and one system have one memory controller per socket):

#define PCI_DEVICE_ID_INTEL_SBRIDGE_SAD0	0x3cf4	/* 12.6 */
#define PCI_DEVICE_ID_INTEL_SBRIDGE_SAD1	0x3cf6	/* 12.7 */
#define PCI_DEVICE_ID_INTEL_SBRIDGE_BR		0x3cf5	/* 13.6 */
#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_HA0	0x3ca0	/* 14.0 */
#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TA	0x3ca8	/* 15.0 */
#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_RAS	0x3c71	/* 15.1 */
#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD0	0x3caa	/* 15.2 */
#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD1	0x3cab	/* 15.3 */
#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD2	0x3cac	/* 15.4 */
#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD3	0x3cad	/* 15.5 */
#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_DDRIO	0x3cb8	/* 17.0 */

The first thing that any EDAC driver needs to do is to get the memory
configuration (number of DIMMs, channels filled, DIMM size, etc).
In the case of sb_edac, the logic is at get_dimm_config(). It needs
to read data on _several_ of the above PCI IDs:

static int get_dimm_config(struct mem_ctl_info *mci)
{
...
	pci_read_config_dword(pvt->pci_br, SAD_TARGET, &reg);		// reads from PCI_DEVICE_ID_INTEL_SBRIDGE_BR
...
	pci_read_config_dword(pvt->pci_br, SAD_CONTROL, &reg);		// reads from PCI_DEVICE_ID_INTEL_SBRIDGE_BR
...
	pci_read_config_dword(pvt->pci_ras, RASENABLES, &reg);		// reads from PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_RAS
...
	pci_read_config_dword(pvt->pci_ta, MCMTR, &pvt->info.mcmtr);	// reads from PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TA
...
	pci_read_config_dword(pvt->pci_ddrio, RANK_CFG_A, &reg);	// reads from PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_DDRIO
...
	for (i = 0; i < NUM_CHANNELS; i++) {
...
		for (j = 0; j < ARRAY_SIZE(mtr_regs); j++) {
...
			pci_read_config_dword(pvt->pci_tad[i],
					      mtr_regs[j], &mtr);	// PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD0 to PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TAD3

So, while the usual design of having one PCI ID entry for each device
works for the vast majority of drivers, as each different PCI ID/function
are typically independent and pci_driver.probe() can be called for every
entry inside the PCI ID table, that's not the case of EDAC.

On EDAC drivers, the probing routine can be called only after calling
pci_get_device() for the entire set of PCI devices that belongs to the
memory controller. 

To make things worse, the PCI IDs for the memory controllers are sometimes
after PCI lastbus.

So, the logic used on almost all drivers there is to use one PCI ID "detect"
device at the table, used to discover if the system has a supported memory
controller chipset. If it matches, the pci_driver.probe() will seek for the
actual PCI devices that are required for it to work, with may or may not be
the same as the "detect" device.

Also, the highest bus corresponds to the first memory controller. So,
bus=255 matches the memory controller for the CPU socket 0,
bus=254 matches the one for CPU socket 1 and so on. That forced the driver
to probe all devices at the same time, on all CPU sockets, in order to reverse
the order when initializing the memory controller EDAC structures.

There are some other odd details there... In the case of i7core_edac, it 
supports 3 different versions of memory controllers; each version has its 
own set of PCI ID's. So, its "real" PCI ID table set has 3 entries:

static const struct pci_id_table pci_dev_table[] = {
	PCI_ID_TABLE_ENTRY(pci_dev_descr_i7core_nehalem),
	PCI_ID_TABLE_ENTRY(pci_dev_descr_lynnfield),
	PCI_ID_TABLE_ENTRY(pci_dev_descr_i7core_westmere),
	{0,}			/* 0 terminated list. */
};

while its PCI ID "detect" table has only two, as the PCI device 8086:342e
(PCI_DEVICE_ID_INTEL_X58_HUB_MGMT) is found on both Nehalem and Westmere:

static DEFINE_PCI_DEVICE_TABLE(i7core_pci_tbl) = {
	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_X58_HUB_MGMT)},
	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LYNNFIELD_QPI_LINK0)},
	{0,}			/* 0 terminated list. */

-- 

Cheers,
Mauro

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Bjorn Helgaas]

On Tue, Feb 5, 2013 at 5:47 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Feb 5, 2013 at 4:19 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>
>> Maybe.  I'd rather not introduce for_each_pci_host_bridge() at all, if
>> we can avoid it.  Every place it's used is a place we have to audit to
>> make sure it's safe.  I think your audit above is correct and
>> complete, but it relies on way too much architecture knowledge.  It's
>> better if we can deduce correctness without knowing which arches
>> support hotplug and which CPUs support EDAC.
>>
>> As soon as for_each_pci_host_bridge() is in the tree, those uses can
>> be copied to even more places.  It's a macro, so it's usable by any
>> module, even out-of-tree ones that we'll never see and can't fix.  So
>> we won't really have a good way to deprecate and remove it.
>
> Now we only have two references in modules.
>
> drivers/edac/i7core_edac.c:     for_each_pci_host_bridge(host_bridge) {
> drivers/pci/hotplug/sgi_hotplug.c:      for_each_pci_host_bridge(host_bridge) {
>
> for the sgi_hotplug.c, it should be same problem that have for acpiphp
> and pciehp.
> need to make it support pci host bridge hotplug anyway.
>
> for edac, we need to check Mauro about their plan.
>
> After those two are addressed, we can drop that EXPORT_SYMBOL_GPL for
> pci_get_next_host_bridge.
>
> We do have pci_get_domain_bus_and_slot() as export symbol.
> So we export pci_get_next_host_bridge should be ok now.
> and it would be better than export root buses list.

I think you're missing the point.

Search the tree for uses of "for_each_pci_dev()."  Almost every
occurrence is a bug because that code doesn't work correctly for
hot-added devices.  That code gets run for devices that are present at
boot, but not for devices hot-added later.

You're proposing to add "for_each_pci_host_bridge()."  That will have
the exact same problem as for_each_pci_dev() already does.  Code that
uses it will work for host bridges present at boot, but not for
bridges hot-added later.

Why would we want to add an interface when every use of that interface
will be a design bug?  I think we should fix the existing users of
pci_root_buses by changing their designs so they will work correctly
with host bridge hotplug.

Bjorn

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Yinghai Lu]

On Wed, Feb 6, 2013 at 9:54 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> I think you're missing the point.
>
> Search the tree for uses of "for_each_pci_dev()."  Almost every
> occurrence is a bug because that code doesn't work correctly for
> hot-added devices.  That code gets run for devices that are present at
> boot, but not for devices hot-added later.
>
> You're proposing to add "for_each_pci_host_bridge()."  That will have
> the exact same problem as for_each_pci_dev() already does.  Code that
> uses it will work for host bridges present at boot, but not for
> bridges hot-added later.
>
> Why would we want to add an interface when every use of that interface
> will be a design bug?  I think we should fix the existing users of
> pci_root_buses by changing their designs so they will work correctly
> with host bridge hotplug.

I'm a little confused about what you want.

In boot stage using for_each_pci_host_bridge or pci_root_buses is fine.

For those cases that it should support host-bridge by nature.
there are two solutions:
1. use for_each_pci_host_bridge, and it is hotplug-safe.
and make sgi_hotplug to use acpi_pci_driver interface.
and acpi_pci_root_add() will call .add in the acpi_pci_driver.

2. make them all to be built-in, and  those acpi_pci_driver should be registered
much early before acpi_pci_root_add is called.
then we don't need to call for_each_host_bridge for it.

So difference between them:
1. still keep the module support, and register acpi_pci_driver later.
2. built-in support only, and need to register acpi_pci_driver early.

Please let me which one you like.

Thanks

Yinghai

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Bjorn Helgaas]

On Wed, Feb 6, 2013 at 11:59 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Wed, Feb 6, 2013 at 9:54 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> I think you're missing the point.
>>
>> Search the tree for uses of "for_each_pci_dev()."  Almost every
>> occurrence is a bug because that code doesn't work correctly for
>> hot-added devices.  That code gets run for devices that are present at
>> boot, but not for devices hot-added later.
>>
>> You're proposing to add "for_each_pci_host_bridge()."  That will have
>> the exact same problem as for_each_pci_dev() already does.  Code that
>> uses it will work for host bridges present at boot, but not for
>> bridges hot-added later.
>>
>> Why would we want to add an interface when every use of that interface
>> will be a design bug?  I think we should fix the existing users of
>> pci_root_buses by changing their designs so they will work correctly
>> with host bridge hotplug.
>
> I'm a little confused about what you want.
>
> In boot stage using for_each_pci_host_bridge or pci_root_buses is fine.

No, it's not.  Boot-time should work exactly the same way as hot-add
time, unless there's a reason it can't.  There might be corner cases
where we can't do that yet, e.g., the pcibios_resource_survey stuff,
but in general I don't think there's anything that *forces* us to
iterate through host bridges at boot-time.

> For those cases that it should support host-bridge by nature.
> there are two solutions:
> 1. use for_each_pci_host_bridge, and it is hotplug-safe.

I'm trying to tell you that for_each_pci_host_bridge() is NOT
hotplug-safe.  Your series makes it safer than it was, in the sense
that it probably fixes stray pointer references when a host bridge
hotplug happens while somebody's traversing pci_root_buses.  But the
whole point of for_each_pci_host_bridge() is to run some code for
every bridge we know about *right now*.  If a host bridge is added
right after the for_each_pci_host_bridge() loop exits, that code
doesn't get run.  In most cases, that's a bug.  The only exception I
know about is the /sys/.../rescan interface.

> and make sgi_hotplug to use acpi_pci_driver interface.
> and acpi_pci_root_add() will call .add in the acpi_pci_driver.
>
> 2. make them all to be built-in, and  those acpi_pci_driver should be registered
> much early before acpi_pci_root_add is called.
> then we don't need to call for_each_host_bridge for it.
>
> So difference between them:
> 1. still keep the module support, and register acpi_pci_driver later.
> 2. built-in support only, and need to register acpi_pci_driver early.

acpi_pci_driver is going away, so I don't want to add any more uses of
it.  Obviously it's only relevant to x86 and ia64 anyway.

What I'd like is a change where for_each_pci_host_bridge() is used
only inside the PCI core and defined somewhere like drivers/pci/pci.h
so it's not even available outside.

The other uses should be changed so they use
pcibios_root_bridge_prepare() or something similar.  That way, it will
be obvious that the code supports hot-added bridges, and when it gets
copied to other places, we'll be copying a working design instead of a
broken one.

I don't want to have to audit these places and figure out "this is
safe because this arch doesn't support host bridge hotplug" or "this
is safe because this CPU doesn't support XYZ."  That's not a
maintainable solution.  The safety should be apparent from the code
itself, without requiring extra platform-specific knowledge.

Bjorn

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Yinghai Lu]

On Wed, Feb 6, 2013 at 12:50 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Feb 6, 2013 at 11:59 AM, Yinghai Lu <yinghai@kernel.org> wrote:
>> On Wed, Feb 6, 2013 at 9:54 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> I think you're missing the point.
>>>
>>> Search the tree for uses of "for_each_pci_dev()."  Almost every
>>> occurrence is a bug because that code doesn't work correctly for
>>> hot-added devices.  That code gets run for devices that are present at
>>> boot, but not for devices hot-added later.
>>>
>>> You're proposing to add "for_each_pci_host_bridge()."  That will have
>>> the exact same problem as for_each_pci_dev() already does.  Code that
>>> uses it will work for host bridges present at boot, but not for
>>> bridges hot-added later.
>>>
>>> Why would we want to add an interface when every use of that interface
>>> will be a design bug?  I think we should fix the existing users of
>>> pci_root_buses by changing their designs so they will work correctly
>>> with host bridge hotplug.
>>
>> I'm a little confused about what you want.
>>
>> In boot stage using for_each_pci_host_bridge or pci_root_buses is fine.
>
> No, it's not.  Boot-time should work exactly the same way as hot-add
> time, unless there's a reason it can't.  There might be corner cases
> where we can't do that yet, e.g., the pcibios_resource_survey stuff,
> but in general I don't think there's anything that *forces* us to
> iterate through host bridges at boot-time.
>
>> For those cases that it should support host-bridge by nature.
>> there are two solutions:
>> 1. use for_each_pci_host_bridge, and it is hotplug-safe.
>
> I'm trying to tell you that for_each_pci_host_bridge() is NOT
> hotplug-safe.  Your series makes it safer than it was, in the sense
> that it probably fixes stray pointer references when a host bridge
> hotplug happens while somebody's traversing pci_root_buses.  But the
> whole point of for_each_pci_host_bridge() is to run some code for
> every bridge we know about *right now*.  If a host bridge is added
> right after the for_each_pci_host_bridge() loop exits, that code
> doesn't get run.  In most cases, that's a bug.  The only exception I
> know about is the /sys/.../rescan interface.
>
>> and make sgi_hotplug to use acpi_pci_driver interface.
>> and acpi_pci_root_add() will call .add in the acpi_pci_driver.
>>
>> 2. make them all to be built-in, and  those acpi_pci_driver should be registered
>> much early before acpi_pci_root_add is called.
>> then we don't need to call for_each_host_bridge for it.
>>
>> So difference between them:
>> 1. still keep the module support, and register acpi_pci_driver later.
>> 2. built-in support only, and need to register acpi_pci_driver early.
>
> acpi_pci_driver is going away, so I don't want to add any more uses of
> it.  Obviously it's only relevant to x86 and ia64 anyway.

when?

I have ioapic and iommu hotplug patch set that need to use them.

>
> What I'd like is a change where for_each_pci_host_bridge() is used
> only inside the PCI core and defined somewhere like drivers/pci/pci.h
> so it's not even available outside.
>
> The other uses should be changed so they use
> pcibios_root_bridge_prepare() or something similar.  That way, it will
> be obvious that the code supports hot-added bridges, and when it gets
> copied to other places, we'll be copying a working design instead of a
> broken one.
>
> I don't want to have to audit these places and figure out "this is
> safe because this arch doesn't support host bridge hotplug" or "this
> is safe because this CPU doesn't support XYZ."  That's not a
> maintainable solution.  The safety should be apparent from the code
> itself, without requiring extra platform-specific knowledge.

so you still did not answer you want 1 or 2 yet:

for sgi_hotplug,

1. still keep the module support, and register acpi_pci_driver later.
2. built-in support only, and need to register acpi_pci_driver early.

Thanks

Yinghai

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Rafael J. Wysocki]

On Wednesday, February 06, 2013 01:28:27 PM Yinghai Lu wrote:
> On Wed, Feb 6, 2013 at 12:50 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Wed, Feb 6, 2013 at 11:59 AM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> On Wed, Feb 6, 2013 at 9:54 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >>> I think you're missing the point.
> >>>
> >>> Search the tree for uses of "for_each_pci_dev()."  Almost every
> >>> occurrence is a bug because that code doesn't work correctly for
> >>> hot-added devices.  That code gets run for devices that are present at
> >>> boot, but not for devices hot-added later.
> >>>
> >>> You're proposing to add "for_each_pci_host_bridge()."  That will have
> >>> the exact same problem as for_each_pci_dev() already does.  Code that
> >>> uses it will work for host bridges present at boot, but not for
> >>> bridges hot-added later.
> >>>
> >>> Why would we want to add an interface when every use of that interface
> >>> will be a design bug?  I think we should fix the existing users of
> >>> pci_root_buses by changing their designs so they will work correctly
> >>> with host bridge hotplug.
> >>
> >> I'm a little confused about what you want.
> >>
> >> In boot stage using for_each_pci_host_bridge or pci_root_buses is fine.
> >
> > No, it's not.  Boot-time should work exactly the same way as hot-add
> > time, unless there's a reason it can't.  There might be corner cases
> > where we can't do that yet, e.g., the pcibios_resource_survey stuff,
> > but in general I don't think there's anything that *forces* us to
> > iterate through host bridges at boot-time.
> >
> >> For those cases that it should support host-bridge by nature.
> >> there are two solutions:
> >> 1. use for_each_pci_host_bridge, and it is hotplug-safe.
> >
> > I'm trying to tell you that for_each_pci_host_bridge() is NOT
> > hotplug-safe.  Your series makes it safer than it was, in the sense
> > that it probably fixes stray pointer references when a host bridge
> > hotplug happens while somebody's traversing pci_root_buses.  But the
> > whole point of for_each_pci_host_bridge() is to run some code for
> > every bridge we know about *right now*.  If a host bridge is added
> > right after the for_each_pci_host_bridge() loop exits, that code
> > doesn't get run.  In most cases, that's a bug.  The only exception I
> > know about is the /sys/.../rescan interface.
> >
> >> and make sgi_hotplug to use acpi_pci_driver interface.
> >> and acpi_pci_root_add() will call .add in the acpi_pci_driver.
> >>
> >> 2. make them all to be built-in, and  those acpi_pci_driver should be registered
> >> much early before acpi_pci_root_add is called.
> >> then we don't need to call for_each_host_bridge for it.
> >>
> >> So difference between them:
> >> 1. still keep the module support, and register acpi_pci_driver later.
> >> 2. built-in support only, and need to register acpi_pci_driver early.
> >
> > acpi_pci_driver is going away, so I don't want to add any more uses of
> > it.  Obviously it's only relevant to x86 and ia64 anyway.
> 
> when?
> 
> I have ioapic and iommu hotplug patch set that need to use them.

I suppose it would be a bit simpler if you tried to fix things up before
adding more stuff on top of them, which only makes it more difficult
to clean them up when this additional stuff is applied.

A patchset to remove acpi_pci_driver has been posted already and it is going
to be submitted again after 3.9-rc1.  So this is the time frame.

> > What I'd like is a change where for_each_pci_host_bridge() is used
> > only inside the PCI core and defined somewhere like drivers/pci/pci.h
> > so it's not even available outside.
> >
> > The other uses should be changed so they use
> > pcibios_root_bridge_prepare() or something similar.  That way, it will
> > be obvious that the code supports hot-added bridges, and when it gets
> > copied to other places, we'll be copying a working design instead of a
> > broken one.
> >
> > I don't want to have to audit these places and figure out "this is
> > safe because this arch doesn't support host bridge hotplug" or "this
> > is safe because this CPU doesn't support XYZ."  That's not a
> > maintainable solution.  The safety should be apparent from the code
> > itself, without requiring extra platform-specific knowledge.
> 
> so you still did not answer you want 1 or 2 yet:
> 
> for sgi_hotplug,
> 
> 1. still keep the module support, and register acpi_pci_driver later.
> 2. built-in support only, and need to register acpi_pci_driver early.

Please work with the assumption that acpi_pci_driver is not going to be there
any more.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Yinghai Lu]

On Wed, Feb 6, 2013 at 1:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, February 06, 2013 01:28:27 PM Yinghai Lu wrote:
>> so you still did not answer you want 1 or 2 yet:
>>
>> for sgi_hotplug,
>>
>> 1. still keep the module support, and register acpi_pci_driver later.
>> 2. built-in support only, and need to register acpi_pci_driver early.
>
> Please work with the assumption that acpi_pci_driver is not going to be there
> any more.
>

I think I could change ioapic and iommu hotplug to weak add/remove because they
should be built-in by nature.

but how about others like sgi_hotplug etc?

Thanks

Yinghai

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Rafael J. Wysocki]

On Wednesday, February 06, 2013 01:53:50 PM Yinghai Lu wrote:
> On Wed, Feb 6, 2013 at 1:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Wednesday, February 06, 2013 01:28:27 PM Yinghai Lu wrote:
> >> so you still did not answer you want 1 or 2 yet:
> >>
> >> for sgi_hotplug,
> >>
> >> 1. still keep the module support, and register acpi_pci_driver later.
> >> 2. built-in support only, and need to register acpi_pci_driver early.
> >
> > Please work with the assumption that acpi_pci_driver is not going to be there
> > any more.
> >
> 
> I think I could change ioapic and iommu hotplug to weak add/remove because they
> should be built-in by nature.
> 
> but how about others like sgi_hotplug etc?

I'd really prefer to wait for the patchset removing acpi_pci_driver (from
Myron) before proceeding with more changes in that area.

Myron, do you have a prototype based on the current linux-next?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Bjorn Helgaas]

On Wed, Feb 6, 2013 at 3:05 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Wednesday, February 06, 2013 01:53:50 PM Yinghai Lu wrote:
>> On Wed, Feb 6, 2013 at 1:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Wednesday, February 06, 2013 01:28:27 PM Yinghai Lu wrote:
>> >> so you still did not answer you want 1 or 2 yet:
>> >>
>> >> for sgi_hotplug,
>> >>
>> >> 1. still keep the module support, and register acpi_pci_driver later.
>> >> 2. built-in support only, and need to register acpi_pci_driver early.
>> >
>> > Please work with the assumption that acpi_pci_driver is not going to be there
>> > any more.
>> >
>>
>> I think I could change ioapic and iommu hotplug to weak add/remove because they
>> should be built-in by nature.
>>
>> but how about others like sgi_hotplug etc?

I think that could be handled with pcibios_root_bridge_prepare() or
something similar -- something that happens in the "add host bridge"
path.  I'm not saying we necessarily have all the right hooks in place
yet.  It's just that I'd rather figure out what the right hooks *are*
and add them if necessary, than just convert pci_root_buses to
for_each_pci_host_bridge().

> I'd really prefer to wait for the patchset removing acpi_pci_driver (from
> Myron) before proceeding with more changes in that area.
>
> Myron, do you have a prototype based on the current linux-next?

I think it's really the acpiphp/pciehp issue that's holding up the
removal of acpi_pci_driver.  I'm not sure if anybody's working on
that.

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Yinghai Lu]

On Wed, Feb 6, 2013 at 3:02 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Feb 6, 2013 at 3:05 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Wednesday, February 06, 2013 01:53:50 PM Yinghai Lu wrote:
>>> On Wed, Feb 6, 2013 at 1:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>> > On Wednesday, February 06, 2013 01:28:27 PM Yinghai Lu wrote:
>>> >> so you still did not answer you want 1 or 2 yet:
>>> >>
>>> >> for sgi_hotplug,
>>> >>
>>> >> 1. still keep the module support, and register acpi_pci_driver later.
>>> >> 2. built-in support only, and need to register acpi_pci_driver early.
>>> >
>>> > Please work with the assumption that acpi_pci_driver is not going to be there
>>> > any more.
>>> >
>>>
>>> I think I could change ioapic and iommu hotplug to weak add/remove because they
>>> should be built-in by nature.
>>>
>>> but how about others like sgi_hotplug etc?
>
> I think that could be handled with pcibios_root_bridge_prepare() or
> something similar -- something that happens in the "add host bridge"
> path.  I'm not saying we necessarily have all the right hooks in place
> yet.  It's just that I'd rather figure out what the right hooks *are*
> and add them if necessary, than just convert pci_root_buses to
> for_each_pci_host_bridge().

so we will have all built-in for apci_pci_driver and call add/remove directly?

BTW, looks like sgi_hotplug may need some time to convert ...
Also not sure who can test it.

>
>> I'd really prefer to wait for the patchset removing acpi_pci_driver (from
>> Myron) before proceeding with more changes in that area.
>>
>> Myron, do you have a prototype based on the current linux-next?
>
> I think it's really the acpiphp/pciehp issue that's holding up the
> removal of acpi_pci_driver.  I'm not sure if anybody's working on
> that.

Myron's last Dec's version would split all acpi_pci_driver's
add/remove into acpi_pci_root_add/remove.

other way is use host-bridge nodifier, looks like that is not favorable.

BTW, looks like I could put add/remove calling directly in
acpi_pci_root_add/remove
for ioapic and iommu.

Thanks

Yinghai

Re: [PATCH v3 00/22] PCI: Iterate pci host bridge instead of pci root bus

[Jiang Liu]

On 2013-2-7 7:02, Bjorn Helgaas wrote:
> On Wed, Feb 6, 2013 at 3:05 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> On Wednesday, February 06, 2013 01:53:50 PM Yinghai Lu wrote:
>>> On Wed, Feb 6, 2013 at 1:43 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>>>> On Wednesday, February 06, 2013 01:28:27 PM Yinghai Lu wrote:
>>>>> so you still did not answer you want 1 or 2 yet:
>>>>>
>>>>> for sgi_hotplug,
>>>>>
>>>>> 1. still keep the module support, and register acpi_pci_driver later.
>>>>> 2. built-in support only, and need to register acpi_pci_driver early.
>>>>
>>>> Please work with the assumption that acpi_pci_driver is not going to be there
>>>> any more.
>>>>
>>>
>>> I think I could change ioapic and iommu hotplug to weak add/remove because they
>>> should be built-in by nature.
>>>
>>> but how about others like sgi_hotplug etc?
> 
> I think that could be handled with pcibios_root_bridge_prepare() or
> something similar -- something that happens in the "add host bridge"
> path.  I'm not saying we necessarily have all the right hooks in place
> yet.  It's just that I'd rather figure out what the right hooks *are*
> and add them if necessary, than just convert pci_root_buses to
> for_each_pci_host_bridge().
> 
>> I'd really prefer to wait for the patchset removing acpi_pci_driver (from
>> Myron) before proceeding with more changes in that area.
>>
>> Myron, do you have a prototype based on the current linux-next?
> 
> I think it's really the acpiphp/pciehp issue that's holding up the
> removal of acpi_pci_driver.  I'm not sure if anybody's working on
> that.
Hi Bjorn,
	I'm working on this topic, but a little busy with other stuff
in last few days. I guess I could should out a version in coming days.
	Thanks!

> 
> .
>