[PATCH 0/3] PCI: Rework config space locking, add INTx masking services

[PATCH 0/3] PCI: Rework config space locking, add INTx masking services

[Jan Kiszka]

This series tries to heal the currently broken locking scheme around PCI
config space accesses.

We have an interface lock out access via sysfs, but that service wrongly
assumes it is only called by one instance at a time for some device. So
two loops doing

echo 1 > /sys/bus/pci/devices/<some-device>/reset

in parallel will trigger a kernel BUG at the moment.

Besides synchronizing with user space, we also need to manage config
space access of generic PCI drivers. They need to mask legacy interrupt
lines while the specific driver runs in user space or a guest OS.

The approach taken here is provide mutex-like locking for general
access - which still requires a special mechanism due to requirements of
the IBM Power RAID SCSI driver. Furthermore, INTx masking is now
available via the PCI core and synchronized via the internal pci_lock.

Not sure who may want to take this, so I'm CC'ing broadly.

CC: Brian King <brking@linux.vnet.ibm.com>

Jan Kiszka (3):
  pci: Rework config space blocking services
  pci: Introduce INTx check & mask API
  uio: Convert uio_generic_pci to new intx masking API

 drivers/pci/access.c          |   76 +++++++++++++++++----------
 drivers/pci/iov.c             |   12 ++--
 drivers/pci/pci.c             |  114 ++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/pci.h             |    2 +
 drivers/scsi/ipr.c            |   66 +++++++++++++++++++++---
 drivers/scsi/ipr.h            |    1 +
 drivers/uio/uio_pci_generic.c |   82 +++---------------------------
 include/linux/pci.h           |   17 ++++--
 8 files changed, 248 insertions(+), 122 deletions(-)

-- 
1.7.3.4

[PATCH 1/3] pci: Rework config space blocking services

[Jan Kiszka]

pci_block_user_cfg_access was designed for the use case that a single
context, the IPR driver, temporarily delays user space accesses to the
config space via sysfs. This assumption became invalid by the time
pci_dev_reset was added as locking instance. Today, if you run two loops
in parallel that reset the same device via sysfs, you end up with a
kernel BUG as pci_block_user_cfg_access detect the broken assumption.

This reworks the pci_block_user_cfg_access to a sleeping service
pci_cfg_access_lock and an atomic-compatible variant called
pci_cfg_access_trylock. The former not only blocks user space access as
before but also waits if access was already locked. The latter service
just returns false in this case, allowing the caller to resolve the
conflict instead of raising a BUG.

Adaptions of the ipr driver were originally written by Brian King.

CC: Brian King <brking@linux.vnet.ibm.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/pci/access.c          |   74 ++++++++++++++++++++++++++--------------
 drivers/pci/iov.c             |   12 +++---
 drivers/pci/pci.c             |    4 +-
 drivers/scsi/ipr.c            |   66 ++++++++++++++++++++++++++++++++----
 drivers/scsi/ipr.h            |    1 +
 drivers/uio/uio_pci_generic.c |   10 +++--
 include/linux/pci.h           |   14 +++++---
 7 files changed, 131 insertions(+), 50 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index fdaa42a..0c4c717 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -127,20 +127,20 @@ EXPORT_SYMBOL(pci_write_vpd);
  * We have a bit per device to indicate it's blocked and a global wait queue
  * for callers to sleep on until devices are unblocked.
  */
-static DECLARE_WAIT_QUEUE_HEAD(pci_ucfg_wait);
+static DECLARE_WAIT_QUEUE_HEAD(pci_cfg_wait);
 
-static noinline void pci_wait_ucfg(struct pci_dev *dev)
+static noinline void pci_wait_cfg(struct pci_dev *dev)
 {
 	DECLARE_WAITQUEUE(wait, current);
 
-	__add_wait_queue(&pci_ucfg_wait, &wait);
+	__add_wait_queue(&pci_cfg_wait, &wait);
 	do {
 		set_current_state(TASK_UNINTERRUPTIBLE);
 		raw_spin_unlock_irq(&pci_lock);
 		schedule();
 		raw_spin_lock_irq(&pci_lock);
-	} while (dev->block_ucfg_access);
-	__remove_wait_queue(&pci_ucfg_wait, &wait);
+	} while (dev->block_cfg_access);
+	__remove_wait_queue(&pci_cfg_wait, &wait);
 }
 
 /* Returns 0 on success, negative values indicate error. */
