On Thu, Feb 26, 2015 at 04:11:55PM +0800, Yijing Wang wrote: > Pci_bus_add_devices() should not be placed in pci_scan_bus(). > Now pci device will be added to driver core once its > creation. All things left in pci_bus_add_devices() are > driver attachment and other trivial sysfs things. > Pci_scan_bus() should be the function responsible for > scanning PCI devices, not including driver attachment. > Other, some callers(m68k,unicore32,alpha) of pci_scan_bus() > will call pci_bus_size_bridges() and pci_bus_assign_resources() > after pci_scan_bus(). > > E.g. > In m68k > mcf_pci_init() > pci_scan_bus() > ... > pci_bus_add_devices() --- try to attach driver > pci_fixup_irqs() > pci_bus_size_bridges() > pci_bus_assign_resources() > > Above code flow make pci scan logic obscure, resources should > be assigned correctly before attaching driver. Because > pci_scan_bus() mostly be called in system boot path, when > before drivers are loaded(in module_init), so no pci scan > issues will occur. Most pci_scan_bus() callers additionally > call pci_bus_size_bridges() and pci_bus_assign_resources() > after pci_scan_bus(), we could rip out pci_bus_add_devices() > for better code readability. After applied this patch, > pci_scan_bus() should be used like: > > pci_scan_bus() (mandatory) > pci_fixup_irqs() (optional) > pci_bus_size_bridges() (optional) > pci_pci_bus_assign_resources() (optional) > pci_bus_add_devices() (mandatory) > > Signed-off-by: Yijing Wang <wangyijing@xxxxxxxxxx> > CC: "David S. Miller" <davem@xxxxxxxxxxxxx> > CC: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> > CC: Guan Xuetao <gxt@xxxxxxxxxxxxxxx> > CC: Richard Henderson <rth@xxxxxxxxxxx> > CC: Ivan Kokshaysky <ink@xxxxxxxxxxxxxxxxxxxx> > CC: Matt Turner <mattst88@xxxxxxxxx> > CC: linux-alpha@xxxxxxxxxxxxxxx > CC: linux-m68k@xxxxxxxxxxxxxxxxxxxx > CC: sparclinux@xxxxxxxxxxxxxxx > --- > arch/alpha/kernel/sys_nautilus.c | 1 + > arch/m68k/coldfire/pci.c | 1 + > arch/sparc/kernel/pcic.c | 2 ++ > arch/unicore32/kernel/pci.c | 11 +++-------- > drivers/pci/hotplug/ibmphp_core.c | 6 ++++-- > drivers/pci/probe.c | 1 - > 6 files changed, 11 insertions(+), 11 deletions(-) > > diff --git a/arch/alpha/kernel/sys_nautilus.c b/arch/alpha/kernel/sys_nautilus.c > index 837c0fa..4ae4a40 100644 > --- a/arch/alpha/kernel/sys_nautilus.c > +++ b/arch/alpha/kernel/sys_nautilus.c > @@ -253,6 +253,7 @@ nautilus_init_pci(void) > for the root bus, so just clear it. */ > bus->self = NULL; > pci_fixup_irqs(alpha_mv.pci_swizzle, alpha_mv.pci_map_irq); > + pci_bus_add_devices(bus); > } > > /* > diff --git a/arch/m68k/coldfire/pci.c b/arch/m68k/coldfire/pci.c > index df96792..d45f087 100644 > --- a/arch/m68k/coldfire/pci.c > +++ b/arch/m68k/coldfire/pci.c > @@ -319,6 +319,7 @@ static int __init mcf_pci_init(void) > pci_fixup_irqs(pci_common_swizzle, mcf_pci_map_irq); > pci_bus_size_bridges(rootbus); > pci_bus_assign_resources(rootbus); > + pci_bus_add_devices(rootbus); > return 0; > } > > diff --git a/arch/sparc/kernel/pcic.c b/arch/sparc/kernel/pcic.c > index 6cc78c2..7a82fe2 100644 > --- a/arch/sparc/kernel/pcic.c > +++ b/arch/sparc/kernel/pcic.c > @@ -391,6 +391,8 @@ static void __init pcic_pbm_scan_bus(struct linux_pcic *pcic) > struct linux_pbm_info *pbm = &pcic->pbm; > > pbm->pci_bus = pci_scan_bus(pbm->pci_first_busno, &pcic_ops, pbm); > + if (pbm->pci_bus) > + pci_bus_add_devices(pbm->pci_bus); > #if 0 /* deadwood transplanted from sparc64 */ > pci_fill_in_pbm_cookies(pbm->pci_bus, pbm, pbm->prom_node); > pci_record_assignments(pbm, pbm->pci_bus); This is correct, but it should look like this instead: if (!pbm->pci_bus) return; #if 0 /* deadwood ... */ ... #endif pci_bus_add_devices(pbm->pci_bus); } That way the mainline code is the normal path, and pci_bus_add_devices() is in the correct place even if the "deadwood" code is resurrected. > diff --git a/arch/unicore32/kernel/pci.c b/arch/unicore32/kernel/pci.c > index 374a055..3d82024 100644 > --- a/arch/unicore32/kernel/pci.c > +++ b/arch/unicore32/kernel/pci.c > @@ -266,17 +266,12 @@ static int __init pci_common_init(void) > pci_fixup_irqs(pci_common_swizzle, pci_puv3_map_irq); > > if (!pci_has_flag(PCI_PROBE_ONLY)) { > - /* > - * Size the bridge windows. > - */ > + /* Size the bridge windows. */ > pci_bus_size_bridges(puv3_bus); > - > - /* > - * Assign resources. > - */ > + /* Assign resources. */ > pci_bus_assign_resources(puv3_bus); > } > - > + pci_bus_add_devices(puv3_bus); > return 0; > } > subsys_initcall(pci_common_init); > diff --git a/drivers/pci/hotplug/ibmphp_core.c b/drivers/pci/hotplug/ibmphp_core.c > index 96c5c72..86e3bfd 100644 > --- a/drivers/pci/hotplug/ibmphp_core.c > +++ b/drivers/pci/hotplug/ibmphp_core.c > @@ -738,7 +738,7 @@ static void ibm_unconfigure_device(struct pci_func *func) > */ > static u8 bus_structure_fixup(u8 busno) > { > - struct pci_bus *bus; > + struct pci_bus *bus, *b; > struct pci_dev *dev; > u16 l; > > @@ -765,7 +765,9 @@ static u8 bus_structure_fixup(u8 busno) > (l != 0x0000) && (l != 0xffff)) { > debug("%s - Inside bus_structure_fixup()\n", > __func__); > - pci_scan_bus(busno, ibmphp_pci_bus->ops, NULL); > + b = pci_scan_bus(busno, ibmphp_pci_bus->ops, NULL); > + if (b) > + pci_bus_add_devices(b); Use this: if (!b) continue; pci_bus_add_devices(b); for the same reason as above. > break; > } > } > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 8d2f400..88604f2 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2123,7 +2123,6 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, > b = pci_create_root_bus(NULL, bus, ops, sysdata, &resources); > if (b) { > pci_scan_child_bus(b); > - pci_bus_add_devices(b); > } else { > pci_free_resource_list(&resources); > } > -- > 1.7.1 > -- 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