[PATCH 1/2] PCI: decrease pci_dev->enable_cnt when no pcie capability found

[PATCH 1/2] PCI: decrease pci_dev->enable_cnt when no pcie capability found

[Yijing Wang]

We should decrease dev->enable_cnt when no pcie port capability
found for balance.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
Cc: Jiang Liu <jiang.liu@huawei.com>
Cc: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Cc: Shengzhou Liu <Shengzhou.Liu@freescale.com>
---
 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

[PATCH 2/2] PCI/IA64: fix pci_dev->enable_cnt balance when doing pci hotplug

[Yijing Wang]

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:
-- 
1.7.1

RE: [PATCH 2/2] PCI/IA64: fix pci_dev->enable_cnt balance when doing pci hotplug

[Luck, Tony]

> 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

Re: [PATCH 2/2] PCI/IA64: fix pci_dev->enable_cnt balance when doing pci hotplug

[Yijing Wang]

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

RE: [PATCH 2/2] PCI/IA64: fix pci_dev->enable_cnt balance when doing pci hotplug

[Luck, 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

Re: [PATCH 2/2] PCI/IA64: fix pci_dev->enable_cnt balance when doing pci hotplug

[Bjorn Helgaas]

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

Re: [PATCH 2/2] PCI/IA64: fix pci_dev->enable_cnt balance when doing pci hotplug

[Yijing Wang]

>> @@ -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