From patchwork Tue Feb 26 15:25:52 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jiang Liu X-Patchwork-Id: 2185851 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 8D3B8DFF33 for ; Tue, 26 Feb 2013 15:33:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932328Ab3BZPck (ORCPT ); Tue, 26 Feb 2013 10:32:40 -0500 Received: from mail-pb0-f52.google.com ([209.85.160.52]:40797 "EHLO mail-pb0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759665Ab3BZPch (ORCPT ); Tue, 26 Feb 2013 10:32:37 -0500 Received: by mail-pb0-f52.google.com with SMTP id ma3so2423682pbc.11 for ; Tue, 26 Feb 2013 07:32:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:from:to:cc:subject:date:message-id:x-mailer:in-reply-to :references; bh=yChcDLVXAB03PpTZrDdfRzZsAhcDsIIiThANZAv891c=; b=NhFRqEl53nssOtqMVFeURm3A0jrQIgWl5X6DG+0F7DBIWB2rZ/peKdU3LITzZ9D4UJ 7G4D9PrM/9zxVIgDCoHT8p7UtRS4w9eNlRkQ+0kgCMeWwmYIcEdvaEc7KLoxhmpTldFf yWwSOAZCqZXaQLsOskW2WcyyGzks4ll3ZxjSCW7AusJFSycyWFMa6OUpmGbwBpbZG/Z3 X5s+G9ybV4NaHUJuJNy0xjXoQNyKqAqF7Lij9M/5vgBUNJl/YQFdtgBn25rfILvbixTl 8cQGEn316mkFVrCOcLQe6m18CWVaUF5JBO4mBKFUEhrSSTbvs9eP/G/J1dmfQW/MmHPF M7Iw== X-Received: by 10.68.41.197 with SMTP id h5mr24205795pbl.88.1361892756929; Tue, 26 Feb 2013 07:32:36 -0800 (PST) Received: from localhost.localdomain ([114.250.77.63]) by mx.google.com with ESMTPS id gg7sm1229055pbc.45.2013.02.26.07.32.27 (version=TLSv1.1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 26 Feb 2013 07:32:35 -0800 (PST) From: Jiang Liu To: Bjorn Helgaas , "Rafael J . Wysocki" Cc: Jiang Liu , Yinghai Lu , Yijing Wang , Jiang Liu , linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, Greg Kroah-Hartman , ACPI Devel Maling List , Toshi Kani , Myron Stowe , "Rafael J. Wysocki" Subject: [PATCH v8 12/13] PCI/acpiphp: protect acpiphp data structures from concurrent updating Date: Tue, 26 Feb 2013 23:25:52 +0800 Message-Id: <1361892353-14786-13-git-send-email-jiang.liu@huawei.com> X-Mailer: git-send-email 1.7.9.5 In-Reply-To: <1361892353-14786-1-git-send-email-jiang.liu@huawei.com> References: <1361892353-14786-1-git-send-email-jiang.liu@huawei.com> Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org Now acpiphp_enumerate_slots() and acpiphp_remove_slots() may be invoked concurrently by the PCI core, so add a bridge_mutex and reference count mechanism to protect acpiphp bridge/slot/function data structures. To avoid deadlock, handle_hotplug_event_bridge() will requeue the hotplug event onto the kacpi_hotplug_wq by calling alloc_acpi_hp_work(). But the workaround has introduced a minor race window because the 'bridge' passed to _handle_hotplug_event_bridge() may have already been destroyed when _handle_hotplug_event_bridge() is actually executed by the kacpi_hotplug_wq. So hold a reference count on the passed 'bridge'. Fix the same issue for handle_hotplug_event_func() too. Signed-off-by: Jiang Liu Signed-off-by: Yijing Wang Cc: Yinghai Lu Cc: "Rafael J. Wysocki" Cc: Toshi Kani Cc: linux-pci@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- drivers/pci/hotplug/acpiphp.h | 1 + drivers/pci/hotplug/acpiphp_glue.c | 95 +++++++++++++++++++++++++++++------- 2 files changed, 78 insertions(+), 18 deletions(-) diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h index 0b30045..7577bb3 100644 --- a/drivers/pci/hotplug/acpiphp.h +++ b/drivers/pci/hotplug/acpiphp.h @@ -74,6 +74,7 @@ static inline const char *slot_name(struct slot *slot) struct acpiphp_bridge { struct list_head list; struct list_head slots; + struct kref ref; acpi_handle handle; /* Ejectable PCI-to-PCI bridge (PCI bridge and PCI function) */ diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index 3566f9a..f3647ae 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -54,6 +54,7 @@ #include "acpiphp.h" static LIST_HEAD(bridge_list); +static DEFINE_MUTEX(bridge_mutex); #define MY_NAME "acpiphp_glue" @@ -61,6 +62,7 @@ static void handle_hotplug_event_bridge (acpi_handle, u32, void *); static void acpiphp_sanitize_bus(struct pci_bus *bus); static void acpiphp_set_hpp_values(struct pci_bus *bus); static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context); +static void free_bridge(struct kref *kref); /* callback routine to check for the existence of a pci dock device */ static acpi_status @@ -76,6 +78,39 @@ is_pci_dock_device(acpi_handle handle, u32 lvl, void *context, void **rv) } } +static inline void get_bridge(struct acpiphp_bridge *bridge) +{ + kref_get(&bridge->ref); +} + +static inline void put_bridge(struct acpiphp_bridge *bridge) +{ + kref_put(&bridge->ref, free_bridge); +} + +static void free_bridge(struct kref *kref) +{ + struct acpiphp_bridge *bridge; + struct acpiphp_slot *slot, *next; + struct acpiphp_func *func, *tmp; + + bridge = container_of(kref, struct acpiphp_bridge, ref); + + list_for_each_entry_safe(slot, next, &bridge->slots, node) { + list_for_each_entry_safe(func, tmp, &slot->funcs, sibling) { + kfree(func); + } + kfree(slot); + } + + /* Release reference acquired by acpiphp_bridge_handle_to_function() */ + if ((bridge->flags & BRIDGE_HAS_EJ0) && bridge->func) + put_bridge(bridge->func->slot->bridge); + put_device(&bridge->pci_bus->dev); + pci_dev_put(bridge->pci_dev); + kfree(bridge); +} + /* * the _DCK method can do funny things... and sometimes not * hah-hah funny. @@ -171,7 +206,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv) device = (adr >> 16) & 0xffff; function = adr & 0xffff; - pdev = pbus->self; + pdev = bridge->pci_dev; if (pdev && device_is_managed_by_native_pciehp(pdev)) return AE_OK; @@ -179,7 +214,6 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv) if (!newfunc) return AE_NO_MEMORY; - INIT_LIST_HEAD(&newfunc->sibling); newfunc->handle = handle; newfunc->function = function; @@ -229,7 +263,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv) INIT_LIST_HEAD(&slot->funcs); mutex_init(&slot->crit_sect); + mutex_lock(&bridge_mutex); list_add_tail(&slot->node, &bridge->slots); + mutex_unlock(&bridge_mutex); bridge->nr_slots++; dbg("found ACPI PCI Hotplug slot %llu at PCI %04x:%02x:%02x\n", @@ -247,7 +283,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv) } newfunc->slot = slot; + mutex_lock(&bridge_mutex); list_add_tail(&newfunc->sibling, &slot->funcs); + mutex_unlock(&bridge_mutex); if (pci_bus_read_dev_vendor_id(pbus, PCI_DEVFN(device, function), &val, 60*1000)) @@ -288,7 +326,9 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv) err_exit: bridge->nr_slots--; + mutex_lock(&bridge_mutex); list_del(&slot->node); + mutex_unlock(&bridge_mutex); kfree(slot); kfree(newfunc); @@ -313,13 +353,17 @@ static void init_bridge_misc(struct acpiphp_bridge *bridge) acpi_status status; /* must be added to the list prior to calling register_slot */ + mutex_lock(&bridge_mutex); list_add(&bridge->list, &bridge_list); + mutex_unlock(&bridge_mutex); /* register all slot objects under this bridge */ status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, (u32)1, register_slot, NULL, bridge, NULL); if (ACPI_FAILURE(status)) { + mutex_lock(&bridge_mutex); list_del(&bridge->list); + mutex_unlock(&bridge_mutex); return; } @@ -349,16 +393,21 @@ static struct acpiphp_func *acpiphp_bridge_handle_to_function(acpi_handle handle { struct acpiphp_bridge *bridge; struct acpiphp_slot *slot; - struct acpiphp_func *func; + struct acpiphp_func *func = NULL; + mutex_lock(&bridge_mutex); list_for_each_entry(bridge, &bridge_list, list) { list_for_each_entry(slot, &bridge->slots, node) { list_for_each_entry(func, &slot->funcs, sibling) { - if (func->handle == handle) + if (func->handle == handle) { + get_bridge(func->slot->bridge); + mutex_unlock(&bridge_mutex); return func; + } } } } + mutex_unlock(&bridge_mutex); return NULL; } @@ -368,17 +417,22 @@ static struct acpiphp_bridge *acpiphp_handle_to_bridge(acpi_handle handle) { struct acpiphp_bridge *bridge; + mutex_lock(&bridge_mutex); list_for_each_entry(bridge, &bridge_list, list) - if (bridge->handle == handle) + if (bridge->handle == handle) { + get_bridge(bridge); + mutex_unlock(&bridge_mutex); return bridge; + } + mutex_unlock(&bridge_mutex); return NULL; } static void cleanup_bridge(struct acpiphp_bridge *bridge) { - struct acpiphp_slot *slot, *next; - struct acpiphp_func *func, *tmp; + struct acpiphp_slot *slot; + struct acpiphp_func *func; acpi_status status; acpi_handle handle = bridge->handle; @@ -399,8 +453,8 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge) err("failed to install interrupt notify handler\n"); } - list_for_each_entry_safe(slot, next, &bridge->slots, node) { - list_for_each_entry_safe(func, tmp, &slot->funcs, sibling) { + list_for_each_entry(slot, &bridge->slots, node) { + list_for_each_entry(func, &slot->funcs, sibling) { if (is_dock_device(func->handle)) { unregister_hotplug_dock_device(func->handle); unregister_dock_notifier(&func->nb); @@ -412,18 +466,13 @@ static void cleanup_bridge(struct acpiphp_bridge *bridge) if (ACPI_FAILURE(status)) err("failed to remove notify handler\n"); } - list_del(&func->sibling); - kfree(func); } acpiphp_unregister_hotplug_slot(slot); - list_del(&slot->funcs); - kfree(slot); } - put_device(&bridge->pci_bus->dev); - pci_dev_put(bridge->pci_dev); + mutex_lock(&bridge_mutex); list_del(&bridge->list); - kfree(bridge); + mutex_unlock(&bridge_mutex); } static int power_on_slot(struct acpiphp_slot *slot) @@ -624,7 +673,6 @@ static int __ref enable_device(struct acpiphp_slot *slot) struct pci_dev *dev; struct pci_bus *bus = slot->bridge->pci_bus; struct acpiphp_func *func; - int retval = 0; int num, max, pass; if (slot->flags & SLOT_ENABLED) @@ -684,7 +732,7 @@ static int __ref enable_device(struct acpiphp_slot *slot) err_exit: - return retval; + return 0; } /* return first device in slot, acquiring a reference on it */ @@ -901,6 +949,7 @@ check_sub_bridges(acpi_handle handle, u32 lvl, void *context, void **rv) dbg("%s: re-enumerating slots under %s\n", __func__, objname); acpiphp_check_bridge(bridge); + put_bridge(bridge); } return AE_OK ; } @@ -975,6 +1024,7 @@ static void _handle_hotplug_event_bridge(struct work_struct *work) } kfree(hp_work); /* allocated in handle_hotplug_event_bridge */ + put_bridge(bridge); } /** @@ -988,6 +1038,8 @@ static void _handle_hotplug_event_bridge(struct work_struct *work) static void handle_hotplug_event_bridge(acpi_handle handle, u32 type, void *context) { + struct acpiphp_bridge *bridge = context; + /* * Currently the code adds all hotplug events to the kacpid_wq * queue when it should add hotplug events to the kacpi_hotplug_wq. @@ -996,6 +1048,7 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type, * For now just re-add this work to the kacpi_hotplug_wq so we * don't deadlock on hotplug actions. */ + get_bridge(bridge); alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge); } @@ -1047,6 +1100,7 @@ static void _handle_hotplug_event_func(struct work_struct *work) } kfree(hp_work); /* allocated in handle_hotplug_event_func */ + put_bridge(func->slot->bridge); } /** @@ -1060,6 +1114,8 @@ static void _handle_hotplug_event_func(struct work_struct *work) static void handle_hotplug_event_func(acpi_handle handle, u32 type, void *context) { + struct acpiphp_func *func = context; + /* * Currently the code adds all hotplug events to the kacpid_wq * queue when it should add hotplug events to the kacpi_hotplug_wq. @@ -1068,6 +1124,7 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type, * For now just re-add this work to the kacpi_hotplug_wq so we * don't deadlock on hotplug actions. */ + get_bridge(func->slot->bridge); alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func); } @@ -1090,6 +1147,7 @@ void acpiphp_enumerate_slots(struct pci_bus *bus, acpi_handle handle) } INIT_LIST_HEAD(&bridge->slots); + kref_init(&bridge->ref); bridge->handle = handle; bridge->pci_dev = pci_dev_get(bus->self); bridge->pci_bus = bus; @@ -1120,6 +1178,7 @@ void acpiphp_remove_slots(struct pci_bus *bus) list_for_each_entry_safe(bridge, tmp, &bridge_list, list) if (bridge->pci_bus == bus) { cleanup_bridge(bridge); + put_bridge(bridge); break; } }