From patchwork Fri Jul 17 21:31:52 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bjorn Helgaas X-Patchwork-Id: 6819401 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.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id BC87E9F2E8 for ; Fri, 17 Jul 2015 21:31:59 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 8C09D2081B for ; Fri, 17 Jul 2015 21:31:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 5088F20810 for ; Fri, 17 Jul 2015 21:31:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752337AbbGQVb4 (ORCPT ); Fri, 17 Jul 2015 17:31:56 -0400 Received: from mail-oi0-f53.google.com ([209.85.218.53]:32874 "EHLO mail-oi0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751977AbbGQVbz (ORCPT ); Fri, 17 Jul 2015 17:31:55 -0400 Received: by oige126 with SMTP id e126so78451672oig.0 for ; Fri, 17 Jul 2015 14:31:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=subject:to:from:cc:date:message-id:in-reply-to:references :user-agent:mime-version:content-type:content-transfer-encoding; bh=P2LL+Hb5nwIV/Dj2jBMOguHFTR4RpTfXr54CpGWuMRs=; b=hpImmAD4evF96+XVqwgt/jXu3TxrdnExKbZZSYCHG13sSVcYDAyZNZD95RXmBeQiWT wvadHQ5KRHOC4zM7kkfkJYAUDfU2m4DuLBZz8lqc1pY3qL2EMMump9wOxO7Kcwh3b4jm l6Fk9YPAI63uqeN2rN3Pt1T9P7OMknBHEX0FzN+8Km37oyyWqusdqpZHyLQ/aX4DwV8q Q6YhZ30jPeDug9IKR7q5AuSExGoMzbBZUhfyb9VF5HfFyLksw2c8+eBIYAm7wHLTb+Ww pm8i+iFcEW/anR3LWVTz7owiT6odfV3O38kZaOJCpsthgkqrKMHdInrmbYkGM3LmawDE m/hg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:from:cc:date:message-id:in-reply-to :references:user-agent:mime-version:content-type :content-transfer-encoding; bh=P2LL+Hb5nwIV/Dj2jBMOguHFTR4RpTfXr54CpGWuMRs=; b=dpeuLjeZUik2MYTRObLz87T/ApiGudRAhYEUGD3aWuCCTDe809DD9Gkb25GKMFVx7X LMLMPHYcuWxNjFoE70+2bg1090HcwfiYdwi6qMothufhK45XLEvqCxf61DYouYIc0aDC OOb/PrW2DySIEGzHkduCXiIvYjgpcp/fxM0X3dWtk7n8Y8885ib/OjKTbqUg35rquU3o MebPbC9WnnBwpjAcc7Ed9xrdv+byFKXjqVVAbuR2JKqzG+fDoOsGlJZp5nfAl1xAfD/P rHmT6GdjK1UC3FSmJuJzSEIFbgWEYaa5IKqSxsQds4j5Ur3mIi/MMTMj3x93jnJVn9tt YVng== X-Gm-Message-State: ALoCoQnag+Yjm125hqhqcZaWQc+j5JrkxR8afr2x7X9QMggY0L42u67SBXzMUpXSTAKLf7MHNVeW X-Received: by 10.60.83.200 with SMTP id s8mr16077895oey.44.1437168715162; Fri, 17 Jul 2015 14:31:55 -0700 (PDT) Received: from localhost ([146.7.4.71]) by smtp.gmail.com with ESMTPSA id kg7sm3304999oeb.1.2015.07.17.14.31.53 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 17 Jul 2015 14:31:53 -0700 (PDT) Subject: [PATCH 1/8] PCI: Allocate ATS struct during enumeration To: linux-pci@vger.kernel.org, Joerg Roedel From: Bjorn Helgaas Cc: Gregor Dick Date: Fri, 17 Jul 2015 16:31:52 -0500 Message-ID: <20150717213152.18379.50159.stgit@bhelgaas-glaptop2.roam.corp.google.com> In-Reply-To: <20150717212759.18379.44858.stgit@bhelgaas-glaptop2.roam.corp.google.com> References: <20150717212759.18379.44858.stgit@bhelgaas-glaptop2.roam.corp.google.com> User-Agent: StGit/0.16 MIME-Version: 1.0 Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org X-Spam-Status: No, score=-8.0 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, 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 Previously, we allocated pci_ats structures when an IOMMU driver called pci_enable_ats(). An SR-IOV VF shares the STU setting with its PF, so when enabling ATS on the VF, we allocated a pci_ats struct for the PF if it didn't already have one. We held the sriov->lock to serialize threads concurrently enabling ATS on several VFS so only one would allocate the PF pci_ats. Gregor reported a deadlock here: pci_enable_sriov sriov_enable virtfn_add mutex_lock(dev->sriov->lock) # acquire sriov->lock pci_device_add device_add BUS_NOTIFY_ADD_DEVICE notifier chain iommu_bus_notifier amd_iommu_add_device # iommu_ops.add_device init_iommu_group iommu_group_get_for_dev iommu_group_add_device __iommu_attach_device amd_iommu_attach_device # iommu_ops.attach_device attach_device pci_enable_ats mutex_lock(dev->sriov->lock) # deadlock There's no reason to delay allocating the pci_ats struct, and if we allocate it for each device at enumeration-time, there's no need for locking in pci_enable_ats(). Allocate pci_ats struct during enumeration, when we initialize other capabilities. Note that this implementation requires ATS to be enabled on the PF first, before on any of the VFs because the PF controls the STU for all the VFs. Link: http://permalink.gmane.org/gmane.linux.kernel.iommu/9433 Reported-by: Gregor Dick Signed-off-by: Bjorn Helgaas --- drivers/pci/ats.c | 81 +++++++++++++++-------------------------------- drivers/pci/probe.c | 3 ++ drivers/pci/remove.c | 1 + include/linux/pci-ats.h | 1 - include/linux/pci.h | 9 +++++ 5 files changed, 38 insertions(+), 57 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 diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c index a8099d4..923759f 100644 --- a/drivers/pci/ats.c +++ b/drivers/pci/ats.c @@ -17,7 +17,7 @@ #include "pci.h" -static int ats_alloc_one(struct pci_dev *dev, int ps) +static void ats_alloc_one(struct pci_dev *dev) { int pos; u16 cap; @@ -25,20 +25,17 @@ static int ats_alloc_one(struct pci_dev *dev, int ps) pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS); if (!pos) - return -ENODEV; + return; ats = kzalloc(sizeof(*ats), GFP_KERNEL); if (!ats) - return -ENOMEM; + return; ats->pos = pos; - ats->stu = ps; pci_read_config_word(dev, pos + PCI_ATS_CAP, &cap); ats->qdep = PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : PCI_ATS_MAX_QDEP; dev->ats = ats; - - return 0; } static void ats_free_one(struct pci_dev *dev) @@ -47,6 +44,16 @@ static void ats_free_one(struct pci_dev *dev) dev->ats = NULL; } +void pci_ats_init(struct pci_dev *dev) +{ + ats_alloc_one(dev); +} + +void pci_ats_free(struct pci_dev *dev) +{ + ats_free_one(dev); +} + /** * pci_enable_ats - enable the ATS capability * @dev: the PCI device @@ -56,43 +63,29 @@ static void ats_free_one(struct pci_dev *dev) */ int pci_enable_ats(struct pci_dev *dev, int ps) { - int rc; u16 ctrl; BUG_ON(dev->ats && dev->ats->is_enabled); - if (ps < PCI_ATS_MIN_STU) + if (!dev->ats) return -EINVAL; - if (dev->is_physfn || dev->is_virtfn) { - struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn; - - mutex_lock(&pdev->sriov->lock); - if (pdev->ats) - rc = pdev->ats->stu == ps ? 0 : -EINVAL; - else - rc = ats_alloc_one(pdev, ps); + if (ps < PCI_ATS_MIN_STU) + return -EINVAL; - if (!rc) - pdev->ats->ref_cnt++; - mutex_unlock(&pdev->sriov->lock); - if (rc) - return rc; - } + ctrl = PCI_ATS_CTRL_ENABLE; + if (dev->is_virtfn) { + struct pci_dev *pdev = dev->physfn; - if (!dev->is_physfn) { - rc = ats_alloc_one(dev, ps); - if (rc) - return rc; + if (pdev->ats->stu != ps) + return -EINVAL; + } else { + dev->ats->stu = ps; + ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU); } - - ctrl = PCI_ATS_CTRL_ENABLE; - if (!dev->is_virtfn) - ctrl |= PCI_ATS_CTRL_STU(ps - PCI_ATS_MIN_STU); pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl); dev->ats->is_enabled = 1; - return 0; } EXPORT_SYMBOL_GPL(pci_enable_ats); @@ -112,19 +105,6 @@ void pci_disable_ats(struct pci_dev *dev) pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl); dev->ats->is_enabled = 0; - - if (dev->is_physfn || dev->is_virtfn) { - struct pci_dev *pdev = dev->is_physfn ? dev : dev->physfn; - - mutex_lock(&pdev->sriov->lock); - pdev->ats->ref_cnt--; - if (!pdev->ats->ref_cnt) - ats_free_one(pdev); - mutex_unlock(&pdev->sriov->lock); - } - - if (!dev->is_physfn) - ats_free_one(dev); } EXPORT_SYMBOL_GPL(pci_disable_ats); @@ -140,7 +120,6 @@ void pci_restore_ats_state(struct pci_dev *dev) ctrl = PCI_ATS_CTRL_ENABLE; if (!dev->is_virtfn) ctrl |= PCI_ATS_CTRL_STU(dev->ats->stu - PCI_ATS_MIN_STU); - pci_write_config_word(dev, dev->ats->pos + PCI_ATS_CTRL, ctrl); } EXPORT_SYMBOL_GPL(pci_restore_ats_state); @@ -159,23 +138,13 @@ EXPORT_SYMBOL_GPL(pci_restore_ats_state); */ int pci_ats_queue_depth(struct pci_dev *dev) { - int pos; - u16 cap; - if (dev->is_virtfn) return 0; if (dev->ats) return dev->ats->qdep; - pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ATS); - if (!pos) - return -ENODEV; - - pci_read_config_word(dev, pos + PCI_ATS_CAP, &cap); - - return PCI_ATS_CAP_QDEP(cap) ? PCI_ATS_CAP_QDEP(cap) : - PCI_ATS_MAX_QDEP; + return -ENODEV; } EXPORT_SYMBOL_GPL(pci_ats_queue_depth); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index cefd636..c206398 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1540,6 +1540,9 @@ static void pci_init_capabilities(struct pci_dev *dev) /* Single Root I/O Virtualization */ pci_iov_init(dev); + /* Address Translation Services */ + pci_ats_init(dev); + /* Enable ACS P2P upstream forwarding */ pci_enable_acs(dev); } diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index 8a280e9..27617b8 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -26,6 +26,7 @@ static void pci_stop_dev(struct pci_dev *dev) dev->is_added = 0; } + pci_ats_free(dev); if (dev->bus->self) pcie_aspm_exit_link_state(dev); } diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h index 7203178..d2e8170 100644 --- a/include/linux/pci-ats.h +++ b/include/linux/pci-ats.h @@ -8,7 +8,6 @@ struct pci_ats { int pos; /* capability position */ int stu; /* Smallest Translation Unit */ int qdep; /* Invalidate Queue Depth */ - int ref_cnt; /* Physical Function reference count */ unsigned int is_enabled:1; /* Enable bit is set */ }; diff --git a/include/linux/pci.h b/include/linux/pci.h index 8a0321a..5c277f1 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1294,6 +1294,15 @@ int ht_create_irq(struct pci_dev *dev, int idx); void ht_destroy_irq(unsigned int irq); #endif /* CONFIG_HT_IRQ */ +#ifdef CONFIG_PCI_ATS +/* Address Translation Service */ +void pci_ats_init(struct pci_dev *dev); +void pci_ats_free(struct pci_dev *dev); +#else +static inline void pci_ats_init(struct pci_dev *) { } +static inline void pci_ats_free(struct pci_dev *) { } +#endif + void pci_cfg_access_lock(struct pci_dev *dev); bool pci_cfg_access_trylock(struct pci_dev *dev); void pci_cfg_access_unlock(struct pci_dev *dev);