Message-ID: <20080702020223.GA32046@ldl.fc.hp.com>
Date:	Tue, 1 Jul 2008 20:02:23 -0600
From:	Alex Chiang <achiang@...com>
To:	jbarnes@...tuousgeek.org, kristen.c.accardi@...el.com
Cc:	garyhade@...ibm.com, linux-pci@...r.kernel-org,
	linux-kernel@...r.kernel.org
Subject: [PATCH] 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 <achiang@...com>

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.

Signed-off-by: Alex Chiang <achiang@...com>
Signed-off-by: Gary Hade <garyhade@...ibm.com>
---
 drivers/pci/hotplug/acpiphp_glue.c |   17 ++++++++++++++---
 1 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 9342c84..a3e4705 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists