Re: [PATCH] Block on access to temporarily unavailable pci device

[Brian King <brking@us.ibm.com>]

Matthew Wilcox wrote:
> 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.

Nack. This changes pci_block_user_cfg_access such that it can now sleep,
which does not work for the ipr driver, which uses it to block during BIST.
The ipr driver needs to be able to call this function at interrupt level
when it receives an interrupt that its scsi adapter has received a fatal
error and requires BIST to recover. The only way I see for ipr to be able
to use the changed interface would require I create a kernel thread in
the ipr driver for this specific purpose.

Brian

> 
> I'll post the patch I used to test blocking device accesses separately.
> 
> Signed-off-by: Matthew Wilcox <matthew@wil.cx>
> 
> 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;


-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/