Re: [PATCH 1/2] pci: Block config access during BIST

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

Greg - do you have any comments on this patch?

Thanks

-Brian

Brian King wrote:
> Benjamin Herrenschmidt wrote:
> 
>>> I thought about that when coding this up and thought it would
>>> be better to simply have the function do what it advertises and no
>>> more. Seems strange that a function called pci_block_config_access
>>> would go and do a bunch of pci config accesses, but we can
>>> certainly add it if you like.
>>
>>
>>
>> Well, considering that when blocked, reads return data from the cache,
>> it makes sense to fill the cache when blocking ... or we'll end up
>> returning crap.
> 
> 
> Here is an updated patch with the added pci_save_state call.
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 
> Some PCI adapters (eg. ipr scsi adapters) have an exposure today in that 
> they issue BIST to the adapter to reset the card. If, during the time
> it takes to complete BIST, userspace attempts to access PCI config space, 
> the host bus bridge will master abort the access since the ipr adapter 
> does not respond on the PCI bus for a brief period of time when running BIST. 
> On PPC64 hardware, this master abort results in the host PCI bridge
> isolating that PCI device from the rest of the system, making the device
> unusable until Linux is rebooted. This patch is an attempt to close that
> exposure by introducing some blocking code in the PCI code. When blocked,
> writes will be humored and reads will return the cached value. Ben
> Herrenschmidt has also mentioned that he plans to use this in PPC power
> management.
> 
> Signed-off-by: Brian King <brking@us.ibm.com>
> ---
> 
> 
> 
> ---
> 
>  linux-2.6.10-rc2-bk5-bjking1/drivers/pci/access.c |  104 ++++++++++++++++++++++
>  linux-2.6.10-rc2-bk5-bjking1/include/linux/pci.h  |   37 ++-----
>  2 files changed, 115 insertions(+), 26 deletions(-)
> 
> diff -puN drivers/pci/access.c~pci_block_config_io_during_bist drivers/pci/access.c
> --- linux-2.6.10-rc2-bk5/drivers/pci/access.c~pci_block_config_io_during_bist	2004-11-20 16:07:19.000000000 -0600
> +++ linux-2.6.10-rc2-bk5-bjking1/drivers/pci/access.c	2004-11-21 10:42:10.000000000 -0600
> @@ -60,3 +60,107 @@ EXPORT_SYMBOL(pci_bus_read_config_dword)
>  EXPORT_SYMBOL(pci_bus_write_config_byte);
>  EXPORT_SYMBOL(pci_bus_write_config_word);
>  EXPORT_SYMBOL(pci_bus_write_config_dword);
> +
> +#define PCI_READ_CONFIG(size,type)	\
> +int pci_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_cfg_access))				\
> +		ret = dev->bus->ops->read(dev->bus, dev->devfn, pos, sizeof(type), &data); \
> +	else if (pos < sizeof(dev->saved_config_space))		\
> +		data = dev->saved_config_space[pos/sizeof(dev->saved_config_space[0])]; \
> +	spin_unlock_irqrestore(&pci_lock, flags);		\
> +	*val = (type)data;					\
> +	return ret;							\
> +}
> +
> +#define PCI_WRITE_CONFIG(size,type)	\
> +int pci_write_config_##size	\
> +	(struct pci_dev *dev, int pos, type val)		\
> +{									\
> +	unsigned long flags;					\
> +	int ret = 0;						\
> +	if (PCI_##size##_BAD) return PCIBIOS_BAD_REGISTER_NUMBER;	\
> +	spin_lock_irqsave(&pci_lock, flags);		\
> +	if (likely(!dev->block_cfg_access))					\
> +		ret = dev->bus->ops->write(dev->bus, dev->devfn, pos, sizeof(type), val); \
> +	spin_unlock_irqrestore(&pci_lock, flags);		\
> +	return ret;							\
> +}
> +
> +PCI_READ_CONFIG(byte, u8)
> +PCI_READ_CONFIG(word, u16)
> +PCI_READ_CONFIG(dword, u32)
> +PCI_WRITE_CONFIG(byte, u8)
> +PCI_WRITE_CONFIG(word, u16)
> +PCI_WRITE_CONFIG(dword, u32)
> +
> +/**
> + * pci_block_config_access - Block PCI config reads/writes
> + * @dev:	pci device struct
> + *
> + * This function blocks any PCI config accesses from occurring.
> + * When blocked, any writes will be humored and reads will return
> + * the data saved using pci_save_state for the first 64 bytes
> + * of config space and return ff's for all other config reads.
> + *
> + * Return value:
> + * 	nothing
> + **/
> +void pci_block_config_access(struct pci_dev *dev)
> +{
> +	unsigned long flags;
> +
> +	pci_save_state(dev);
> +	spin_lock_irqsave(&pci_lock, flags);
> +	dev->block_cfg_access = 1;
> +	spin_unlock_irqrestore(&pci_lock, flags);
> +}
> +
> +/**
> + * pci_unblock_config_access - Unblock PCI config reads/writes
> + * @dev:	pci device struct
> + *
> + * This function allows PCI config accesses to resume.
> + *
> + * Return value:
> + * 	nothing
> + **/
> +void pci_unblock_config_access(struct pci_dev *dev)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&pci_lock, flags);
> +	dev->block_cfg_access = 0;
> +	spin_unlock_irqrestore(&pci_lock, flags);
> +}
> +
> +/**
> + * pci_start_bist - Start BIST on a PCI device
> + * @dev:	pci device struct
> + *
> + * This function allows a device driver to start BIST
> + * when PCI config accesses are disabled.
> + *
> + * Return value:
> + * 	nothing
> + **/
> +int pci_start_bist(struct pci_dev *dev)
> +{
> +	return pci_bus_write_config_byte(dev->bus, dev->devfn, PCI_BIST, PCI_BIST_START);
> +}
> +
> +EXPORT_SYMBOL(pci_read_config_byte);
> +EXPORT_SYMBOL(pci_read_config_word);
> +EXPORT_SYMBOL(pci_read_config_dword);
> +EXPORT_SYMBOL(pci_write_config_byte);
> +EXPORT_SYMBOL(pci_write_config_word);
> +EXPORT_SYMBOL(pci_write_config_dword);
> +EXPORT_SYMBOL(pci_start_bist);
> +EXPORT_SYMBOL(pci_block_config_access);
> +EXPORT_SYMBOL(pci_unblock_config_access);
> diff -puN include/linux/pci.h~pci_block_config_io_during_bist include/linux/pci.h
> --- linux-2.6.10-rc2-bk5/include/linux/pci.h~pci_block_config_io_during_bist	2004-11-20 16:07:19.000000000 -0600
> +++ linux-2.6.10-rc2-bk5-bjking1/include/linux/pci.h	2004-11-20 16:07:19.000000000 -0600
> @@ -535,7 +535,8 @@ struct pci_dev {
>  	/* keep track of device state */
>  	unsigned int	is_enabled:1;	/* pci_enable_device has been called */
>  	unsigned int	is_busmaster:1; /* device is busmaster */
> -	
> +	unsigned int	block_cfg_access:1;	/* config space access is blocked */
> +
>  	u32		saved_config_space[16]; /* config space saved at suspend time */
>  	struct bin_attribute *rom_attr; /* attribute descriptor for sysfs ROM entry */
>  	int rom_attr_enabled;		/* has display of the rom attribute been enabled? */
> @@ -750,31 +751,12 @@ int pci_bus_read_config_dword (struct pc
>  int pci_bus_write_config_byte (struct pci_bus *bus, unsigned int devfn, int where, u8 val);
>  int pci_bus_write_config_word (struct pci_bus *bus, unsigned int devfn, int where, u16 val);
>  int pci_bus_write_config_dword (struct pci_bus *bus, unsigned int devfn, int where, u32 val);
> -
> -static inline int pci_read_config_byte(struct pci_dev *dev, int where, u8 *val)
> -{
> -	return pci_bus_read_config_byte (dev->bus, dev->devfn, where, val);
> -}
> -static inline int pci_read_config_word(struct pci_dev *dev, int where, u16 *val)
> -{
> -	return pci_bus_read_config_word (dev->bus, dev->devfn, where, val);
> -}
> -static inline int pci_read_config_dword(struct pci_dev *dev, int where, u32 *val)
> -{
> -	return pci_bus_read_config_dword (dev->bus, dev->devfn, where, val);
> -}
> -static inline int pci_write_config_byte(struct pci_dev *dev, int where, u8 val)
> -{
> -	return pci_bus_write_config_byte (dev->bus, dev->devfn, where, val);
> -}
> -static inline int pci_write_config_word(struct pci_dev *dev, int where, u16 val)
> -{
> -	return pci_bus_write_config_word (dev->bus, dev->devfn, where, val);
> -}
> -static inline int pci_write_config_dword(struct pci_dev *dev, int where, u32 val)
> -{
> -	return pci_bus_write_config_dword (dev->bus, dev->devfn, where, val);
> -}
> +int pci_read_config_byte(struct pci_dev *dev, int where, u8 *val);
> +int pci_read_config_word(struct pci_dev *dev, int where, u16 *val);
> +int pci_read_config_dword(struct pci_dev *dev, int where, u32 *val);
> +int pci_write_config_byte(struct pci_dev *dev, int where, u8 val);
> +int pci_write_config_word(struct pci_dev *dev, int where, u16 val);
> +int pci_write_config_dword(struct pci_dev *dev, int where, u32 val);
>  
>  int pci_enable_device(struct pci_dev *dev);
>  int pci_enable_device_bars(struct pci_dev *dev, int mask);
> @@ -870,6 +852,9 @@ extern void pci_disable_msix(struct pci_
>  extern void msi_remove_pci_irq_vectors(struct pci_dev *dev);
>  #endif
>  
> +extern int pci_start_bist(struct pci_dev *dev);
> +extern void pci_block_config_access(struct pci_dev *dev);
> +extern void pci_unblock_config_access(struct pci_dev *dev);
>  #endif /* CONFIG_PCI */
>  
>  /* Include architecture-dependent settings and functions */
> 
> _

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center