PCI/ACPI: fix wrong ref count handling in acpi_pci_bind()

PATCH In review Kenji Kaneshige 4 participants 7 messages May 26, 2009 – May 28, 2009

Subsystems (2)

ACPI PCI

lore

Share & tools

lore

Kenji Kaneshige

author

2009-05-26 00:08 UTC #0 lore raw

PATCH 1/1 PCI/ACPI: fix wrong ref count handling in acpi_pci_bind()

Fix wrong struct pci_dev reference counter handling in acpi_pci_bind().

The 'dev' field of struct acpi_pci_data is having a pointer to struct pci_dev without incrementing the reference counter. Because of this, I got the following kernel oops when I was doing some pci hotplug operations. This patch fixes this bug by replacing wrong hand-made pci_find_slot() with pci_get_slot() in acpi_pci_bind().

[  206.427004] BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
[  206.427076] IP: [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
[  206.427076] PGD 8225ad067 PUD 82258b067 PMD 0
[  206.427076] Oops: 0000 [#1] SMP
[  206.427076] last sysfs file: /sys/bus/pci/slots/1/power
[  206.427076] CPU 2
[  206.427076] Modules linked in: acpiphp ipv6 autofs4 hidp rfcomm l2cap bluetooth sunrpc dm_mirror dm_region_hash dm_log dm_multipath scsi_dh dm_mod sbs sbshc pci_slot battery ac parport_pc lp parport sg mptspi mptscsih mptbase scsi_transport_spi sr_mod cdrom e1000e serio_raw button i2c_i801 i2c_core shpchp pcspkr ata_piix libata megaraid_sas sd_mod scsi_mod crc_t10dif ext3 jbd uhci_hcd ohci_hcd ehci_hcd [last unloaded: microcode]
[  206.427076] Pid: 10367, comm: bash Not tainted 2.6.30-rc4-kk #10 PRIMERGY
[  206.427076] RIP: 0010:[<ffffffff803f0e9b>]  [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
[  206.427076] RSP: 0018:ffff8808225a9d68  EFLAGS: 00010206
[  206.427076] RAX: 0000000000000000 RBX: ffff88083c547800 RCX: 0000000000000006
[  206.427076] RDX: ffff88083ce36ca0 RSI: ffff880822508768 RDI: 0000000000000000
[  206.427076] RBP: ffff8808225a9d98 R08: 0000000000000000 R09: 0000000000000000
[  206.427076] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000001
[  206.427076] R13: 0000000000000000 R14: 0000000000000001 R15: ffff8808225a9e28
[  206.427076] FS:  00007f376c4ee6e0(0000) GS:ffff880054636000(0000) knlGS:0000000000000000
[  206.427076] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  206.427076] CR2: 00000000000000e8 CR3: 0000000822590000 CR4: 00000000000006e0
[  206.427076] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  206.427076] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  206.427076] Process bash (pid: 10367, threadinfo ffff8808225a8000, task ffff880822508000)
[  206.427076] Stack:
[  206.427076]  000000000000001f ffff88083addd5a0 ffff8808225a9d98 ffff88083ce36ca0
[  206.427076]  ffff88083c547800 ffff88083c547800 ffff8808225a9db8 ffffffff803ecee4
[  206.427076]  ffff88083c547800 ffff88083c547000 ffff8808225a9e08 ffffffff803ecf6d
[  206.427076] Call Trace:
[  206.427076]  [<ffffffff803ecee4>] acpi_bus_remove+0x54/0x68
[  206.427076]  [<ffffffff803ecf6d>] acpi_bus_trim+0x75/0xe3
[  206.427076]  [<ffffffffa0345ddd>] acpiphp_disable_slot+0x16d/0x1e0 [acpiphp]
[  206.427076]  [<ffffffffa03441f0>] disable_slot+0x20/0x60 [acpiphp]
[  206.427076]  [<ffffffff803cfc18>] power_write_file+0xc8/0x110
[  206.427076]  [<ffffffff803c6a54>] pci_slot_attr_store+0x24/0x30
[  206.427076]  [<ffffffff803469ce>] sysfs_write_file+0xce/0x140
[  206.427076]  [<ffffffff802e94e7>] vfs_write+0xc7/0x170
[  206.427076]  [<ffffffff802e9aa0>] sys_write+0x50/0x90
[  206.427076]  [<ffffffff8020bd6b>] system_call_fastpath+0x16/0x1b
[  206.427076] Code: be 2b 01 00 00 48 c7 c7 d0 c6 5a 80 31 c0 e8 c3 8f 01 00 eb 36 48 8b 55 e8 48 8b 42 10 48 83 78 18 00 74 13 48 8b 42 08 0f b7 3a <0f> b6 b0 e8 00 00 00 e8 ab f8 ff ff 48 8b 7d e8 e8 30 cf ee ff
[  206.427076] RIP  [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
[  206.427076]  RSP <ffff8808225a9d68>
[  206.427076] CR2: 00000000000000e8
[  206.440158] ---[ end trace 1ca3974fa717e665 ]---

Signed-off-by Kenji Kaneshige <[email protected]>

 drivers/acpi/pci_bind.c |   21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

drivers/acpi/pci_bind.c +6 ~~-15~~

Index: 20090521/drivers/acpi/pci_bind.c
===================================================================
@@ -116,9 +116,6 @@ int acpi_pci_bind(struct acpi_device *de
 	struct acpi_pci_data *pdata;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	acpi_handle handle;
-	struct pci_dev *dev;
-	struct pci_bus *bus;
-
 
 	if (!device || !device->parent)
 		return -EINVAL;

@@ -180,16 +177,8 @@ int acpi_pci_bind(struct acpi_device *de
 	 * PCI devices are added to the global pci list when the root
 	 * bridge start ops are run, which may not have happened yet.
 	 */
-	bus = pci_find_bus(data->id.segment, data->id.bus);
-	if (bus) {
-		list_for_each_entry(dev, &bus->devices, bus_list) {
-			if (dev->devfn == PCI_DEVFN(data->id.device,
-						    data->id.function)) {
-				data->dev = dev;
-				break;
-			}
-		}
-	}
+	data->dev = pci_get_slot(pdata->bus,
+				PCI_DEVFN(data->id.device, data->id.function));
 	if (!data->dev) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Device %04x:%02x:%02x.%d not present in PCI namespace\n",

@@ -259,9 +248,10 @@ int acpi_pci_bind(struct acpi_device *de
 
       end:
 	kfree(buffer.pointer);
-	if (result)
+	if (result) {
+		pci_dev_put(data->dev);
 		kfree(data);
-
+	}
 	return result;
 }

@@ -303,6 +293,7 @@ static int acpi_pci_unbind(struct acpi_d
 	if (data->dev->subordinate) {
 		acpi_pci_irq_del_prt(data->id.segment, data->bus->number);
 	}
+	pci_dev_put(data->dev);
 	kfree(data);
 
       end:

Bjorn Helgaas

2009-05-26 15:45 UTC #1 lore raw

Quoted 7 lines · Kenji Kaneshige

Fix wrong struct pci_dev reference counter handling in acpi_pci_bind().
The 'dev' field of struct acpi_pci_data is having a pointer to struct pci_dev without incrementing the reference counter. Because of this, I got the following kernel oops when I was doing some pci hotplug operations. This patch fixes this bug by replacing wrong hand-made pci_find_slot() with pci_get_slot() in acpi_pci_bind().

I don't like this ACPI/PCI bind thing in general because having the extra .bind and .unbind methods is ugly and somewhat non-obvious, and I'm nervous about object lifetime issues like this one.

But I don't have a better alternative to offer, and there's definitely a problem here, so thanks for fixing and testing it. I do have one question below about whether the comment in the existing code, which seems to be an excuse for doing the hand-made pci_find_slot(), is still relevant, or should just be removed.

Bjorn

Quoted 62 lines

[  206.427004] BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
[  206.427076] IP: [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
[  206.427076] PGD 8225ad067 PUD 82258b067 PMD 0
[  206.427076] Oops: 0000 [#1] SMP
[  206.427076] last sysfs file: /sys/bus/pci/slots/1/power
[  206.427076] CPU 2
[  206.427076] Modules linked in: acpiphp ipv6 autofs4 hidp rfcomm l2cap bluetooth sunrpc dm_mirror dm_region_hash dm_log dm_multipath scsi_dh dm_mod sbs sbshc pci_slot battery ac parport_pc lp parport sg mptspi mptscsih mptbase scsi_transport_spi sr_mod cdrom e1000e serio_raw button i2c_i801 i2c_core shpchp pcspkr ata_piix libata megaraid_sas sd_mod scsi_mod crc_t10dif ext3 jbd uhci_hcd ohci_hcd ehci_hcd [last unloaded: microcode]
[  206.427076] Pid: 10367, comm: bash Not tainted 2.6.30-rc4-kk #10 PRIMERGY
[  206.427076] RIP: 0010:[<ffffffff803f0e9b>]  [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
[  206.427076] RSP: 0018:ffff8808225a9d68  EFLAGS: 00010206
[  206.427076] RAX: 0000000000000000 RBX: ffff88083c547800 RCX: 0000000000000006
[  206.427076] RDX: ffff88083ce36ca0 RSI: ffff880822508768 RDI: 0000000000000000
[  206.427076] RBP: ffff8808225a9d98 R08: 0000000000000000 R09: 0000000000000000
[  206.427076] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000001
[  206.427076] R13: 0000000000000000 R14: 0000000000000001 R15: ffff8808225a9e28
[  206.427076] FS:  00007f376c4ee6e0(0000) GS:ffff880054636000(0000) knlGS:0000000000000000
[  206.427076] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  206.427076] CR2: 00000000000000e8 CR3: 0000000822590000 CR4: 00000000000006e0
[  206.427076] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  206.427076] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  206.427076] Process bash (pid: 10367, threadinfo ffff8808225a8000, task ffff880822508000)
[  206.427076] Stack:
[  206.427076]  000000000000001f ffff88083addd5a0 ffff8808225a9d98 ffff88083ce36ca0
[  206.427076]  ffff88083c547800 ffff88083c547800 ffff8808225a9db8 ffffffff803ecee4
[  206.427076]  ffff88083c547800 ffff88083c547000 ffff8808225a9e08 ffffffff803ecf6d
[  206.427076] Call Trace:
[  206.427076]  [<ffffffff803ecee4>] acpi_bus_remove+0x54/0x68
[  206.427076]  [<ffffffff803ecf6d>] acpi_bus_trim+0x75/0xe3
[  206.427076]  [<ffffffffa0345ddd>] acpiphp_disable_slot+0x16d/0x1e0 [acpiphp]
[  206.427076]  [<ffffffffa03441f0>] disable_slot+0x20/0x60 [acpiphp]
[  206.427076]  [<ffffffff803cfc18>] power_write_file+0xc8/0x110
[  206.427076]  [<ffffffff803c6a54>] pci_slot_attr_store+0x24/0x30
[  206.427076]  [<ffffffff803469ce>] sysfs_write_file+0xce/0x140
[  206.427076]  [<ffffffff802e94e7>] vfs_write+0xc7/0x170
[  206.427076]  [<ffffffff802e9aa0>] sys_write+0x50/0x90
[  206.427076]  [<ffffffff8020bd6b>] system_call_fastpath+0x16/0x1b
[  206.427076] Code: be 2b 01 00 00 48 c7 c7 d0 c6 5a 80 31 c0 e8 c3 8f 01 00 eb 36 48 8b 55 e8 48 8b 42 10 48 83 78 18 00 74 13 48 8b 42 08 0f b7 3a <0f> b6 b0 e8 00 00 00 e8 ab f8 ff ff 48 8b 7d e8 e8 30 cf ee ff
[  206.427076] RIP  [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
[  206.427076]  RSP <ffff8808225a9d68>
[  206.427076] CR2: 00000000000000e8
[  206.440158] ---[ end trace 1ca3974fa717e665 ]---

Signed-off-by Kenji Kaneshige <[email protected]>

 drivers/acpi/pci_bind.c |   21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

Index: 20090521/drivers/acpi/pci_bind.c
===================================================================

drivers/acpi/pci_bind.c +0 -3

@@ -116,9 +116,6 @@ int acpi_pci_bind(struct acpi_device *de
 	struct acpi_pci_data *pdata;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	acpi_handle handle;
-	struct pci_dev *dev;
-	struct pci_bus *bus;
-
 
 	if (!device || !device->parent)
 		return -EINVAL;

drivers/acpi/pci_bind.c int acpi_pci_bind(struct acpi_device *de · line 178

Quoted 2 lines

drivers/acpi/pci_bind.c +0 -0

@@ -180,16 +177,8 @@ int acpi_pci_bind(struct acpi_device *de
 	 * PCI devices are added to the global pci list when the root
 	 * bridge start ops are run, which may not have happened yet.

Please update or remove this comment, which claims that "we cannot simply search the global pci device list." I don't know whether the comment (a) explains why we can't use pci_get_slot(), (b) explains why we can't use pci_find_slot() or some other interface, (c) refers to an ordering problem that doesn't exist on your system, or (d) is just no longer applicable at all.

Quoted 37 lines

drivers/acpi/pci_bind.c +6 ~~-12~~

 	 */
-	bus = pci_find_bus(data->id.segment, data->id.bus);
-	if (bus) {
-		list_for_each_entry(dev, &bus->devices, bus_list) {
-			if (dev->devfn == PCI_DEVFN(data->id.device,
-						    data->id.function)) {
-				data->dev = dev;
-				break;
-			}
-		}
-	}
+	data->dev = pci_get_slot(pdata->bus,
+				PCI_DEVFN(data->id.device, data->id.function));
 	if (!data->dev) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Device %04x:%02x:%02x.%d not present in PCI namespace\n",

@@ -259,9 +248,10 @@ int acpi_pci_bind(struct acpi_device *de
 
       end:
 	kfree(buffer.pointer);
-	if (result)
+	if (result) {
+		pci_dev_put(data->dev);
 		kfree(data);
-
+	}
 	return result;
 }

@@ -303,6 +293,7 @@ static int acpi_pci_unbind(struct acpi_d
 	if (data->dev->subordinate) {
 		acpi_pci_irq_del_prt(data->id.segment, data->bus->number);
 	}
+	pci_dev_put(data->dev);
 	kfree(data);
 
       end:

Alex Chiang

Reviewed-by Tested-by

2009-05-26 23:41 UTC #2 lore raw

Adding Len because this should probably go through his tree, not Jesse's.

Len, we're discussing a patch that I feel should go into 2.6.30, because it fixes an oops that I introduced in the beginning of the merge window, and that we've been working on since. We just now have a patch to fix it, along with another patch that I wrote:

	http://patchwork.kernel.org/patch/25296/

This current patch is here:

	http://patchwork.kernel.org/patch/25895/

Discussion follows.

* Bjorn Helgaas <[email protected]>:

Quoted 13 lines · Kenji Kaneshige

Quoted 7 lines
Fix wrong struct pci_dev reference counter handling in acpi_pci_bind().
The 'dev' field of struct acpi_pci_data is having a pointer to struct pci_dev without incrementing the reference counter. Because of this, I got the following kernel oops when I was doing some pci hotplug operations. This patch fixes this bug by replacing wrong hand-made pci_find_slot() with pci_get_slot() in acpi_pci_bind().
I don't like this ACPI/PCI bind thing in general because having the extra .bind and .unbind methods is ugly and somewhat non-obvious, and I'm nervous about object lifetime issues like this one.

Quoted 5 lines

But I don't have a better alternative to offer, and there's definitely a problem here, so thanks for fixing and testing it. I do have one question below about whether the comment in the existing code, which seems to be an excuse for doing the hand-made pci_find_slot(), is still relevant, or should just be removed.

I reviewed and successfully tested this patch on our ia64 machine.

Reviewed-by Alex Chiang <[email protected]>

Tested-by Alex Chiang <[email protected]>

Quoted 10 lines

drivers/acpi/pci_bind.c int acpi_pci_bind(struct acpi_device *de · line 178
Quoted 3 lines
drivers/acpi/pci_bind.c +0 -0
@@ -180,16 +177,8 @@ int acpi_pci_bind(struct acpi_device *de
 	 * PCI devices are added to the global pci list when the root
 	 * bridge start ops are run, which may not have happened yet.
Please update or remove this comment, which claims that "we cannot simply search the global pci device list." I don't know whether the comment (a) explains why we can't use pci_get_slot(), (b) explains why we can't use pci_find_slot() or some other interface, (c) refers to an ordering problem that doesn't exist on your system, or (d) is just no longer applicable at all.

I think the comment is simply no longer applicable.

During boot, we exercise this path in acpi_pci_root_add():

acpi_pci_root_add
pci_acpi_scan_root
acpi_pci_bind_root
acpi_pci_bridge_scan
acpi_pci_bind

pci_acpi_scan_root will create the PCI namespace for us before we attempt to bind the devices, so we know we will find the pci_dev on the pci_bus->devices list.

During hotplug, we exercise this path:

acpiphp_enable_slot
enable_device
pci_scan_slot
pci_scan_bridge acpiphp_bus_add
acpi_bus_add
acpi_add_single_object
acpi_pci_bind

pci_scan_slot() will put the new pci_devs onto pci_bus->devices, so by the time we get to acpi_pci_bind, the call to pci_get_slot will be successful.

There is the case where we hotplug a bridge device:

handle_hotplug_event_bridge
handle_bridge_insertion
acpi_bus_add
acpi_add_single_object
acpi_pci_bind

And this does confuse me a little bit, because I'm not seeing how the bridge device gets added to the parent pci_bus->devices list before we get to acpi_pci_bind, but...

Kenji's patch isn't changing the semantics on _how_ we find a device:

- bus = pci_find_bus(data->id.segment, data->id.bus);
- if (bus) {
- list_for_each_entry(dev, &bus->devices, bus_list) {
- if (dev->devfn == PCI_DEVFN(data->id.device,
- data->id.function)) {
- data->dev = dev;
- break;
- }
- }
- }
+ data->dev = pci_get_slot(pdata->bus,

+                               PCI_DEVFN(data->id.device, data->id.function));
        if (!data->dev) {
                ACPI_DEBUG_PRINT((ACPI_DB_INFO,
                                  "Device %04x:%02x:%02x.%d not present in PCI namespace\n",

pci_get_slot() iterates across bus->devices too (except that it correctly grabs the pci_bus_sem first) in addition to the obvious refcount on the pci_dev.

Given the above, I feel pretty comfortable with Kenji-san's change, and I'd recommend that he just get rid of that confusing comment entirely.

The only other suggestion I have is that he could trim down the oops output a bit to just get the stack trace:

BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
 IP: [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
 Call Trace:
  [<ffffffff803ecee4>] acpi_bus_remove+0x54/0x68
  [<ffffffff803ecf6d>] acpi_bus_trim+0x75/0xe3
  [<ffffffffa0345ddd>] acpiphp_disable_slot+0x16d/0x1e0 [acpiphp]
  [<ffffffffa03441f0>] disable_slot+0x20/0x60 [acpiphp]
  [<ffffffff803cfc18>] power_write_file+0xc8/0x110
  [<ffffffff803c6a54>] pci_slot_attr_store+0x24/0x30
  [<ffffffff803469ce>] sysfs_write_file+0xce/0x140
  [<ffffffff802e94e7>] vfs_write+0xc7/0x170
  [<ffffffff802e9aa0>] sys_write+0x50/0x90
  [<ffffffff8020bd6b>] system_call_fastpath+0x16/0x1b

Thanks.

/ac

Kenji Kaneshige

author

2009-05-26 23:44 UTC #6 lore raw

In reply to Bjorn Helgaas

Quoted 80 lines · Bjorn Helgaas

Quoted 7 lines · Kenji Kaneshige

Fix wrong struct pci_dev reference counter handling in acpi_pci_bind().
The 'dev' field of struct acpi_pci_data is having a pointer to struct pci_dev without incrementing the reference counter. Because of this, I got the following kernel oops when I was doing some pci hotplug operations. This patch fixes this bug by replacing wrong hand-made pci_find_slot() with pci_get_slot() in acpi_pci_bind().

I don't like this ACPI/PCI bind thing in general because having the extra .bind and .unbind methods is ugly and somewhat non-obvious, and I'm nervous about object lifetime issues like this one.

Bjorn

Quoted 59 lines

[  206.427004] BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
[  206.427076] IP: [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
[  206.427076] PGD 8225ad067 PUD 82258b067 PMD 0
[  206.427076] Oops: 0000 [#1] SMP
[  206.427076] last sysfs file: /sys/bus/pci/slots/1/power
[  206.427076] CPU 2
[  206.427076] Modules linked in: acpiphp ipv6 autofs4 hidp rfcomm l2cap bluetooth sunrpc dm_mirror dm_region_hash dm_log dm_multipath scsi_dh dm_mod sbs sbshc pci_slot battery ac parport_pc lp parport sg mptspi mptscsih mptbase scsi_transport_spi sr_mod cdrom e1000e serio_raw button i2c_i801 i2c_core shpchp pcspkr ata_piix libata megaraid_sas sd_mod scsi_mod crc_t10dif ext3 jbd uhci_hcd ohci_hcd ehci_hcd [last unloaded: microcode]
[  206.427076] Pid: 10367, comm: bash Not tainted 2.6.30-rc4-kk #10 PRIMERGY
[  206.427076] RIP: 0010:[<ffffffff803f0e9b>]  [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
[  206.427076] RSP: 0018:ffff8808225a9d68  EFLAGS: 00010206
[  206.427076] RAX: 0000000000000000 RBX: ffff88083c547800 RCX: 0000000000000006
[  206.427076] RDX: ffff88083ce36ca0 RSI: ffff880822508768 RDI: 0000000000000000
[  206.427076] RBP: ffff8808225a9d98 R08: 0000000000000000 R09: 0000000000000000
[  206.427076] R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000001
[  206.427076] R13: 0000000000000000 R14: 0000000000000001 R15: ffff8808225a9e28
[  206.427076] FS:  00007f376c4ee6e0(0000) GS:ffff880054636000(0000) knlGS:0000000000000000
[  206.427076] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  206.427076] CR2: 00000000000000e8 CR3: 0000000822590000 CR4: 00000000000006e0
[  206.427076] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  206.427076] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  206.427076] Process bash (pid: 10367, threadinfo ffff8808225a8000, task ffff880822508000)
[  206.427076] Stack:
[  206.427076]  000000000000001f ffff88083addd5a0 ffff8808225a9d98 ffff88083ce36ca0
[  206.427076]  ffff88083c547800 ffff88083c547800 ffff8808225a9db8 ffffffff803ecee4
[  206.427076]  ffff88083c547800 ffff88083c547000 ffff8808225a9e08 ffffffff803ecf6d
[  206.427076] Call Trace:
[  206.427076]  [<ffffffff803ecee4>] acpi_bus_remove+0x54/0x68
[  206.427076]  [<ffffffff803ecf6d>] acpi_bus_trim+0x75/0xe3
[  206.427076]  [<ffffffffa0345ddd>] acpiphp_disable_slot+0x16d/0x1e0 [acpiphp]
[  206.427076]  [<ffffffffa03441f0>] disable_slot+0x20/0x60 [acpiphp]
[  206.427076]  [<ffffffff803cfc18>] power_write_file+0xc8/0x110
[  206.427076]  [<ffffffff803c6a54>] pci_slot_attr_store+0x24/0x30
[  206.427076]  [<ffffffff803469ce>] sysfs_write_file+0xce/0x140
[  206.427076]  [<ffffffff802e94e7>] vfs_write+0xc7/0x170
[  206.427076]  [<ffffffff802e9aa0>] sys_write+0x50/0x90
[  206.427076]  [<ffffffff8020bd6b>] system_call_fastpath+0x16/0x1b
[  206.427076] Code: be 2b 01 00 00 48 c7 c7 d0 c6 5a 80 31 c0 e8 c3 8f 01 00 eb 36 48 8b 55 e8 48 8b 42 10 48 83 78 18 00 74 13 48 8b 42 08 0f b7 3a <0f> b6 b0 e8 00 00 00 e8 ab f8 ff ff 48 8b 7d e8 e8 30 cf ee ff
[  206.427076] RIP  [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
[  206.427076]  RSP <ffff8808225a9d68>
[  206.427076] CR2: 00000000000000e8
[  206.440158] ---[ end trace 1ca3974fa717e665 ]---

Signed-off-by Kenji Kaneshige <[email protected]>

 drivers/acpi/pci_bind.c |   21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

Index: 20090521/drivers/acpi/pci_bind.c
===================================================================

drivers/acpi/pci_bind.c +0 -3

@@ -116,9 +116,6 @@ int acpi_pci_bind(struct acpi_device *de
 	struct acpi_pci_data *pdata;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	acpi_handle handle;
-	struct pci_dev *dev;
-	struct pci_bus *bus;
-

Quoted 13 lines

drivers/acpi/pci_bind.c int acpi_pci_bind(struct acpi_device *de · line 178
Quoted 5 lines
drivers/acpi/pci_bind.c +0 -0
 	if (!device || !device->parent)
 		return -EINVAL;
@@ -180,16 +177,8 @@ int acpi_pci_bind(struct acpi_device *de
 	 * PCI devices are added to the global pci list when the root
 	 * bridge start ops are run, which may not have happened yet.
Please update or remove this comment, which claims that "we cannot simply search the global pci device list." I don't know whether the comment (a) explains why we can't use pci_get_slot(), (b) explains why we can't use pci_find_slot() or some other interface, (c) refers to an ordering problem that doesn't exist on your system, or (d) is just no longer applicable at all.

My answer is (d).

In the past, PCI core used to have two link lists to which struct pci_dev are linked, one is per PCI bus list and the another is global list. And the global list doesn't exist now, it was removed several months/years(?) ago. At the acpi_pci_bind() time, struct pci_dev is already on the per PCI bus list, but not on the global list (struct pci_dev used to be added to global list at .start time of ACPI PCI root driver). So I think this comment just explains we cannot find struct pci_dev on the global list at this point.

I'll make an additional patch to remove this obsolete comment.

Thanks,
Kenji Kaneshige

Kenji Kaneshige

author

2009-05-27 00:02 UTC #3 lore raw

In reply to Alex Chiang

Quoted 122 lines · Alex Chiang

Adding Len because this should probably go through his tree, not Jesse's.
Len, we're discussing a patch that I feel should go into 2.6.30, because it fixes an oops that I introduced in the beginning of the merge window, and that we've been working on since. We just now have a patch to fix it, along with another patch that I wrote:
	http://patchwork.kernel.org/patch/25296/
This current patch is here:
	http://patchwork.kernel.org/patch/25895/
Discussion follows.
* Bjorn Helgaas <[email protected]>:
Quoted 12 lines · Kenji Kaneshige
Quoted 7 lines
Fix wrong struct pci_dev reference counter handling in acpi_pci_bind().
The 'dev' field of struct acpi_pci_data is having a pointer to struct pci_dev without incrementing the reference counter. Because of this, I got the following kernel oops when I was doing some pci hotplug operations. This patch fixes this bug by replacing wrong hand-made pci_find_slot() with pci_get_slot() in acpi_pci_bind().
I don't like this ACPI/PCI bind thing in general because having the extra .bind and .unbind methods is ugly and somewhat non-obvious, and I'm nervous about object lifetime issues like this one.
Quoted 5 lines
But I don't have a better alternative to offer, and there's definitely a problem here, so thanks for fixing and testing it. I do have one question below about whether the comment in the existing code, which seems to be an excuse for doing the hand-made pci_find_slot(), is still relevant, or should just be removed.
I reviewed and successfully tested this patch on our ia64 machine.
Reviewed-by Alex Chiang <[email protected]>
Tested-by Alex Chiang <[email protected]>
Quoted 9 lines
drivers/acpi/pci_bind.c int acpi_pci_bind(struct acpi_device *de · line 178
Quoted 3 lines
drivers/acpi/pci_bind.c +0 -0
@@ -180,16 +177,8 @@ int acpi_pci_bind(struct acpi_device *de
 	 * PCI devices are added to the global pci list when the root
 	 * bridge start ops are run, which may not have happened yet.
Please update or remove this comment, which claims that "we cannot simply search the global pci device list." I don't know whether the comment (a) explains why we can't use pci_get_slot(), (b) explains why we can't use pci_find_slot() or some other interface, (c) refers to an ordering problem that doesn't exist on your system, or (d) is just no longer applicable at all.
I think the comment is simply no longer applicable.
During boot, we exercise this path in acpi_pci_root_add():
acpi_pci_root_add
pci_acpi_scan_root
acpi_pci_bind_root
acpi_pci_bridge_scan
acpi_pci_bind
pci_acpi_scan_root will create the PCI namespace for us before we attempt to bind the devices, so we know we will find the pci_dev on the pci_bus->devices list.
During hotplug, we exercise this path:
acpiphp_enable_slot
enable_device
pci_scan_slot
pci_scan_bridge acpiphp_bus_add
acpi_bus_add
acpi_add_single_object
acpi_pci_bind
pci_scan_slot() will put the new pci_devs onto pci_bus->devices, so by the time we get to acpi_pci_bind, the call to pci_get_slot will be successful.
There is the case where we hotplug a bridge device:
handle_hotplug_event_bridge
handle_bridge_insertion
acpi_bus_add
acpi_add_single_object
acpi_pci_bind
And this does confuse me a little bit, because I'm not seeing how the bridge device gets added to the parent pci_bus->devices list before we get to acpi_pci_bind, but...
Kenji's patch isn't changing the semantics on _how_ we find a device:
drivers/acpi/pci_bind.c +2 ~~-10~~
-       bus = pci_find_bus(data->id.segment, data->id.bus);
-       if (bus) {
-               list_for_each_entry(dev, &bus->devices, bus_list) {
-                       if (dev->devfn == PCI_DEVFN(data->id.device,
-                                                   data->id.function)) {
-                               data->dev = dev;
-                               break;
-                       }
-               }
-       }
+       data->dev = pci_get_slot(pdata->bus,
+                               PCI_DEVFN(data->id.device, data->id.function));
        if (!data->dev) {
                ACPI_DEBUG_PRINT((ACPI_DB_INFO,
                                  "Device %04x:%02x:%02x.%d not present in PCI namespace\n",
pci_get_slot() iterates across bus->devices too (except that it correctly grabs the pci_bus_sem first) in addition to the obvious refcount on the pci_dev.
Given the above, I feel pretty comfortable with Kenji-san's change, and I'd recommend that he just get rid of that confusing comment entirely.
The only other suggestion I have is that he could trim down the oops output a bit to just get the stack trace:

Quoted 14 lines

BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
 IP: [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd
 Call Trace:
  [<ffffffff803ecee4>] acpi_bus_remove+0x54/0x68
  [<ffffffff803ecf6d>] acpi_bus_trim+0x75/0xe3
  [<ffffffffa0345ddd>] acpiphp_disable_slot+0x16d/0x1e0 [acpiphp]
  [<ffffffffa03441f0>] disable_slot+0x20/0x60 [acpiphp]
  [<ffffffff803cfc18>] power_write_file+0xc8/0x110
  [<ffffffff803c6a54>] pci_slot_attr_store+0x24/0x30
  [<ffffffff803469ce>] sysfs_write_file+0xce/0x140
  [<ffffffff802e94e7>] vfs_write+0xc7/0x170
  [<ffffffff802e9aa0>] sys_write+0x50/0x90
  [<ffffffff8020bd6b>] system_call_fastpath+0x16/0x1b

Thank you very much for detailed analysis. I agree with you. I'll send an updated patch (instead of additional patch to remove obsolete comment), but it will be tomorrow because I'm day off today. Sorry...

Thanks,
Kenji Kaneshige

Len Brown

reviewer Reviewed-by Bjorn Helgaas Reviewed-by Alex Chiang Tested-by Alex Chiang

2009-05-27 21:56 UTC #4 lore raw

Quoted 1 line

From dacd2549ca61ddbdd1ed62a76ca95dea3f0e02c6 Mon Sep 17 00:00:00 2001

From: Kenji Kaneshige <[email protected]>
Date: Tue, 26 May 2009 09:08:03 +0900
Subject: [PATCH] PCI/ACPI: fix wrong ref count handling in acpi_pci_bind()

 BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
 IP: [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd

 Call Trace:
  [<ffffffff803ecee4>] acpi_bus_remove+0x54/0x68
  [<ffffffff803ecf6d>] acpi_bus_trim+0x75/0xe3
  [<ffffffffa0345ddd>] acpiphp_disable_slot+0x16d/0x1e0 [acpiphp]
  [<ffffffffa03441f0>] disable_slot+0x20/0x60 [acpiphp]
  [<ffffffff803cfc18>] power_write_file+0xc8/0x110
  [<ffffffff803c6a54>] pci_slot_attr_store+0x24/0x30
  [<ffffffff803469ce>] sysfs_write_file+0xce/0x140
  [<ffffffff802e94e7>] vfs_write+0xc7/0x170
  [<ffffffff802e9aa0>] sys_write+0x50/0x90
  [<ffffffff8020bd6b>] system_call_fastpath+0x16/0x1b

Signed-off-by Kenji Kaneshige <[email protected]>

Reviewed-by Bjorn Helgaas <[email protected]>

Reviewed-by Alex Chiang <[email protected]>

Tested-by Alex Chiang <[email protected]>

Signed-off-by Len Brown <[email protected]>

---

as applied to the acpi tree...

cheers,
-Len

 drivers/acpi/pci_bind.c |   24 ++++++------------------
 1 files changed, 6 insertions(+), 18 deletions(-)

drivers/acpi/pci_bind.c +6 ~~-18~~

index 95650f8..bc46de3 100644
@@ -116,9 +116,6 @@ int acpi_pci_bind(struct acpi_device *device)
 	struct acpi_pci_data *pdata;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	acpi_handle handle;
-	struct pci_dev *dev;
-	struct pci_bus *bus;
-
 
 	if (!device || !device->parent)
 		return -EINVAL;

@@ -176,20 +173,9 @@ int acpi_pci_bind(struct acpi_device *device)
 	 * Locate matching device in PCI namespace.  If it doesn't exist
 	 * this typically means that the device isn't currently inserted
 	 * (e.g. docking station, port replicator, etc.).
-	 * We cannot simply search the global pci device list, since
-	 * PCI devices are added to the global pci list when the root
-	 * bridge start ops are run, which may not have happened yet.
 	 */
-	bus = pci_find_bus(data->id.segment, data->id.bus);
-	if (bus) {
-		list_for_each_entry(dev, &bus->devices, bus_list) {
-			if (dev->devfn == PCI_DEVFN(data->id.device,
-						    data->id.function)) {
-				data->dev = dev;
-				break;
-			}
-		}
-	}
+	data->dev = pci_get_slot(pdata->bus,
+				PCI_DEVFN(data->id.device, data->id.function));
 	if (!data->dev) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Device %04x:%02x:%02x.%d not present in PCI namespace\n",

@@ -259,9 +245,10 @@ int acpi_pci_bind(struct acpi_device *device)
 
       end:
 	kfree(buffer.pointer);
-	if (result)
+	if (result) {
+		pci_dev_put(data->dev);
 		kfree(data);
-
+	}
 	return result;
 }

@@ -303,6 +290,7 @@ static int acpi_pci_unbind(struct acpi_device *device)
 	if (data->dev->subordinate) {
 		acpi_pci_irq_del_prt(data->id.segment, data->bus->number);
 	}
+	pci_dev_put(data->dev);
 	kfree(data);
 
       end:

Signature · 1 line

1.6.3.1.169.g33fd

Kenji Kaneshige

author

2009-05-28 00:09 UTC #5 lore raw

Thank you for updating the patch.
It looks good to me.

Thanks,
Kenji Kaneshige

Quoted 99 lines · Len Brown

Quoted 1 line

From dacd2549ca61ddbdd1ed62a76ca95dea3f0e02c6 Mon Sep 17 00:00:00 2001

From: Kenji Kaneshige <[email protected]>
Date: Tue, 26 May 2009 09:08:03 +0900
Subject: [PATCH] PCI/ACPI: fix wrong ref count handling in acpi_pci_bind()

 BUG: unable to handle kernel NULL pointer dereference at 00000000000000e8
 IP: [<ffffffff803f0e9b>] acpi_pci_unbind+0xb1/0xdd

 Call Trace:
  [<ffffffff803ecee4>] acpi_bus_remove+0x54/0x68
  [<ffffffff803ecf6d>] acpi_bus_trim+0x75/0xe3
  [<ffffffffa0345ddd>] acpiphp_disable_slot+0x16d/0x1e0 [acpiphp]
  [<ffffffffa03441f0>] disable_slot+0x20/0x60 [acpiphp]
  [<ffffffff803cfc18>] power_write_file+0xc8/0x110
  [<ffffffff803c6a54>] pci_slot_attr_store+0x24/0x30
  [<ffffffff803469ce>] sysfs_write_file+0xce/0x140
  [<ffffffff802e94e7>] vfs_write+0xc7/0x170
  [<ffffffff802e9aa0>] sys_write+0x50/0x90
  [<ffffffff8020bd6b>] system_call_fastpath+0x16/0x1b

Signed-off-by Kenji Kaneshige <[email protected]>

Reviewed-by Bjorn Helgaas <[email protected]>

Reviewed-by Alex Chiang <[email protected]>

Tested-by Alex Chiang <[email protected]>

Signed-off-by Len Brown <[email protected]>

---

as applied to the acpi tree...

cheers,

(quoted diff) +0 -1

-Len
 
 drivers/acpi/pci_bind.c |   24 ++++++------------------
 1 files changed, 6 insertions(+), 18 deletions(-)

drivers/acpi/pci_bind.c +6 ~~-18~~

index 95650f8..bc46de3 100644
@@ -116,9 +116,6 @@ int acpi_pci_bind(struct acpi_device *device)
 	struct acpi_pci_data *pdata;
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	acpi_handle handle;
-	struct pci_dev *dev;
-	struct pci_bus *bus;
-
 
 	if (!device || !device->parent)
 		return -EINVAL;

@@ -176,20 +173,9 @@ int acpi_pci_bind(struct acpi_device *device)
 	 * Locate matching device in PCI namespace.  If it doesn't exist
 	 * this typically means that the device isn't currently inserted
 	 * (e.g. docking station, port replicator, etc.).
-	 * We cannot simply search the global pci device list, since
-	 * PCI devices are added to the global pci list when the root
-	 * bridge start ops are run, which may not have happened yet.
 	 */
-	bus = pci_find_bus(data->id.segment, data->id.bus);
-	if (bus) {
-		list_for_each_entry(dev, &bus->devices, bus_list) {
-			if (dev->devfn == PCI_DEVFN(data->id.device,
-						    data->id.function)) {
-				data->dev = dev;
-				break;
-			}
-		}
-	}
+	data->dev = pci_get_slot(pdata->bus,
+				PCI_DEVFN(data->id.device, data->id.function));
 	if (!data->dev) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				  "Device %04x:%02x:%02x.%d not present in PCI namespace\n",

@@ -259,9 +245,10 @@ int acpi_pci_bind(struct acpi_device *device)
 
       end:
 	kfree(buffer.pointer);
-	if (result)
+	if (result) {
+		pci_dev_put(data->dev);
 		kfree(data);
-
+	}
 	return result;
 }

@@ -303,6 +290,7 @@ static int acpi_pci_unbind(struct acpi_device *device)
 	if (data->dev->subordinate) {
 		acpi_pci_irq_del_prt(data->id.segment, data->bus->number);
 	}
+	pci_dev_put(data->dev);
 	kfree(data);
 
       end: