| Message ID | 1364805775-12396-2-git-send-email-wangyijing@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
> In IA64 platform, we don't call pci_enable_bridges() > when scan all pci buses during system boot up. But in > X86 we do it in Your patch looks plausible ... but I have a question. X86 doesn't *directly* call pci_enable_bridges() from any arch/x86/* file. Do we need this in an arch/ia64 file because our PCI support is getting old and stale? "git grep" says that arm, m68k, mips and sh all make direct calls. -Tony -- 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 2013/4/2 6:23, Luck, Tony wrote: >> In IA64 platform, we don't call pci_enable_bridges() >> when scan all pci buses during system boot up. But in >> X86 we do it in > > Your patch looks plausible ... but I have a question. X86 doesn't > *directly* call pci_enable_bridges() from any arch/x86/* file. > Hi Tony, In x86, we will use pcibios_assign_resources() in arch/x86/pci/i386.c to assign pci device resource. And at the end of pci_assign_unassigned_resources() function we enable pci bridges. So X86 call pci_enable_bridges() when doing device resource assignment. But in IA64, we assign device resource according BIOS setting in PCI BARs. No additional resource reassignment code support after scan all pci buses. In this respects, resource reassignment support is weak in IA64. But this patch mainly to fix the unbalanced dev->enable_cnt in IA64 which will print WARNING Calltrace in dmesg. If you think it is valuable, I will try to improve resource assignment in IA64 like other arch (eg arm, m68k, mips and sh..) in another patch. Thanks! Yijing. > Do we need this in an arch/ia64 file because our PCI support > is getting old and stale? "git grep" says that arm, m68k, mips > and sh all make direct calls. I think so. > > -Tony > > . >
> But this patch mainly to fix the unbalanced dev->enable_cnt in IA64 which will print WARNING Calltrace > in dmesg. Thanks for the explanation. > If you think it is valuable, I will try to improve resource assignment in IA64 like other arch (eg arm, m68k, mips and sh..) > in another patch. Making the ia64 code more like the x86 code might help avoid such problems in the future (lots more people look at x86 than ia64 - if ours is the same, or very similar, then it is likely that changes made to x86 will be correct for ia64 too). Only you can decide how much this is worth to you and your company - perhaps there will be no more changes that break ia64 even with the code differences. Or perhaps it will be easier for you to just fix things as they break than to undertake a restructure of the code. -Tony -- 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 Mon, Apr 1, 2013 at 2:42 AM, Yijing Wang <wangyijing@huawei.com> wrote: > In IA64 platform, we don't call pci_enable_bridges() > when scan all pci buses during system boot up. But in > X86 we do it in > > pcibios_assign_resources() > pci_assign_unassigned_resources() > ........... > pci_enable_bridges() > > Then when we doing hot remove > > acpiphp_disable_slot() > pci_stop_and_remove_bus_device() > pci_stop_bus_device() > ............. > pcie_portdrv_remove() > pcie_port_device_remove() > pci_disable_device() first decrease enable_cnt here > pci_disable_device() second decrease enable_cnt > So pci_dev->enable_cnt is unbalanced in IA64. > > Following Warning info found under IA64 when doing pci hotplug. > > ------------[ cut here ]------------ > WARNING: at drivers/pci/pci.c:1397 pci_disable_device+0x1c0/0x220() > Hardware name: MH8900 > Device pcieport > disabling already-disabled device > Modules linked in: acpiphp ipv6 ipmi_si(+) ipmi_devintf ipmi_msghandler fuse vfaa > t fat dm_mod iTCO_wdt iTCO_vendor_support lpc_ich i2c_i801 mfd_core i2c_core sg > sd_mod crc_t10dif ext3 mbcache jbd ata_piix > > Call Trace: > [<a000000100015c00>] show_stack+0x80/0xa0 > sp=e000000fd629fc00 bsp=e000000fd62996e0 > [<a000000100a9ca20>] dump_stack+0x30/0x50 > sp=e000000fd629fdd0 bsp=e000000fd62996c8 > [<a000000100061960>] warn_slowpath_common+0xc0/0x100 > sp=e000000fd629fdd0 bsp=e000000fd6299688 > [<a000000100061b50>] warn_slowpath_fmt+0x90/0xc0 > sp=e000000fd629fdd0 bsp=e000000fd6299628 > [<a000000100495be0>] pci_disable_device+0x1c0/0x220 > sp=e000000fd629fe10 bsp=e000000fd62995e8 > [<a0000001004b3ba0>] pcie_portdrv_remove+0xc0/0xe0 > sp=e000000fd629fe10 bsp=e000000fd62995c8 > [<a000000100499670>] pci_device_remove+0x90/0x1e0 > sp=e000000fd629fe10 bsp=e000000fd6299598 > [<a000000100636490>] __device_release_driver+0x150/0x280 > sp=e000000fd629fe10 bsp=e000000fd6299560 > [<a000000100636830>] device_release_driver+0x30/0x60 > sp=e000000fd629fe10 bsp=e000000fd6299538 > [<a000000100634a40>] bus_remove_device+0x2c0/0x3c0 > sp=e000000fd629fe10 bsp=e000000fd62994f0 > [<a0000001006306d0>] device_del+0x290/0x440 > sp=e000000fd629fe10 bsp=e000000fd62994a8 > [<a00000010048d550>] pci_stop_bus_device+0x150/0x200 > sp=e000000fd629fe10 bsp=e000000fd6299478 > [<a00000010048d470>] pci_stop_bus_device+0x70/0x200 > sp=e000000fd629fe10 bsp=e000000fd6299448 > [<a00000010048d470>] pci_stop_bus_device+0x70/0x200 > sp=e000000fd629fe10 bsp=e000000fd6299418 > [<a00000010048d9a0>] pci_stop_and_remove_bus_device+0x20/0x60 > sp=e000000fd629fe10 bsp=e000000fd62993f0 > [<a0000002089aa100>] acpiphp_disable_slot+0x240/0x4e0 [acpiphp] > sp=e000000fd629fe10 bsp=e000000fd62993a0 > [<a0000002089a8a30>] disable_slot+0x50/0x160 [acpiphp] > sp=e000000fd629fe20 bsp=e000000fd6299378 > [<a0000001004ba080>] power_write_file+0x140/0x2a0 > sp=e000000fd629fe20 bsp=e000000fd6299348 > [<a0000001004a6320>] pci_slot_attr_store+0x60/0xa0 > sp=e000000fd629fe20 bsp=e000000fd6299310 > [<a00000010032a100>] sysfs_write_file+0x240/0x340 > sp=e000000fd629fe20 bsp=e000000fd62992b8 > [<a00000010023c910>] vfs_write+0x1b0/0x3a0 > sp=e000000fd629fe20 bsp=e000000fd6299270 > [<a00000010023cc90>] sys_write+0x90/0xe0 > sp=e000000fd629fe20 bsp=e000000fd62991f0 > [<a00000010000bce0>] ia64_ret_from_syscall+0x0/0x20 > sp=e000000fd629fe30 bsp=e000000fd62991f0 > [<a000000000040720>] __kernel_syscall_via_break+0x0/0x20 > sp=e000000fd62a0000 bsp=e000000fd62991f0 > ---[ end trace 34d87c78dbff78ce ]--- > GSI 37 (level, low) -> CPU 15 (0x01e0) vector 68 unregistered > pcie_pme 0000:00:07.0:pcie01: unloading service driver pcie_pme > aer 0000:00:07.0:pcie02: unloading service driver aer > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > Cc: Fenghua Yu <fenghua.yu@intel.com> > Cc: Yinghai Lu <yinghai@kernel.org> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Thierry Reding <thierry.reding@avionic-design.de> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> > --- > arch/ia64/pci/pci.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c > index 60532ab..a557096 100644 > --- a/arch/ia64/pci/pci.c > +++ b/arch/ia64/pci/pci.c > @@ -383,6 +383,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > } > > pci_scan_child_bus(pbus); > + pci_enable_bridges(pbus); > return pbus; > > out3: I think that with this patch, if you hot-add a PCI host bridge, you will call pci_enable_bridges() twice (once in pci_acpi_scan_root() and again in acpi_pci_root_add()), so there will be an enable_cnt error in the opposite direction. I'd like to see the pci_enable_bridges() calls pushed up into the generic code because I don't think there's anything arch-specific about it. Bjorn -- 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
>> @@ -383,6 +383,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) >> } >> >> pci_scan_child_bus(pbus); >> + pci_enable_bridges(pbus); >> return pbus; >> >> out3: > > I think that with this patch, if you hot-add a PCI host bridge, you > will call pci_enable_bridges() twice (once in pci_acpi_scan_root() and > again in acpi_pci_root_add()), so there will be an enable_cnt error in > the opposite direction. > > I'd like to see the pci_enable_bridges() calls pushed up into the > generic code because I don't think there's anything arch-specific > about it. Hi Bjorn, Thanks for your review and comments! This is my fault, I forgot we will enable pci bridges when we hot add host bridge. Push pci_enable_bridges() into the generic code is a good idea, so we don't need to consider enabling bridge in pci arch-specific code. In IA64 we don't assign the unassigned resources like other arch. This is also a weak point. I will update this patch and resend soon. Thanks! Yijing. > > . >
diff --git a/arch/ia64/pci/pci.c b/arch/ia64/pci/pci.c index 60532ab..a557096 100644 --- a/arch/ia64/pci/pci.c +++ b/arch/ia64/pci/pci.c @@ -383,6 +383,7 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) } pci_scan_child_bus(pbus); + pci_enable_bridges(pbus); return pbus; out3:
In IA64 platform, we don't call pci_enable_bridges() when scan all pci buses during system boot up. But in X86 we do it in pcibios_assign_resources() pci_assign_unassigned_resources() ........... pci_enable_bridges() Then when we doing hot remove acpiphp_disable_slot() pci_stop_and_remove_bus_device() pci_stop_bus_device() ............. pcie_portdrv_remove() pcie_port_device_remove() pci_disable_device() first decrease enable_cnt here pci_disable_device() second decrease enable_cnt So pci_dev->enable_cnt is unbalanced in IA64. Following Warning info found under IA64 when doing pci hotplug. ------------[ cut here ]------------ WARNING: at drivers/pci/pci.c:1397 pci_disable_device+0x1c0/0x220() Hardware name: MH8900 Device pcieport disabling already-disabled device Modules linked in: acpiphp ipv6 ipmi_si(+) ipmi_devintf ipmi_msghandler fuse vfaa t fat dm_mod iTCO_wdt iTCO_vendor_support lpc_ich i2c_i801 mfd_core i2c_core sg sd_mod crc_t10dif ext3 mbcache jbd ata_piix Call Trace: [<a000000100015c00>] show_stack+0x80/0xa0 sp=e000000fd629fc00 bsp=e000000fd62996e0 [<a000000100a9ca20>] dump_stack+0x30/0x50 sp=e000000fd629fdd0 bsp=e000000fd62996c8 [<a000000100061960>] warn_slowpath_common+0xc0/0x100 sp=e000000fd629fdd0 bsp=e000000fd6299688 [<a000000100061b50>] warn_slowpath_fmt+0x90/0xc0 sp=e000000fd629fdd0 bsp=e000000fd6299628 [<a000000100495be0>] pci_disable_device+0x1c0/0x220 sp=e000000fd629fe10 bsp=e000000fd62995e8 [<a0000001004b3ba0>] pcie_portdrv_remove+0xc0/0xe0 sp=e000000fd629fe10 bsp=e000000fd62995c8 [<a000000100499670>] pci_device_remove+0x90/0x1e0 sp=e000000fd629fe10 bsp=e000000fd6299598 [<a000000100636490>] __device_release_driver+0x150/0x280 sp=e000000fd629fe10 bsp=e000000fd6299560 [<a000000100636830>] device_release_driver+0x30/0x60 sp=e000000fd629fe10 bsp=e000000fd6299538 [<a000000100634a40>] bus_remove_device+0x2c0/0x3c0 sp=e000000fd629fe10 bsp=e000000fd62994f0 [<a0000001006306d0>] device_del+0x290/0x440 sp=e000000fd629fe10 bsp=e000000fd62994a8 [<a00000010048d550>] pci_stop_bus_device+0x150/0x200 sp=e000000fd629fe10 bsp=e000000fd6299478 [<a00000010048d470>] pci_stop_bus_device+0x70/0x200 sp=e000000fd629fe10 bsp=e000000fd6299448 [<a00000010048d470>] pci_stop_bus_device+0x70/0x200 sp=e000000fd629fe10 bsp=e000000fd6299418 [<a00000010048d9a0>] pci_stop_and_remove_bus_device+0x20/0x60 sp=e000000fd629fe10 bsp=e000000fd62993f0 [<a0000002089aa100>] acpiphp_disable_slot+0x240/0x4e0 [acpiphp] sp=e000000fd629fe10 bsp=e000000fd62993a0 [<a0000002089a8a30>] disable_slot+0x50/0x160 [acpiphp] sp=e000000fd629fe20 bsp=e000000fd6299378 [<a0000001004ba080>] power_write_file+0x140/0x2a0 sp=e000000fd629fe20 bsp=e000000fd6299348 [<a0000001004a6320>] pci_slot_attr_store+0x60/0xa0 sp=e000000fd629fe20 bsp=e000000fd6299310 [<a00000010032a100>] sysfs_write_file+0x240/0x340 sp=e000000fd629fe20 bsp=e000000fd62992b8 [<a00000010023c910>] vfs_write+0x1b0/0x3a0 sp=e000000fd629fe20 bsp=e000000fd6299270 [<a00000010023cc90>] sys_write+0x90/0xe0 sp=e000000fd629fe20 bsp=e000000fd62991f0 [<a00000010000bce0>] ia64_ret_from_syscall+0x0/0x20 sp=e000000fd629fe30 bsp=e000000fd62991f0 [<a000000000040720>] __kernel_syscall_via_break+0x0/0x20 sp=e000000fd62a0000 bsp=e000000fd62991f0 ---[ end trace 34d87c78dbff78ce ]--- GSI 37 (level, low) -> CPU 15 (0x01e0) vector 68 unregistered pcie_pme 0000:00:07.0:pcie01: unloading service driver pcie_pme aer 0000:00:07.0:pcie02: unloading service driver aer Signed-off-by: Yijing Wang <wangyijing@huawei.com> Cc: Fenghua Yu <fenghua.yu@intel.com> Cc: Yinghai Lu <yinghai@kernel.org> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Thierry Reding <thierry.reding@avionic-design.de> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> --- arch/ia64/pci/pci.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)