diff options
author | Hari Vyas <hari.vyas@broadcom.com> | 2018-07-03 05:05:41 -0400 |
---|---|---|
committer | Bjorn Helgaas <bhelgaas@google.com> | 2018-07-31 12:27:54 -0400 |
commit | 44bda4b7d26e9fffed6d7152d98a2e9edaeb2a76 (patch) | |
tree | 46ed284871234e825bc3211dd5bf0d960b7c3cda | |
parent | a54e43f993f8ec2f063b616a0e4d2b09e08d78a5 (diff) |
PCI: Fix is_added/is_busmaster race condition
When a PCI device is detected, pdev->is_added is set to 1 and proc and
sysfs entries are created.
When the device is removed, pdev->is_added is checked for one and then
device is detached with clearing of proc and sys entries and at end,
pdev->is_added is set to 0.
is_added and is_busmaster are bit fields in pci_dev structure sharing same
memory location.
A strange issue was observed with multiple removal and rescan of a PCIe
NVMe device using sysfs commands where is_added flag was observed as zero
instead of one while removing device and proc,sys entries are not cleared.
This causes issue in later device addition with warning message
"proc_dir_entry" already registered.
Debugging revealed a race condition between the PCI core setting the
is_added bit in pci_bus_add_device() and the NVMe driver reset work-queue
setting the is_busmaster bit in pci_set_master(). As these fields are not
handled atomically, that clears the is_added bit.
Move the is_added bit to a separate private flag variable and use atomic
functions to set and retrieve the device addition state. This avoids the
race because is_added no longer shares a memory location with is_busmaster.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=200283
Signed-off-by: Hari Vyas <hari.vyas@broadcom.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Lukas Wunner <lukas@wunner.de>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
-rw-r--r-- | arch/powerpc/kernel/pci-common.c | 4 | ||||
-rw-r--r-- | arch/powerpc/platforms/powernv/pci-ioda.c | 3 | ||||
-rw-r--r-- | arch/powerpc/platforms/pseries/setup.c | 3 | ||||
-rw-r--r-- | drivers/pci/bus.c | 6 | ||||
-rw-r--r-- | drivers/pci/hotplug/acpiphp_glue.c | 2 | ||||
-rw-r--r-- | drivers/pci/pci.h | 11 | ||||
-rw-r--r-- | drivers/pci/probe.c | 4 | ||||
-rw-r--r-- | drivers/pci/remove.c | 5 | ||||
-rw-r--r-- | include/linux/pci.h | 1 |
9 files changed, 27 insertions, 12 deletions
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c index fe9733ffffaa..471aac313b89 100644 --- a/arch/powerpc/kernel/pci-common.c +++ b/arch/powerpc/kernel/pci-common.c | |||
@@ -42,6 +42,8 @@ | |||
42 | #include <asm/ppc-pci.h> | 42 | #include <asm/ppc-pci.h> |
43 | #include <asm/eeh.h> | 43 | #include <asm/eeh.h> |
44 | 44 | ||
45 | #include "../../../drivers/pci/pci.h" | ||
46 | |||
45 | /* hose_spinlock protects accesses to the the phb_bitmap. */ | 47 | /* hose_spinlock protects accesses to the the phb_bitmap. */ |
46 | static DEFINE_SPINLOCK(hose_spinlock); | 48 | static DEFINE_SPINLOCK(hose_spinlock); |
47 | LIST_HEAD(hose_list); | 49 | LIST_HEAD(hose_list); |
@@ -1014,7 +1016,7 @@ void pcibios_setup_bus_devices(struct pci_bus *bus) | |||
1014 | /* Cardbus can call us to add new devices to a bus, so ignore | 1016 | /* Cardbus can call us to add new devices to a bus, so ignore |
1015 | * those who are already fully discovered | 1017 | * those who are already fully discovered |
1016 | */ | 1018 | */ |
1017 | if (dev->is_added) | 1019 | if (pci_dev_is_added(dev)) |
1018 | continue; | 1020 | continue; |
1019 | 1021 | ||
1020 | pcibios_setup_device(dev); | 1022 | pcibios_setup_device(dev); |
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c index 5bd0eb6681bc..70b2e1e0f23c 100644 --- a/arch/powerpc/platforms/powernv/pci-ioda.c +++ b/arch/powerpc/platforms/powernv/pci-ioda.c | |||
@@ -46,6 +46,7 @@ | |||
46 | 46 | ||
47 | #include "powernv.h" | 47 | #include "powernv.h" |
48 | #include "pci.h" | 48 | #include "pci.h" |
49 | #include "../../../../drivers/pci/pci.h" | ||
49 | 50 | ||
50 | #define PNV_IODA1_M64_NUM 16 /* Number of M64 BARs */ | 51 | #define PNV_IODA1_M64_NUM 16 /* Number of M64 BARs */ |
51 | #define PNV_IODA1_M64_SEGS 8 /* Segments per M64 BAR */ | 52 | #define PNV_IODA1_M64_SEGS 8 /* Segments per M64 BAR */ |
@@ -3138,7 +3139,7 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev) | |||
3138 | struct pci_dn *pdn; | 3139 | struct pci_dn *pdn; |
3139 | int mul, total_vfs; | 3140 | int mul, total_vfs; |
3140 | 3141 | ||
3141 | if (!pdev->is_physfn || pdev->is_added) | 3142 | if (!pdev->is_physfn || pci_dev_is_added(pdev)) |
3142 | return; | 3143 | return; |
3143 | 3144 | ||
3144 | pdn = pci_get_pdn(pdev); | 3145 | pdn = pci_get_pdn(pdev); |
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c index 139f0af6c3d9..8a4868a3964b 100644 --- a/arch/powerpc/platforms/pseries/setup.c +++ b/arch/powerpc/platforms/pseries/setup.c | |||
@@ -71,6 +71,7 @@ | |||
71 | #include <asm/security_features.h> | 71 | #include <asm/security_features.h> |
72 | 72 | ||
73 | #include "pseries.h" | 73 | #include "pseries.h" |
74 | #include "../../../../drivers/pci/pci.h" | ||
74 | 75 | ||
75 | int CMO_PrPSP = -1; | 76 | int CMO_PrPSP = -1; |
76 | int CMO_SecPSP = -1; | 77 | int CMO_SecPSP = -1; |
@@ -664,7 +665,7 @@ static void pseries_pci_fixup_iov_resources(struct pci_dev *pdev) | |||
664 | const int *indexes; | 665 | const int *indexes; |
665 | struct device_node *dn = pci_device_to_OF_node(pdev); | 666 | struct device_node *dn = pci_device_to_OF_node(pdev); |
666 | 667 | ||
667 | if (!pdev->is_physfn || pdev->is_added) | 668 | if (!pdev->is_physfn || pci_dev_is_added(pdev)) |
668 | return; | 669 | return; |
669 | /*Firmware must support open sriov otherwise dont configure*/ | 670 | /*Firmware must support open sriov otherwise dont configure*/ |
670 | indexes = of_get_property(dn, "ibm,open-sriov-vf-bar-info", NULL); | 671 | 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 35b7fc87eac5..5cb40b2518f9 100644 --- a/drivers/pci/bus.c +++ b/drivers/pci/bus.c | |||
@@ -330,7 +330,7 @@ void pci_bus_add_device(struct pci_dev *dev) | |||
330 | return; | 330 | return; |
331 | } | 331 | } |
332 | 332 | ||
333 | dev->is_added = 1; | 333 | pci_dev_assign_added(dev, true); |
334 | } | 334 | } |
335 | EXPORT_SYMBOL_GPL(pci_bus_add_device); | 335 | EXPORT_SYMBOL_GPL(pci_bus_add_device); |
336 | 336 | ||
@@ -347,14 +347,14 @@ void pci_bus_add_devices(const struct pci_bus *bus) | |||
347 | 347 | ||
348 | list_for_each_entry(dev, &bus->devices, bus_list) { | 348 | list_for_each_entry(dev, &bus->devices, bus_list) { |
349 | /* Skip already-added devices */ | 349 | /* Skip already-added devices */ |
350 | if (dev->is_added) | 350 | if (pci_dev_is_added(dev)) |
351 | continue; | 351 | continue; |
352 | pci_bus_add_device(dev); | 352 | pci_bus_add_device(dev); |
353 | } | 353 | } |
354 | 354 | ||
355 | list_for_each_entry(dev, &bus->devices, bus_list) { | 355 | list_for_each_entry(dev, &bus->devices, bus_list) { |
356 | /* Skip if device attach failed */ | 356 | /* Skip if device attach failed */ |
357 | if (!dev->is_added) | 357 | if (!pci_dev_is_added(dev)) |
358 | continue; | 358 | continue; |
359 | child = dev->subordinate; | 359 | child = dev->subordinate; |
360 | if (child) | 360 | if (child) |
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c index 3a17b290df5d..ef0b1b6ba86f 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) | |||
509 | 509 | ||
510 | list_for_each_entry(dev, &bus->devices, bus_list) { | 510 | list_for_each_entry(dev, &bus->devices, bus_list) { |
511 | /* Assume that newly added devices are powered on already. */ | 511 | /* Assume that newly added devices are powered on already. */ |
512 | if (!dev->is_added) | 512 | if (!pci_dev_is_added(dev)) |
513 | dev->current_state = PCI_D0; | 513 | dev->current_state = PCI_D0; |
514 | } | 514 | } |
515 | 515 | ||
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 882f1f9596df..08817253c8a2 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h | |||
@@ -288,6 +288,7 @@ struct pci_sriov { | |||
288 | 288 | ||
289 | /* pci_dev priv_flags */ | 289 | /* pci_dev priv_flags */ |
290 | #define PCI_DEV_DISCONNECTED 0 | 290 | #define PCI_DEV_DISCONNECTED 0 |
291 | #define PCI_DEV_ADDED 1 | ||
291 | 292 | ||
292 | static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused) | 293 | static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused) |
293 | { | 294 | { |
@@ -300,6 +301,16 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev) | |||
300 | return test_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags); | 301 | return test_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags); |
301 | } | 302 | } |
302 | 303 | ||
304 | static inline void pci_dev_assign_added(struct pci_dev *dev, bool added) | ||
305 | { | ||
306 | assign_bit(PCI_DEV_ADDED, &dev->priv_flags, added); | ||
307 | } | ||
308 | |||
309 | static inline bool pci_dev_is_added(const struct pci_dev *dev) | ||
310 | { | ||
311 | return test_bit(PCI_DEV_ADDED, &dev->priv_flags); | ||
312 | } | ||
313 | |||
303 | #ifdef CONFIG_PCI_ATS | 314 | #ifdef CONFIG_PCI_ATS |
304 | void pci_restore_ats_state(struct pci_dev *dev); | 315 | void pci_restore_ats_state(struct pci_dev *dev); |
305 | #else | 316 | #else |
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index ac876e32de4b..611adcd9c169 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c | |||
@@ -2433,13 +2433,13 @@ int pci_scan_slot(struct pci_bus *bus, int devfn) | |||
2433 | dev = pci_scan_single_device(bus, devfn); | 2433 | dev = pci_scan_single_device(bus, devfn); |
2434 | if (!dev) | 2434 | if (!dev) |
2435 | return 0; | 2435 | return 0; |
2436 | if (!dev->is_added) | 2436 | if (!pci_dev_is_added(dev)) |
2437 | nr++; | 2437 | nr++; |
2438 | 2438 | ||
2439 | for (fn = next_fn(bus, dev, 0); fn > 0; fn = next_fn(bus, dev, fn)) { | 2439 | for (fn = next_fn(bus, dev, 0); fn > 0; fn = next_fn(bus, dev, fn)) { |
2440 | dev = pci_scan_single_device(bus, devfn + fn); | 2440 | dev = pci_scan_single_device(bus, devfn + fn); |
2441 | if (dev) { | 2441 | if (dev) { |
2442 | if (!dev->is_added) | 2442 | if (!pci_dev_is_added(dev)) |
2443 | nr++; | 2443 | nr++; |
2444 | dev->multifunction = 1; | 2444 | dev->multifunction = 1; |
2445 | } | 2445 | } |
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index 6f072eae4f7a..5e3d0dced2b8 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c | |||
@@ -19,11 +19,12 @@ static void pci_stop_dev(struct pci_dev *dev) | |||
19 | { | 19 | { |
20 | pci_pme_active(dev, false); | 20 | pci_pme_active(dev, false); |
21 | 21 | ||
22 | if (dev->is_added) { | 22 | if (pci_dev_is_added(dev)) { |
23 | device_release_driver(&dev->dev); | 23 | device_release_driver(&dev->dev); |
24 | pci_proc_detach_device(dev); | 24 | pci_proc_detach_device(dev); |
25 | pci_remove_sysfs_dev_files(dev); | 25 | pci_remove_sysfs_dev_files(dev); |
26 | dev->is_added = 0; | 26 | |
27 | pci_dev_assign_added(dev, false); | ||
27 | } | 28 | } |
28 | 29 | ||
29 | if (dev->bus->self) | 30 | if (dev->bus->self) |
diff --git a/include/linux/pci.h b/include/linux/pci.h index abd5d5e17aee..c133ccfa002e 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h | |||
@@ -368,7 +368,6 @@ struct pci_dev { | |||
368 | unsigned int transparent:1; /* Subtractive decode bridge */ | 368 | unsigned int transparent:1; /* Subtractive decode bridge */ |
369 | unsigned int multifunction:1; /* Multi-function device */ | 369 | unsigned int multifunction:1; /* Multi-function device */ |
370 | 370 | ||
371 | unsigned int is_added:1; | ||
372 | unsigned int is_busmaster:1; /* Is busmaster */ | 371 | unsigned int is_busmaster:1; /* Is busmaster */ |
373 | unsigned int no_msi:1; /* May not use MSI */ | 372 | unsigned int no_msi:1; /* May not use MSI */ |
374 | unsigned int no_64bit_msi:1; /* May only use 32-bit MSIs */ | 373 | unsigned int no_64bit_msi:1; /* May only use 32-bit MSIs */ |