From: Rafael J. Wysocki <[email protected]>
After relatively recent changes in the ACPI-based PCI hotplug
(ACPIPHP) code, the acpiphp_check_host_bridge() executed for PCI
host bridges via acpi_pci_root_scan_dependent() doesn't do anything
useful, because those bridges do not have hotplug contexts. That
happens by mistake, so fix it by making acpiphp_enumerate_slots()
add hotplug contexts to PCI host bridges too and modify
acpiphp_remove_slots() to drop those contexts for host bridges
as appropriate.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=76901
Fixes: 2d8b1d566a5f (ACPI / hotplug / PCI: Get rid of check_sub_bridges())
Reported-and-tested-by: Gavin Guo <[email protected]>
Cc: 3.15+ <[email protected]> # 3.15+
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/pci/hotplug/acpiphp.h | 10 ++++++
drivers/pci/hotplug/acpiphp_glue.c | 60 +++++++++++++++++++++++++------------
2 files changed, 52 insertions(+), 18 deletions(-)
Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
@@ -373,17 +373,13 @@ static acpi_status acpiphp_add_context(a
static struct acpiphp_bridge *acpiphp_dev_to_bridge(struct acpi_device *adev)
{
- struct acpiphp_context *context;
struct acpiphp_bridge *bridge = NULL;
acpi_lock_hp_context();
- context = acpiphp_get_context(adev);
- if (context) {
- bridge = context->bridge;
+ if (adev->hp) {
+ bridge = to_acpiphp_root_context(adev->hp)->root_bridge;
if (bridge)
get_bridge(bridge);
-
- acpiphp_put_context(context);
}
acpi_unlock_hp_context();
return bridge;
@@ -881,7 +877,17 @@ void acpiphp_enumerate_slots(struct pci_
*/
get_device(&bus->dev);
- if (!pci_is_root_bus(bridge->pci_bus)) {
+ acpi_lock_hp_context();
+ if (pci_is_root_bus(bridge->pci_bus)) {
+ struct acpiphp_root_context *root_context;
+
+ root_context = kzalloc(sizeof(*root_context), GFP_KERNEL);
+ if (!root_context)
+ goto err;
+
+ root_context->root_bridge = bridge;
+ acpi_set_hp_context(adev, &root_context->hp, NULL, NULL, NULL);
+ } else {
struct acpiphp_context *context;
/*
@@ -890,21 +896,16 @@ void acpiphp_enumerate_slots(struct pci_
* parent is going to be handled by pciehp, in which case this
* bridge is not interesting to us either.
*/
- acpi_lock_hp_context();
context = acpiphp_get_context(adev);
- if (!context) {
- acpi_unlock_hp_context();
- put_device(&bus->dev);
- pci_dev_put(bridge->pci_dev);
- kfree(bridge);
- return;
- }
+ if (!context)
+ goto err;
+
bridge->context = context;
context->bridge = bridge;
/* Get a reference to the parent bridge. */
get_bridge(context->func.parent);
- acpi_unlock_hp_context();
}
+ acpi_unlock_hp_context();
/* Must be added to the list prior to calling acpiphp_add_context(). */
mutex_lock(&bridge_mutex);
@@ -919,6 +920,30 @@ void acpiphp_enumerate_slots(struct pci_
cleanup_bridge(bridge);
put_bridge(bridge);
}
+ return;
+
+ err:
+ acpi_unlock_hp_context();
+ put_device(&bus->dev);
+ pci_dev_put(bridge->pci_dev);
+ kfree(bridge);
+}
+
+void acpiphp_drop_bridge(struct acpiphp_bridge *bridge)
+{
+ if (pci_is_root_bus(bridge->pci_bus)) {
+ struct acpiphp_root_context *root_context;
+ struct acpi_device *adev;
+
+ acpi_lock_hp_context();
+ adev = ACPI_COMPANION(bridge->pci_bus->bridge);
+ root_context = to_acpiphp_root_context(adev->hp);
+ adev->hp = NULL;
+ acpi_unlock_hp_context();
+ kfree(root_context);
+ }
+ cleanup_bridge(bridge);
+ put_bridge(bridge);
}
/**
@@ -936,8 +961,7 @@ void acpiphp_remove_slots(struct pci_bus
list_for_each_entry(bridge, &bridge_list, list)
if (bridge->pci_bus == bus) {
mutex_unlock(&bridge_mutex);
- cleanup_bridge(bridge);
- put_bridge(bridge);
+ acpiphp_drop_bridge(bridge);
return;
}
Index: linux-pm/drivers/pci/hotplug/acpiphp.h
===================================================================
--- linux-pm.orig/drivers/pci/hotplug/acpiphp.h
+++ linux-pm/drivers/pci/hotplug/acpiphp.h
@@ -142,6 +142,16 @@ static inline acpi_handle func_to_handle
return func_to_acpi_device(func)->handle;
}
+struct acpiphp_root_context {
+ struct acpi_hotplug_context hp;
+ struct acpiphp_bridge *root_bridge;
+};
+
+static inline struct acpiphp_root_context *to_acpiphp_root_context(struct acpi_hotplug_context *hp)
+{
+ return container_of(hp, struct acpiphp_root_context, hp);
+}
+
/*
* struct acpiphp_attention_info - device specific attention registration
*
On Tue, Jun 10, 2014 at 2:51 PM, Rafael J. Wysocki <[email protected]> wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> After relatively recent changes in the ACPI-based PCI hotplug
> (ACPIPHP) code, the acpiphp_check_host_bridge() executed for PCI
> host bridges via acpi_pci_root_scan_dependent() doesn't do anything
> useful, because those bridges do not have hotplug contexts. That
> happens by mistake, so fix it by making acpiphp_enumerate_slots()
> add hotplug contexts to PCI host bridges too and modify
> acpiphp_remove_slots() to drop those contexts for host bridges
> as appropriate.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=76901
> Fixes: 2d8b1d566a5f (ACPI / hotplug / PCI: Get rid of check_sub_bridges())
> Reported-and-tested-by: Gavin Guo <[email protected]>
> Cc: 3.15+ <[email protected]> # 3.15+
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Acked-by: Bjorn Helgaas <[email protected]>
Rafael, do you want to merge this via your tree, since you merged the
original acpiphp rework?
I do have a small cleanup of acpiphp_glue.c in my queue, but it won't
conflict with this.
Thanks a lot for fixing this!
Bjorn
> ---
> drivers/pci/hotplug/acpiphp.h | 10 ++++++
> drivers/pci/hotplug/acpiphp_glue.c | 60 +++++++++++++++++++++++++------------
> 2 files changed, 52 insertions(+), 18 deletions(-)
>
> Index: linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp_glue.c
> +++ linux-pm/drivers/pci/hotplug/acpiphp_glue.c
> @@ -373,17 +373,13 @@ static acpi_status acpiphp_add_context(a
>
> static struct acpiphp_bridge *acpiphp_dev_to_bridge(struct acpi_device *adev)
> {
> - struct acpiphp_context *context;
> struct acpiphp_bridge *bridge = NULL;
>
> acpi_lock_hp_context();
> - context = acpiphp_get_context(adev);
> - if (context) {
> - bridge = context->bridge;
> + if (adev->hp) {
> + bridge = to_acpiphp_root_context(adev->hp)->root_bridge;
> if (bridge)
> get_bridge(bridge);
> -
> - acpiphp_put_context(context);
> }
> acpi_unlock_hp_context();
> return bridge;
> @@ -881,7 +877,17 @@ void acpiphp_enumerate_slots(struct pci_
> */
> get_device(&bus->dev);
>
> - if (!pci_is_root_bus(bridge->pci_bus)) {
> + acpi_lock_hp_context();
> + if (pci_is_root_bus(bridge->pci_bus)) {
> + struct acpiphp_root_context *root_context;
> +
> + root_context = kzalloc(sizeof(*root_context), GFP_KERNEL);
> + if (!root_context)
> + goto err;
> +
> + root_context->root_bridge = bridge;
> + acpi_set_hp_context(adev, &root_context->hp, NULL, NULL, NULL);
> + } else {
> struct acpiphp_context *context;
>
> /*
> @@ -890,21 +896,16 @@ void acpiphp_enumerate_slots(struct pci_
> * parent is going to be handled by pciehp, in which case this
> * bridge is not interesting to us either.
> */
> - acpi_lock_hp_context();
> context = acpiphp_get_context(adev);
> - if (!context) {
> - acpi_unlock_hp_context();
> - put_device(&bus->dev);
> - pci_dev_put(bridge->pci_dev);
> - kfree(bridge);
> - return;
> - }
> + if (!context)
> + goto err;
> +
> bridge->context = context;
> context->bridge = bridge;
> /* Get a reference to the parent bridge. */
> get_bridge(context->func.parent);
> - acpi_unlock_hp_context();
> }
> + acpi_unlock_hp_context();
>
> /* Must be added to the list prior to calling acpiphp_add_context(). */
> mutex_lock(&bridge_mutex);
> @@ -919,6 +920,30 @@ void acpiphp_enumerate_slots(struct pci_
> cleanup_bridge(bridge);
> put_bridge(bridge);
> }
> + return;
> +
> + err:
> + acpi_unlock_hp_context();
> + put_device(&bus->dev);
> + pci_dev_put(bridge->pci_dev);
> + kfree(bridge);
> +}
> +
> +void acpiphp_drop_bridge(struct acpiphp_bridge *bridge)
> +{
> + if (pci_is_root_bus(bridge->pci_bus)) {
> + struct acpiphp_root_context *root_context;
> + struct acpi_device *adev;
> +
> + acpi_lock_hp_context();
> + adev = ACPI_COMPANION(bridge->pci_bus->bridge);
> + root_context = to_acpiphp_root_context(adev->hp);
> + adev->hp = NULL;
> + acpi_unlock_hp_context();
> + kfree(root_context);
> + }
> + cleanup_bridge(bridge);
> + put_bridge(bridge);
> }
>
> /**
> @@ -936,8 +961,7 @@ void acpiphp_remove_slots(struct pci_bus
> list_for_each_entry(bridge, &bridge_list, list)
> if (bridge->pci_bus == bus) {
> mutex_unlock(&bridge_mutex);
> - cleanup_bridge(bridge);
> - put_bridge(bridge);
> + acpiphp_drop_bridge(bridge);
> return;
> }
>
> Index: linux-pm/drivers/pci/hotplug/acpiphp.h
> ===================================================================
> --- linux-pm.orig/drivers/pci/hotplug/acpiphp.h
> +++ linux-pm/drivers/pci/hotplug/acpiphp.h
> @@ -142,6 +142,16 @@ static inline acpi_handle func_to_handle
> return func_to_acpi_device(func)->handle;
> }
>
> +struct acpiphp_root_context {
> + struct acpi_hotplug_context hp;
> + struct acpiphp_bridge *root_bridge;
> +};
> +
> +static inline struct acpiphp_root_context *to_acpiphp_root_context(struct acpi_hotplug_context *hp)
> +{
> + return container_of(hp, struct acpiphp_root_context, hp);
> +}
> +
> /*
> * struct acpiphp_attention_info - device specific attention registration
> *
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 10, 2014 at 10:51:51PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> After relatively recent changes in the ACPI-based PCI hotplug
> (ACPIPHP) code, the acpiphp_check_host_bridge() executed for PCI
> host bridges via acpi_pci_root_scan_dependent() doesn't do anything
> useful, because those bridges do not have hotplug contexts. That
> happens by mistake, so fix it by making acpiphp_enumerate_slots()
> add hotplug contexts to PCI host bridges too and modify
> acpiphp_remove_slots() to drop those contexts for host bridges
> as appropriate.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=76901
> Fixes: 2d8b1d566a5f (ACPI / hotplug / PCI: Get rid of check_sub_bridges())
> Reported-and-tested-by: Gavin Guo <[email protected]>
> Cc: 3.15+ <[email protected]> # 3.15+
> Signed-off-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Thanks for taking care of this!
On Wed, Jun 11, 2014 at 4:29 AM, Bjorn Helgaas <[email protected]> wrote:
> On Tue, Jun 10, 2014 at 2:51 PM, Rafael J. Wysocki <[email protected]> wrote:
>> From: Rafael J. Wysocki <[email protected]>
>>
>> After relatively recent changes in the ACPI-based PCI hotplug
>> (ACPIPHP) code, the acpiphp_check_host_bridge() executed for PCI
>> host bridges via acpi_pci_root_scan_dependent() doesn't do anything
>> useful, because those bridges do not have hotplug contexts. That
>> happens by mistake, so fix it by making acpiphp_enumerate_slots()
>> add hotplug contexts to PCI host bridges too and modify
>> acpiphp_remove_slots() to drop those contexts for host bridges
>> as appropriate.
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=76901
>> Fixes: 2d8b1d566a5f (ACPI / hotplug / PCI: Get rid of check_sub_bridges())
>> Reported-and-tested-by: Gavin Guo <[email protected]>
>> Cc: 3.15+ <[email protected]> # 3.15+
>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>
> Acked-by: Bjorn Helgaas <[email protected]>
Thanks!
> Rafael, do you want to merge this via your tree, since you merged the
> original acpiphp rework?
Yes, I'll take care of this.
> I do have a small cleanup of acpiphp_glue.c in my queue, but it won't
> conflict with this.
>
> Thanks a lot for fixing this!
You're very welcome. :-)
Rafael