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@xxxxxxxxxx> wrote:
> On Sat, Feb 2, 2013 at 9:05 PM, Yinghai Lu <yinghai@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/