From patchwork Tue Nov 26 20:13:50 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yinghai Lu X-Patchwork-Id: 3241011 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: patchwork-linux-pci@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 368379F3AE for ; Tue, 26 Nov 2013 20:13:55 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2547D20451 for ; Tue, 26 Nov 2013 20:13:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E6633203FB for ; Tue, 26 Nov 2013 20:13:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757128Ab3KZUNv (ORCPT ); Tue, 26 Nov 2013 15:13:51 -0500 Received: from mail-ie0-f172.google.com ([209.85.223.172]:49682 "EHLO mail-ie0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754304Ab3KZUNu (ORCPT ); Tue, 26 Nov 2013 15:13:50 -0500 Received: by mail-ie0-f172.google.com with SMTP id qd12so10201221ieb.31 for ; Tue, 26 Nov 2013 12:13:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=8sjhM2veK/j+XsSu1p87W339TRJ6NFYL7wC4crmzQDY=; b=xEW0DamdYkMd9GXk0FqoL730Gk+1kFr+pD8uUuykV7cUKtYcHctqb1zs52v58W4W7I Y9CbnjJpBjVOlPXEoTlxNYokH4q6rhFq1AUgbLcw5XUQL5sXpu8G/1xf9mUN63QlSzrt FhB6I45ih/tL4CcAqlA1DhWvSLhAPbZhxJsRwMLxBID4lws1mh0+JOvOPIMMbAe9PMN+ IxsXCm27l04tlAPPLsR9+JVNPu2kxwyvZin/5yI07IDFmmyZxFIpj0t9d+KqF1nmPL/J yMzP7Qx9VnaU4iw+NS1W4EGSgMcE2kVJC4peDYhkaV1wMQgONrhxZp+okqDUV8/atOFo YH6w== MIME-Version: 1.0 X-Received: by 10.42.148.200 with SMTP id s8mr2604529icv.67.1385496830244; Tue, 26 Nov 2013 12:13:50 -0800 (PST) Received: by 10.64.235.70 with HTTP; Tue, 26 Nov 2013 12:13:50 -0800 (PST) In-Reply-To: References: <1385429290-25397-1-git-send-email-yinghai@kernel.org> <1385429290-25397-5-git-send-email-yinghai@kernel.org> Date: Tue, 26 Nov 2013 12:13:50 -0800 X-Google-Sender-Auth: l4qsAmJhaQ3C4xi4sKZ8sj1oMbQ Message-ID: Subject: Re: [PATCH v2 04/10] PCI: Destroy pci dev only once From: Yinghai Lu To: Bjorn Helgaas Cc: "Rafael J. Wysocki" , Gu Zheng , Guo Chao , "linux-pci@vger.kernel.org" , "linux-kernel@vger.kernel.org" Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Nov 26, 2013 at 11:34 AM, Yinghai Lu wrote: > On Mon, Nov 25, 2013 at 7:38 PM, Bjorn Helgaas wrote: >> On Mon, Nov 25, 2013 at 6:28 PM, Yinghai Lu wrote: >>> Mutliple removing via /sys will call pci_destroy_dev two times. >>> >>> | When concurent removing pci devices which are in the same pci subtree >>> | via sysfs, such as: >>> | echo -n 1 > /sys/bus/pci/devices/0000\:10\:00.0/remove ; echo -n 1 > >>> | /sys/bus/pci/devices/0000\:1a\:01.0/remove >>> | (1a:01.0 device is downstream from the 10:00.0 bridge) >>> | >>> | the following warning will show: >>> | [ 1799.280918] ------------[ cut here ]------------ >>> | [ 1799.336199] WARNING: CPU: 7 PID: 126 at lib/list_debug.c:53 __list_del_entry+0x63/0xd0() >>> | [ 1799.433093] list_del corruption, ffff8807b4a7c000->next is LIST_POISON1 (dead000000100100) >>> | [ 1800.276623] CPU: 7 PID: 126 Comm: kworker/u512:1 Tainted: G W 3.12.0-rc5+ #196 >>> | [ 1800.508918] Workqueue: sysfsd sysfs_schedule_callback_work >>> | [ 1800.574703] 0000000000000009 ffff8807adbadbd8 ffffffff8168b26c ffff8807c27d08a8 >>> | [ 1800.663860] ffff8807adbadc28 ffff8807adbadc18 ffffffff810711dc ffff8807adbadc68 >>> | [ 1800.753130] ffff8807b4a7c000 ffff8807b4a7c000 ffff8807ad089c00 0000000000000000 >>> | [ 1800.842282] Call Trace: >>> | [ 1800.871651] [] dump_stack+0x55/0x76 >>> | [ 1800.933301] [] warn_slowpath_common+0x8c/0xc0 >>> | [ 1801.005283] [] warn_slowpath_fmt+0x46/0x50 >>> | [ 1801.074081] [] __list_del_entry+0x63/0xd0 >>> | [ 1801.141839] [] list_del+0x11/0x40 >>> | [ 1801.201320] [] pci_remove_bus_device+0x6a/0xe0 >>> | [ 1801.274279] [] pci_stop_and_remove_bus_device+0x1e/0x30 >>> | [ 1801.356606] [] remove_callback+0x2b/0x40 >>> | [ 1801.423412] [] sysfs_schedule_callback_work+0x18/0x60 >>> | [ 1801.503744] [] process_one_work+0x1f5/0x540 >>> | [ 1801.573640] [] ? process_one_work+0x193/0x540 >>> | [ 1801.645616] [] worker_thread+0x11c/0x370 >>> | [ 1801.712337] [] ? rescuer_thread+0x350/0x350 >>> | [ 1801.782178] [] kthread+0xed/0x100 >>> | [ 1801.841661] [] ? kthread_create_on_node+0x160/0x160 >>> | [ 1801.919919] [] ret_from_fork+0x7c/0xb0 >>> | [ 1801.984608] [] ? kthread_create_on_node+0x160/0x160 >>> | [ 1802.062825] ---[ end trace d77f2054de000fb7 ]--- >>> | >>> | This issue is related to the bug 54411: >>> | https://bugzilla.kernel.org/show_bug.cgi?id=54411 >>> >>> Add is_removed to record if pci_destroy_dev is called already. >>> >>> During second calling, still have extra dev ref hold via >>> device_schedule_call, so we are safe to check dev->is_removed. >>> >>> It fixs the problem In Gu's test. >>> >>> -v2: add partial changelog from Gu Zheng >>> refresh after patch of moving device_del from Rafael. >>> >>> Signed-off-by: Yinghai Lu >>> --- >>> drivers/pci/remove.c | 8 +++++--- >>> include/linux/pci.h | 1 + >>> 2 files changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c >>> index f452148..b090cec 100644 >>> --- a/drivers/pci/remove.c >>> +++ b/drivers/pci/remove.c >>> @@ -20,9 +20,11 @@ static void pci_stop_dev(struct pci_dev *dev) >>> >>> static void pci_destroy_dev(struct pci_dev *dev) >>> { >>> - device_del(&dev->dev); >>> - >>> - put_device(&dev->dev); >>> + if (!dev->is_removed) { >>> + device_del(&dev->dev); >>> + dev->is_removed = 1; >> >> As Rafael pointed out, this looks like a race. What prevents two >> concurrent calls to pci_destroy_dev() from seeing "dev->is_removed == >> 0" and both calling device_del() on the same device? > hope you are happy with this one: -v3: use atomic operations to prevent racing that Rafael and Bjorn concern. Signed-off-by: Yinghai Lu --- drivers/pci/probe.c | 2 ++ drivers/pci/remove.c | 8 +++++--- include/linux/pci.h | 1 + 3 files changed, 8 insertions(+), 3 deletions(-) -- 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 Index: linux-2.6/drivers/pci/remove.c =================================================================== --- linux-2.6.orig/drivers/pci/remove.c +++ linux-2.6/drivers/pci/remove.c @@ -20,9 +20,11 @@ static void pci_stop_dev(struct pci_dev static void pci_destroy_dev(struct pci_dev *dev) { - device_del(&dev->dev); - - put_device(&dev->dev); + if (atomic_inc_and_test(&dev->removed_count)) { + device_del(&dev->dev); + put_device(&dev->dev); + } else + atomic_dec(&dev->removed_count); } void pci_remove_bus(struct pci_bus *bus) Index: linux-2.6/include/linux/pci.h =================================================================== --- linux-2.6.orig/include/linux/pci.h +++ linux-2.6/include/linux/pci.h @@ -316,6 +316,7 @@ struct pci_dev { struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */ bool match_driver; /* Skip attaching driver */ + atomic_t removed_count; /* pci_destroy_dev is called */ /* These fields are used by common fixups */ unsigned int transparent:1; /* Subtractive decode PCI bridge */ unsigned int multifunction:1;/* Part of multi-function device */ Index: linux-2.6/drivers/pci/probe.c =================================================================== --- linux-2.6.orig/drivers/pci/probe.c +++ linux-2.6/drivers/pci/probe.c @@ -1398,6 +1398,8 @@ void pci_device_add(struct pci_dev *dev, dev->match_driver = false; ret = device_add(&dev->dev); WARN_ON(ret < 0); + + atomic_set(&dev->removed_count, -1); } struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)