@@ -153,7 +153,8 @@ int pci_user_read_config_##size						\
 	if (PCI_##size##_BAD)						\
 		return -EINVAL;						\
 	raw_spin_lock_irq(&pci_lock);				\
-	if (unlikely(dev->block_ucfg_access)) pci_wait_ucfg(dev);	\
+	if (unlikely(dev->block_cfg_access))				\
+		pci_wait_cfg(dev);					\
 	ret = dev->bus->ops->read(dev->bus, dev->devfn,			\
 					pos, sizeof(type), &data);	\
 	raw_spin_unlock_irq(&pci_lock);				\
@@ -172,7 +173,8 @@ int pci_user_write_config_##size					\
 	if (PCI_##size##_BAD)						\
 		return -EINVAL;						\
 	raw_spin_lock_irq(&pci_lock);				\
-	if (unlikely(dev->block_ucfg_access)) pci_wait_ucfg(dev);	\
+	if (unlikely(dev->block_cfg_access))				\
+		pci_wait_cfg(dev);					\
 	ret = dev->bus->ops->write(dev->bus, dev->devfn,		\
 					pos, sizeof(type), val);	\
 	raw_spin_unlock_irq(&pci_lock);				\
@@ -401,36 +403,56 @@ int pci_vpd_truncate(struct pci_dev *dev, size_t size)
 EXPORT_SYMBOL(pci_vpd_truncate);
 
 /**
- * pci_block_user_cfg_access - Block userspace PCI config reads/writes
+ * pci_cfg_access_lock - Lock PCI config reads/writes
  * @dev:	pci device struct
  *
- * When user access is blocked, any reads or writes to config space will
- * sleep until access is unblocked again.  We don't allow nesting of
- * block/unblock calls.
+ * When access is locked, any userspace reads or writes to config
+ * space and concurrent lock requests will sleep until access is
+ * allowed via pci_cfg_access_unlocked again.
  */
-void pci_block_user_cfg_access(struct pci_dev *dev)
+void pci_cfg_access_lock(struct pci_dev *dev)
+{
+	might_sleep();
+
+	raw_spin_lock_irq(&pci_lock);
+	if (dev->block_cfg_access)
+		pci_wait_cfg(dev);
+	dev->block_cfg_access = 1;
+	raw_spin_unlock_irq(&pci_lock);
+}
+EXPORT_SYMBOL_GPL(pci_cfg_access_lock);
+
+/**
+ * pci_cfg_access_trylock - try to lock PCI config reads/writes
+ * @dev:	pci device struct
+ *
+ * Same as pci_cfg_access_lock, but will return 0 if access is
+ * already locked, 1 otherwise. This function can be used from
+ * atomic contexts.
+ */
+bool pci_cfg_access_trylock(struct pci_dev *dev)
 {
 	unsigned long flags;
-	int was_blocked;
+	bool locked = true;
 
 	raw_spin_lock_irqsave(&pci_lock, flags);
-	was_blocked = dev->block_ucfg_access;
-	dev->block_ucfg_access = 1;
+	if (dev->block_cfg_access)
+		locked = false;
+	else
+		dev->block_cfg_access = 1;
 	raw_spin_unlock_irqrestore(&pci_lock, flags);
 
-	/* If we BUG() inside the pci_lock, we're guaranteed to hose
-	 * the machine */
-	BUG_ON(was_blocked);
+	return locked;
 }
-EXPORT_SYMBOL_GPL(pci_block_user_cfg_access);
+EXPORT_SYMBOL_GPL(pci_cfg_access_trylock);
 
 /**
- * pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes
+ * pci_cfg_access_unlock - Unlock PCI config reads/writes
  * @dev:	pci device struct
  *
- * This function allows userspace PCI config accesses to resume.
+ * This function allows PCI config accesses to resume.
  */
-void pci_unblock_user_cfg_access(struct pci_dev *dev)
+void pci_cfg_access_unlock(struct pci_dev *dev)
 {
 	unsigned long flags;
 
@@ -438,10 +460,10 @@ void pci_unblock_user_cfg_access(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_ucfg_access);
+	WARN_ON(!dev->block_cfg_access);
 
-	dev->block_ucfg_access = 0;
-	wake_up_all(&pci_ucfg_wait);
+	dev->block_cfg_access = 0;
+	wake_up_all(&pci_cfg_wait);
 	raw_spin_unlock_irqrestore(&pci_lock, flags);
 }
-EXPORT_SYMBOL_GPL(pci_unblock_user_cfg_access);
+EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index 42fae47..e737b91 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -340,10 +340,10 @@ static int sriov_enable(struct pci_dev *dev, int nr_virtfn)
 	}
 
 	iov->ctrl |= PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE;
-	pci_block_user_cfg_access(dev);
+	pci_cfg_access_lock(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
 	msleep(100);
-	pci_unblock_user_cfg_access(dev);
+	pci_cfg_access_unlock(dev);
 
 	iov->initial = initial;
 	if (nr_virtfn < initial)
@@ -371,10 +371,10 @@ failed:
 		virtfn_remove(dev, j, 0);
 
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
-	pci_block_user_cfg_access(dev);
+	pci_cfg_access_lock(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
 	ssleep(1);
-	pci_unblock_user_cfg_access(dev);
+	pci_cfg_access_unlock(dev);
 
 	if (iov->link != dev->devfn)
 		sysfs_remove_link(&dev->dev.kobj, "dep_link");
@@ -397,10 +397,10 @@ static void sriov_disable(struct pci_dev *dev)
 		virtfn_remove(dev, i, 0);
 
 	iov->ctrl &= ~(PCI_SRIOV_CTRL_VFE | PCI_SRIOV_CTRL_MSE);
-	pci_block_user_cfg_access(dev);
+	pci_cfg_access_lock(dev);
 	pci_write_config_word(dev, iov->pos + PCI_SRIOV_CTRL, iov->ctrl);
 	ssleep(1);
-	pci_unblock_user_cfg_access(dev);
+	pci_cfg_access_unlock(dev);
 
 	if (iov->link != dev->devfn)
 		sysfs_remove_link(&dev->dev.kobj, "dep_link");
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 0ce6742..208fe21 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2959,7 +2959,7 @@ static int pci_dev_reset(struct pci_dev *dev, int probe)
 	might_sleep();
 
 	if (!probe) {
-		pci_block_user_cfg_access(dev);
+		pci_cfg_access_lock(dev);
 		/* block PM suspend, driver probe, etc. */
 		device_lock(&dev->dev);
 	}
@@ -2984,7 +2984,7 @@ static int pci_dev_reset(struct pci_dev *dev, int probe)
 done:
 	if (!probe) {
 		device_unlock(&dev->dev);
-		pci_unblock_user_cfg_access(dev);
+		pci_cfg_access_unlock(dev);
 	}
 
 	return rc;
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 8d63630..167c3dd 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -7639,8 +7639,12 @@ static int ipr_reset_restore_cfg_space(struct ipr_cmnd *ipr_cmd)
  **/
 static int ipr_reset_bist_done(struct ipr_cmnd *ipr_cmd)
 {
+	struct ipr_ioa_cfg *ioa_cfg = ipr_cmd->ioa_cfg;
+
 	ENTER;
-	pci_unblock_user_cfg_access(ipr_cmd->ioa_cfg->pdev);
+	if (ioa_cfg->cfg_locked)
+		pci_cfg_access_unlock(ioa_cfg->pdev);
+	ioa_cfg->cfg_locked = 0;
 	ipr_cmd->job_step = ipr_reset_restore_cfg_space;
 	LEAVE;
 	return IPR_RC_JOB_CONTINUE;
@@ -7661,8 +7665,6 @@ static int ipr_reset_start_bist(struct ipr_cmnd *ipr_cmd)
 	int rc = PCIBIOS_SUCCESSFUL;
 
 	ENTER;
-	pci_block_user_cfg_access(ioa_cfg->pdev);
-
 	if (ioa_cfg->ipr_chip->bist_method == IPR_MMIO)
 		writel(IPR_UPROCI_SIS64_START_BIST,
 		       ioa_cfg->regs.set_uproc_interrupt_reg32);
@@ -7674,7 +7676,9 @@ static int ipr_reset_start_bist(struct ipr_cmnd *ipr_cmd)
 		ipr_reset_start_timer(ipr_cmd, IPR_WAIT_FOR_BIST_TIMEOUT);
 		rc = IPR_RC_JOB_RETURN;
 	} else {
-		pci_unblock_user_cfg_access(ipr_cmd->ioa_cfg->pdev);
+		if (ioa_cfg->cfg_locked)
+			pci_cfg_access_unlock(ipr_cmd->ioa_cfg->pdev);
+		ioa_cfg->cfg_locked = 0;
 		ipr_cmd->s.ioasa.hdr.ioasc = cpu_to_be32(IPR_IOASC_PCI_ACCESS_ERROR);
 		rc = IPR_RC_JOB_CONTINUE;
 	}
@@ -7717,7 +7721,6 @@ static int ipr_reset_slot_reset(struct ipr_cmnd *ipr_cmd)
 	struct pci_dev *pdev = ioa_cfg->pdev;
 
 	ENTER;
-	pci_block_user_cfg_access(pdev);
 	pci_set_pcie_reset_state(pdev, pcie_warm_reset);
 	ipr_cmd->job_step = ipr_reset_slot_reset_done;
 	ipr_reset_start_timer(ipr_cmd, IPR_PCI_RESET_TIMEOUT);
@@ -7726,6 +7729,55 @@ static int ipr_reset_slot_reset(struct ipr_cmnd *ipr_cmd)
 }
 
 /**
+ * ipr_reset_block_config_access_wait - Wait for permission to block config access
+ * @ipr_cmd:	ipr command struct
+ *
+ * Description: This attempts to block config access to the IOA.
+ *
+ * Return value:
+ * 	IPR_RC_JOB_CONTINUE / IPR_RC_JOB_RETURN
+ **/
+static int ipr_reset_block_config_access_wait(struct ipr_cmnd *ipr_cmd)
+{
+	struct ipr_ioa_cfg *ioa_cfg = ipr_cmd->ioa_cfg;
+	int rc = IPR_RC_JOB_CONTINUE;
+
+	if (pci_cfg_access_trylock(ioa_cfg->pdev)) {
+		ioa_cfg->cfg_locked = 1;
+		ipr_cmd->job_step = ioa_cfg->reset;
+	} else {
+		if (ipr_cmd->u.time_left) {
+			rc = IPR_RC_JOB_RETURN;
+			ipr_cmd->u.time_left -= IPR_CHECK_FOR_RESET_TIMEOUT;
+			ipr_reset_start_timer(ipr_cmd, IPR_CHECK_FOR_RESET_TIMEOUT);
+		} else {
+			ipr_cmd->job_step = ioa_cfg->reset;
+			dev_err(&ioa_cfg->pdev->dev,
+				"Timed out waiting to lock config access. Resetting anyway.\n");
+		}
+	}
+
+	return rc;
+}
+
+/**
+ * ipr_reset_block_config_access - Block config access to the IOA
+ * @ipr_cmd:	ipr command struct
+ *
+ * Description: This attempts to block config access to the IOA
+ *
+ * Return value:
+ * 	IPR_RC_JOB_CONTINUE
+ **/
+static int ipr_reset_block_config_access(struct ipr_cmnd *ipr_cmd)
+{
+	ipr_cmd->ioa_cfg->cfg_locked = 0;
+	ipr_cmd->job_step = ipr_reset_block_config_access_wait;
+	ipr_cmd->u.time_left = IPR_WAIT_FOR_RESET_TIMEOUT;
+	return IPR_RC_JOB_CONTINUE;
+}
+
+/**
  * ipr_reset_allowed - Query whether or not IOA can be reset
  * @ioa_cfg:	ioa config struct
  *
@@ -7764,7 +7816,7 @@ static int ipr_reset_wait_to_start_bist(struct ipr_cmnd *ipr_cmd)
 		ipr_cmd->u.time_left -= IPR_CHECK_FOR_RESET_TIMEOUT;
 		ipr_reset_start_timer(ipr_cmd, IPR_CHECK_FOR_RESET_TIMEOUT);
 	} else {
-		ipr_cmd->job_step = ioa_cfg->reset;
+		ipr_cmd->job_step = ipr_reset_block_config_access;
 		rc = IPR_RC_JOB_CONTINUE;
 	}
 
@@ -7797,7 +7849,7 @@ static int ipr_reset_alert(struct ipr_cmnd *ipr_cmd)
 		writel(IPR_UPROCI_RESET_ALERT, ioa_cfg->regs.set_uproc_interrupt_reg32);
 		ipr_cmd->job_step = ipr_reset_wait_to_start_bist;
 	} else {
-		ipr_cmd->job_step = ioa_cfg->reset;
+		ipr_cmd->job_step = ipr_reset_block_config_access;
 	}
 
 	ipr_cmd->u.time_left = IPR_WAIT_FOR_RESET_TIMEOUT;
diff --git a/drivers/scsi/ipr.h b/drivers/scsi/ipr.h
index f93f863..dbb40c0 100644
--- a/drivers/scsi/ipr.h
+++ b/drivers/scsi/ipr.h
@@ -1384,6 +1384,7 @@ struct ipr_ioa_cfg {
 	u8 needs_warm_reset:1;
 	u8 msi_received:1;
 	u8 sis64:1;
+	u8 cfg_locked:1;
 
 	u8 revid;
 
diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index fc22e1e..67535f9 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -58,7 +58,8 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
 
 	spin_lock_irq(&gdev->lock);
-	pci_block_user_cfg_access(pdev);
+	if (!pci_cfg_access_trylock(pdev))
+		goto error;
 
 	/* Read both command and status registers in a single 32-bit operation.
 	 * Note: we could cache the value for command and move the status read
@@ -82,7 +83,8 @@ static irqreturn_t irqhandler(int irq, struct uio_info *info)
 	ret = IRQ_HANDLED;
 done:
 
-	pci_unblock_user_cfg_access(pdev);
+	pci_cfg_access_lock(pdev);
+error:
 	spin_unlock_irq(&gdev->lock);
 	return ret;
 }
@@ -95,7 +97,7 @@ static int __devinit verify_pci_2_3(struct pci_dev *pdev)
 	u16 orig, new;
 	int err = 0;
 
-	pci_block_user_cfg_access(pdev);
+	pci_cfg_access_lock(pdev);
 	pci_read_config_word(pdev, PCI_COMMAND, &orig);
 	pci_write_config_word(pdev, PCI_COMMAND,
 			      orig ^ PCI_COMMAND_INTX_DISABLE);
@@ -118,7 +120,7 @@ static int __devinit verify_pci_2_3(struct pci_dev *pdev)
 	/* Now restore the original value. */
 	pci_write_config_word(pdev, PCI_COMMAND, orig);
 err:
-	pci_unblock_user_cfg_access(pdev);
+	pci_cfg_access_unlock(pdev);
 	return err;
 }
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8c230cb..0e84de6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -305,7 +305,7 @@ struct pci_dev {
 	unsigned int	is_added:1;
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	unsigned int	no_msi:1;	/* device may not use msi */
-	unsigned int	block_ucfg_access:1;	/* userspace config space access is blocked */
+	unsigned int	block_cfg_access:1;	/* config space access is blocked */
 	unsigned int	broken_parity_status:1;	/* Device generates false positive parity */
 	unsigned int	irq_reroute_variant:2;	/* device needs IRQ rerouting variant */
 	unsigned int 	msi_enabled:1;
@@ -1079,8 +1079,9 @@ int  ht_create_irq(struct pci_dev *dev, int idx);
 void ht_destroy_irq(unsigned int irq);
 #endif /* CONFIG_HT_IRQ */
 
-extern void pci_block_user_cfg_access(struct pci_dev *dev);
-extern void pci_unblock_user_cfg_access(struct pci_dev *dev);
+extern void pci_cfg_access_lock(struct pci_dev *dev);
+extern bool pci_cfg_access_trylock(struct pci_dev *dev);
+extern void pci_cfg_access_unlock(struct pci_dev *dev);
 
 /*
  * PCI domain support.  Sometimes called PCI segment (eg by ACPI),
@@ -1277,10 +1278,13 @@ static inline void pci_release_regions(struct pci_dev *dev)
 
 #define pci_dma_burst_advice(pdev, strat, strategy_parameter) do { } while (0)
 
-static inline void pci_block_user_cfg_access(struct pci_dev *dev)
+static inline void pci_block_cfg_access(struct pci_dev *dev)
 { }
 
-static inline void pci_unblock_user_cfg_access(struct pci_dev *dev)
+static inline int pci_block_cfg_access_in_atomic(struct pci_dev *dev)
+{ return 0; }
+
+static inline void pci_unblock_cfg_access(struct pci_dev *dev)
 { }
 
 static inline struct pci_bus *pci_find_next_bus(const struct pci_bus *from)
-- 
1.7.3.4

[PATCH 2/3] pci: Introduce INTx check & mask API

[Jan Kiszka]

These new PCI services allow to probe for 2.3-compliant INTx masking
support and then use the feature from PCI interrupt handlers. The
services are properly synchronized with concurrent config space access
via sysfs or on device reset.

This enables generic PCI device drivers like uio_pci_generic or KVM's
device assignment to implement the necessary kernel-side IRQ handling
without any knowledge about device-specific interrupt status and control
registers.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/pci/access.c |    2 +-
 drivers/pci/pci.c    |  110 ++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h    |    2 +
 include/linux/pci.h  |    3 +
 4 files changed, 116 insertions(+), 1 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 0c4c717..2a58164 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -13,7 +13,7 @@
  * configuration space.
  */
 
-static DEFINE_RAW_SPINLOCK(pci_lock);
+DEFINE_RAW_SPINLOCK(pci_lock);
 
 /*
  *  Wrappers for all PCI configuration access functions.  They just check
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 208fe21..2bff7c9 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2762,6 +2762,116 @@ pci_intx(struct pci_dev *pdev, int enable)
 }
 
 /**
+ * pci_intx_mask_supported - probe for INTx masking support
+ * @pdev: the PCI device to operate on
+ *
+ * Check if the device dev support INTx masking via the config space
+ * command word.
+ */
+bool pci_intx_mask_supported(struct pci_dev *dev)
+{
+	bool mask_supported = false;
+	u16 orig, new;
+
+	pci_cfg_access_lock(dev);
+
+	pci_read_config_word(dev, PCI_COMMAND, &orig);
+	pci_write_config_word(dev, PCI_COMMAND,
+			      orig ^ PCI_COMMAND_INTX_DISABLE);
+	pci_read_config_word(dev, PCI_COMMAND, &new);
+
+	/*
+	 * There's no way to protect against hardware bugs or detect them
+	 * reliably, but as long as we know what the value should be, let's
+	 * go ahead and check it.
+	 */
+	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
+		dev_err(&dev->dev, "Command register changed from "
+			"0x%x to 0x%x: driver or hardware bug?\n", orig, new);
+	} else if ((new ^ orig) & PCI_COMMAND_INTX_DISABLE) {
+		mask_supported = true;
+		pci_write_config_word(dev, PCI_COMMAND, orig);
+	}
+
+	pci_cfg_access_unlock(dev);
+	return mask_supported;
+}
+EXPORT_SYMBOL_GPL(pci_intx_mask_supported);
+
+static bool pci_check_and_set_intx_mask(struct pci_dev *dev, bool mask)
+{
+	struct pci_bus *bus = dev->bus;
+	bool mask_updated = true;
+	u32 cmd_status_dword;
+	u16 origcmd, newcmd;
+	unsigned long flags;
+	bool irq_pending;
+
+	/*
+	 * We do a single dword read to retrieve both command and status.
+	 * Document assumptions that make this possible.
+	 */
+	BUILD_BUG_ON(PCI_COMMAND % 4);
+	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
+
+	raw_spin_lock_irqsave(&pci_lock, flags);
+
+	bus->ops->read(bus, dev->devfn, PCI_COMMAND, 4, &cmd_status_dword);
+
+	irq_pending = (cmd_status_dword >> 16) & PCI_STATUS_INTERRUPT;
+
+	/*
+	 * Check interrupt status register to see whether our device
+	 * triggered the interrupt (when masking) or the next IRQ is
+	 * already pending (when unmasking).
+	 */
+	if (mask != irq_pending) {
+		mask_updated = false;
+		goto done;
+	}
+
+	origcmd = cmd_status_dword;
+	newcmd = origcmd & ~PCI_COMMAND_INTX_DISABLE;
+	if (mask)
+		newcmd |= PCI_COMMAND_INTX_DISABLE;
+	if (newcmd != origcmd)
+		bus->ops->write(bus, dev->devfn, PCI_COMMAND, 2, newcmd);
+
+done:
+	raw_spin_unlock_irqrestore(&pci_lock, flags);
+
+	return mask_updated;
+}
+
+/**
+ * pci_check_and_mask_intx - mask INTx on pending interrupt
+ * @pdev: the PCI device to operate on
+ *
+ * Check if the device dev has its INTx line asserted, mask it and
+ * return true in that case. False is returned if not interrupt was
+ * pending.
+ */
+bool pci_check_and_mask_intx(struct pci_dev *dev)
+{
+	return pci_check_and_set_intx_mask(dev, true);
+}
+EXPORT_SYMBOL_GPL(pci_check_and_mask_intx);
+
+/**
+ * pci_check_and_mask_intx - unmask INTx of no interrupt is pending
+ * @pdev: the PCI device to operate on
+ *
+ * Check if the device dev has its INTx line asserted, unmask it if not
+ * and return true. False is returned and the mask remains active if
+ * there was still an interrupt pending.
+ */
+bool pci_check_and_unmask_intx(struct pci_dev *dev)
+{
+	return pci_check_and_set_intx_mask(dev, false);
+}
+EXPORT_SYMBOL_GPL(pci_check_and_unmask_intx);
+
+/**
  * pci_msi_off - disables any msi or msix capabilities
  * @dev: the PCI device to operate on
  *
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index b74084e..3b6e4ed 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -136,6 +136,8 @@ static inline void pci_remove_legacy_files(struct pci_bus *bus) { return; }
 /* Lock for read/write access to pci device and bus lists */
 extern struct rw_semaphore pci_bus_sem;
 
+extern raw_spinlock_t pci_lock;
+
 extern unsigned int pci_pm_d3_delay;
 
 #ifdef CONFIG_PCI_MSI
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0e84de6..5b294ac 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -799,6 +799,9 @@ int __must_check pci_set_mwi(struct pci_dev *dev);
 int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
+bool pci_intx_mask_supported(struct pci_dev *dev);
+bool pci_check_and_mask_intx(struct pci_dev *dev);
+bool pci_check_and_unmask_intx(struct pci_dev *dev);
 void pci_msi_off(struct pci_dev *dev);
 int pci_set_dma_max_seg_size(struct pci_dev *dev, unsigned int size);
 int pci_set_dma_seg_boundary(struct pci_dev *dev, unsigned long mask);
-- 
1.7.3.4

[PATCH 3/3] uio: Convert uio_generic_pci to new intx masking API

[Jan Kiszka]

The new PCI API provides both generic probing for 2.3 masking support
and check&mask in the interrupt handler.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/uio/uio_pci_generic.c |   84 +++-------------------------------------
 1 files changed, 7 insertions(+), 77 deletions(-)

diff --git a/drivers/uio/uio_pci_generic.c b/drivers/uio/uio_pci_generic.c
index 67535f9..33f7dfb 100644
--- a/drivers/uio/uio_pci_generic.c
+++ b/drivers/uio/uio_pci_generic.c
@@ -33,7 +33,6 @@
 struct uio_pci_generic_dev {
 	struct uio_info info;
 	struct pci_dev *pdev;
-	spinlock_t lock; /* guards command register accesses */
 };
 
 static inline struct uio_pci_generic_dev *
@@ -47,81 +46,12 @@ to_uio_pci_generic_dev(struct uio_info *info)
 static irqreturn_t irqhandler(int irq, struct uio_info *info)
 {
 	struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info);
-	struct pci_dev *pdev = gdev->pdev;
-	irqreturn_t ret = IRQ_NONE;
-	u32 cmd_status_dword;
-	u16 origcmd, newcmd, status;
-
-	/* We do a single dword read to retrieve both command and status.
-	 * Document assumptions that make this possible. */
-	BUILD_BUG_ON(PCI_COMMAND % 4);
-	BUILD_BUG_ON(PCI_COMMAND + 2 != PCI_STATUS);
-
-	spin_lock_irq(&gdev->lock);
-	if (!pci_cfg_access_trylock(pdev))
-		goto error;
-
-	/* Read both command and status registers in a single 32-bit operation.
-	 * Note: we could cache the value for command and move the status read
-	 * out of the lock if there was a way to get notified of user changes
-	 * to command register through sysfs. Should be good for shared irqs. */
-	pci_read_config_dword(pdev, PCI_COMMAND, &cmd_status_dword);
-	origcmd = cmd_status_dword;
-	status = cmd_status_dword >> 16;
-
-	/* Check interrupt status register to see whether our device
-	 * triggered the interrupt. */
-	if (!(status & PCI_STATUS_INTERRUPT))
-		goto done;
-
-	/* We triggered the interrupt, disable it. */
-	newcmd = origcmd | PCI_COMMAND_INTX_DISABLE;
-	if (newcmd != origcmd)
-		pci_write_config_word(pdev, PCI_COMMAND, newcmd);
 
-	/* UIO core will signal the user process. */
-	ret = IRQ_HANDLED;
-done:
-
-	pci_cfg_access_lock(pdev);
-error:
-	spin_unlock_irq(&gdev->lock);
-	return ret;
-}
+	if (!pci_check_and_mask_intx(gdev->pdev))
+		return IRQ_NONE;
 
