[PATCH v2 0/3] PCI core logical hotplug cleanup

[PATCH v2 0/3] PCI core logical hotplug cleanup

[Alex Chiang]

This is v2 of cleaning up the fallout between core logical hotplug and
physical hotplug drivers.

Thanks.

/ac

v1->v2:
	- clarify changelog
	- just use struct pci_slot->bus directly

---

Alex Chiang (3):
      PCI: pci_slot: grab refcount on slot's bus
      PCI Hotplug: acpiphp: grab refcount on p2p subordinate bus
      PCI: allow PCI core hotplug to remove PCI root bus


 drivers/acpi/pci_slot.c            |    5 +++++
 drivers/pci/hotplug/acpiphp_glue.c |   14 ++++++++++++++
 drivers/pci/pci-sysfs.c            |    4 ----
 3 files changed, 19 insertions(+), 4 deletions(-)

[PATCH v2 1/3] PCI: allow PCI core hotplug to remove PCI root bus

[Alex Chiang]

There is no reason to prevent removal of root bus devices. A subsequent
rescan will find them just fine.

Signed-off-by: Alex Chiang <achiang@hp.com>
---

 drivers/pci/pci-sysfs.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index e9a8706..7b2cb27 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -277,14 +277,10 @@ remove_store(struct device *dev, struct device_attribute *dummy,
 {
 	int ret = 0;
 	unsigned long val;
-	struct pci_dev *pdev = to_pci_dev(dev);
 
 	if (strict_strtoul(buf, 0, &val) < 0)
 		return -EINVAL;
 
-	if (pci_is_root_bus(pdev->bus))
-		return -EBUSY;
-
 	/* An attribute cannot be unregistered by one of its own methods,
 	 * so we have to use this roundabout approach.
 	 */

Re: [PATCH v2 1/3] PCI: allow PCI core hotplug to remove PCI root bus

[Kenji Kaneshige]

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

Thanks,
Kenji Kaneshige


Alex Chiang wrote:
> There is no reason to prevent removal of root bus devices. A subsequent
> rescan will find them just fine.
> 
> Signed-off-by: Alex Chiang <achiang@hp.com>
> ---
> 
>  drivers/pci/pci-sysfs.c |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index e9a8706..7b2cb27 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -277,14 +277,10 @@ remove_store(struct device *dev, struct device_attribute *dummy,
>  {
>  	int ret = 0;
>  	unsigned long val;
> -	struct pci_dev *pdev = to_pci_dev(dev);
>  
>  	if (strict_strtoul(buf, 0, &val) < 0)
>  		return -EINVAL;
>  
> -	if (pci_is_root_bus(pdev->bus))
> -		return -EBUSY;
> -
>  	/* An attribute cannot be unregistered by one of its own methods,
>  	 * so we have to use this roundabout approach.
>  	 */
> 
> --
> 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
> 
> 

Re: [PATCH v2 1/3] PCI: allow PCI core hotplug to remove PCI root bus

[Jesse Barnes]

On Mon, 30 Mar 2009 10:50:09 -0600
Alex Chiang <achiang@hp.com> wrote:

> There is no reason to prevent removal of root bus devices. A
> subsequent rescan will find them just fine.
> 
> Signed-off-by: Alex Chiang <achiang@hp.com>

Applied this series, thanks.

-- 
Jesse Barnes, Intel Open Source Technology Center

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

[Alex Chiang]

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);

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

[Kenji Kaneshige]

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

[PATCH v2 3/3] PCI: pci_slot: grab refcount on slot's bus

[Alex Chiang]

If a logical hot unplug (remove) is performed on a physical PCI slot's
parent bridge, and then pci_slot is unloaded, we will encounter an oops:

  [<ffffffff803a788a>] kobject_release+0x9a/0x290
  [<ffffffff803a77f0>] ? kobject_release+0x0/0x290
  [<ffffffff803a8ce7>] kref_put+0x37/0x80
  [<ffffffff803a76f7>] kobject_put+0x27/0x60
  [<ffffffff803bebcc>] ? pci_destroy_slot+0x3c/0xc0
  [<ffffffff803bebd5>] pci_destroy_slot+0x45/0xc0
  [<ffffffffa000f05c>] acpi_pci_slot_remove+0x5c/0x91 [pci_slot]
  [<ffffffff8040064b>] acpi_pci_unregister_driver+0x4b/0x62
  [<ffffffffa000f5c8>] acpi_pci_slot_exit+0x10/0x12 [pci_slot]
  [<ffffffff80276ce1>] sys_delete_module+0x161/0x250

We need to grab a reference to the parent PCI bus, which will pin
the bus and prevent it from being released until pci_slot is unloaded.

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

 drivers/acpi/pci_slot.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index cd1f446..12158e0 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -164,6 +164,8 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	list_add(&slot->list, &slot_list);
 	mutex_unlock(&slot_list_lock);
 
+	get_device(&pci_bus->dev);
+
 	dbg("pci_slot: %p, pci_bus: %x, device: %d, name: %s\n",
 		pci_slot, pci_bus->number, device, name);
 
@@ -310,12 +312,15 @@ static void
 acpi_pci_slot_remove(acpi_handle handle)
 {
 	struct acpi_pci_slot *slot, *tmp;
+	struct pci_bus *pbus;
 
 	mutex_lock(&slot_list_lock);
 	list_for_each_entry_safe(slot, tmp, &slot_list, list) {
 		if (slot->root_handle == handle) {
 			list_del(&slot->list);
+			pbus = slot->pci_slot->bus;
 			pci_destroy_slot(slot->pci_slot);
+			put_device(&pbus->dev);
 			kfree(slot);
 		}
 	}

Re: [PATCH v2 3/3] PCI: pci_slot: grab refcount on slot's bus

[Kenji Kaneshige]

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 pci_slot are
remaining even after the parent bridge/bus of the slots are
removed. At this point, pci_slot is working with struct pci_bus
for the already disabled pci bus. This might cause something
problem. So I think we also need something mechanism to
unregister slots by pci_slot when the parent bus is removed.

Thanks,
Kenji Kaneshige


Alex Chiang wrote:
> If a logical hot unplug (remove) is performed on a physical PCI slot's
> parent bridge, and then pci_slot is unloaded, we will encounter an oops:
> 
>   [<ffffffff803a788a>] kobject_release+0x9a/0x290
>   [<ffffffff803a77f0>] ? kobject_release+0x0/0x290
>   [<ffffffff803a8ce7>] kref_put+0x37/0x80
>   [<ffffffff803a76f7>] kobject_put+0x27/0x60
>   [<ffffffff803bebcc>] ? pci_destroy_slot+0x3c/0xc0
>   [<ffffffff803bebd5>] pci_destroy_slot+0x45/0xc0
>   [<ffffffffa000f05c>] acpi_pci_slot_remove+0x5c/0x91 [pci_slot]
>   [<ffffffff8040064b>] acpi_pci_unregister_driver+0x4b/0x62
>   [<ffffffffa000f5c8>] acpi_pci_slot_exit+0x10/0x12 [pci_slot]
>   [<ffffffff80276ce1>] sys_delete_module+0x161/0x250
> 
> We need to grab a reference to the parent PCI bus, which will pin
> the bus and prevent it from being released until pci_slot is unloaded.
> 
> Cc: lenb@kernel.org
> Reported-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> Signed-off-by: Alex Chiang <achiang@hp.com>
> ---
> 
>  drivers/acpi/pci_slot.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> index cd1f446..12158e0 100644
> --- a/drivers/acpi/pci_slot.c
> +++ b/drivers/acpi/pci_slot.c
> @@ -164,6 +164,8 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>  	list_add(&slot->list, &slot_list);
>  	mutex_unlock(&slot_list_lock);
>  
> +	get_device(&pci_bus->dev);
> +
>  	dbg("pci_slot: %p, pci_bus: %x, device: %d, name: %s\n",
>  		pci_slot, pci_bus->number, device, name);
>  
> @@ -310,12 +312,15 @@ static void
>  acpi_pci_slot_remove(acpi_handle handle)
>  {
>  	struct acpi_pci_slot *slot, *tmp;
> +	struct pci_bus *pbus;
>  
>  	mutex_lock(&slot_list_lock);
>  	list_for_each_entry_safe(slot, tmp, &slot_list, list) {
>  		if (slot->root_handle == handle) {
>  			list_del(&slot->list);
> +			pbus = slot->pci_slot->bus;
>  			pci_destroy_slot(slot->pci_slot);
> +			put_device(&pbus->dev);
>  			kfree(slot);
>  		}
>  	}
> 
> --
> 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
> 
>