From 40f11adc7cd9281227f0a6a627d966dd0a5f0cd9 Mon Sep 17 00:00:00 2001 From: Srinath Mannam Date: Fri, 18 Aug 2017 21:50:48 -0500 Subject: [PATCH] PCI: Avoid race while enabling upstream bridges When we enable a device, we first enable any upstream bridges. If a bridge has multiple downstream devices and we enable them simultaneously, the race to enable the upstream bridge may cause problems. Consider this hierarchy: bridge A --+-- device B +-- device C If drivers for B and C call pci_enable_device() simultaneously, both will attempt to enable A, which involves setting PCI_COMMAND_MASTER via pci_set_master() and PCI_COMMAND_MEMORY via pci_enable_resources(). In the following sequence, B's update to set A's PCI_COMMAND_MEMORY is lost, and neither B nor C will work correctly: B C pci_set_master(A) cmd = read(A, PCI_COMMAND) cmd |= PCI_COMMAND_MASTER pci_set_master(A) cmd = read(A, PCI_COMMAND) cmd |= PCI_COMMAND_MASTER write(A, PCI_COMMAND, cmd) pci_enable_device(A) pci_enable_resources(A) cmd = read(A, PCI_COMMAND) cmd |= PCI_COMMAND_MEMORY write(A, PCI_COMMAND, cmd) write(A, PCI_COMMAND, cmd) Avoid this race by holding a new pci_bridge_mutex while enabling a bridge. This ensures that both PCI_COMMAND_MASTER and PCI_COMMAND_MEMORY will be updated before another thread can start enabling the bridge. Note that although pci_enable_bridge() is recursive, it enables any upstream bridges *before* acquiring the mutex. When it acquires the mutex and calls pci_set_master() and pci_enable_device(), any upstream bridges have already been enabled so pci_enable_device() will not deadlock by calling pci_enable_bridge() again. Signed-off-by: Srinath Mannam [bhelgaas: changelog, comment] Signed-off-by: Bjorn Helgaas --- drivers/pci/pci.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index af0cc3456dc1..7cb29a223b73 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -52,6 +52,7 @@ static void pci_pme_list_scan(struct work_struct *work); static LIST_HEAD(pci_pme_list); static DEFINE_MUTEX(pci_pme_list_mutex); static DECLARE_DELAYED_WORK(pci_pme_work, pci_pme_list_scan); +static DEFINE_MUTEX(pci_bridge_mutex); struct pci_pme_device { struct list_head list; @@ -1348,10 +1349,16 @@ static void pci_enable_bridge(struct pci_dev *dev) if (bridge) pci_enable_bridge(bridge); + /* + * Hold pci_bridge_mutex to prevent a race when enabling two + * devices below the bridge simultaneously. The race may cause a + * PCI_COMMAND_MEMORY update to be lost (see changelog). + */ + mutex_lock(&pci_bridge_mutex); if (pci_is_enabled(dev)) { if (!dev->is_busmaster) pci_set_master(dev); - return; + goto end; } retval = pci_enable_device(dev); @@ -1359,6 +1366,8 @@ static void pci_enable_bridge(struct pci_dev *dev) dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n", retval); pci_set_master(dev); +end: + mutex_unlock(&pci_bridge_mutex); } static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) @@ -1383,7 +1392,7 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags) return 0; /* already enabled */ bridge = pci_upstream_bridge(dev); - if (bridge) + if (bridge && !pci_is_enabled(bridge)) pci_enable_bridge(bridge); /* only skip sriov related */ -- 2.20.1