[PATCH] Block on access to temporarily unavailable pci device

[Matthew Wilcox]

The existing implementation of pci_block_user_cfg_access() was recently
criticised for providing out of date information and for returning errors
on write, which applications won't be expecting.

This reimplementation uses an rw semaphore to block accesses.  I examined
a couple of other alternatives (a mutex, which would unnecessarily
serialise kernel BIST users; a per-device mutex, which would take another
16 bytes per pci device; a custom queue), I felt the rwsem provided the
best tradeoffs.

I'll post the patch I used to test blocking device accesses separately.

Signed-off-by: Matthew Wilcox <matthew@xxxxxx>

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ea16805..29581a2 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -1,6 +1,7 @@
 #include <linux/pci.h>
 #include <linux/module.h>
 #include <linux/ioport.h>
+#include <linux/rwsem.h>
 
 #include "pci.h"
 
@@ -12,6 +13,18 @@ #include "pci.h"
 static DEFINE_SPINLOCK(pci_lock);
 
 /*
+ * Prevent the user from accessing PCI config space when it's unsafe to do
+ * so.  Some devices require this during BIST and we're required to prevent
+ * it during D-state transitions.  It could be made per-device, but it doesn't
+ * seem worthwhile for such a rare occurrence.
+ *
+ * User accesses are the 'writer' as only one is allowed at a time.  Kernel
+ * blocking the user is the 'reader' as multiple devices can be blocked at
+ * the same time.
+ */
+static DECLARE_RWSEM(pci_user_sem);
+
+/*
  *  Wrappers for all PCI configuration access functions.  They just check
  *  alignment, do locking and call the low-level functions pointed to
  *  by pci_dev->ops.
@@ -63,15 +76,6 @@ EXPORT_SYMBOL(pci_bus_write_config_byte)
 EXPORT_SYMBOL(pci_bus_write_config_word);
 EXPORT_SYMBOL(pci_bus_write_config_dword);
 
-static u32 pci_user_cached_config(struct pci_dev *dev, int pos)
-{
-	u32 data;
-
-	data = dev->saved_config_space[pos/sizeof(dev->saved_config_space[0])];
-	data >>= (pos % sizeof(dev->saved_config_space[0])) * 8;
-	return data;
-}
-
 #define PCI_USER_READ_CONFIG(size,type)					\
 int pci_user_read_config_##size						\
 	(struct pci_dev *dev, int pos, type *val)			\
@@ -80,13 +84,12 @@ int pci_user_read_config_##size						\
 	int ret = 0;							\
 	u32 data = -1;							\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
+	down_write(&pci_user_sem);					\
 	spin_lock_irqsave(&pci_lock, flags);				\
-	if (likely(!dev->block_ucfg_access))				\
-		ret = dev->bus->ops->read(dev->bus, dev->devfn,		\
+	ret = dev->bus->ops->read(dev->bus, dev->devfn,			\
 					pos, sizeof(type), &data);	\
-	else if (pos < sizeof(dev->saved_config_space))			\
-		data = pci_user_cached_config(dev, pos); 		\
 	spin_unlock_irqrestore(&pci_lock, flags);			\
+	up_write(&pci_user_sem);					\
 	*val = (type)data;						\
 	return ret;							\
 }
@@ -98,11 +101,12 @@ int pci_user_write_config_##size					\
 	unsigned long flags;						\
 	int ret = -EIO;							\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
+	down_write(&pci_user_sem);					\
 	spin_lock_irqsave(&pci_lock, flags);				\
-	if (likely(!dev->block_ucfg_access))				\
-		ret = dev->bus->ops->write(dev->bus, dev->devfn,	\
+	ret = dev->bus->ops->write(dev->bus, dev->devfn,		\
 					pos, sizeof(type), val);	\
 	spin_unlock_irqrestore(&pci_lock, flags);			\
+	up_write(&pci_user_sem);					\
 	return ret;							\
 }
 
@@ -117,21 +121,12 @@ PCI_USER_WRITE_CONFIG(dword, u32)
  * pci_block_user_cfg_access - Block userspace PCI config reads/writes
  * @dev:	pci device struct
  *
- * This function blocks any userspace PCI config accesses from occurring.
- * When blocked, any writes will be bit bucketed and reads will return the
- * data saved using pci_save_state for the first 64 bytes of config
- * space and return 0xff for all other config reads.
- **/
+ * When user access is blocked, any reads or writes to config space will
+ * sleep until access is unblocked again.
+ */
 void pci_block_user_cfg_access(struct pci_dev *dev)
 {
-	unsigned long flags;
-
-	pci_save_state(dev);
-
-	/* spinlock to synchronize with anyone reading config space now */
-	spin_lock_irqsave(&pci_lock, flags);
-	dev->block_ucfg_access = 1;
-	spin_unlock_irqrestore(&pci_lock, flags);
+	down_read(&pci_user_sem);
 }
 EXPORT_SYMBOL_GPL(pci_block_user_cfg_access);
 
@@ -140,14 +135,9 @@ EXPORT_SYMBOL_GPL(pci_block_user_cfg_acc
  * @dev:	pci device struct
  *
  * This function allows userspace PCI config accesses to resume.
- **/
+ */
 void pci_unblock_user_cfg_access(struct pci_dev *dev)
 {
-	unsigned long flags;
-
-	/* spinlock to synchronize with anyone reading saved config space */
-	spin_lock_irqsave(&pci_lock, flags);
-	dev->block_ucfg_access = 0;
-	spin_unlock_irqrestore(&pci_lock, flags);
+	up_read(&pci_user_sem);
 }
 EXPORT_SYMBOL_GPL(pci_unblock_user_cfg_access);
diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 2dde821..5d06837 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -6174,6 +6174,7 @@ static int ipr_reset_start_bist(struct i
 	int rc;
 
 	ENTER;
+	pci_save_state(ioa_cfg->pdev);
 	pci_block_user_cfg_access(ioa_cfg->pdev);
 	rc = pci_write_config_byte(ioa_cfg->pdev, PCI_BIST, PCI_BIST_START);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3632282..4dbcbbd 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -174,7 +174,6 @@ struct pci_dev {
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	unsigned int	no_msi:1;	/* device may not use msi */
 	unsigned int	no_d1d2:1;   /* only allow d0 or d3 */
-	unsigned int	block_ucfg_access:1;	/* userspace config space access is blocked */
 	unsigned int	broken_parity_status:1;	/* Device generates false positive parity */
 	unsigned int 	msi_enabled:1;
 	unsigned int	msix_enabled:1;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/