From: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> To: <linux-pci@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org> Cc: Bjorn Helgaas <helgaas@kernel.org>, <linux@yadro.com>, Sergey Miroshnichenko <s.miroshnichenko@yadro.com>, Srinath Mannam <srinath.mannam@broadcom.com>, Marta Rybczynska <mrybczyn@kalray.eu> Subject: [PATCH v5 01/23] PCI: Fix race condition in pci_enable/disable_device() Date: Fri, 16 Aug 2019 19:50:39 +0300 [thread overview] Message-ID: <20190816165101.911-2-s.miroshnichenko@yadro.com> (raw) In-Reply-To: <20190816165101.911-1-s.miroshnichenko@yadro.com> This is a yet another approach to fix an old [1-2] concurrency issue, when: - two or more devices are being hot-added into a bridge which was initially empty; - a bridge with two or more devices is being hot-added; - during boot, if BIOS/bootloader/firmware doesn't pre-enable bridges. The problem is that a bridge is reported as enabled before the MEM/IO bits are actually written to the PCI_COMMAND register, so another driver thread starts memory requests through the not-yet-enabled bridge: CPU0 CPU1 pci_enable_device_mem() pci_enable_device_mem() pci_enable_bridge() pci_enable_bridge() pci_is_enabled() return false; atomic_inc_return(enable_cnt) Start actual enabling the bridge ... pci_is_enabled() ... return true; ... Start memory requests <-- FAIL ... Set the PCI_COMMAND_MEMORY bit <-- Must wait for this Protect the pci_enable/disable_device() and pci_enable_bridge(), which is similar to the previous solution from commit 40f11adc7cd9 ("PCI: Avoid race while enabling upstream bridges"), but adding a per-device mutexes and preventing the dev->enable_cnt from from incrementing early. CC: Srinath Mannam <srinath.mannam@broadcom.com> CC: Marta Rybczynska <mrybczyn@kalray.eu> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> [1] https://lore.kernel.org/linux-pci/1501858648-22228-1-git-send-email-srinath.mannam@broadcom.com/T/#u [RFC PATCH v3] pci: Concurrency issue during pci enable bridge [2] https://lore.kernel.org/linux-pci/744877924.5841545.1521630049567.JavaMail.zimbra@kalray.eu/T/#u [RFC PATCH] nvme: avoid race-conditions when enabling devices --- drivers/pci/pci.c | 26 ++++++++++++++++++++++---- drivers/pci/probe.c | 1 + include/linux/pci.h | 1 + 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 1b27b5af3d55..e7f8c354e644 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1645,6 +1645,8 @@ static void pci_enable_bridge(struct pci_dev *dev) struct pci_dev *bridge; int retval; + mutex_lock(&dev->enable_mutex); + bridge = pci_upstream_bridge(dev); if (bridge) pci_enable_bridge(bridge); @@ -1652,6 +1654,7 @@ static void pci_enable_bridge(struct pci_dev *dev) if (pci_is_enabled(dev)) { if (!dev->is_busmaster) pci_set_master(dev); + mutex_unlock(&dev->enable_mutex); return; } @@ -1660,11 +1663,14 @@ static void pci_enable_bridge(struct pci_dev *dev) pci_err(dev, "Error enabling bridge (%d), continuing\n", retval); pci_set_master(dev); + mutex_unlock(&dev->enable_mutex); } static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) { struct pci_dev *bridge; + /* Enable-locking of bridges is performed within the pci_enable_bridge() */ + bool need_lock = !dev->subordinate; int err; int i, bars = 0; @@ -1680,8 +1686,13 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK); } - if (atomic_inc_return(&dev->enable_cnt) > 1) + if (need_lock) + mutex_lock(&dev->enable_mutex); + if (pci_is_enabled(dev)) { + if (need_lock) + mutex_unlock(&dev->enable_mutex); return 0; /* already enabled */ + } bridge = pci_upstream_bridge(dev); if (bridge) @@ -1696,8 +1707,10 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) bars |= (1 << i); err = do_pci_enable_device(dev, bars); - if (err < 0) - atomic_dec(&dev->enable_cnt); + if (err >= 0) + atomic_inc(&dev->enable_cnt); + if (need_lock) + mutex_unlock(&dev->enable_mutex); return err; } @@ -1941,15 +1954,20 @@ void pci_disable_device(struct pci_dev *dev) if (dr) dr->enabled = 0; + mutex_lock(&dev->enable_mutex); dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= 0, "disabling already-disabled device"); - if (atomic_dec_return(&dev->enable_cnt) != 0) + if (atomic_dec_return(&dev->enable_cnt) != 0) { + mutex_unlock(&dev->enable_mutex); return; + } do_pci_disable_device(dev); dev->is_busmaster = 0; + + mutex_unlock(&dev->enable_mutex); } EXPORT_SYMBOL(pci_disable_device); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a3c7338fad86..2e58ece820e8 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2427,6 +2427,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->enable_mutex); return dev; } diff --git a/include/linux/pci.h b/include/linux/pci.h index 9e700d9f9f28..d3a72159722d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -425,6 +425,7 @@ struct pci_dev { unsigned int no_vf_scan:1; /* Don't scan for VFs after IOV enablement */ pci_dev_flags_t dev_flags; atomic_t enable_cnt; /* pci_enable_device has been called */ + struct mutex enable_mutex; u32 saved_config_space[16]; /* Config space saved at suspend time */ struct hlist_head saved_cap_space; -- 2.21.0
WARNING: multiple messages have this Message-ID (diff)
From: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> To: <linux-pci@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org> Cc: Marta Rybczynska <mrybczyn@kalray.eu>, Sergey Miroshnichenko <s.miroshnichenko@yadro.com>, Srinath Mannam <srinath.mannam@broadcom.com>, Bjorn Helgaas <helgaas@kernel.org>, linux@yadro.com Subject: [PATCH v5 01/23] PCI: Fix race condition in pci_enable/disable_device() Date: Fri, 16 Aug 2019 19:50:39 +0300 [thread overview] Message-ID: <20190816165101.911-2-s.miroshnichenko@yadro.com> (raw) In-Reply-To: <20190816165101.911-1-s.miroshnichenko@yadro.com> This is a yet another approach to fix an old [1-2] concurrency issue, when: - two or more devices are being hot-added into a bridge which was initially empty; - a bridge with two or more devices is being hot-added; - during boot, if BIOS/bootloader/firmware doesn't pre-enable bridges. The problem is that a bridge is reported as enabled before the MEM/IO bits are actually written to the PCI_COMMAND register, so another driver thread starts memory requests through the not-yet-enabled bridge: CPU0 CPU1 pci_enable_device_mem() pci_enable_device_mem() pci_enable_bridge() pci_enable_bridge() pci_is_enabled() return false; atomic_inc_return(enable_cnt) Start actual enabling the bridge ... pci_is_enabled() ... return true; ... Start memory requests <-- FAIL ... Set the PCI_COMMAND_MEMORY bit <-- Must wait for this Protect the pci_enable/disable_device() and pci_enable_bridge(), which is similar to the previous solution from commit 40f11adc7cd9 ("PCI: Avoid race while enabling upstream bridges"), but adding a per-device mutexes and preventing the dev->enable_cnt from from incrementing early. CC: Srinath Mannam <srinath.mannam@broadcom.com> CC: Marta Rybczynska <mrybczyn@kalray.eu> Signed-off-by: Sergey Miroshnichenko <s.miroshnichenko@yadro.com> [1] https://lore.kernel.org/linux-pci/1501858648-22228-1-git-send-email-srinath.mannam@broadcom.com/T/#u [RFC PATCH v3] pci: Concurrency issue during pci enable bridge [2] https://lore.kernel.org/linux-pci/744877924.5841545.1521630049567.JavaMail.zimbra@kalray.eu/T/#u [RFC PATCH] nvme: avoid race-conditions when enabling devices --- drivers/pci/pci.c | 26 ++++++++++++++++++++++---- drivers/pci/probe.c | 1 + include/linux/pci.h | 1 + 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 1b27b5af3d55..e7f8c354e644 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1645,6 +1645,8 @@ static void pci_enable_bridge(struct pci_dev *dev) struct pci_dev *bridge; int retval; + mutex_lock(&dev->enable_mutex); + bridge = pci_upstream_bridge(dev); if (bridge) pci_enable_bridge(bridge); @@ -1652,6 +1654,7 @@ static void pci_enable_bridge(struct pci_dev *dev) if (pci_is_enabled(dev)) { if (!dev->is_busmaster) pci_set_master(dev); + mutex_unlock(&dev->enable_mutex); return; } @@ -1660,11 +1663,14 @@ static void pci_enable_bridge(struct pci_dev *dev) pci_err(dev, "Error enabling bridge (%d), continuing\n", retval); pci_set_master(dev); + mutex_unlock(&dev->enable_mutex); } static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) { struct pci_dev *bridge; + /* Enable-locking of bridges is performed within the pci_enable_bridge() */ + bool need_lock = !dev->subordinate; int err; int i, bars = 0; @@ -1680,8 +1686,13 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK); } - if (atomic_inc_return(&dev->enable_cnt) > 1) + if (need_lock) + mutex_lock(&dev->enable_mutex); + if (pci_is_enabled(dev)) { + if (need_lock) + mutex_unlock(&dev->enable_mutex); return 0; /* already enabled */ + } bridge = pci_upstream_bridge(dev); if (bridge) @@ -1696,8 +1707,10 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) bars |= (1 << i); err = do_pci_enable_device(dev, bars); - if (err < 0) - atomic_dec(&dev->enable_cnt); + if (err >= 0) + atomic_inc(&dev->enable_cnt); + if (need_lock) + mutex_unlock(&dev->enable_mutex); return err; } @@ -1941,15 +1954,20 @@ void pci_disable_device(struct pci_dev *dev) if (dr) dr->enabled = 0; + mutex_lock(&dev->enable_mutex); dev_WARN_ONCE(&dev->dev, atomic_read(&dev->enable_cnt) <= 0, "disabling already-disabled device"); - if (atomic_dec_return(&dev->enable_cnt) != 0) + if (atomic_dec_return(&dev->enable_cnt) != 0) { + mutex_unlock(&dev->enable_mutex); return; + } do_pci_disable_device(dev); dev->is_busmaster = 0; + + mutex_unlock(&dev->enable_mutex); } EXPORT_SYMBOL(pci_disable_device); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a3c7338fad86..2e58ece820e8 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2427,6 +2427,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->enable_mutex); return dev; } diff --git a/include/linux/pci.h b/include/linux/pci.h index 9e700d9f9f28..d3a72159722d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -425,6 +425,7 @@ struct pci_dev { unsigned int no_vf_scan:1; /* Don't scan for VFs after IOV enablement */ pci_dev_flags_t dev_flags; atomic_t enable_cnt; /* pci_enable_device has been called */ + struct mutex enable_mutex; u32 saved_config_space[16]; /* Config space saved at suspend time */ struct hlist_head saved_cap_space; -- 2.21.0
next prev parent reply other threads:[~2019-08-16 16:51 UTC|newest] Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-16 16:50 [PATCH v5 00/23] PCI: Allow BAR movement during hotplug Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko [this message] 2019-08-16 16:50 ` [PATCH v5 01/23] PCI: Fix race condition in pci_enable/disable_device() Sergey Miroshnichenko 2019-08-22 12:37 ` Marta Rybczynska 2019-08-22 12:37 ` Marta Rybczynska 2019-09-27 21:59 ` Bjorn Helgaas 2019-09-27 21:59 ` Bjorn Helgaas 2019-09-30 8:53 ` Sergey Miroshnichenko 2019-09-30 8:53 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 02/23] PCI: Enable bridge's I/O and MEM access for hotplugged devices Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-09-27 22:01 ` Bjorn Helgaas 2019-08-16 16:50 ` [PATCH v5 03/23] PCI: hotplug: Add a flag for the movable BARs feature Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-09-27 22:02 ` Bjorn Helgaas 2019-09-27 22:02 ` Bjorn Helgaas 2019-09-30 8:44 ` David Laight 2019-09-30 16:17 ` Sergey Miroshnichenko 2019-09-30 12:59 ` Sergey Miroshnichenko 2019-09-30 12:59 ` Sergey Miroshnichenko 2019-10-15 22:14 ` Bjorn Helgaas 2019-10-15 22:14 ` Bjorn Helgaas 2019-10-16 15:50 ` Sergey Miroshnichenko 2019-10-16 15:50 ` Sergey Miroshnichenko 2019-10-16 17:29 ` Bjorn Helgaas 2019-10-16 17:29 ` Bjorn Helgaas 2019-08-16 16:50 ` [PATCH v5 04/23] PCI: Define PCI-specific version of the release_child_resources() Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 05/23] PCI: hotplug: movable BARs: Fix reassigning the released bridge windows Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 06/23] PCI: hotplug: movable BARs: Recalculate all bridge windows during rescan Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 07/23] PCI: hotplug: movable BARs: Don't allow added devices to steal resources Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 08/23] PCI: Include fixed and immovable BARs into the bus size calculating Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 09/23] PCI: Prohibit assigning BARs and bridge windows to non-direct parents Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 10/23] PCI: hotplug: movable BARs: Try to assign unassigned resources only once Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 11/23] PCI: hotplug: movable BARs: Calculate immovable parts of bridge windows Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 12/23] PCI: hotplug: movable BARs: Compute limits for relocated " Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 13/23] PCI: Make sure bridge windows include their fixed BARs Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 14/23] PCI: Fix assigning the fixed prefetchable resources Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 15/23] PCI: hotplug: movable BARs: Assign fixed and immovable BARs before others Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 16/23] PCI: hotplug: movable BARs: Don't reserve IO/mem bus space Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-09-04 5:42 ` Oliver O'Halloran 2019-09-04 5:42 ` Oliver O'Halloran 2019-09-04 11:22 ` Sergey Miroshnichenko 2019-09-04 11:22 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 17/23] powerpc/pci: Fix crash with enabled movable BARs Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 18/23] powerpc/pci: Handle BAR movement Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-09-04 5:37 ` Oliver O'Halloran 2019-09-04 5:37 ` Oliver O'Halloran 2019-09-06 16:24 ` Sergey Miroshnichenko 2019-09-06 16:24 ` Sergey Miroshnichenko 2019-09-09 14:02 ` Oliver O'Halloran 2019-09-09 14:02 ` Oliver O'Halloran 2019-08-16 16:50 ` [PATCH v5 19/23] PCI: hotplug: Configure MPS for hot-added bridges during bus rescan Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 20/23] PCI: hotplug: movable BARs: Enable the feature by default Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` [PATCH v5 21/23] nvme-pci: Handle movable BARs Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:50 ` Sergey Miroshnichenko 2019-08-16 16:51 ` [PATCH v5 22/23] PCI/portdrv: Declare support of " Sergey Miroshnichenko 2019-08-16 16:51 ` Sergey Miroshnichenko 2019-08-16 16:51 ` [PATCH v5 23/23] PCI: pciehp: movable BARs: Trigger a domain rescan on hp events Sergey Miroshnichenko 2019-08-16 16:51 ` Sergey Miroshnichenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190816165101.911-2-s.miroshnichenko@yadro.com \ --to=s.miroshnichenko@yadro.com \ --cc=helgaas@kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux@yadro.com \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mrybczyn@kalray.eu \ --cc=srinath.mannam@broadcom.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.