Previously we use bit field for block_cfg_access and msi_enabled/msix_enabled, which is non-atomic and they may race each other as they share the same memory region. A race condition is met between driver bind vs FLR through sysfs: for driver bind side in thread 1: ... device_lock() ... ->probe() pci_alloc_irq_vectors_affinity() __pci_enable_msi_range() msi_capability_init() dev->msi_enabled=1 <---set here request_irq(pci_irq_vector(),...) when echo 1 > reset in thread 2: pci_reset_function() pci_dev_lock() pci_cfg_access_lock() dev->block_cfg_access=1 <---may overwrite msi_enabled bit device_lock() The msi_enabled bit may be overwritten to 0 and will trigger the WARN assert in pci_irq_vector(). A similar issue has been addressed in commit 44bda4b7d26e ("PCI: Fix is_added/is_busmaster race condition"). Move the block_cfg_access to the pci_dev->priv_flags and use atomic bit operations to avoid the race condition. Signed-off-by: Yicong Yang <yangyicong@xxxxxxxxxxxxx> --- drivers/pci/access.c | 20 ++++++++++---------- drivers/pci/pci.h | 11 +++++++++++ include/linux/pci.h | 1 - 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/drivers/pci/access.c b/drivers/pci/access.c index 4693569..5826962 100644 --- a/drivers/pci/access.c +++ b/drivers/pci/access.c @@ -208,9 +208,9 @@ static noinline void pci_wait_cfg(struct pci_dev *dev) { do { raw_spin_unlock_irq(&pci_lock); - wait_event(pci_cfg_wait, !dev->block_cfg_access); + wait_event(pci_cfg_wait, !pci_dev_is_cfg_access_blocked(dev)); raw_spin_lock_irq(&pci_lock); - } while (dev->block_cfg_access); + } while (pci_dev_is_cfg_access_blocked(dev)); } /* Returns 0 on success, negative values indicate error. */ @@ -223,7 +223,7 @@ int pci_user_read_config_##size \ if (PCI_##size##_BAD) \ return -EINVAL; \ raw_spin_lock_irq(&pci_lock); \ - if (unlikely(dev->block_cfg_access)) \ + if (unlikely(pci_dev_is_cfg_access_blocked(dev))) \ pci_wait_cfg(dev); \ ret = dev->bus->ops->read(dev->bus, dev->devfn, \ pos, sizeof(type), &data); \ @@ -242,7 +242,7 @@ int pci_user_write_config_##size \ if (PCI_##size##_BAD) \ return -EINVAL; \ raw_spin_lock_irq(&pci_lock); \ - if (unlikely(dev->block_cfg_access)) \ + if (unlikely(pci_dev_is_cfg_access_blocked(dev))) \ pci_wait_cfg(dev); \ ret = dev->bus->ops->write(dev->bus, dev->devfn, \ pos, sizeof(type), val); \ @@ -271,9 +271,9 @@ void pci_cfg_access_lock(struct pci_dev *dev) might_sleep(); raw_spin_lock_irq(&pci_lock); - if (dev->block_cfg_access) + if (pci_dev_is_cfg_access_blocked(dev)) pci_wait_cfg(dev); - dev->block_cfg_access = 1; + pci_dev_block_cfg_access(dev, true); raw_spin_unlock_irq(&pci_lock); } EXPORT_SYMBOL_GPL(pci_cfg_access_lock); @@ -292,10 +292,10 @@ bool pci_cfg_access_trylock(struct pci_dev *dev) bool locked = true; raw_spin_lock_irqsave(&pci_lock, flags); - if (dev->block_cfg_access) + if (pci_dev_is_cfg_access_blocked(dev)) locked = false; else - dev->block_cfg_access = 1; + pci_dev_block_cfg_access(dev, true); raw_spin_unlock_irqrestore(&pci_lock, flags); return locked; @@ -318,9 +318,9 @@ void pci_cfg_access_unlock(struct pci_dev *dev) * This indicates a problem in the caller, but we don't need * to kill them, unlike a double-block above. */ - WARN_ON(!dev->block_cfg_access); + WARN_ON(!pci_dev_is_cfg_access_blocked(dev)); - dev->block_cfg_access = 0; + pci_dev_block_cfg_access(dev, false); raw_spin_unlock_irqrestore(&pci_lock, flags); wake_up_all(&pci_cfg_wait); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 73740dd..1cf3122 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -410,6 +410,7 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev) /* pci_dev priv_flags */ #define PCI_DEV_ADDED 0 +#define PCI_DEV_BLOCK_CFG_ACCESS 1 /* Config space access blocked */ static inline void pci_dev_assign_added(struct pci_dev *dev, bool added) { @@ -421,6 +422,16 @@ static inline bool pci_dev_is_added(const struct pci_dev *dev) return test_bit(PCI_DEV_ADDED, &dev->priv_flags); } +static inline void pci_dev_block_cfg_access(struct pci_dev *dev, bool block) +{ + assign_bit(PCI_DEV_BLOCK_CFG_ACCESS, &dev->priv_flags, block); +} + +static inline bool pci_dev_is_cfg_access_blocked(struct pci_dev *dev) +{ + return test_bit(PCI_DEV_BLOCK_CFG_ACCESS, &dev->priv_flags); +} + #ifdef CONFIG_PCIEAER #include <linux/aer.h> diff --git a/include/linux/pci.h b/include/linux/pci.h index 8355306..4ffb588 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -406,7 +406,6 @@ struct pci_dev { 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 */ unsigned int broken_parity_status:1; /* Generates false positive parity */ unsigned int irq_reroute_variant:2; /* Needs IRQ rerouting variant */ unsigned int msi_enabled:1; -- 2.8.1