-/* Verify that the device supports Interrupt Disable bit in command register,
- * per PCI 2.3, by flipping this bit and reading it back: this bit was readonly
- * in PCI 2.2. */
-static int __devinit verify_pci_2_3(struct pci_dev *pdev)
-{
-	u16 orig, new;
-	int err = 0;
-
-	pci_cfg_access_lock(pdev);
-	pci_read_config_word(pdev, PCI_COMMAND, &orig);
-	pci_write_config_word(pdev, PCI_COMMAND,
-			      orig ^ PCI_COMMAND_INTX_DISABLE);
-	pci_read_config_word(pdev, PCI_COMMAND, &new);
-	/* There's no way to protect against
-	 * hardware bugs or detect them reliably, but as long as we know
-	 * what the value should be, let's go ahead and check it. */
-	if ((new ^ orig) & ~PCI_COMMAND_INTX_DISABLE) {
-		err = -EBUSY;
-		dev_err(&pdev->dev, "Command changed from 0x%x to 0x%x: "
-			"driver or HW bug?\n", orig, new);
-		goto err;
-	}
-	if (!((new ^ orig) & PCI_COMMAND_INTX_DISABLE)) {
-		dev_warn(&pdev->dev, "Device does not support "
-			 "disabling interrupts: unable to bind.\n");
-		err = -ENODEV;
-		goto err;
-	}
-	/* Now restore the original value. */
-	pci_write_config_word(pdev, PCI_COMMAND, orig);
-err:
-	pci_cfg_access_unlock(pdev);
-	return err;
+	/* UIO core will signal the user process. */
+	return IRQ_HANDLED;
 }
 
 static int __devinit probe(struct pci_dev *pdev,
@@ -144,9 +74,10 @@ static int __devinit probe(struct pci_dev *pdev,
 		return -ENODEV;
 	}
 
-	err = verify_pci_2_3(pdev);
-	if (err)
+	if (!pci_intx_mask_supported(pdev)) {
+		err = -ENODEV;
 		goto err_verify;
+	}
 
 	gdev = kzalloc(sizeof(struct uio_pci_generic_dev), GFP_KERNEL);
 	if (!gdev) {
@@ -160,7 +91,6 @@ static int __devinit probe(struct pci_dev *pdev,
 	gdev->info.irq_flags = IRQF_SHARED;
 	gdev->info.handler = irqhandler;
 	gdev->pdev = pdev;
-	spin_lock_init(&gdev->lock);
 
 	if (uio_register_device(&pdev->dev, &gdev->info))
 		goto err_register;
-- 
1.7.3.4

Re: [PATCH 0/3] PCI: Rework config space locking, add INTx masking services

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

On Mon, 12 Sep 2011 18:54:01 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> This series tries to heal the currently broken locking scheme around PCI
> config space accesses.
> 
> We have an interface lock out access via sysfs, but that service wrongly
> assumes it is only called by one instance at a time for some device. So
> two loops doing
> 
> echo 1 > /sys/bus/pci/devices/<some-device>/reset
> 
> in parallel will trigger a kernel BUG at the moment.
> 
> Besides synchronizing with user space, we also need to manage config
> space access of generic PCI drivers. They need to mask legacy interrupt
> lines while the specific driver runs in user space or a guest OS.
> 
> The approach taken here is provide mutex-like locking for general
> access - which still requires a special mechanism due to requirements of
> the IBM Power RAID SCSI driver. Furthermore, INTx masking is now
> available via the PCI core and synchronized via the internal pci_lock.
> 
> Not sure who may want to take this, so I'm CC'ing broadly.

ISTR a bunch of discussion about this (just back from lots of work
travel and vacation, sorry I missed most of it).

Is this the agreed upon way of handling it?  If so, can I get some
Reviewed/Acked-bys from people?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/3] PCI: Rework config space locking, add INTx masking services

[Jan Kiszka]

On 2011-10-06 17:48, Jesse Barnes wrote:
> On Mon, 12 Sep 2011 18:54:01 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> This series tries to heal the currently broken locking scheme around PCI
>> config space accesses.
>>
>> We have an interface lock out access via sysfs, but that service wrongly
>> assumes it is only called by one instance at a time for some device. So
>> two loops doing
>>
>> echo 1 > /sys/bus/pci/devices/<some-device>/reset
>>
>> in parallel will trigger a kernel BUG at the moment.
>>
>> Besides synchronizing with user space, we also need to manage config
>> space access of generic PCI drivers. They need to mask legacy interrupt
>> lines while the specific driver runs in user space or a guest OS.
>>
>> The approach taken here is provide mutex-like locking for general
>> access - which still requires a special mechanism due to requirements of
>> the IBM Power RAID SCSI driver. Furthermore, INTx masking is now
>> available via the PCI core and synchronized via the internal pci_lock.
>>
>> Not sure who may want to take this, so I'm CC'ing broadly.
> 
> ISTR a bunch of discussion about this (just back from lots of work
> travel and vacation, sorry I missed most of it).
> 
> Is this the agreed upon way of handling it?  If so, can I get some
> Reviewed/Acked-bys from people?

I hope this is acceptable. These changes are required for further
improvements of the KVM device assignment support (INTx sharing). So I
would appreciate any ack or whatever feedback as well.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 0/3] PCI: Rework config space locking, add INTx masking services

[Brian King]

On 10/06/2011 10:48 AM, Jesse Barnes wrote:
> On Mon, 12 Sep 2011 18:54:01 +0200
> Jan Kiszka <jan.kiszka@siemens.com> wrote:
> 
>> This series tries to heal the currently broken locking scheme around PCI
>> config space accesses.
>>
>> We have an interface lock out access via sysfs, but that service wrongly
>> assumes it is only called by one instance at a time for some device. So
>> two loops doing
>>
>> echo 1 > /sys/bus/pci/devices/<some-device>/reset
>>
>> in parallel will trigger a kernel BUG at the moment.
>>
>> Besides synchronizing with user space, we also need to manage config
>> space access of generic PCI drivers. They need to mask legacy interrupt
>> lines while the specific driver runs in user space or a guest OS.
>>
>> The approach taken here is provide mutex-like locking for general
>> access - which still requires a special mechanism due to requirements of
>> the IBM Power RAID SCSI driver. Furthermore, INTx masking is now
>> available via the PCI core and synchronized via the internal pci_lock.
>>
>> Not sure who may want to take this, so I'm CC'ing broadly.
> 
> ISTR a bunch of discussion about this (just back from lots of work
> travel and vacation, sorry I missed most of it).
> 
> Is this the agreed upon way of handling it?  If so, can I get some
> Reviewed/Acked-bys from people?

Acked-by: Brian King <brking@linux.vnet.ibm.com>

-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center

Re: [PATCH 0/3] PCI: Rework config space locking, add INTx masking services

[Michael S. Tsirkin]

On Mon, Sep 12, 2011 at 06:54:01PM +0200, Jan Kiszka wrote:
> This series tries to heal the currently broken locking scheme around PCI
> config space accesses.
> 
> We have an interface lock out access via sysfs, but that service wrongly
> assumes it is only called by one instance at a time for some device. So
> two loops doing
> 
> echo 1 > /sys/bus/pci/devices/<some-device>/reset
> 
> in parallel will trigger a kernel BUG at the moment.
> 
> Besides synchronizing with user space, we also need to manage config
> space access of generic PCI drivers. They need to mask legacy interrupt
> lines while the specific driver runs in user space or a guest OS.
> 
> The approach taken here is provide mutex-like locking for general
> access - which still requires a special mechanism due to requirements of
> the IBM Power RAID SCSI driver. Furthermore, INTx masking is now
> available via the PCI core and synchronized via the internal pci_lock.
> 
> Not sure who may want to take this, so I'm CC'ing broadly.
> 
> CC: Brian King <brking@linux.vnet.ibm.com>
> 
> Jan Kiszka (3):
>   pci: Rework config space blocking services
>   pci: Introduce INTx check & mask API
>   uio: Convert uio_generic_pci to new intx masking API

For the series:

Acked-by: Michael S. Tsirkin <mst@redhat.com>


>  drivers/pci/access.c          |   76 +++++++++++++++++----------
>  drivers/pci/iov.c             |   12 ++--
>  drivers/pci/pci.c             |  114 ++++++++++++++++++++++++++++++++++++++++-
>  drivers/pci/pci.h             |    2 +
>  drivers/scsi/ipr.c            |   66 +++++++++++++++++++++---
>  drivers/scsi/ipr.h            |    1 +
>  drivers/uio/uio_pci_generic.c |   82 +++---------------------------
>  include/linux/pci.h           |   17 ++++--
>  8 files changed, 248 insertions(+), 122 deletions(-)
> 
> -- 
> 1.7.3.4

Re: [PATCH 0/3] PCI: Rework config space locking, add INTx masking services

[Jan Kiszka]

On 2011-10-14 16:48, Jan Kiszka wrote:
> On 2011-10-06 17:48, Jesse Barnes wrote:
>> On Mon, 12 Sep 2011 18:54:01 +0200
>> Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>>> This series tries to heal the currently broken locking scheme around PCI
>>> config space accesses.
>>>
>>> We have an interface lock out access via sysfs, but that service wrongly
>>> assumes it is only called by one instance at a time for some device. So
>>> two loops doing
>>>
>>> echo 1 > /sys/bus/pci/devices/<some-device>/reset
>>>
>>> in parallel will trigger a kernel BUG at the moment.
>>>
>>> Besides synchronizing with user space, we also need to manage config
>>> space access of generic PCI drivers. They need to mask legacy interrupt
>>> lines while the specific driver runs in user space or a guest OS.
>>>
>>> The approach taken here is provide mutex-like locking for general
>>> access - which still requires a special mechanism due to requirements of
>>> the IBM Power RAID SCSI driver. Furthermore, INTx masking is now
>>> available via the PCI core and synchronized via the internal pci_lock.
>>>
>>> Not sure who may want to take this, so I'm CC'ing broadly.
>>
>> ISTR a bunch of discussion about this (just back from lots of work
>> travel and vacation, sorry I missed most of it).
>>
>> Is this the agreed upon way of handling it?  If so, can I get some
>> Reviewed/Acked-bys from people?
> 
> I hope this is acceptable. These changes are required for further
> improvements of the KVM device assignment support (INTx sharing). So I
> would appreciate any ack or whatever feedback as well.

At least two acks arrived by now. But I guess this series is not yet
lined up for the 3.2 window, is it? Can we get it merged for 3.3? Just
let me know if any rebasing is required and against which tree.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 1/3] pci: Rework config space blocking services

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

On Mon, 12 Sep 2011 18:54:02 +0200
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> pci_block_user_cfg_access was designed for the use case that a single
> context, the IPR driver, temporarily delays user space accesses to the
> config space via sysfs. This assumption became invalid by the time
> pci_dev_reset was added as locking instance. Today, if you run two loops
> in parallel that reset the same device via sysfs, you end up with a
> kernel BUG as pci_block_user_cfg_access detect the broken assumption.
> 
> This reworks the pci_block_user_cfg_access to a sleeping service
> pci_cfg_access_lock and an atomic-compatible variant called
> pci_cfg_access_trylock. The former not only blocks user space access as
> before but also waits if access was already locked. The latter service
> just returns false in this case, allowing the caller to resolve the
> conflict instead of raising a BUG.
> 
> Adaptions of the ipr driver were originally written by Brian King.

Sorry to ask you to refresh these after so long, Jan, but can you
respin against my linux-next branch?

My inbox is a poor substitute for patchwork, but that's no excuse for
leaving you hanging so long.  Sorry!

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]