| Message ID | CAErSpo6NOhTV0HX1dw5-mRkUtfO7XH_iCGVwiddBPpQvPZL=7g@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Sep 25, 2012 at 10:37 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Tue, Sep 25, 2012 at 10:29 AM, Yinghai Lu <yinghai@kernel.org> wrote: >>> >>> I'd prefer a design where the PCI core provides an interface that >>> means "call this function for every host bridge we know about now >>> *and* for every one that's added in the future." >> >> yes, that is the point to add pci_root_bridge_bus_type. We can register >> bus notifier on that. > > I guess I missed your point. In the patch below (20/29 from your > series), you're still iterating through all the host bridges, so there > would have to be something else to handle hot-added host bridges. > > Are you saying you plan future patches to change this again to > something using a bus notifier? > > --- a/arch/sparc/kernel/pci.c > +++ b/arch/sparc/kernel/pci.c > @@ -997,11 +997,13 @@ static void __devinit pci_bus_slot_names(struct > device_node *node, > > static int __init of_pci_slot_init(void) > { > - struct pci_bus *pbus = NULL; > + struct pci_host_bridge *host_bridge = NULL; > + struct pci_bus *pbus; > > - while ((pbus = pci_find_next_bus(pbus)) != NULL) { > + for_each_pci_host_bridge(host_bridge) { > struct device_node *node; > > + pbus = hot_bridge->bus; > if (pbus->self) { > /* PCI->PCI bridge */ > node = pbus->self->dev.of_node; that is for initial booting path. for hot add/remove notifier add need to be done case by case. -Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 25, 2012 at 12:06 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Tue, Sep 25, 2012 at 10:37 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Tue, Sep 25, 2012 at 10:29 AM, Yinghai Lu <yinghai@kernel.org> wrote: >>>> >>>> I'd prefer a design where the PCI core provides an interface that >>>> means "call this function for every host bridge we know about now >>>> *and* for every one that's added in the future." >>> >>> yes, that is the point to add pci_root_bridge_bus_type. We can register >>> bus notifier on that. >> >> I guess I missed your point. In the patch below (20/29 from your >> series), you're still iterating through all the host bridges, so there >> would have to be something else to handle hot-added host bridges. >> >> Are you saying you plan future patches to change this again to >> something using a bus notifier? >> >> --- a/arch/sparc/kernel/pci.c >> +++ b/arch/sparc/kernel/pci.c >> @@ -997,11 +997,13 @@ static void __devinit pci_bus_slot_names(struct >> device_node *node, >> >> static int __init of_pci_slot_init(void) >> { >> - struct pci_bus *pbus = NULL; >> + struct pci_host_bridge *host_bridge = NULL; >> + struct pci_bus *pbus; >> >> - while ((pbus = pci_find_next_bus(pbus)) != NULL) { >> + for_each_pci_host_bridge(host_bridge) { >> struct device_node *node; >> >> + pbus = hot_bridge->bus; >> if (pbus->self) { >> /* PCI->PCI bridge */ >> node = pbus->self->dev.of_node; > > that is for initial booting path. > > for hot add/remove notifier add need to be done case by case. Can initial boot be done the same as hot-add? If we add interfaces like for_each_pci_host_bridge(), people will just copy that for use at run-time. So it would be better to have the same interfaces for use at boot-time and at hot add-time. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 25, 2012 at 11:19 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Tue, Sep 25, 2012 at 12:06 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> that is for initial booting path. >> >> for hot add/remove notifier add need to be done case by case. > > Can initial boot be done the same as hot-add? If we add interfaces > like for_each_pci_host_bridge(), people will just copy that for use at > run-time. So it would be better to have the same interfaces for use > at boot-time and at hot add-time. still need to check them case by case. some function may be too early to be called in work_fn in notifier. that need to find out when to get that bus notifier get register. I have one draft patch that will delay bridge enabling to BUS_ADD for pci bridge... that will need to get that register rather later otherwise that bridge can not be enabled because resources are not reserved/allocated for initial booting path. please refer to the attached patch. you should notice that fs_initcall is used for registration until boot path already have that pci bridges before. +static struct notifier_block pci_hp_nb = { + .notifier_call = &pci_hp_notifier, +}; + +static int __init pci_hp_init(void) +{ + return bus_register_notifier(&pci_bus_type, &pci_hp_nb); +} + +fs_initcall(pci_hp_init); Also using that for_each_pci_host_bridge in run-time is still safe. -Yinghai
On Tue, Sep 25, 2012 at 1:17 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Tue, Sep 25, 2012 at 11:19 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Tue, Sep 25, 2012 at 12:06 PM, Yinghai Lu <yinghai@kernel.org> wrote: > >>> that is for initial booting path. >>> >>> for hot add/remove notifier add need to be done case by case. >> >> Can initial boot be done the same as hot-add? If we add interfaces >> like for_each_pci_host_bridge(), people will just copy that for use at >> run-time. So it would be better to have the same interfaces for use >> at boot-time and at hot add-time. > > still need to check them case by case. some function may be too early > to be called in work_fn in notifier. that need to find out when to get > that bus notifier get > register. > > I have one draft patch that will delay bridge enabling to BUS_ADD for > pci bridge... > that will need to get that register rather later otherwise that bridge > can not be enabled because resources are not reserved/allocated for > initial booting path. > please refer to the attached patch. you should notice that fs_initcall > is used for registration until boot path already have that pci bridges > before. > > +static struct notifier_block pci_hp_nb = { > + .notifier_call = &pci_hp_notifier, > +}; > + > +static int __init pci_hp_init(void) > +{ > + return bus_register_notifier(&pci_bus_type, &pci_hp_nb); > +} > + > +fs_initcall(pci_hp_init); > > Also using that for_each_pci_host_bridge in run-time is still safe. Sure, it might be *safe*. But it's not *sufficient*. If you use for_each_pci_host_bridge(), you also need to do something else to deal with hot-added host bridges. It's hard to make sure that every caller does both parts correctly: 1) for_each_pci_host_bridge() for things we've already seen and 2) some sort of notifier for hot-added bridges That's why I'd prefer a single interface. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 25, 2012 at 12:45 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > Sure, it might be *safe*. But it's not *sufficient*. If you use > for_each_pci_host_bridge(), you also need to do something else to deal > with hot-added host bridges. It's hard to make sure that every caller > does both parts correctly: > > 1) for_each_pci_host_bridge() for things we've already seen and > 2) some sort of notifier for hot-added bridges yes. > > That's why I'd prefer a single interface. that will have draw backs too: 1. too much changes to the code of current caller. 2. have to checking system_state back and forth. 3. some variable function call may only need for _init path could not be freed. or add annoying _ref_ok etc. -Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 25, 2012 at 1:53 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Tue, Sep 25, 2012 at 12:45 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > >> Sure, it might be *safe*. But it's not *sufficient*. If you use >> for_each_pci_host_bridge(), you also need to do something else to deal >> with hot-added host bridges. It's hard to make sure that every caller >> does both parts correctly: >> >> 1) for_each_pci_host_bridge() for things we've already seen and >> 2) some sort of notifier for hot-added bridges > > yes. > >> >> That's why I'd prefer a single interface. > > that will have draw backs too: > 1. too much changes to the code of current caller. > 2. have to checking system_state back and forth. > 3. some variable function call may only need for _init path could not be freed. > or add annoying _ref_ok etc. We already have to make significant changes to the callers. Your changes only address part 1. More changes will be needed for part 2, and they will look very much like the approach I'm suggesting. I'm frankly dubious about your fears of system_state and __init complexity. PCI enumeration and driver binding already happens late enough that the system is almost completely initialized. But I guess we won't know for sure until we try out both ideas. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 25, 2012 at 1:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Tue, Sep 25, 2012 at 1:53 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Tue, Sep 25, 2012 at 12:45 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> >>> Sure, it might be *safe*. But it's not *sufficient*. If you use >>> for_each_pci_host_bridge(), you also need to do something else to deal >>> with hot-added host bridges. It's hard to make sure that every caller >>> does both parts correctly: >>> >>> 1) for_each_pci_host_bridge() for things we've already seen and >>> 2) some sort of notifier for hot-added bridges >> >> yes. >> >>> >>> That's why I'd prefer a single interface. >> >> that will have draw backs too: >> 1. too much changes to the code of current caller. >> 2. have to checking system_state back and forth. >> 3. some variable function call may only need for _init path could not be freed. >> or add annoying _ref_ok etc. > > We already have to make significant changes to the callers. Your > changes only address part 1. More changes will be needed for part 2, > and they will look very much like the approach I'm suggesting. not that much, We just update the loop function from the caller. > > I'm frankly dubious about your fears of system_state and __init > complexity. PCI enumeration and driver binding already happens late > enough that the system is almost completely initialized. But I guess > we won't know for sure until we try out both ideas. it is all up to the caller to decide it initial path for_ loop is enough, or they still need part 2, or later they could even remove part1 and only keep part2 if the those function can survive the initial boot path. -Yinghai -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/arch/sparc/kernel/pci.c +++ b/arch/sparc/kernel/pci.c @@ -997,11 +997,13 @@ static void __devinit pci_bus_slot_names(struct device_node *node, static int __init of_pci_slot_init(void) { - struct pci_bus *pbus = NULL; + struct pci_host_bridge *host_bridge = NULL; + struct pci_bus *pbus; - while ((pbus = pci_find_next_bus(pbus)) != NULL) { + for_each_pci_host_bridge(host_bridge) { struct device_node *node; + pbus = hot_bridge->bus; if (pbus->self) { /* PCI->PCI bridge */