On Wed, Feb 6, 2013 at 12:50 PM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote: > On Wed, Feb 6, 2013 at 11:59 AM, Yinghai Lu <yinghai@xxxxxxxxxx> wrote: >> On Wed, Feb 6, 2013 at 9:54 AM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
