diff mbox

[v2,2/3] PCI Hotplug: acpiphp: grab refcount on p2p subordinate bus

Message ID 20090330165014.18855.67275.stgit@bob.kio (mailing list archive)
State Accepted, archived
Headers show

Commit Message

Alexander Chiang March 30, 2009, 4:50 p.m. UTC
If a logical hot unplug (remove) is performed on a bridge claimed
by acpiphp and then acpiphp is unloaded, we will encounter an oops.

This is because acpiphp will access the bridge's subordinate bus,
which was released by the user's prior hot unplug.

The solution is to grab a reference on the subordinate PCI bus.
This will prevent the bus from release until acpiphp is unloaded.

Reported-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Signed-off-by: Alex Chiang <achiang@hp.com>
---

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


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Kenji Kaneshige March 31, 2009, 4:37 a.m. UTC | #1
I confirmed this patch fix the kernel oops problem I reported.

Reviewed-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
Tested-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

By the way, /sys/bus/pci/slots/<slot> directory by acpiphp are
remaining even after the parent bridge/bus of the slots are
removed. At this point, acpiphp is working with struct pci_bus
for the already disabled pci bus. I guess some operation against
the files under /sys/bus/pci/slots/<slot> directory would cause
something problems. So I think we also need something mechanism
to unregister acpiphp slots when the parent bus is removed.

Thanks,
Kenji Kaneshige


Alex Chiang wrote:
> If a logical hot unplug (remove) is performed on a bridge claimed
> by acpiphp and then acpiphp is unloaded, we will encounter an oops.
> 
> This is because acpiphp will access the bridge's subordinate bus,
> which was released by the user's prior hot unplug.
> 
> The solution is to grab a reference on the subordinate PCI bus.
> This will prevent the bus from release until acpiphp is unloaded.
> 
> Reported-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> Signed-off-by: Alex Chiang <achiang@hp.com>
> ---
> 
>  drivers/pci/hotplug/acpiphp_glue.c |   14 ++++++++++++++
>  1 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 803d9dd..a33794d 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -38,6 +38,8 @@
>   *  - The one in acpiphp_bridge has its refcount elevated by pci_get_slot()
>   *    when the bridge is scanned and it loses a refcount when the bridge
>   *    is removed.
> + *  - When a P2P bridge is present, we elevate the refcount on the subordinate
> + *    bus. It loses the refcount when the the driver unloads.
>   */
>  
>  #include <linux/init.h>
> @@ -440,6 +442,12 @@ static void add_p2p_bridge(acpi_handle *handle, struct pci_dev *pci_dev)
>  		goto err;
>  	}
>  
> +	/*
> +	 * Grab a ref to the subordinate PCI bus in case the bus is
> +	 * removed via PCI core logical hotplug. The ref pins the bus
> +	 * (which we access during module unload).
> +	 */
> +	get_device(&bridge->pci_bus->dev);
>  	spin_lock_init(&bridge->res_lock);
>  
>  	init_bridge_misc(bridge);
> @@ -619,6 +627,12 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge)
>  		slot = next;
>  	}
>  
> +	/*
> +	 * Only P2P bridges have a pci_dev
> +	 */
> +	if (bridge->pci_dev)
> +		put_device(&bridge->pci_bus->dev);
> +
>  	pci_dev_put(bridge->pci_dev);
>  	list_del(&bridge->list);
>  	kfree(bridge);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 803d9dd..a33794d 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -38,6 +38,8 @@ 
  *  - The one in acpiphp_bridge has its refcount elevated by pci_get_slot()
  *    when the bridge is scanned and it loses a refcount when the bridge
  *    is removed.
+ *  - When a P2P bridge is present, we elevate the refcount on the subordinate
+ *    bus. It loses the refcount when the the driver unloads.
  */
 
 #include <linux/init.h>
@@ -440,6 +442,12 @@  static void add_p2p_bridge(acpi_handle *handle, struct pci_dev *pci_dev)
 		goto err;
 	}
 
+	/*
+	 * Grab a ref to the subordinate PCI bus in case the bus is
+	 * removed via PCI core logical hotplug. The ref pins the bus
+	 * (which we access during module unload).
+	 */
+	get_device(&bridge->pci_bus->dev);
 	spin_lock_init(&bridge->res_lock);
 
 	init_bridge_misc(bridge);
@@ -619,6 +627,12 @@  static void cleanup_bridge(struct acpiphp_bridge *bridge)
 		slot = next;
 	}
 
+	/*
+	 * Only P2P bridges have a pci_dev
+	 */
+	if (bridge->pci_dev)
+		put_device(&bridge->pci_bus->dev);
+
 	pci_dev_put(bridge->pci_dev);
 	list_del(&bridge->list);
 	kfree(bridge);