PCI Hotplug: acpiphp: cleanup notify handler on all root bridges

Hi Jesse, Kristen,

During the development of the physical PCI slot patch series, Gary Hade kept on reporting strange oopses due to interactions between pci_slot and acpiphp.

	http://lkml.org/lkml/2007/11/28/319

He got busy and went away for a while, and that's when I was able to sneak my patchset into Jesse's linux-next branch. ;)

Recently, Gary got some time to test again on his x3950 M2 system, and together, we finally figured out the oops.

This bug has always been present in acpiphp so it's not a regression. So if you want to hold off until the next merge window, I'm ok with that.

Otherwise, I feel it's pretty low-risk and could go into the next -rc. Totally your call, I have no strong feelings either way.

Incidentally, figuring out this oops makes me feel a lot more confident about the pci_slot changes, as this has been in the back of my mind for a while. (famous last words?)

Thanks,

/ac

From: Alex Chiang <[email protected]>

find_root_bridges() unconditionally installs handle_hotplug_event_bridge() as an ACPI_SYSTEM_NOTIFY handler for all root bridges.

However, during module cleanup, remove_bridge() will only remove the notify handler iff the root bridge had a hot-pluggable slot directly underneath. That is:

	root bridge -> hotplug slot

But, if the topology looks like either of the following:

	root bridge -> non-hotplug slot
	root bridge -> p2p bridge -> hotplug slot

Then we currently do not remove the notify handler from that root bridge.

This can cause a kernel oops if we modprobe acpiphp later and it gets loaded somewhere else in memory. If the root bridge then receives a hotplug event, it will then attempt to call a stale, non-existent notify handler and we blow up.

Much thanks goes to Gary Hade for his persistent debugging efforts.

---
 drivers/pci/hotplug/acpiphp_glue.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

index 9342c84..a3e4705 100644
@@ -705,9 +705,10 @@ cleanup_p2p_bridge(acpi_handle handle, u32 lvl, void *context, void **rv)
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, (u32)1,
 				cleanup_p2p_bridge, NULL, NULL);
 
-	if (!(bridge = acpiphp_handle_to_bridge(handle)))
-		return AE_OK;
-	cleanup_bridge(bridge);
+	bridge = acpiphp_handle_to_bridge(handle);
+	if (bridge)
+		cleanup_bridge(bridge);
+
 	return AE_OK;
 }
 

@@ -720,9 +721,19 @@ static void remove_bridge(acpi_handle handle)
 	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle,
 				(u32)1, cleanup_p2p_bridge, NULL, NULL);
 
+	/*
+	 * On root bridges with hotplug slots directly underneath (ie,
+	 * no p2p bridge inbetween), we call cleanup_bridge(). 
+	 *
+	 * The else clause cleans up root bridges that either had no
+	 * hotplug slots at all, or had a p2p bridge underneath.
+	 */
 	bridge = acpiphp_handle_to_bridge(handle);
 	if (bridge)
 		cleanup_bridge(bridge);
+	else
+		acpi_remove_notify_handler(handle, ACPI_SYSTEM_NOTIFY,
+					   handle_hotplug_event_bridge);
 }
 
 static struct pci_dev * get_apic_pci_info(acpi_handle handle)

1.5.3.1.1.g1e61

given that it fixes some pretty bad behavior, I went ahead and pushed this into my pull request for Linus. Thanks for cleaning it up, the hotplug code is looking much better these days thanks to yours and Kenji-san's efforts.

Jesse