[PATCH 0/3] PCI core logical hotplug cleanup

[PATCH 0/3] PCI core logical hotplug cleanup

[Alex Chiang]

Hi Jesse,

This series cleans up some of the fallout from the poor interaction
between PCI core logical hotplug and acpiphp/pci_slot.

I'm taking a piecemeal approach for now because I don't have a good
solution that can fix all the drivers at once.

I tested pci_slot and acpiphp on my machine, and it seems to solve the
issue that Kenji-san reported.

Thanks.

/ac

---

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 1/3] PCI: allow PCI core hotplug to remove PCI root bus

[Alex Chiang]

There is no reason to prevent root bus removal. We never actually
remove the node from the pci_root_buses list, so a rescan will correctly
rediscover the root bus.

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 1/3] PCI: allow PCI core hotplug to remove PCI root bus

[Kenji Kaneshige]

Alex Chiang wrote:
> There is no reason to prevent root bus removal. We never actually
> remove the node from the pci_root_buses list, so a rescan will correctly
> rediscover the root bus.
> 

I'm a little confused about the description. I don't think the
patch is for allowing pci root bus removal. I think it is for
allowing removal of pci devices on pci root buses.

Thanks,
Kenji Kaneshige


> 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 1/3] PCI: allow PCI core hotplug to remove PCI root bus

[Alex Chiang]

* Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
> Alex Chiang wrote:
> > There is no reason to prevent root bus removal. We never actually
> > remove the node from the pci_root_buses list, so a rescan will correctly
> > rediscover the root bus.
> > 
> 
> I'm a little confused about the description. I don't think the
> patch is for allowing pci root bus removal. I think it is for
> allowing removal of pci devices on pci root buses.

Thanks for the review. I'll change the changelog text.

/ac

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

[PATCH 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..c7ad9f2 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -63,6 +63,7 @@ struct acpi_pci_slot {
 	acpi_handle root_handle;	/* handle of the root bridge */
 	struct pci_slot *pci_slot;	/* corresponding pci_slot */
 	struct list_head list;		/* node in the list of slots */
+	struct pci_bus *bus;		/* bus the slot is on */
 };
 
 static int acpi_pci_slot_add(acpi_handle handle);
@@ -159,11 +160,14 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 
 	slot->root_handle = parent_context->root_handle;
 	slot->pci_slot = pci_slot;
+	slot->bus = pci_bus;
 	INIT_LIST_HEAD(&slot->list);
 	mutex_lock(&slot_list_lock);
 	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);
 
@@ -316,6 +320,7 @@ acpi_pci_slot_remove(acpi_handle handle)
 		if (slot->root_handle == handle) {
 			list_del(&slot->list);
 			pci_destroy_slot(slot->pci_slot);
+			put_device(&slot->bus->dev);
 			kfree(slot);
 		}
 	}

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

[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..c7ad9f2 100644
> --- a/drivers/acpi/pci_slot.c
> +++ b/drivers/acpi/pci_slot.c
> @@ -63,6 +63,7 @@ struct acpi_pci_slot {
>  	acpi_handle root_handle;	/* handle of the root bridge */
>  	struct pci_slot *pci_slot;	/* corresponding pci_slot */
>  	struct list_head list;		/* node in the list of slots */
> +	struct pci_bus *bus;		/* bus the slot is on */
>  };

I don't think we need an additional 'bus' field in the struct
acpi_pci_slot. At the register_slot(), we already know the
address of struct pci_bus. At the acpi_pci_slot_remove(), I
think we can get the address of struct pci_bus as follows.

		if (slot->root_handle == handle) {
			struct pci_bus *pbus = slot->pci_slot->bus;
			list_del(&slot->list);
			pci_destroy_slot(slot->pci_slot);
			put_device(&pbus->dev);
			kfree(slot);
		}

Thanks,
Kenji Kaneshige


>  
>  static int acpi_pci_slot_add(acpi_handle handle);
> @@ -159,11 +160,14 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
>  
>  	slot->root_handle = parent_context->root_handle;
>  	slot->pci_slot = pci_slot;
> +	slot->bus = pci_bus;
>  	INIT_LIST_HEAD(&slot->list);
>  	mutex_lock(&slot_list_lock);
>  	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);
>  
> @@ -316,6 +320,7 @@ acpi_pci_slot_remove(acpi_handle handle)
>  		if (slot->root_handle == handle) {
>  			list_del(&slot->list);
>  			pci_destroy_slot(slot->pci_slot);
> +			put_device(&slot->bus->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
> 
> 

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

[Alex Chiang]

* Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
> Alex Chiang wrote:
> > diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
> > index cd1f446..c7ad9f2 100644
> > --- a/drivers/acpi/pci_slot.c
> > +++ b/drivers/acpi/pci_slot.c
> > @@ -63,6 +63,7 @@ struct acpi_pci_slot {
> >  	acpi_handle root_handle;	/* handle of the root bridge */
> >  	struct pci_slot *pci_slot;	/* corresponding pci_slot */
> >  	struct list_head list;		/* node in the list of slots */
> > +	struct pci_bus *bus;		/* bus the slot is on */
> >  };
> 
> I don't think we need an additional 'bus' field in the struct
> acpi_pci_slot. At the register_slot(), we already know the
> address of struct pci_bus. At the acpi_pci_slot_remove(), I
> think we can get the address of struct pci_bus as follows.
> 
> 		if (slot->root_handle == handle) {
> 			struct pci_bus *pbus = slot->pci_slot->bus;
> 			list_del(&slot->list);
> 			pci_destroy_slot(slot->pci_slot);
> 			put_device(&pbus->dev);
> 			kfree(slot);
> 		}

Ok, that is a little cleaner, I'll do it your way.

Thanks for the review.

/ac