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. 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 -- 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
