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(-)
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: [email protected]
Reported-by: Kenji Kaneshige <[email protected]>
Signed-off-by: Alex Chiang <[email protected]>
---
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);
}
}
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 <[email protected]>
Signed-off-by: Alex Chiang <[email protected]>
---
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);
There is no reason to prevent removal of root bus devices. A subsequent
rescan will find them just fine.
Signed-off-by: Alex Chiang <[email protected]>
---
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.
*/
Reviewed-by: Kenji Kaneshige <[email protected]>
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 <[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
I confirmed this patch fix the kernel oops problem I reported.
Reviewed-by: Kenji Kaneshige <[email protected]>
Tested-by: Kenji Kaneshige <[email protected]>
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 <[email protected]>
> Signed-off-by: Alex Chiang <[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
I confirmed this patch fix the kernel oops problem I reported.
Reviewed-by: Kenji Kaneshige <[email protected]>
Tested-by: Kenji Kaneshige <[email protected]>
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: [email protected]
> Reported-by: Kenji Kaneshige <[email protected]>
> Signed-off-by: Alex Chiang <[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
On Mon, 30 Mar 2009 10:50:09 -0600
Alex Chiang <[email protected]> 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 <[email protected]>
Applied this series, thanks.
--
Jesse Barnes, Intel Open Source Technology Center