| Message ID | 1530268061-17324-2-git-send-email-hari.vyas@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Hari,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pci/next]
[also build test ERROR on v4.18-rc2 next-20180629]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Hari-Vyas/PCI-Data-corruption-happening-due-to-race-condition/20180629-203647
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: x86_64-randconfig-x005-201825 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
Note: the linux-review/Hari-Vyas/PCI-Data-corruption-happening-due-to-race-condition/20180629-203647 HEAD 75ee48282af54fe77cb0ac092623577327440033 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
drivers/pci/hotplug/acpiphp_glue.c: In function 'enable_slot':
>> drivers/pci/hotplug/acpiphp_glue.c:512:13: error: 'struct pci_dev' has no member named 'is_added'; did you mean 'is_managed'?
if (!dev->is_added)
^~~~~~~~
is_managed
vim +512 drivers/pci/hotplug/acpiphp_glue.c
84c8b58ed3 Mika Westerberg 2018-05-29 456
^1da177e4c Linus Torvalds 2005-04-16 457 /**
a1d0abcea8 Rafael J. Wysocki 2013-07-13 458 * enable_slot - enable, configure a slot
^1da177e4c Linus Torvalds 2005-04-16 459 * @slot: slot to be enabled
^1da177e4c Linus Torvalds 2005-04-16 460 *
^1da177e4c Linus Torvalds 2005-04-16 461 * This function should be called per *physical slot*,
^1da177e4c Linus Torvalds 2005-04-16 462 * not per each slot object in ACPI namespace.
^1da177e4c Linus Torvalds 2005-04-16 463 */
10874f5a00 Bjorn Helgaas 2014-04-14 464 static void enable_slot(struct acpiphp_slot *slot)
^1da177e4c Linus Torvalds 2005-04-16 465 {
^1da177e4c Linus Torvalds 2005-04-16 466 struct pci_dev *dev;
bda46dbb66 Rafael J. Wysocki 2013-07-13 467 struct pci_bus *bus = slot->bus;
^1da177e4c Linus Torvalds 2005-04-16 468 struct acpiphp_func *func;
84c8b58ed3 Mika Westerberg 2018-05-29 469
84c8b58ed3 Mika Westerberg 2018-05-29 470 if (bus->self && hotplug_is_native(bus->self)) {
84c8b58ed3 Mika Westerberg 2018-05-29 471 /*
84c8b58ed3 Mika Westerberg 2018-05-29 472 * If native hotplug is used, it will take care of hotplug
84c8b58ed3 Mika Westerberg 2018-05-29 473 * slot management and resource allocation for hotplug
84c8b58ed3 Mika Westerberg 2018-05-29 474 * bridges. However, ACPI hotplug may still be used for
84c8b58ed3 Mika Westerberg 2018-05-29 475 * non-hotplug bridges to bring in additional devices such
84c8b58ed3 Mika Westerberg 2018-05-29 476 * as a Thunderbolt host controller.
84c8b58ed3 Mika Westerberg 2018-05-29 477 */
84c8b58ed3 Mika Westerberg 2018-05-29 478 for_each_pci_bridge(dev, bus) {
84c8b58ed3 Mika Westerberg 2018-05-29 479 if (PCI_SLOT(dev->devfn) == slot->device)
84c8b58ed3 Mika Westerberg 2018-05-29 480 acpiphp_native_scan_bridge(dev);
84c8b58ed3 Mika Westerberg 2018-05-29 481 }
84c8b58ed3 Mika Westerberg 2018-05-29 482 pci_assign_unassigned_bridge_resources(bus->self);
84c8b58ed3 Mika Westerberg 2018-05-29 483 } else {
d66ecb7220 Jiang Liu 2013-06-23 484 LIST_HEAD(add_list);
84c8b58ed3 Mika Westerberg 2018-05-29 485 int max, pass;
^1da177e4c Linus Torvalds 2005-04-16 486
ab1225901d Mika Westerberg 2013-10-30 487 acpiphp_rescan_slot(slot);
15a1ae7487 Kristen Accardi 2006-02-23 488 max = acpiphp_max_busnr(bus);
42f49a6ae5 Rajesh Shah 2005-04-28 489 for (pass = 0; pass < 2; pass++) {
24a0c654d7 Andy Shevchenko 2017-10-20 490 for_each_pci_bridge(dev, bus) {
42f49a6ae5 Rajesh Shah 2005-04-28 491 if (PCI_SLOT(dev->devfn) != slot->device)
42f49a6ae5 Rajesh Shah 2005-04-28 492 continue;
a1d0abcea8 Rafael J. Wysocki 2013-07-13 493
42f49a6ae5 Rajesh Shah 2005-04-28 494 max = pci_scan_bridge(bus, dev, max, pass);
1f96a965e3 Yinghai Lu 2013-01-21 495 if (pass && dev->subordinate) {
1f96a965e3 Yinghai Lu 2013-01-21 496 check_hotplug_bridge(slot, dev);
d66ecb7220 Jiang Liu 2013-06-23 497 pcibios_resource_survey_bus(dev->subordinate);
84c8b58ed3 Mika Westerberg 2018-05-29 498 __pci_bus_size_bridges(dev->subordinate,
84c8b58ed3 Mika Westerberg 2018-05-29 499 &add_list);
c64b5eead9 Kristen Accardi 2005-12-14 500 }
42f49a6ae5 Rajesh Shah 2005-04-28 501 }
1f96a965e3 Yinghai Lu 2013-01-21 502 }
d66ecb7220 Jiang Liu 2013-06-23 503 __pci_bus_assign_resources(bus, &add_list, NULL);
84c8b58ed3 Mika Westerberg 2018-05-29 504 }
2dc41281b1 Rafael J. Wysocki 2013-09-06 505
8e5dce3522 Kristen Accardi 2005-10-18 506 acpiphp_sanitize_bus(bus);
81ee57326c Bjorn Helgaas 2014-08-28 507 pcie_bus_configure_settings(bus);
d060705091 Shaohua Li 2010-02-25 508 acpiphp_set_acpi_region(slot);
69643e4829 Ian Campbell 2011-05-11 509
69643e4829 Ian Campbell 2011-05-11 510 list_for_each_entry(dev, &bus->devices, bus_list) {
69643e4829 Ian Campbell 2011-05-11 511 /* Assume that newly added devices are powered on already. */
69643e4829 Ian Campbell 2011-05-11 @512 if (!dev->is_added)
69643e4829 Ian Campbell 2011-05-11 513 dev->current_state = PCI_D0;
69643e4829 Ian Campbell 2011-05-11 514 }
69643e4829 Ian Campbell 2011-05-11 515
42f49a6ae5 Rajesh Shah 2005-04-28 516 pci_bus_add_devices(bus);
42f49a6ae5 Rajesh Shah 2005-04-28 517
f382a086f3 Amos Kong 2011-11-25 518 slot->flags |= SLOT_ENABLED;
58c08628c4 Alex Chiang 2009-10-26 519 list_for_each_entry(func, &slot->funcs, sibling) {
9d911d7903 Alex Chiang 2009-05-21 520 dev = pci_get_slot(bus, PCI_DEVFN(slot->device,
^1da177e4c Linus Torvalds 2005-04-16 521 func->function));
f382a086f3 Amos Kong 2011-11-25 522 if (!dev) {
f382a086f3 Amos Kong 2011-11-25 523 /* Do not set SLOT_ENABLED flag if some funcs
f382a086f3 Amos Kong 2011-11-25 524 are not added. */
9337a49362 Mika Westerberg 2018-05-24 525 slot->flags &= ~SLOT_ENABLED;
551bcb75b3 MUNEDA Takahiro 2006-03-22 526 continue;
f382a086f3 Amos Kong 2011-11-25 527 }
^1da177e4c Linus Torvalds 2005-04-16 528 }
^1da177e4c Linus Torvalds 2005-04-16 529 }
^1da177e4c Linus Torvalds 2005-04-16 530
:::::: The code at line 512 was first introduced by commit
:::::: 69643e4829c5cd13bafe44a6b9f3eb2086e0f618 PCI hotplug: acpiphp: assume device is in state D0 after powering on a slot.
:::::: TO: Ian Campbell <ian.campbell@citrix.com>
:::::: CC: Jesse Barnes <jbarnes@virtuousgeek.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Hari,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on pci/next]
[also build test ERROR on v4.18-rc2 next-20180629]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Hari-Vyas/PCI-Data-corruption-happening-due-to-race-condition/20180629-203647
base: https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=powerpc
Note: the linux-review/Hari-Vyas/PCI-Data-corruption-happening-due-to-race-condition/20180629-203647 HEAD 75ee48282af54fe77cb0ac092623577327440033 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
arch/powerpc/kernel/pci-common.c: In function 'pcibios_setup_bus_devices':
>> arch/powerpc/kernel/pci-common.c:1017:12: error: 'struct pci_dev' has no member named 'is_added'; did you mean 'is_managed'?
if (dev->is_added)
^~~~~~~~
is_managed
vim +1017 arch/powerpc/kernel/pci-common.c
7846de40 Guenter Roeck 2013-06-10 1005
37f02195 Yuanquan Chen 2013-04-02 1006 void pcibios_setup_bus_devices(struct pci_bus *bus)
37f02195 Yuanquan Chen 2013-04-02 1007 {
37f02195 Yuanquan Chen 2013-04-02 1008 struct pci_dev *dev;
37f02195 Yuanquan Chen 2013-04-02 1009
37f02195 Yuanquan Chen 2013-04-02 1010 pr_debug("PCI: Fixup bus devices %d (%s)\n",
37f02195 Yuanquan Chen 2013-04-02 1011 bus->number, bus->self ? pci_name(bus->self) : "PHB");
37f02195 Yuanquan Chen 2013-04-02 1012
37f02195 Yuanquan Chen 2013-04-02 1013 list_for_each_entry(dev, &bus->devices, bus_list) {
37f02195 Yuanquan Chen 2013-04-02 1014 /* Cardbus can call us to add new devices to a bus, so ignore
37f02195 Yuanquan Chen 2013-04-02 1015 * those who are already fully discovered
37f02195 Yuanquan Chen 2013-04-02 1016 */
37f02195 Yuanquan Chen 2013-04-02 @1017 if (dev->is_added)
37f02195 Yuanquan Chen 2013-04-02 1018 continue;
37f02195 Yuanquan Chen 2013-04-02 1019
37f02195 Yuanquan Chen 2013-04-02 1020 pcibios_setup_device(dev);
37f02195 Yuanquan Chen 2013-04-02 1021 }
7eef440a Benjamin Herrenschmidt 2008-10-27 1022 }
7eef440a Benjamin Herrenschmidt 2008-10-27 1023
:::::: The code at line 1017 was first introduced by commit
:::::: 37f02195bee9c25ce44e25204f40b7961a6d7c9d powerpc/pci: fix PCI-e devices rescan issue on powerpc platform
:::::: TO: Yuanquan Chen <Yuanquan.Chen@freescale.com>
:::::: CC: Michael Ellerman <michael@ellerman.id.au>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, Jun 29, 2018 at 03:57:39PM +0530, Hari Vyas wrote: > Fix moves device addition is_added bit to separate private flag > variable and use different atomic functions to set, clear and > retrieve device addition state. As is_added shares different > memory location so race condition is avoided. As 0-day has already discovered, you need to squash all 3 patches together to avoid breaking the build. > +static inline int pci_dev_set_added(struct pci_dev *dev, void *unused) > +{ > + set_bit(PCI_DEV_ADDED, &dev->priv_flags); > + return 0; > +} > + > +static inline int pci_dev_clear_added(struct pci_dev *dev, void *unused) > +{ > + clear_bit(PCI_DEV_ADDED, &dev->priv_flags); > + return 0; > +} You don't need the "unused" parameter here and you can return void. pci_dev_set_disconnected() has the parameter because the function is passed in to pci_walk_bus() in a few places, but you're not doing that AFAICS. What you *could* do however is collapse pci_dev_set_added() and pci_dev_clear_added() into a single function, pass in a bool "added", then use assign_bit() to set or clear it. It would save 6 LoC. Thanks, Lukas
On Sun, Jul 1, 2018 at 7:36 PM, Lukas Wunner <lukas@wunner.de> wrote: > On Fri, Jun 29, 2018 at 03:57:39PM +0530, Hari Vyas wrote: >> Fix moves device addition is_added bit to separate private flag >> variable and use different atomic functions to set, clear and >> retrieve device addition state. As is_added shares different >> memory location so race condition is avoided. > > As 0-day has already discovered, you need to squash all 3 patches > together to avoid breaking the build. > > Agreed. I will squash and raise a new patch >> +static inline int pci_dev_set_added(struct pci_dev *dev, void *unused) >> +{ >> + set_bit(PCI_DEV_ADDED, &dev->priv_flags); >> + return 0; >> +} >> + >> +static inline int pci_dev_clear_added(struct pci_dev *dev, void *unused) >> +{ >> + clear_bit(PCI_DEV_ADDED, &dev->priv_flags); >> + return 0; >> +} > > You don't need the "unused" parameter here and you can return void. > pci_dev_set_disconnected() has the parameter because the function is > passed in to pci_walk_bus() in a few places, but you're not doing that > AFAICS. > > What you *could* do however is collapse pci_dev_set_added() and > pci_dev_clear_added() into a single function, pass in a bool "added", > then use assign_bit() to set or clear it. It would save 6 LoC. > Agreed. Just for symmetry I did that one. Will incorporate change. > Thanks, > > Lukas
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c index 35b7fc8..8674019 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; } - dev->is_added = 1; + pci_dev_set_added(dev, NULL); } 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 (dev->is_added) + if (pci_dev_is_added(dev)) continue; pci_bus_add_device(dev); } list_for_each_entry(dev, &bus->devices, bus_list) { /* Skip if device attach failed */ - if (!dev->is_added) + if (!pci_dev_is_added(dev)) continue; child = dev->subordinate; if (child) diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 97acba7..baefd55 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3675,6 +3675,7 @@ static void __pci_set_master(struct pci_dev *dev, bool enable) enable ? "enabling" : "disabling"); pci_write_config_word(dev, PCI_COMMAND, cmd); } + dev->is_busmaster = enable; } diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index c358e7a0..c924a4c 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -288,6 +288,7 @@ 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) { @@ -300,6 +301,23 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev) return test_bit(PCI_DEV_DISCONNECTED, &dev->priv_flags); } +static inline int pci_dev_set_added(struct pci_dev *dev, void *unused) +{ + set_bit(PCI_DEV_ADDED, &dev->priv_flags); + return 0; +} + +static inline int pci_dev_clear_added(struct pci_dev *dev, void *unused) +{ + clear_bit(PCI_DEV_ADDED, &dev->priv_flags); + return 0; +} + +static inline bool pci_dev_is_added(const struct pci_dev *dev) +{ + return test_bit(PCI_DEV_ADDED, &dev->priv_flags); +} + #ifdef CONFIG_PCI_ATS void pci_restore_ats_state(struct pci_dev *dev); #else diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index ac876e3..611adcd 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) dev = pci_scan_single_device(bus, devfn); if (!dev) return 0; - if (!dev->is_added) + if (!pci_dev_is_added(dev)) 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 (!dev->is_added) + if (!pci_dev_is_added(dev)) nr++; dev->multifunction = 1; } diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c index 6f072ea..a272cdc 100644 --- a/drivers/pci/remove.c +++ b/drivers/pci/remove.c @@ -19,11 +19,12 @@ static void pci_stop_dev(struct pci_dev *dev) { pci_pme_active(dev, false); - if (dev->is_added) { + if (pci_dev_is_added(dev)) { device_release_driver(&dev->dev); pci_proc_detach_device(dev); pci_remove_sysfs_dev_files(dev); - dev->is_added = 0; + + pci_dev_clear_added(dev, NULL); } if (dev->bus->self) diff --git a/include/linux/pci.h b/include/linux/pci.h index 340029b..506125b 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -368,7 +368,6 @@ 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 */
When a pci device is detected, a variable is_added is set to 1 in pci device structure and proc, sys entries are created. When a pci device is removed, first is_added is checked for one and then device is detached with clearing of proc and sys entries and at end, 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 times 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 pcie core driver enabling is_added bit(pci_bus_add_device()) and nvme driver reset work-queue enabling is_busmaster bit (by pci_set_master()). As both fields are not handled in atomic manner and that clears is_added bit. Fix moves device addition is_added bit to separate private flag variable and use different atomic functions to set, clear and retrieve device addition state. As is_added shares different memory location so race condition is avoided. Signed-off-by: Hari Vyas <hari.vyas@broadcom.com> --- drivers/pci/bus.c | 6 +++--- drivers/pci/pci.c | 1 + drivers/pci/pci.h | 18 ++++++++++++++++++ drivers/pci/probe.c | 4 ++-- drivers/pci/remove.c | 5 +++-- include/linux/pci.h | 1 - 6 files changed, 27 insertions(+), 8 deletions(-)