Re: [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 a global wait queue and a bit per device.
I've open-coded prepare_to_wait() / finish_wait() as I could optimise
it significantly by knowing that the pci_lock protected us at all points.

It looked a bit funny to be doing a spin_unlock_irqsave(); schedule(),
so I used spin_lock_irq() for the _user versions of pci_read_config and
pci_write_config.  Not carrying a flags pointer around made the code
much less nasty.

I also addressed the potential issue with nested attempts to block.
Now pci_block_user_cfg_access() can return -EBUSY if it's already blocked,
and pci_unblock_user_cfg_access() will WARN if you try to unblock an
already unblocked device.

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

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ea16805..a27a6c0 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/wait.h>
 
 #include "pci.h"
 
@@ -63,30 +64,42 @@ 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;
+/*
+ * The following routines are to 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.
+ *
+ * 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);
 
-	data = dev->saved_config_space[pos/sizeof(dev->saved_config_space[0])];
-	data >>= (pos % sizeof(dev->saved_config_space[0])) * 8;
-	return data;
+static noinline void pci_wait_ucfg(struct pci_dev *dev)
+{
+	DECLARE_WAITQUEUE(wait, current);
+
+	__add_wait_queue(&pci_ucfg_wait, &wait);
+	do {
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		spin_unlock_irq(&pci_lock);
+		schedule();
+		spin_lock_irq(&pci_lock);
+	} while (dev->block_ucfg_access);
+	__remove_wait_queue(&pci_ucfg_wait, &wait);
 }
 
 #define PCI_USER_READ_CONFIG(size,type)					\
 int pci_user_read_config_##size						\
 	(struct pci_dev *dev, int pos, type *val)			\
 {									\
-	unsigned long flags;						\
 	int ret = 0;							\
 	u32 data = -1;							\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
-	spin_lock_irqsave(&pci_lock, flags);				\
-	if (likely(!dev->block_ucfg_access))				\
-		ret = dev->bus->ops->read(dev->bus, dev->devfn,		\
+	spin_lock_irq(&pci_lock);					\
+	if (unlikely(dev->block_ucfg_access)) pci_wait_ucfg(dev);	\
+	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);			\
+	spin_unlock_irq(&pci_lock);					\
 	*val = (type)data;						\
 	return ret;							\
 }
@@ -95,14 +108,13 @@ #define PCI_USER_WRITE_CONFIG(size,type)
 int pci_user_write_config_##size					\
 	(struct pci_dev *dev, int pos, type val)			\
 {									\
-	unsigned long flags;						\
 	int ret = -EIO;							\
 	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
-	spin_lock_irqsave(&pci_lock, flags);				\
-	if (likely(!dev->block_ucfg_access))				\
-		ret = dev->bus->ops->write(dev->bus, dev->devfn,	\
+	spin_lock_irq(&pci_lock);					\
+	if (unlikely(dev->block_ucfg_access)) pci_wait_ucfg(dev);	\
+	ret = dev->bus->ops->write(dev->bus, dev->devfn,		\
 					pos, sizeof(type), val);	\
-	spin_unlock_irqrestore(&pci_lock, flags);			\
+	spin_unlock_irq(&pci_lock);					\
 	return ret;							\
 }
 
@@ -117,21 +129,21 @@ 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.
- **/
-void pci_block_user_cfg_access(struct pci_dev *dev)
+ * 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.
+ */
+int pci_block_user_cfg_access(struct pci_dev *dev)
 {
 	unsigned long flags;
+	int result;
 
-	pci_save_state(dev);
-
-	/* spinlock to synchronize with anyone reading config space now */
 	spin_lock_irqsave(&pci_lock, flags);
+	result = dev->block_ucfg_access ? -EBUSY : 0;
 	dev->block_ucfg_access = 1;
 	spin_unlock_irqrestore(&pci_lock, flags);
+
+	return result;
 }
 EXPORT_SYMBOL_GPL(pci_block_user_cfg_access);
 
@@ -140,14 +152,19 @@ 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);
+
+	/* A second unblock implies a failure to notice an attempt to nested
+	 * block, which we don't support. */
+	WARN_ON(!dev->block_ucfg_access);
+
 	dev->block_ucfg_access = 0;
+	wake_up_all(&pci_ucfg_wait);
 	spin_unlock_irqrestore(&pci_lock, flags);
 }
 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..8725d1f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -634,7 +634,7 @@ int  ht_create_irq(struct pci_dev *dev, 
 void ht_destroy_irq(unsigned int irq);
 #endif /* CONFIG_HT_IRQ */
 
-extern void pci_block_user_cfg_access(struct pci_dev *dev);
+extern int __must_check pci_block_user_cfg_access(struct pci_dev *dev);
 extern void pci_unblock_user_cfg_access(struct pci_dev *dev);
 
 /*
-
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/