Re: [PATCH v3, part2 01/20] PCI: introduce hotplug-safe PCI bus iterators

[Bjorn Helgaas]

On Thu, Jun 20, 2013 at 10:18 AM, Jiang Liu <liuj97@xxxxxxxxx> wrote:
> On 06/18/2013 04:06 AM, Bjorn Helgaas wrote:
>> On Sun, May 26, 2013 at 11:52:58PM +0800, Jiang Liu wrote:
>>> Introduce hotplug-safe PCI bus iterators as below, which hold a
>>> reference on the returned PCI bus object.
>>> bool pci_bus_exists(int domain, int busnr);
>>> struct pci_bus *pci_get_bus(int domain, int busnr);
>>> struct pci_bus *pci_get_next_bus(struct pci_bus *from);
>>> struct pci_bus *pci_get_next_root_bus(struct pci_bus *from);
>>> #define for_each_pci_bus(b) for (b = NULL; (b = pci_get_next_bus(b)); )
>>> #define for_each_pci_root_bus(b)  \
>>>              for (b = NULL; (b = pci_get_next_root_bus(b)); )
>>>
>>> The long-term goal is to remove hotplug-unsafe pci_find_bus(),
>>> pci_find_next_bus() and the global pci_root_buses list.
>>
>> I think you should mark the unsafe interfaces as __deprecated so
>> users will get compile-time warnings.
>>
>> I don't think pci_bus_exists() is a safe interface, because the value
>> it returns may be incorrect before the caller can look at it.  The
>> only safe thing would be to make it so we try to create the bus
>> and return failure if it already exists.  Then the mutex can be in
>> the code that creates the bus.
>>
>> I don't see any uses of for_each_pci_bus(), so please remove that.
>>
>> It sounds like we don't have a consensus on how to iterate over
>> PCI root buses.  If you separate that from the pci_get_bus()
>> changes, maybe we can at least move forward on the pci_get_bus()
>> stuff.
> Hi Bjorn and Yinghai,
>     I have thought about the way to implement pci_for_each_root_bus()
> again. And there are several possible ways here:
> 1) Yinghai has a patch set implementing an iterator for PCI host
> bridges, but we can't safely refer the PCI root bus associated with a
> host bridge device because the host bridge doesn't hold a reference to
> associated PCI root bus. So we need to find a safe way to refer the PCI
> root bus associated with a PCI host bridge.
> 2) Unexport pci_root_buses and convert it to klist, then we could walk
> all root buses effectively. This solution is straight-forward, but it
> may break out of tree drivers.
> 3) Keep current implementation, which does waste some computation cycles:(
>
> So what's your thoughts about above solutions? Or any other suggestions?
> Regards!
> Gerry

Iteration is generally the wrong approach because it doesn't fit well
with hot plug.  I recognize that it may be impossible to fix all the
places that currently iterate over root buses, so we may have to
accept iteration at least in the short term.

Sometimes when we fix the things we *know* how to fix, it makes it
easier to see how to fix the hard problem.  Your pci_bus ref counting
fixes seem like a clear step forward, and I'd like to get them in
ASAP, even if the root bus iteration isn't figured out yet.

So my advice is, let's do the simple stuff we know how to do now, then
worry about the harder stuff later.

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/