We should decrease dev->enable_cnt when no pcie port capability
found for balance.
Signed-off-by: Yijing Wang <[email protected]>
Cc: Jiang Liu <[email protected]>
Cc: Kenji Kaneshige <[email protected]>
Cc: Shengzhou Liu <[email protected]>
---
drivers/pci/pcie/portdrv_core.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 31063ac..dd95d6f 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -369,8 +369,10 @@ int pcie_port_device_register(struct pci_dev *dev)
/* Get and check PCI Express port services */
capabilities = get_port_device_capability(dev);
- if (!capabilities)
+ if (!capabilities) {
+ pci_disable_device(dev);
return 0;
+ }
pci_set_master(dev);
/*
--
1.7.1
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 <[email protected]>
Cc: Fenghua Yu <[email protected]>
Cc: Yinghai Lu <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Thierry Reding <[email protected]>
Cc: "Rafael J. Wysocki" <[email protected]>
---
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:
--
1.7.1
> 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
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
>
> .
>
--
Thanks!
Yijing
> 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
On Mon, Apr 1, 2013 at 2:42 AM, Yijing Wang <[email protected]> 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 <[email protected]>
> Cc: Fenghua Yu <[email protected]>
> Cc: Yinghai Lu <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Thierry Reding <[email protected]>
> Cc: "Rafael J. Wysocki" <[email protected]>
> ---
> 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
>> @@ -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.
>
> .
>
--
Thanks!
Yijing