| Message ID | 1421372666-12288-2-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Jan 15, 2015 at 5:43 PM, Yijing Wang <wangyijing@huawei.com> 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() > > It is not correct, resources should be assigned correctly > before attaching driver. No, at that time pci drivers are loaded yet. Thanks 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 Fri, Jan 16, 2015 at 3:15 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Thu, Jan 15, 2015 at 5:43 PM, Yijing Wang <wangyijing@huawei.com> 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() >> >> It is not correct, resources should be assigned correctly >> before attaching driver. > No, for booting path, at that time pci drivers are *NOT* loaded yet. Thanks 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 2015/1/17 7:16, Yinghai Lu wrote: > On Fri, Jan 16, 2015 at 3:15 PM, Yinghai Lu <yinghai@kernel.org> wrote: >> On Thu, Jan 15, 2015 at 5:43 PM, Yijing Wang <wangyijing@huawei.com> 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() >>> >>> It is not correct, resources should be assigned correctly >>> before attaching driver. >> > No, for booting path, at that time pci drivers are *NOT* loaded yet. Hi Yinghai, I knew code flow here would not cause problems, sorry the log confused you, I will refresh it. But I think pci_scan_bus()/pci_scan_root_bus() which could only be used during system boot up(before module_init) make the pci scan logic obscure. Because most callers additionally call pci_bus_size_bridges() and pci_bus_assign_resources() later, so rip out pci_bus_add_devices() from pci_scan_bus()/pci_scan_root_bus() make code have better readability. Thanks! Yijing. > > Thanks > > Yinghai > > . >
On 19/01/15 12:04, Yijing Wang wrote: > On 2015/1/17 7:16, Yinghai Lu wrote: >> On Fri, Jan 16, 2015 at 3:15 PM, Yinghai Lu <yinghai@kernel.org> wrote: >>> On Thu, Jan 15, 2015 at 5:43 PM, Yijing Wang <wangyijing@huawei.com> 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() >>>> >>>> It is not correct, resources should be assigned correctly >>>> before attaching driver. >>> >> No, for booting path, at that time pci drivers are *NOT* loaded yet. > > Hi Yinghai, I knew code flow here would not cause problems, sorry the log > confused you, I will refresh it. But I think pci_scan_bus()/pci_scan_root_bus() > which could only be used during system boot up(before module_init) make > the pci scan logic obscure. Because most callers additionally call > pci_bus_size_bridges() and pci_bus_assign_resources() later, > so rip out pci_bus_add_devices() from pci_scan_bus()/pci_scan_root_bus() > make code have better readability. I agree that ordering seems odd. I recall there was a reason I had to put it in that order (at that time - which is a few years back now). I can't remember exactly now, but it was something like bridges didn't get resourced properly without a pci scan first. Regards Greg -- 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 Friday 16 January 2015 15:16:28 Yinghai Lu wrote: > On Fri, Jan 16, 2015 at 3:15 PM, Yinghai Lu <yinghai@kernel.org> wrote: > > On Thu, Jan 15, 2015 at 5:43 PM, Yijing Wang <wangyijing@huawei.com> 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() > >> > >> It is not correct, resources should be assigned correctly > >> before attaching driver. > > > No, for booting path, at that time pci drivers are *NOT* loaded yet. This may be true for the m68k example, and traditionally for all PCI hosts, but as we move to a more modular architecture with drivers/pci/hosts, you can have PCI host drivers that are loadable modules and get loaded after the built-in drivers are initialized, or you can have host drivers that depend on a resource (clock, regulator, reset, gpio, ...) that in turn comes from a driver that gets initialized later. Arnd -- 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
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); 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); break; } } diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 23212f8..053c0f4 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); }
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() It is not correct, resources should be assigned correctly before attaching driver. So we should rip out pci_bus_add_devices() for better code design. After applied this patch, pci_scan_bus() should be used in flow 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@huawei.com> CC: "David S. Miller" <davem@davemloft.net> CC: Geert Uytterhoeven <geert@linux-m68k.org> CC: Guan Xuetao <gxt@mprc.pku.edu.cn> CC: linux-alpha@vger.kernel.org CC: linux-m68k@lists.linux-m68k.org CC: sparclinux@vger.kernel.org --- 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(-)