From patchwork Fri Aug 17 04:48:57 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Benjamin Herrenschmidt X-Patchwork-Id: 958646 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=linux-pci-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 41s9jm5khlz9s3x for ; Fri, 17 Aug 2018 14:50:04 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726294AbeHQHv4 (ORCPT ); Fri, 17 Aug 2018 03:51:56 -0400 Received: from gate.crashing.org ([63.228.1.57]:46733 "EHLO gate.crashing.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725833AbeHQHv4 (ORCPT ); Fri, 17 Aug 2018 03:51:56 -0400 Received: from pasglop.ozlabs.ibm.com (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id w7H4nAsE017258; Thu, 16 Aug 2018 23:49:17 -0500 From: Benjamin Herrenschmidt To: Bjorn Helgaas , linux-pci@vger.kernel.org Cc: Hari Vyas , Ray Jui , Srinath Mannam , Guenter Roeck , Jens Axboe , Lukas Wunner , Konstantin Khlebnikov , Marta Rybczynska , Pierre-Yves Kerbrat , linux-kernel@vger.kernel.org, Benjamin Herrenschmidt Subject: [RFC PATCH 1/6] Revert "PCI: Fix is_added/is_busmaster race condition" Date: Fri, 17 Aug 2018 14:48:57 +1000 Message-Id: <20180817044902.31420-2-benh@kernel.crashing.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180817044902.31420-1-benh@kernel.crashing.org> References: <20180817044902.31420-1-benh@kernel.crashing.org> Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org This reverts commit 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76. The new pci state mutex provides a simpler way of addressing this race and avoids the relative includes added to the powerpc code. Signed-off-by: Benjamin Herrenschmidt --- arch/powerpc/kernel/pci-common.c | 4 +--- arch/powerpc/platforms/powernv/pci-ioda.c | 3 +-- arch/powerpc/platforms/pseries/setup.c | 3 +-- drivers/pci/bus.c | 6 +++--- drivers/pci/hotplug/acpiphp_glue.c | 2 +- drivers/pci/pci.h | 11 ----------- drivers/pci/probe.c | 4 ++-- drivers/pci/remove.c | 5 ++--- include/linux/pci.h | 1 + 9 files changed, 12 insertions(+), 27 deletions(-) diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index 471aac313b89..fe9733ffffaa 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c @@ -42,8 +42,6 @@ #include #include -#include "../../../drivers/pci/pci.h" - /* hose_spinlock protects accesses to the the phb_bitmap. */ static DEFINE_SPINLOCK(hose_spinlock); LIST_HEAD(hose_list); @@ -1016,7 +1014,7 @@ void pcibios_setup_bus_devices(struct pci_bus *bus) /* Cardbus can call us to add new devices to a bus, so ignore * those who are already fully discovered */ - if (pci_dev_is_added(dev)) + if (dev->is_added) continue; pcibios_setup_device(dev); diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index 70b2e1e0f23c..5bd0eb6681bc 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c @@ -46,7 +46,6 @@ #include "powernv.h" #include "pci.h" -#include "../../../../drivers/pci/pci.h" #define PNV_IODA1_M64_NUM 16 /* Number of M64 BARs */ #define PNV_IODA1_M64_SEGS 8 /* Segments per M64 BAR */ @@ -3139,7 +3138,7 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) struct pci_dn *pdn; int mul, total_vfs; - if (!pdev->is_physfn || pci_dev_is_added(pdev)) + if (!pdev->is_physfn || pdev->is_added) return; pdn = pci_get_pdn(pdev); diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c index 8a4868a3964b..139f0af6c3d9 100644 --- a/arch/powerpc/platforms/pseries/setup.c +++ b/arch/powerpc/platforms/pseries/setup.c @@ -71,7 +71,6 @@ #include #include "pseries.h" -#include "../../../../drivers/pci/pci.h" int CMO_PrPSP = -1; int CMO_SecPSP = -1; @@ -665,7 +664,7 @@ static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev) const int *indexes; struct device_node *dn = pci_device_to_OF_node(pdev); - if (!pdev->is_physfn || pci_dev_is_added(pdev)) + if (!pdev->is_physfn || pdev->is_added) return; /*Firmware must support open sriov otherwise dont configure*/ indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL); diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index 5cb40b2518f9..35b7fc87eac5 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c @@ -330,7 +330,7 @@ void pci_bus_add_device(struct pci_dev *dev) return; } - pci_dev_assign_added(dev, true); + dev->is_added = 1; } EXPORT_SYMBOL_GPL(pci_bus_add_device); @@ -347,14 +347,14 @@ void pci_bus_add_devices(const struct pci_bus *bus) list_for_each_entry(dev, &bus->devices, bus_list) { /* Skip already-added devices */ - if (pci_dev_is_added(dev)) + if (dev->is_added) continue; pci_bus_add_device(dev); } list_for_each_entry(dev, &bus->devices, bus_list) { /* Skip if device attach failed */ - if (!pci_dev_is_added(dev)) + if (!dev->is_added) continue; child = dev->subordinate; if (child) diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index ef0b1b6ba86f..3a17b290df5d 100644 --- a/drivers/pci/hotplug/acpiphp_glue.c +++ b/drivers/pci/hotplug/acpiphp_glue.c @@ -509,7 +509,7 @@ static void enable_slot(struct acpiphp_slot *slot) list_for_each_entry(dev, &bus->devices, bus_list) { /* Assume that newly added devices are powered on already. */ - if (!pci_dev_is_added(dev)) + if (!dev->is_added) dev->current_state = PCI_D0; } diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 6e0d1528d471..473aa10a5dbf 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -295,7 +295,6 @@ struct pci_sriov { /* pci_dev priv_flags */ #define PCI_DEV_DISCONNECTED 0 -#define PCI_DEV_ADDED 1 static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused) { @@ -308,16 +307,6 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev) return test_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags); } -static inline void pci_dev_assign_added(struct pci_dev *dev, bool added) -{ - assign_bit(PCI_DEV_ADDED, &dev->priv_flags, added); -} - -static inline bool pci_dev_is_added(const struct pci_dev *dev) -{ - return test_bit(PCI_DEV_ADDED, &dev->priv_flags); -} - #ifdef CONFIG_PCIEAER #include diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index ec784009a36b..440445ac7dfa 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2525,13 +2525,13 @@ int pci_scan_slot(struct pci_bus *bus, int devfn) dev = pci_scan_single_device(bus, devfn); if (!dev) return 0; - if (!pci_dev_is_added(dev)) + if (!dev->is_added) nr++; for (fn = next_fn(bus, dev, 0); fn > 0; fn = next_fn(bus, dev, fn)) { dev = pci_scan_single_device(bus, devfn + fn); if (dev) { - if (!pci_dev_is_added(dev)) + if (!dev->is_added) nr++; dev->multifunction = 1; } diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index 461e7fd2756f..01ec7fcb5634 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -18,12 +18,11 @@ static void pci_stop_dev(struct pci_dev *dev) { pci_pme_active(dev, false); - if (pci_dev_is_added(dev)) { + if (dev->is_added) { device_release_driver(&dev->dev); pci_proc_detach_device(dev); pci_remove_sysfs_dev_files(dev); - - pci_dev_assign_added(dev, false); + dev->is_added = 0; } if (dev->bus->self) diff --git a/include/linux/pci.h b/include/linux/pci.h index 9b87f1936906..9799109c5e25 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -373,6 +373,7 @@ struct pci_dev { unsigned int transparent:1; /* Subtractive decode bridge */ unsigned int multifunction:1; /* Multi-function device */ + unsigned int is_added:1; unsigned int is_busmaster:1; /* Is busmaster */ unsigned int no_msi:1; /* May not use MSI */ unsigned int no_64bit_msi:1; /* May only use 32-bit MSIs */ From patchwork Fri Aug 17 04:48:58 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Benjamin Herrenschmidt X-Patchwork-Id: 958650 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=linux-pci-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 41sBBb5FrVz9s2P for ; Fri, 17 Aug 2018 15:11:35 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726226AbeHQIN3 (ORCPT ); Fri, 17 Aug 2018 04:13:29 -0400 Received: from gate.crashing.org ([63.228.1.57]:57649 "EHLO gate.crashing.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726134AbeHQIN3 (ORCPT ); Fri, 17 Aug 2018 04:13:29 -0400 Received: from pasglop.ozlabs.ibm.com (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id w7H4nAsF017258; Thu, 16 Aug 2018 23:49:22 -0500 From: Benjamin Herrenschmidt To: Bjorn Helgaas , linux-pci@vger.kernel.org Cc: Hari Vyas , Ray Jui , Srinath Mannam , Guenter Roeck , Jens Axboe , Lukas Wunner , Konstantin Khlebnikov , Marta Rybczynska , Pierre-Yves Kerbrat , linux-kernel@vger.kernel.org, Benjamin Herrenschmidt Subject: [RFC PATCH 2/6] pci: Set pci_dev->is_added before calling device_add Date: Fri, 17 Aug 2018 14:48:58 +1000 Message-Id: <20180817044902.31420-3-benh@kernel.crashing.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180817044902.31420-1-benh@kernel.crashing.org> References: <20180817044902.31420-1-benh@kernel.crashing.org> Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org This re-fixes the bug reported by Hari Vyas after my revert of his commit but in a much simpler way. The main issues is that is_added was being set after the driver got bound and started, and thus setting it could race with other changes to struct pci_dev. This fixes it by setting the flag first, which also has the advantage of matching the fact that we are clearing it *after* unbinding in the remove path, thus the flag is now symtetric and always set while the driver code is running. Signed-off-by: Benjamin Herrenschmidt --- drivers/pci/bus.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index 35b7fc87eac5..48ae63673aa8 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c @@ -321,16 +321,16 @@ void pci_bus_add_device(struct pci_dev *dev) pci_proc_attach_device(dev); pci_bridge_d3_update(dev); + dev->is_added = 1; dev->match_driver = true; retval = device_attach(&dev->dev); if (retval < 0 && retval != -EPROBE_DEFER) { + dev->is_added = 0; pci_warn(dev, "device attach failed (%d)\n", retval); pci_proc_detach_device(dev); pci_remove_sysfs_dev_files(dev); return; } - - dev->is_added = 1; } EXPORT_SYMBOL_GPL(pci_bus_add_device); From patchwork Fri Aug 17 05:13:49 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Benjamin Herrenschmidt X-Patchwork-Id: 958651 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=linux-pci-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 41sBFb0gCxz9s2P for ; Fri, 17 Aug 2018 15:14:11 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726329AbeHQIQF (ORCPT ); Fri, 17 Aug 2018 04:16:05 -0400 Received: from gate.crashing.org ([63.228.1.57]:46508 "EHLO gate.crashing.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726134AbeHQIQF (ORCPT ); Fri, 17 Aug 2018 04:16:05 -0400 Received: from localhost (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id w7H5Dog9018116; Fri, 17 Aug 2018 00:13:51 -0500 Message-ID: Subject: [RFC PATCH v2 3/6] pci: Remove priv_flags and use dev->error_state for "disconnected" status From: Benjamin Herrenschmidt To: Bjorn Helgaas , linux-pci@vger.kernel.org Cc: Hari Vyas , Ray Jui , Srinath Mannam , Guenter Roeck , Jens Axboe , Lukas Wunner , Konstantin Khlebnikov , Marta Rybczynska , Pierre-Yves Kerbrat , linux-kernel@vger.kernel.org Date: Fri, 17 Aug 2018 15:13:49 +1000 In-Reply-To: <20180817044902.31420-4-benh@kernel.crashing.org> References: <20180817044902.31420-1-benh@kernel.crashing.org> <20180817044902.31420-4-benh@kernel.crashing.org> X-Mailer: Evolution 3.28.5 (3.28.5-1.fc28) Mime-Version: 1.0 Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org This already represents whether a device is accessible or not, creating a new flag isn't particularly helpful. dev->error_state being an int, assigning it doesn't require an atomic operation per-se. The existing atomic bitop only protects the field, not anything else anyway. Signed-off-by: Benjamin Herrenschmidt --- v2. Initialize the io channel state before we try to read the header drivers/pci/access.c | 12 ++++++------ drivers/pci/msi.c | 4 ++-- drivers/pci/pci.c | 2 +- drivers/pci/pci.h | 11 ++--------- drivers/pci/probe.c | 4 +++- include/linux/pci.h | 2 -- 6 files changed, 14 insertions(+), 21 deletions(-) diff --git a/drivers/pci/access.c b/drivers/pci/access.c index a3ad2fe185b9..ad97a3544636 100644 --- a/drivers/pci/access.c +++ b/drivers/pci/access.c @@ -535,7 +535,7 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword); int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val) { - if (pci_dev_is_disconnected(dev)) { + if (pci_channel_offline(dev)) { *val = ~0; return PCIBIOS_DEVICE_NOT_FOUND; } @@ -545,7 +545,7 @@ EXPORT_SYMBOL(pci_read_config_byte); int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val) { - if (pci_dev_is_disconnected(dev)) { + if (pci_channel_offline(dev)) { *val = ~0; return PCIBIOS_DEVICE_NOT_FOUND; } @@ -556,7 +556,7 @@ EXPORT_SYMBOL(pci_read_config_word); int pci_read_config_dword(const struct pci_dev *dev, int where, u32 *val) { - if (pci_dev_is_disconnected(dev)) { + if (pci_channel_offline(dev)) { *val = ~0; return PCIBIOS_DEVICE_NOT_FOUND; } @@ -566,7 +566,7 @@ EXPORT_SYMBOL(pci_read_config_dword); int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val) { - if (pci_dev_is_disconnected(dev)) + if (pci_channel_offline(dev)) return PCIBIOS_DEVICE_NOT_FOUND; return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val); } @@ -574,7 +574,7 @@ EXPORT_SYMBOL(pci_write_config_byte); int pci_write_config_word(const struct pci_dev *dev, int where, u16 val) { - if (pci_dev_is_disconnected(dev)) + if (pci_channel_offline(dev)) return PCIBIOS_DEVICE_NOT_FOUND; return pci_bus_write_config_word(dev->bus, dev->devfn, where, val); } @@ -583,7 +583,7 @@ EXPORT_SYMBOL(pci_write_config_word); int pci_write_config_dword(const struct pci_dev *dev, int where, u32 val) { - if (pci_dev_is_disconnected(dev)) + if (pci_channel_offline(dev)) return PCIBIOS_DEVICE_NOT_FOUND; return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val); } diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index f2ef896464b3..2f872db40042 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -298,7 +298,7 @@ void __pci_write_msi_msg(struct msi_desc *entry, struct msi_msg *msg) { struct pci_dev *dev = msi_desc_to_pci_dev(entry); - if (dev->current_state != PCI_D0 || pci_dev_is_disconnected(dev)) { + if (dev->current_state != PCI_D0 || pci_channel_offline(dev)) { /* Don't touch the hardware now */ } else if (entry->msi_attrib.is_msix) { void __iomem *base = pci_msix_desc_addr(entry); @@ -975,7 +975,7 @@ static void pci_msix_shutdown(struct pci_dev *dev) if (!pci_msi_enable || !dev || !dev->msix_enabled) return; - if (pci_dev_is_disconnected(dev)) { + if (pci_channel_offline(dev)) { dev->msix_enabled = 0; return; } diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 29ff9619b5fa..62591c153999 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -5713,7 +5713,7 @@ bool pci_device_is_present(struct pci_dev *pdev) { u32 v; - if (pci_dev_is_disconnected(pdev)) + if (pci_channel_offline(pdev)) return false; return pci_bus_read_dev_vendor_id(pdev->bus, pdev->devfn, &v, 0); } diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 473aa10a5dbf..f7704333e6a1 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -293,18 +293,11 @@ struct pci_sriov { bool drivers_autoprobe; /* Auto probing of VFs by driver */ }; -/* pci_dev priv_flags */ -#define PCI_DEV_DISCONNECTED 0 - static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused) { - set_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags); - return 0; -} + dev->error_state = pci_channel_io_perm_failure; -static inline bool pci_dev_is_disconnected(const struct pci_dev *dev) -{ - return test_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags); + return 0; } #ifdef CONFIG_PCIEAER diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 440445ac7dfa..5cf982b3031b 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1581,6 +1581,9 @@ int pci_setup_device(struct pci_dev *dev) struct pci_bus_region region; struct resource *res; + /* This must be set before any config space access */ + dev->error_state = pci_channel_io_normal; + hdr_type = pci_hdr_type(dev); dev->sysdata = dev->bus->sysdata; @@ -1588,7 +1591,6 @@ int pci_setup_device(struct pci_dev *dev) dev->dev.bus = &pci_bus_type; dev->hdr_type = hdr_type & 0x7f; dev->multifunction = !!(hdr_type & 0x80); - dev->error_state = pci_channel_io_normal; set_pcie_port_type(dev); pci_dev_assign_slot(dev); diff --git a/include/linux/pci.h b/include/linux/pci.h index 9799109c5e25..f58bda204f09 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -443,8 +443,6 @@ struct pci_dev { phys_addr_t rom; /* Physical address if not from BAR */ size_t romlen; /* Length if not from BAR */ char *driver_override; /* Driver name to force a match */ - - unsigned long priv_flags; /* Private flags for the PCI driver */ }; static inline struct pci_dev *pci_physfn(struct pci_dev *dev) From patchwork Fri Aug 17 04:49:00 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Benjamin Herrenschmidt X-Patchwork-Id: 958669 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=linux-pci-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 41sC2z3pmLz9s4Z for ; Fri, 17 Aug 2018 15:50:03 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726208AbeHQIwC (ORCPT ); Fri, 17 Aug 2018 04:52:02 -0400 Received: from gate.crashing.org ([63.228.1.57]:57329 "EHLO gate.crashing.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726134AbeHQIwC (ORCPT ); Fri, 17 Aug 2018 04:52:02 -0400 Received: from pasglop.ozlabs.ibm.com (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id w7H4nAsH017258; Thu, 16 Aug 2018 23:49:30 -0500 From: Benjamin Herrenschmidt To: Bjorn Helgaas , linux-pci@vger.kernel.org Cc: Hari Vyas , Ray Jui , Srinath Mannam , Guenter Roeck , Jens Axboe , Lukas Wunner , Konstantin Khlebnikov , Marta Rybczynska , Pierre-Yves Kerbrat , linux-kernel@vger.kernel.org, Benjamin Herrenschmidt Subject: [RFC PATCH 4/6] pci: Add a mutex to pci_dev to protect device state Date: Fri, 17 Aug 2018 14:49:00 +1000 Message-Id: <20180817044902.31420-5-benh@kernel.crashing.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180817044902.31420-1-benh@kernel.crashing.org> References: <20180817044902.31420-1-benh@kernel.crashing.org> Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org This adds a pci_dev mutex which will be used to fix a number of races related to the various state bits maintained in that structure. We call it "state_lock" with similarly named accessors to avoid confusion with the pci_dev_lock() which uses the device_lock(). This is a low level mutex meant to protect the mapping between the state fields and the hardware state, for example enabling disabling, setting/clearing bus master etc... These operations can happen while the device_lock() is already held (but don't have to) so a separate mutex is preferable. Signed-off-by: Benjamin Herrenschmidt --- drivers/pci/probe.c | 1 + include/linux/pci.h | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 440445ac7dfa..3ce287ab6150 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2155,6 +2155,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus) INIT_LIST_HEAD(&dev->bus_list); dev->dev.type = &pci_dev_type; dev->bus = pci_bus_get(bus); + mutex_init(&dev->state_lock); return dev; } diff --git a/include/linux/pci.h b/include/linux/pci.h index f58bda204f09..0d4fc22df190 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -33,6 +33,7 @@ #include #include #include +#include #include @@ -443,8 +444,24 @@ struct pci_dev { phys_addr_t rom; /* Physical address if not from BAR */ size_t romlen; /* Length if not from BAR */ char *driver_override; /* Driver name to force a match */ + + unsigned long priv_flags; /* Private flags for the PCI driver */ + + struct mutex state_lock; /* Protect local state bits */ + + /* --- Fields below this line are protected by the state_lock mutex */ }; +static inline void pci_dev_state_lock(struct pci_dev *dev) +{ + mutex_lock(&dev->state_lock); +} + +static inline void pci_dev_state_unlock(struct pci_dev *dev) +{ + mutex_unlock(&dev->state_lock); +} + static inline struct pci_dev *pci_physfn(struct pci_dev *dev) { #ifdef CONFIG_PCI_IOV From patchwork Fri Aug 17 04:49:01 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Benjamin Herrenschmidt X-Patchwork-Id: 958648 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=linux-pci-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 41sB4y1F8Qz9s47 for ; Fri, 17 Aug 2018 15:06:42 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726213AbeHQIIf (ORCPT ); Fri, 17 Aug 2018 04:08:35 -0400 Received: from gate.crashing.org ([63.228.1.57]:41662 "EHLO gate.crashing.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726134AbeHQIIf (ORCPT ); Fri, 17 Aug 2018 04:08:35 -0400 Received: from pasglop.ozlabs.ibm.com (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id w7H4nAsI017258; Thu, 16 Aug 2018 23:49:35 -0500 From: Benjamin Herrenschmidt To: Bjorn Helgaas , linux-pci@vger.kernel.org Cc: Hari Vyas , Ray Jui , Srinath Mannam , Guenter Roeck , Jens Axboe , Lukas Wunner , Konstantin Khlebnikov , Marta Rybczynska , Pierre-Yves Kerbrat , linux-kernel@vger.kernel.org, Benjamin Herrenschmidt Subject: [RFC PATCH 5/6] pci: Protect the enable/disable state of pci_dev using the state mutex Date: Fri, 17 Aug 2018 14:49:01 +1000 Message-Id: <20180817044902.31420-6-benh@kernel.crashing.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180817044902.31420-1-benh@kernel.crashing.org> References: <20180817044902.31420-1-benh@kernel.crashing.org> Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org This protects enable/disable operations using the state mutex to avoid races with, for example, concurrent enables on a bridge. The bus hierarchy is walked first before taking the lock to avoid lock nesting (though it would be ok if we ensured that we always nest them bottom-up, it is better to just avoid the issue alltogether, especially as we might find cases where we want to take it top-down later). Signed-off-by: Benjamin Herrenschmidt --- drivers/pci/pci.c | 51 ++++++++++++++++++++++++++++++++--------------- 1 file changed, 35 insertions(+), 16 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 62591c153999..68152de2b5a0 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1540,26 +1540,33 @@ static int do_pci_enable_device(struct pci_dev *dev, int bars) */ int pci_reenable_device(struct pci_dev *dev) { + int rc = 0; + + pci_dev_state_lock(dev); if (pci_is_enabled(dev)) - return do_pci_enable_device(dev, (1 << PCI_NUM_RESOURCES) - 1); - return 0; + rc = do_pci_enable_device(dev, (1 << PCI_NUM_RESOURCES) - 1); + pci_dev_state_unlock(dev); + return rc; } EXPORT_SYMBOL(pci_reenable_device); static void pci_enable_bridge(struct pci_dev *dev) { struct pci_dev *bridge; - int retval; + int retval, enabled; bridge = pci_upstream_bridge(dev); if (bridge) pci_enable_bridge(bridge); - if (pci_is_enabled(dev)) { - if (!dev->is_busmaster) - pci_set_master(dev); + /* Already enabled ? */ + pci_dev_state_lock(dev); + enabled = pci_is_enabled(dev); + if (enabled && !dev->is_busmaster) + pci_set_master(dev); + pci_dev_state_unlock(dev); + if (enabled) return; - } retval = pci_enable_device(dev); if (retval) @@ -1571,9 +1578,16 @@ static void pci_enable_bridge(struct pci_dev *dev) static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) { struct pci_dev *bridge; - int err; + int err = 0; int i, bars = 0; + /* Handle upstream bridges first to avoid locking issues */ + bridge = pci_upstream_bridge(dev); + if (bridge) + pci_enable_bridge(bridge); + + pci_dev_state_lock(dev); + /* * Power state could be unknown at this point, either due to a fresh * boot or a device removal call. So get the current power state @@ -1586,12 +1600,9 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK); } + /* Already enabled ? */ if (atomic_inc_return(&dev->enable_cnt) > 1) - return 0; /* already enabled */ - - bridge = pci_upstream_bridge(dev); - if (bridge) - pci_enable_bridge(bridge); + goto bail; /* only skip sriov related */ for (i = 0; i <= PCI_ROM_RESOURCE; i++) @@ -1604,6 +1615,8 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) err = do_pci_enable_device(dev, bars); if (err < 0) atomic_dec(&dev->enable_cnt); + bail: + pci_dev_state_unlock(dev); return err; } @@ -1820,12 +1833,16 @@ static void do_pci_disable_device(struct pci_dev *dev) * @dev: PCI device to disable * * NOTE: This function is a backend of PCI power management routines and is - * not supposed to be called drivers. + * not supposed to be called drivers. It will keep enable_cnt and is_busmaster + * unmodified so that the resume code knows how to restore the corresponding + * command register bits. */ void pci_disable_enabled_device(struct pci_dev *dev) { + pci_dev_state_lock(dev); if (pci_is_enabled(dev)) do_pci_disable_device(dev); + pci_dev_state_unlock(dev); } /** @@ -1849,12 +1866,14 @@ void pci_disable_device(struct pci_dev *dev) dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= 0, "disabling already-disabled device"); + pci_dev_state_lock(dev); if (atomic_dec_return(&dev->enable_cnt) != 0) - return; + goto bail; do_pci_disable_device(dev); - dev->is_busmaster = 0; + bail: + pci_dev_state_unlock(dev); } EXPORT_SYMBOL(pci_disable_device); From patchwork Fri Aug 17 04:49:02 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Benjamin Herrenschmidt X-Patchwork-Id: 958659 X-Patchwork-Delegate: bhelgaas@google.com Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=linux-pci-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 41sBTQ6lygz9s4v for ; Fri, 17 Aug 2018 15:24:26 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726134AbeHQI0G (ORCPT ); Fri, 17 Aug 2018 04:26:06 -0400 Received: from gate.crashing.org ([63.228.1.57]:48469 "EHLO gate.crashing.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726327AbeHQI0G (ORCPT ); Fri, 17 Aug 2018 04:26:06 -0400 Received: from pasglop.ozlabs.ibm.com (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id w7H4nAsJ017258; Thu, 16 Aug 2018 23:49:41 -0500 From: Benjamin Herrenschmidt To: Bjorn Helgaas , linux-pci@vger.kernel.org Cc: Hari Vyas , Ray Jui , Srinath Mannam , Guenter Roeck , Jens Axboe , Lukas Wunner , Konstantin Khlebnikov , Marta Rybczynska , Pierre-Yves Kerbrat , linux-kernel@vger.kernel.org, Benjamin Herrenschmidt Subject: [RFC PATCH 6/6] pci: Protect is_busmaster using the state lock Date: Fri, 17 Aug 2018 14:49:02 +1000 Message-Id: <20180817044902.31420-7-benh@kernel.crashing.org> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20180817044902.31420-1-benh@kernel.crashing.org> References: <20180817044902.31420-1-benh@kernel.crashing.org> Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org This wraps pci_set_master() and pci_clear_master() with the pci_dev state lock. The clearing of is_busmaster in pci_disable_device() is already covered. This also adds a comment explaining why is_busmaster must not be checked in pci_set_master() due to how the power management code uses it. Signed-off-by: Benjamin Herrenschmidt --- drivers/pci/pci.c | 9 +++++++++ include/linux/pci.h | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 68152de2b5a0..13d988d5b2a3 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4052,11 +4052,18 @@ void __weak pcibios_set_master(struct pci_dev *dev) * * Enables bus-mastering on the device and calls pcibios_set_master() * to do the needed arch specific settings. + * + * Note: This must not check dev->is_busmaster because the power management + * code will call this in order to restore the config space to the + * state of is_busmaster, thus is_busmaster might be set but the + * config space bit cleared. */ void pci_set_master(struct pci_dev *dev) { + pci_dev_state_lock(dev); __pci_set_master(dev, true); pcibios_set_master(dev); + pci_dev_state_unlock(dev); } EXPORT_SYMBOL(pci_set_master); @@ -4066,7 +4073,9 @@ EXPORT_SYMBOL(pci_set_master); */ void pci_clear_master(struct pci_dev *dev) { + pci_dev_state_lock(dev); __pci_set_master(dev, false); + pci_dev_state_unlock(dev); } EXPORT_SYMBOL(pci_clear_master); diff --git a/include/linux/pci.h b/include/linux/pci.h index 0d4fc22df190..a5bac5b21454 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -375,7 +375,6 @@ struct pci_dev { unsigned int multifunction:1; /* Multi-function device */ unsigned int is_added:1; - unsigned int is_busmaster:1; /* Is busmaster */ unsigned int no_msi:1; /* May not use MSI */ unsigned int no_64bit_msi:1; /* May only use 32-bit MSIs */ unsigned int block_cfg_access:1; /* Config space access blocked */ @@ -450,6 +449,7 @@ struct pci_dev { struct mutex state_lock; /* Protect local state bits */ /* --- Fields below this line are protected by the state_lock mutex */ + unsigned int is_busmaster:1; /* Is busmaster */ }; static inline void pci_dev_state_lock(struct pci_dev *dev)