pci: Block config access during BIST (resend)
Subsystems (3)
[email protected]
maintainerPATCH 1/1 pci: Block config access during BIST (resend)
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.
---
linux-2.6.10-bk13-bjking1/drivers/pci/access.c | 104 +++++++++++++++++++++++++ linux-2.6.10-bk13-bjking1/include/linux/pci.h | 37 ++------ 2 files changed, 115 insertions(+), 26 deletions(-)
include/linux/pci.h renamed from include/linux/pci.h~pci_block_config_io_during_bist +11 -26
@@ -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 pcint 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 */
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_config_io_during_bist +104 -0
107 lines not on this page · raw message
_
Quoted 4 lines · [email protected] writes:
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
Quoted 8 lines
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.
I think it would be better to do this check higher level in the driver. IMHO pci_*_config should stay lean and fast low level functions without such baggage.
-Andi
Brian King
maintainerQuoted 13 lines · Andi Kleen
Quoted 8 lines · [email protected] writes:
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.
Quoted 3 lines
I think it would be better to do this check higher level in the driver. IMHO pci_*_config should stay lean and fast low level functions without such baggage.
The problem I am trying to solve is the userspace PCI access methods accessing my config space when the adapter is not able to handle such an access. Today these accesses bypass the device driver altogether and there is no way to stop them. An alternative to this patch would be to only do these checks for the PCI config accesses initiated from the various userspace mechanisms, but I'm not sure the patch would then be usable for what benh wanted it for. Ben?
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
Quoted 4 lines
The problem I am trying to solve is the userspace PCI access methods accessing my config space when the adapter is not able to handle such an access. Today these accesses bypass the device driver altogether and there is no way to stop them. An alternative to this patch would be to
For this I would add a semaphore or a lock bit to pci_dev. Probably a simple flag is good enough that is checked by sysfs/proc and return EBUSY when set.
This assumes that the driver controls the device during BIST time.
-Andi
Brian King
maintainerQuoted 6 lines · Andi Kleen
Quoted 1 line
The problem I am trying to solve is the userspace PCI access methods accessing my config space when the adapter is not able to handle such an access. Today these accesses bypass the device driver altogether and there is no way to stop them. An alternative to this patch would be to
For this I would add a semaphore or a lock bit to
pci_dev. Probably a simple flag is good enough that is checked by sysfs/proc and return EBUSY when set.
How about something like this... (only compile tested at this point)
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
linux-2.6.10-bk13-bjking1/drivers/pci/access.c | 83 ++++++++++++++++++++++ linux-2.6.10-bk13-bjking1/drivers/pci/pci-sysfs.c | 10 +- linux-2.6.10-bk13-bjking1/drivers/pci/proc.c | 28 +++---- linux-2.6.10-bk13-bjking1/drivers/pci/syscall.c | 12 +-- linux-2.6.10-bk13-bjking1/include/linux/pci.h | 12 ++- 5 files changed, 119 insertions(+), 26 deletions(-)
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist +5 -5
@@ -109,7 +109,7 @@ pci_read_config(struct kobject *kobj, chwhile (off &3 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++;if (--size ==0 )
@@ -118,7 +118,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {unsigned int val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); buf[off - init_off] = val &0xff ; buf[off - init_off +1 ] = (val >>8 ) &0xff ; buf[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -129,7 +129,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >0 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++; --size;
@@ -153,7 +153,7 @@ pci_write_config(struct kobject *kobj, c }while (off &3 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++;if (--size ==0 )break ;
@@ -170,7 +170,7 @@ pci_write_config(struct kobject *kobj, c }while (size >0 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++; --size; }
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist +83 -0
86 lines not on this page · raw message
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist +11 -1
@@ -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_ucfg_access:1; /* userspace 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? */
@@ -776,6 +777,13 @@ static inline int pci_write_config_dwordreturn pci_bus_write_config_dword (dev->bus, dev->devfn, where, val); } +int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +int pci_user_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);void pci_disable_device (struct pci_dev *dev);
@@ -870,6 +878,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist +6 -6
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist +14 -14
@@ -79,7 +79,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -88,7 +88,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -97,7 +97,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -106,7 +106,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -115,7 +115,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -150,7 +150,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -159,7 +159,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -168,7 +168,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -177,7 +177,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -186,7 +186,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -474,10 +474,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
_
Quoted 5 lines · Brian King
Quoted 3 lines
For this I would add a semaphore or a lock bit to
pci_dev. Probably a simple flag is good enough that is checked by sysfs/proc and return EBUSY when set.How about something like this... (only compile tested at this point)
User space does not expect to get dumped with -EBUSY randomly on PCI accesses. Not a viable option in that form _but_ making them sleep would work - even with a simple global wait queue
for the pci_unblock_.. path
ie add the following (oh and uninlined probably for compatcness)
static int pci_user_wait_access(struct pci_dev *pdev) {
wait_event(&pci_ucfg_wait, dev->block_ucfg_access == 0);
}Quoted 9 lines · Alan Cox
Quoted 5 lines · Brian King
Quoted 3 lines
For this I would add a semaphore or a lock bit to
pci_dev. Probably a simple flag is good enough that is checked by sysfs/proc and return EBUSY when set.How about something like this... (only compile tested at this point)
User space does not expect to get dumped with
-EBUSYrandomly on PCI
I think it's a reasonable thing to do. If you prefer you could fake a 0xffffffff read, that would look like busy or non existing hardware. But the errno would seem to be cleaner to me.
There may be other reasons to have error codes here in the future too - e.g. with the upcomming support for real PCI error handling it would make a lot of sense to return EIO in some cases. User space will just have to cope with that.
Quoted 9 lines
accesses. Not a viable option in that form _but_ making them sleep would work - even with a simple global wait queue
for thepci_unblock_.. pathie add the following (oh and uninlined probably for compatcness)
static int pci_user_wait_access(struct pci_dev *pdev) { wait_event(&pci_ucfg_wait, dev->block_ucfg_access == 0); }
I don't like this very much. What happens when the device doesn't get out of BIST for some reason?
I think it's better to keep this simple, and an error is fine for that.
-Andi
Brian King
maintainerQuoted 11 lines · Andi Kleen
Quoted 9 lines · Alan Cox
Quoted 5 lines · Brian King
Quoted 3 lines
For this I would add a semaphore or a lock bit to
pci_dev. Probably a simple flag is good enough that is checked by sysfs/proc and return EBUSY when set.How about something like this... (only compile tested at this point)
Quoted 6 lines
Quoted 1 line
User space does not expect to get dumped with
-EBUSYrandomly on PCII think it's a reasonable thing to do. If you prefer you could fake a
0xffffffffread, that would look like busy or non existing hardware. But the errno would seem to be cleaner to me.
We can certainly go either way. I decided to go the way I did simply because that was what was suggested.
Quoted 12 lines
There may be other reasons to have error codes here in the future too - e.g. with the upcomming support for real PCI error handling it would make a lot of sense to return EIO in some cases. User space will just have to cope with that.
Quoted 6 lines
accesses. Not a viable option in that form _but_ making them sleep would work - even with a simple global wait queue
for thepci_unblock_.. pathie add the following (oh and uninlined probably for compatcness)
Quoted 8 lines
Quoted 3 lines
static int pci_user_wait_access(struct pci_dev *pdev) { wait_event(&pci_ucfg_wait, dev->block_ucfg_access == 0); }I don't like this very much. What happens when the device doesn't get out of BIST for some reason?
I think it's better to keep this simple, and an error is fine for that.
Also, Ben Herrenschmidt expressed a desire to use this interface in dealing with some power management issues on G5's. The issue is that when some devices are powered off on G5 they will not respond to config cycles and may even deadlock the system. This usage would require the ability to block userspace for an indefinite period of time and also make use of the config space caching code that is in my patch.
-Brian
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
Quoted 3 lines · Brian King
Andi Kleen wrote:
We can certainly go either way. I decided to go the way I did simply because that was what was suggested.
The error case breaks X11. The cached approach of Ben's would probably hide that nicely although it might cause some random crashes during powerdown. Its hard to fix in user space because X has no idea how to tell what is going on portably in the 'it broke' case other than spin trying to for a while. The kernel knows what is up and can make an intelligent decision - if the device doesn't come back from bist or we need to mark devices as "very gone away" then maybe a second flag so we have two user checked flags
In BIST - wait
Dead - error
Quoted 3 lines
cycles and may even deadlock the system. This usage would require the ability to block userspace for an indefinite period of time and also make use of the config space caching code that is in my patch.
What if the user space you block holds a resource that is preventing power down completing ? I can see how the kernel side stuff needs to be more robust (bad news btw.. over 99% of pci config calls in the kernel don't check the return according to a quick grep count. Good news is they are almost all reads so caching ought to work for the main config space stuff at least)
Caching doesn't however work for the cases like IRQ handlers but that seems not to be problematic as the only "other device" stuff people should be sticking their noses in except for bridge fixup stuff is things like the BAR registers.
Alan
Quoted 5 lines · Andi Kleen
Quoted 1 line
User space does not expect to get dumped with
-EBUSYrandomly on PCII think it's a reasonable thing to do. If you prefer you could fake a
0xffffffffread, that would look like busy or non existing hardware. But the errno would seem to be cleaner to me.
Either will break X.
Quoted 6 lines
Quoted 3 lines
static int pci_user_wait_access(struct pci_dev *pdev) { wait_event(&pci_ucfg_wait, dev->block_ucfg_access == 0); }I don't like this very much. What happens when the device doesn't get out of BIST for some reason?
Then you need to switch to wait_event_timeout(). Its not terribly hard
8)
Quoted 8 lines · Alan Cox
Quoted 5 lines · Andi Kleen
Quoted 1 line
User space does not expect to get dumped with
-EBUSYrandomly on PCII think it's a reasonable thing to do. If you prefer you could fake a
0xffffffffread, that would look like busy or non existing hardware. But the errno would seem to be cleaner to me.Either will break X.
You are saying that X during its own private broken PCI scan stops scanning when it sees an errno?
That sounds incredibly broken if true. I'm not sure how much effort the kernel should really take to work around such user breakage. I suppose an ffffffff return would work.
Quoted 10 lines
Quoted 7 lines
Quoted 4 lines
static int pci_user_wait_access(struct pci_dev *pdev) { wait_event(&pci_ucfg_wait, dev->block_ucfg_access == 0); }I don't like this very much. What happens when the device doesn't get out of BIST for some reason?
Then you need to switch to
wait_event_timeout(). Its not terribly hard
8)
Just complicating something that should be very simple.
-Andi
Quoted 6 lines · Andi Kleen
You are saying that X during its own private broken PCI scan stops scanning when it sees an errno?
That sounds incredibly broken if true. I'm not sure how much effort the kernel should really take to work around such user breakage. I suppose an ffffffff return would work.
X needs to be able to find the device layout in order to build its PCI mappings. Cached data is probably quite sufficient for this.
Quoted 4 lines
Quoted 2 lines
Then you need to switch to
wait_event_timeout(). Its not terribly hard
8)Just complicating something that should be very simple.
You are breaking an established user space API. Its not suprising this will break applications is it.
Quoted 10 lines · Alan Cox
Quoted 6 lines · Andi Kleen
You are saying that X during its own private broken PCI scan stops scanning when it sees an errno?
That sounds incredibly broken if true. I'm not sure how much effort the kernel should really take to work around such user breakage. I suppose an ffffffff return would work.
X needs to be able to find the device layout in order to build its PCI mappings. Cached data is probably quite sufficient for this.
I mean i would expect it to continue scanning other entries when it sees an error on one. Is that not true?
The devices we're talking about here that do BIST are SCSI controllers etc. that are normally of no interest to X.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Then you need to switch to
wait_event_timeout(). Its not terribly hard
8)Just complicating something that should be very simple.
You are breaking an established user space API. Its not suprising this will break applications is it.
Are you sure these devices even return something useful during BIST?
As Brian said the device he was working with would just not answer, leading to a bus abort. This would get ffffffff on a PC. You could simulate this if you want, although I think a EBUSY or EIO is better.
-Andi
Quoted 5 lines · Andi Kleen
Quoted 2 lines
X needs to be able to find the device layout in order to build its PCI mappings. Cached data is probably quite sufficient for this.
I mean i would expect it to continue scanning other entries when it sees an error on one. Is that not true?
X needs to be able to find the device layout in order to build its PCI mappings. If there are things mysteriously vanishing now and then its not going to have valid mappings
Quoted 10 lines · Alan Cox
Quoted 5 lines · Andi Kleen
Quoted 2 lines
X needs to be able to find the device layout in order to build its PCI mappings. Cached data is probably quite sufficient for this.
I mean i would expect it to continue scanning other entries when it sees an error on one. Is that not true?
X needs to be able to find the device layout in order to build its PCI mappings. If there are things mysteriously vanishing now and then its not going to have valid mappings
I could see it as a problem when it happens on a bridge, but why should it be a problem when some arbitary, nothing to do with X leaf is temporarily not available?
-Andi
Quoted 3 lines · Andi Kleen
I could see it as a problem when it happens on a bridge, but why should it be a problem when some arbitary, nothing to do with X leaf is temporarily not available?
Because X will believe that PCI address range is free and right now in some circumstances older X may want to play with PCI layout itself.
Alan
Quoted 7 lines · Alan Cox
Quoted 3 lines · Andi Kleen
I could see it as a problem when it happens on a bridge, but why should it be a problem when some arbitary, nothing to do with X leaf is temporarily not available?
Because X will believe that PCI address range is free and right now in some circumstances older X may want to play with PCI layout itself.
Yuck. sounds as broken as it can be. Hopefully nobody uses such X servers anymore.
Then it won't work with this BIST hardware anyways - if it tries to read config space of a device that is currently in BIST it will just get a bus abort and no useful information.
The only point of this whole patch exercise is to avoid the bus abort to satisfy the more strict hardware error checking on PPC64. On PCs it really won't make any difference.
-Andi
Quoted 3 lines · Andi Kleen
Then it won't work with this BIST hardware anyways - if it tries to read config space of a device that is currently in BIST it will just get a bus abort and no useful information.
So it should wait to preseve a sane API at least for a short while and if the user hasn't specified O_NDELAY. Its a compatibility consideration
Quoted 3 lines
The only point of this whole patch exercise is to avoid the bus abort to satisfy the more strict hardware error checking on PPC64. On PCs it really won't make any difference.
I thought Ben wanted to do this for other PPC stuff ?
Quoted 5 lines · Alan Cox
Quoted 3 lines · Andi Kleen
Then it won't work with this BIST hardware anyways - if it tries to read config space of a device that is currently in BIST it will just get a bus abort and no useful information.
Quoted 8 lines
So it should wait to preseve a sane API at least for a short while and if the user hasn't specified
O_NDELAY. Its a compatibility considerationQuoted 3 lines
The only point of this whole patch exercise is to avoid the bus abort to satisfy the more strict hardware error checking on PPC64. On PCs it really won't make any difference.
I thought Ben wanted to do this for other PPC stuff ?
Yes. On various models, I can turn off some ASIC cells, some of them being PCI devices. I do that dynamically for power management typically.
Depending on the chipset, tapping one of those "disabled" cells with config space accesses results in either all 1's or a lockup. On the former, I currently do nothing special (but that cause problems with various distro HW detection/configuration tools or the possible problem with X you mentioned, among others), on the later, I have a special filter in my pmac low level config space access routines to block access to those sleeping devices (and currently to return all 1's).
I'm pretty sure similar situations can happen on other archs when pushing a bit on power management, especially things like handhelds (though not much of them are PCI based for now).
That's why a "generic" mecanism to hide such devices while providing cached data on config space read's would be useful to me as well.
Ben.
Quoted 6 lines · Benjamin Herrenschmidt
I'm pretty sure similar situations can happen on other archs when pushing a bit on power management, especially things like handhelds (though not much of them are PCI based for now).
That's why a "generic" mecanism to hide such devices while providing cached data on config space read's would be useful to me as well.
That makes a lot of sense. So we need both a "blocked, will be back soon" and "this PCI device is invisible" flags. A device going into blocked and not coming back would presumably transition into "invisible". I'm assuming we can't just delete the PCI device because the kernel needs to know that cell is there for future use/abuse.
Quoted 5 lines · Alan Cox
Quoted 4 lines · Benjamin Herrenschmidt
I'm pretty sure similar situations can happen on other archs when pushing a bit on power management, especially things like handhelds (though not much of them are PCI based for now).
Quoted 8 lines
Quoted 2 lines
That's why a "generic" mecanism to hide such devices while providing cached data on config space read's would be useful to me as well.
That makes a lot of sense. So we need both a "blocked, will be back soon" and "this PCI device is invisible" flags. A device going into blocked and not coming back would presumably transition into "invisible". I'm assuming we can't just delete the PCI device because the kernel needs to know that cell is there for future use/abuse.
Right. Though I think the "will be back soon" and "is invisible" are pretty much the same thing. That is, in both our cases (BIST and pmac PM), we want the device to still be visible to userland, as it actually exist, should be properly detected by userland config tools etc..., but may only be actually enabled when the interface is opened/used for PM reasons.
Ben.
Quoted 6 lines
Right. Though I think the "will be back soon" and "is invisible" are pretty much the same thing. That is, in both our cases (BIST and pmac PM), we want the device to still be visible to userland, as it actually exist, should be properly detected by userland config tools etc..., but may only be actually enabled when the interface is opened/used for PM reasons.
I just request that this shouldn't be done in the low level pci_config_read_* functions. Please keep them simple and lean. If you want such complex semantics for user space do it in a separate layer.
-Andi
Quoted 10 lines · Andi Kleen
Quoted 6 lines
Right. Though I think the "will be back soon" and "is invisible" are pretty much the same thing. That is, in both our cases (BIST and pmac PM), we want the device to still be visible to userland, as it actually exist, should be properly detected by userland config tools etc..., but may only be actually enabled when the interface is opened/used for PM reasons.
I just request that this shouldn't be done in the low level pci_config_read_* functions. Please keep them simple and lean. If you want such complex semantics for user space do it in a separate layer.
What is complex in there ? I agree it's not convenient to do this from the very low level ones that don't take the pci_dev * as an argument, but from the higher level ones that does, the overhead is basically to test a flag in the pci_dev, I doubt it will be significant in any way performance wise, especially compared to the cost of a config space access...
Ben.
Quoted 6 lines
What is complex in there ? I agree it's not convenient to do this from the very low level ones that don't take the
pci_dev* as an argument, but from the higher level ones that does, the overhead is basically to test a flag in thepci_dev, I doubt it will be significant in any way performance wise, especially compared to the cost of a config space access...
For once you cannot block in them. There are even setups that need to (have to) do config space accesses in interrupt handlers. The operations done there should be rather light weight.
-Andi
Quoted 10 lines · Andi Kleen
Quoted 6 lines
What is complex in there ? I agree it's not convenient to do this from the very low level ones that don't take the
pci_dev* as an argument, but from the higher level ones that does, the overhead is basically to test a flag in thepci_dev, I doubt it will be significant in any way performance wise, especially compared to the cost of a config space access...For once you cannot block in them. There are even setups that need to (have to) do config space accesses in interrupt handlers. The operations done there should be rather light weight.
I don't think we ever want to block in that sense. I think all we need is the "filter" mecanism, that is drop writes and return cached data on reads when the device is "offlined"...
Ben.
Quoted 3 lines · Andi Kleen
I just request that this shouldn't be done in the low level pci_config_read_* functions. Please keep them simple and lean. If you want such complex semantics for user space do it in a separate layer.
It seems reasonable not to implement the wait (if not essential) in the pci_config_* cases just in the pci user ones (as Brian was doing in the later patches).
Brian King
maintainerQuoted 4 lines · Andi Kleen
As Brian said the device he was working with would just not answer, leading to a bus abort. This would get ffffffff on a PC. You could simulate this if you want, although I think a EBUSY or EIO is better.
Alan - are you satisfied with the most recent patch, or would you prefer the patch not returning failure return codes and just bit bucketing writes and returning all ff's on reads? Either way works for me.
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
Quoted 9 lines · Brian King
Quoted 4 lines · Andi Kleen
As Brian said the device he was working with would just not answer, leading to a bus abort. This would get ffffffff on a PC. You could simulate this if you want, although I think a EBUSY or EIO is better.
Alan - are you satisfied with the most recent patch, or would you prefer the patch not returning failure return codes and just bit bucketing writes and returning all ff's on reads? Either way works for me.
Hmm, I think i haven't seen a recent patch. But as long as it doesn't block in pci_config_* and is light weight there it's fine for me.
-Andi
Brian King
maintainerQuoted 9 lines · Andi Kleen
Quoted 7 lines · Brian King
Quoted 4 lines · Andi Kleen
As Brian said the device he was working with would just not answer, leading to a bus abort. This would get ffffffff on a PC. You could simulate this if you want, although I think a EBUSY or EIO is better.
Quoted 8 lines
Quoted 1 line
Alan - are you satisfied with the most recent patch, or would you prefer the patch not returning failure return codes and just bit bucketing writes and returning all ff's on reads? Either way works for me.
Hmm, I think i haven't seen a recent patch. But as long as it doesn't block in pci_config_* and is light weight there it's fine for me.
-Andi
Here is my latest patch.
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
---
linux-2.6.11-rc1-bjking1/drivers/pci/access.c | 81 +++++++++++++++++++++++ linux-2.6.11-rc1-bjking1/drivers/pci/pci-sysfs.c | 10 +- linux-2.6.11-rc1-bjking1/drivers/pci/proc.c | 28 +++---- linux-2.6.11-rc1-bjking1/drivers/pci/syscall.c | 12 +-- linux-2.6.11-rc1-bjking1/include/linux/pci.h | 12 +++ 5 files changed, 117 insertions(+), 26 deletions(-)
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +81 -0
@@ -60,3 +60,84 @@ 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_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, 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_USER_WRITE_CONFIG (size,type) \ +int pci_user_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_ucfg_access)) \ + ret = dev->bus->ops->write(dev->bus, dev->devfn, pos, sizeof(type), val); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +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 return -EBUSY and reads will return the +* data saved using pci_save_state for the first 64 bytes of config +* space and return -EBUSY for all other config reads. +* +* Return value: +* nothing + **/ +void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL (pci_block_user_cfg_access); + +/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @dev: pci device struct +* +* This function allows userspace PCI config accesses to resume. +* +* Return value: +* nothing +**/ +void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL (pci_unblock_user_cfg_access);
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist_again +5 -5
@@ -110,7 +110,7 @@ pci_read_config(struct kobject *kobj, chwhile (off &3 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++;if (--size ==0 )
@@ -119,7 +119,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {unsigned int val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); buf[off - init_off] = val &0xff ; buf[off - init_off +1 ] = (val >>8 ) &0xff ; buf[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -130,7 +130,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >0 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++; --size;
@@ -154,7 +154,7 @@ pci_write_config(struct kobject *kobj, c }while (off &3 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++;if (--size ==0 )break ;
@@ -171,7 +171,7 @@ pci_write_config(struct kobject *kobj, c }while (size >0 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++; --size; }
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist_again +14 -14
@@ -79,7 +79,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -88,7 +88,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -97,7 +97,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -106,7 +106,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -115,7 +115,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -150,7 +150,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -159,7 +159,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -168,7 +168,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -177,7 +177,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -186,7 +186,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -474,10 +474,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist_again +6 -6
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +11 -1
@@ -557,7 +557,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_ucfg_access:1; /* userspace 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? */
@@ -801,6 +802,13 @@ static inline int pci_write_config_dwordreturn pci_bus_write_config_dword (dev->bus, dev->devfn, where, val); } +int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +int pci_user_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);void pci_disable_device (struct pci_dev *dev);
@@ -896,6 +904,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
_
Quoted 3 lines · Brian King
Alan - are you satisfied with the most recent patch, or would you prefer the patch not returning failure return codes and just bit bucketing writes and returning all ff's on reads? Either way works for me.
Which was the last one. For userspace we need to block while we finish the BIST.
Brian King
maintainerQuoted 9 lines · Alan Cox
Quoted 1 line · Brian King
Alan - are you satisfied with the most recent patch, or would you prefer the patch not returning failure return codes and just bit bucketing writes and returning all ff's on reads? Either way works for me.
Which was the last one. For userspace we need to block while we finish the BIST.
Here is the last one. I've looked at making userspace sleep until BIST is finished. The downside I see to this is that is complicates the patch due to the following reasons:
1. In order to also make this work for Ben's PPC power management usage would require an additional flag and additional APIs to set and clear the flag.
2. Since BIST can be run at interrupt context, the interfaces to block and unblock userspace accesses across BIST must be callable from interrupt context. This prevents the usage of semaphores or simple wait_event macros and requires new macros that carefully check the new pci device flag and manage the spinlock.
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
---
linux-2.6.11-rc1-bjking1/drivers/pci/access.c | 81 +++++++++++++++++++++++ linux-2.6.11-rc1-bjking1/drivers/pci/pci-sysfs.c | 10 +- linux-2.6.11-rc1-bjking1/drivers/pci/proc.c | 28 +++---- linux-2.6.11-rc1-bjking1/drivers/pci/syscall.c | 12 +-- linux-2.6.11-rc1-bjking1/include/linux/pci.h | 12 +++ 5 files changed, 117 insertions(+), 26 deletions(-)
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +81 -0
@@ -60,3 +60,84 @@ 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_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, 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_USER_WRITE_CONFIG (size,type) \ +int pci_user_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_ucfg_access)) \ + ret = dev->bus->ops->write(dev->bus, dev->devfn, pos, sizeof(type), val); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +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 return -EBUSY and reads will return the +* data saved using pci_save_state for the first 64 bytes of config +* space and return -EBUSY for all other config reads. +* +* Return value: +* nothing + **/ +void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL (pci_block_user_cfg_access); + +/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @dev: pci device struct +* +* This function allows userspace PCI config accesses to resume. +* +* Return value: +* nothing +**/ +void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL (pci_unblock_user_cfg_access);
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist_again +5 -5
@@ -110,7 +110,7 @@ pci_read_config(struct kobject *kobj, chwhile (off &3 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++;if (--size ==0 )
@@ -119,7 +119,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {unsigned int val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); buf[off - init_off] = val &0xff ; buf[off - init_off +1 ] = (val >>8 ) &0xff ; buf[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -130,7 +130,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >0 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++; --size;
@@ -154,7 +154,7 @@ pci_write_config(struct kobject *kobj, c }while (off &3 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++;if (--size ==0 )break ;
@@ -171,7 +171,7 @@ pci_write_config(struct kobject *kobj, c }while (size >0 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++; --size; }
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist_again +14 -14
@@ -79,7 +79,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -88,7 +88,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -97,7 +97,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -106,7 +106,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -115,7 +115,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -150,7 +150,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -159,7 +159,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -168,7 +168,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -177,7 +177,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -186,7 +186,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -474,10 +474,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist_again +6 -6
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +11 -1
@@ -557,7 +557,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_ucfg_access:1; /* userspace 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? */
@@ -801,6 +802,13 @@ static inline int pci_write_config_dwordreturn pci_bus_write_config_dword (dev->bus, dev->devfn, where, val); } +int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +int pci_user_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);void pci_disable_device (struct pci_dev *dev);
@@ -896,6 +904,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
_
Quoted 4 lines · Brian King
Here is the last one. I've looked at making userspace sleep until BIST is finished. The downside I see to this is that is complicates the patch due to the following reasons:
Quoted 9 lines
1. In order to also make this work for Ben's PPC power management usage would require an additional flag and additional APIs to set and clear the flag.
2. Since BIST can be run at interrupt context, the interfaces to block and unblock userspace accesses across BIST must be callable from interrupt context. This prevents the usage of semaphores or simplewait_eventmacros and requires new macros that carefully check the new pci device flag and manage the spinlock.
Well, I honestly think that this is unnecessary burden. I think that just dropping writes & returning data from the cache on reads is enough, blocking userspace isn't necessary, but then, I may be wrong ;)
Ben.
Quoted 4 lines · Benjamin Herrenschmidt
On Wed, 2005-01-26 at 10:34 -0600, Brian King wrote:
Well, I honestly think that this is unnecessary burden. I think that just dropping writes & returning data from the cache on reads is enough, blocking userspace isn't necessary, but then, I may be wrong ;)
Providing the BARs, cmd register and bridge VGA_EN are cached then I think you
are right.
Alan
Brian King
maintainerQuoted 11 lines · Alan Cox
Quoted 4 lines · Benjamin Herrenschmidt
On Wed, 2005-01-26 at 10:34 -0600, Brian King wrote:
Well, I honestly think that this is unnecessary burden. I think that just dropping writes & returning data from the cache on reads is enough, blocking userspace isn't necessary, but then, I may be wrong ;)Providing the BARs, cmd register and bridge
VGA_ENare cached then I think you
are right.
The first 64 bytes of config space are cached, so this should handle the registers you mention above.
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
Quoted 9 lines · Alan Cox
Quoted 4 lines · Benjamin Herrenschmidt
On Wed, 2005-01-26 at 10:34 -0600, Brian King wrote:
Well, I honestly think that this is unnecessary burden. I think that just dropping writes & returning data from the cache on reads is enough, blocking userspace isn't necessary, but then, I may be wrong ;)Providing the BARs, cmd register and bridge
VGA_ENare cached then I think you
are right.
There might be one problem with dropping of writes tho, which has to do with userland like X doing VGA flip-flip (VGA_EN on bridges and CMD_IO on devices). But I think that's a non-issues because:
- For now, nobody is using this interface to hide a VGA device or a
bridge, and I don't see that happening in the near future
- Eventually, the control of who owns VGA is to be moved to a kernel
driver doing proper arbitration as we discussed previously on several lists. (BTW. Alan, I've been a bit out of touch with that and Jon is too busy lately, do you know if there have been progress or at least prototype code one could take over from for that ?)
Ben.
Brian King
maintainerQuoted 11 lines · Alan Cox
Quoted 4 lines · Benjamin Herrenschmidt
On Wed, 2005-01-26 at 10:34 -0600, Brian King wrote:
Well, I honestly think that this is unnecessary burden. I think that just dropping writes & returning data from the cache on reads is enough, blocking userspace isn't necessary, but then, I may be wrong ;)Providing the BARs, cmd register and bridge
VGA_ENare cached then I think you
are right.
Everyone seems satisfied with the patch now. Greg, can you please apply?
Thanks
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
---
linux-2.6.11-rc1-bjking1/drivers/pci/access.c | 81 +++++++++++++++++++++++ linux-2.6.11-rc1-bjking1/drivers/pci/pci-sysfs.c | 10 +- linux-2.6.11-rc1-bjking1/drivers/pci/proc.c | 28 +++---- linux-2.6.11-rc1-bjking1/drivers/pci/syscall.c | 12 +-- linux-2.6.11-rc1-bjking1/include/linux/pci.h | 12 +++ 5 files changed, 117 insertions(+), 26 deletions(-)
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +81 -0
@@ -60,3 +60,84 @@ 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_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, 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_USER_WRITE_CONFIG (size,type) \ +int pci_user_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_ucfg_access)) \ + ret = dev->bus->ops->write(dev->bus, dev->devfn, pos, sizeof(type), val); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +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 return -EBUSY and reads will return the +* data saved using pci_save_state for the first 64 bytes of config +* space and return -EBUSY for all other config reads. +* +* Return value: +* nothing + **/ +void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL (pci_block_user_cfg_access); + +/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @dev: pci device struct +* +* This function allows userspace PCI config accesses to resume. +* +* Return value: +* nothing +**/ +void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL (pci_unblock_user_cfg_access);
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist_again +5 -5
@@ -110,7 +110,7 @@ pci_read_config(struct kobject *kobj, chwhile (off &3 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++;if (--size ==0 )
@@ -119,7 +119,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {unsigned int val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); buf[off - init_off] = val &0xff ; buf[off - init_off +1 ] = (val >>8 ) &0xff ; buf[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -130,7 +130,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >0 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++; --size;
@@ -154,7 +154,7 @@ pci_write_config(struct kobject *kobj, c }while (off &3 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++;if (--size ==0 )break ;
@@ -171,7 +171,7 @@ pci_write_config(struct kobject *kobj, c }while (size >0 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++; --size; }
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist_again +14 -14
@@ -79,7 +79,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -88,7 +88,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -97,7 +97,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -106,7 +106,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -115,7 +115,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -150,7 +150,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -159,7 +159,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -168,7 +168,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -177,7 +177,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -186,7 +186,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -474,10 +474,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist_again +6 -6
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +11 -1
@@ -557,7 +557,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_ucfg_access:1; /* userspace 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? */
@@ -801,6 +802,13 @@ static inline int pci_write_config_dwordreturn pci_bus_write_config_dword (dev->bus, dev->devfn, where, val); } +int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +int pci_user_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);void pci_disable_device (struct pci_dev *dev);
@@ -896,6 +904,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
_
Quoted 28 lines · Brian King
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +28
-0+#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, 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_USER_WRITE_CONFIG (size,type) \ +int pci_user_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_ucfg_access)) \ + ret = dev->bus->ops->write(dev->bus, dev->devfn, pos, sizeof(type), val); \
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 101
Quoted 10 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +10
-0+spin_unlock_irqrestore (&pci_lock, flags); \ +return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +PCI_USER_WRITE_CONFIG (dword, u32)
Global but not exported? If so, they are local to the pci core, right? And if so, please put them in the drivers/pci/pci.h file and not the include/linux/pci.h file.
Quoted 11 lines
+/**
+ *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 return-EBUSYand reads will return the
+ * data saved usingpci_save_statefor the first 64 bytes of config
+ * space and return-EBUSYfor all other config reads.
+ *
+ * Return value:
+ * nothing
We know the return value is not needed by the way the function is defined, these two lines are unneeded.
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 124
Quoted 10 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +10
-0+void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL (pci_block_user_cfg_access);
EXPORT_SYMBOL_GPL() please?
Quoted 8 lines
+/**
+ *pci_unblock_user_cfg_access- Unblock userspace PCI config reads/writes
+ * @dev: pci device struct
+ *
+ * This function allows userspace PCI config accesses to resume.
+ *
+ * Return value:
+ * nothing
Same here as above.
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 143
Quoted 9 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +9
-0+void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL (pci_unblock_user_cfg_access);
Same as above.
include/linux/pci.h extern void pci_disable_msix(struct pci_ · line 909
Quoted 7 lines
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +2
-0@@ -896,6 +904,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */
Don't need empty functions for these if CONFIG_PCI is not enabled? Who would be calling these functions, drivers? If so, please create the empty functions.
Also, please CC the linux-pci mailing list for pci specific patches like this.
thanks,
greg k-h
Brian King
maintainerThanks for looking at this, Greg.
Quoted 12 lines · Greg KH
Review
Quoted 6 lines · Brian King
(quoted diff) +6
-0+PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +PCI_USER_WRITE_CONFIG(dword, u32)Global but not exported? If so, they are local to the pci core, right? And if so, please put them in the
drivers/pci/pci.hfile and not theinclude/linux/pci.hfile.
Ok. They are now local to pci core.
Quoted 13 lines
Quoted 11 lines
+/**
+ *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 return-EBUSYand reads will return the
+ * data saved usingpci_save_statefor the first 64 bytes of config
+ * space and return-EBUSYfor all other config reads.
+ *
+ * Return value:
+ * nothing
Quoted 2 lines
We know the return value is not needed by the way the function is defined, these two lines are unneeded.
Deleted.
Quoted 5 lines
Quoted 5 lines
(quoted diff) +5
-0+void pci_block_user_cfg_access(struct pci_dev *dev) +{ + unsigned long flags; + + pci_save_state(dev);
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ spin_lock_irqsave(&pci_lock, flags); + dev->block_ucfg_access = 1; + spin_unlock_irqrestore(&pci_lock, flags); +} +EXPORT_SYMBOL(pci_block_user_cfg_access);
EXPORT_SYMBOL_GPL()please?
Ok.
Quoted 11 lines
Quoted 8 lines
+/**
+ *pci_unblock_user_cfg_access- Unblock userspace PCI config reads/writes
+ * @dev: pci device struct
+ *
+ * This function allows userspace PCI config accesses to resume.
+ *
+ * Return value:
+ * nothingSame here as above.
Done.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+void pci_unblock_user_cfg_access(struct pci_dev *dev) +{ + unsigned long flags; +
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ spin_lock_irqsave(&pci_lock, flags); + dev->block_ucfg_access = 0; + spin_unlock_irqrestore(&pci_lock, flags); +} +EXPORT_SYMBOL(pci_unblock_user_cfg_access);Same as above.
Done.
Quoted 4 lines
Quoted 4 lines
(quoted diff) +0
-0@@ -896,6 +904,8 @@ extern void pci_disable_msix(struct pci_ extern void msi_remove_pci_irq_vectors(struct pci_dev *dev); #endif
Quoted 8 lines
Review extern void pci_disable_msix(struct pci_ · line 909
Quoted 3 lines
(quoted diff) +2
-0+extern void pci_block_user_cfg_access(struct pci_dev *dev); +extern void pci_unblock_user_cfg_access(struct pci_dev *dev); #endif /* CONFIG_PCI */Don't need empty functions for these if
CONFIG_PCIis not enabled? Who would be calling these functions, drivers? If so, please create the empty functions.
Yes, device drivers would be one of the potential users of these functions. Added the empty functions.
Quoted 2 lines
Also, please CC the linux-pci mailing list for pci specific patches like this.
Done.
Here is an updated patch...
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
---
linux-2.6.11-rc2-bk9-bjking1/drivers/pci/access.c | 75 +++++++++++++++++++ linux-2.6.11-rc2-bk9-bjking1/drivers/pci/pci-sysfs.c | 10 +- linux-2.6.11-rc2-bk9-bjking1/drivers/pci/pci.h | 7 + linux-2.6.11-rc2-bk9-bjking1/drivers/pci/proc.c | 28 +++---- linux-2.6.11-rc2-bk9-bjking1/drivers/pci/syscall.c | 12 +-- linux-2.6.11-rc2-bk9-bjking1/include/linux/pci.h | 7 + 6 files changed, 113 insertions(+), 26 deletions(-)
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +75 -0
@@ -60,3 +60,78 @@ 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_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, 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_USER_WRITE_CONFIG (size,type) \ +int pci_user_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_ucfg_access)) \ + ret = dev->bus->ops->write(dev->bus, dev->devfn, pos, sizeof(type), val); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +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 return -EBUSY and reads will return the +* data saved using pci_save_state for the first 64 bytes of config +* space and return -EBUSY for all other config reads. + **/ +void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL_GPL (pci_block_user_cfg_access); + +/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @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; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL_GPL (pci_unblock_user_cfg_access);
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist_again +5 -5
@@ -110,7 +110,7 @@ pci_read_config(struct kobject *kobj, chwhile (off &3 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++;if (--size ==0 )
@@ -119,7 +119,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {unsigned int val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); buf[off - init_off] = val &0xff ; buf[off - init_off +1 ] = (val >>8 ) &0xff ; buf[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -130,7 +130,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >0 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++; --size;
@@ -154,7 +154,7 @@ pci_write_config(struct kobject *kobj, c }while (off &3 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++;if (--size ==0 )break ;
@@ -171,7 +171,7 @@ pci_write_config(struct kobject *kobj, c }while (size >0 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++; --size; }
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist_again +14 -14
@@ -79,7 +79,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -88,7 +88,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -97,7 +97,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -106,7 +106,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -115,7 +115,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -150,7 +150,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -159,7 +159,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -168,7 +168,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -177,7 +177,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -186,7 +186,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -474,10 +474,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist_again +6 -6
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +6 -1
@@ -557,7 +557,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_ucfg_access:1; /* userspace 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? */
@@ -896,6 +897,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
@@ -947,6 +950,8 @@ static inline void pci_unregister_driverstatic inline int pci_find_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline int pci_find_ext_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline const struct pci_device_id *pci_match_device (const struct pci_device_id *ids,const struct pci_dev *dev) {return NULL ; } +static inline void pci_block_user_cfg_access (struct pci_dev *dev) { } +static inline void pci_unblock_user_cfg_access (struct pci_dev *dev) { }/* Power management related routines */ static inline int pci_save_state (struct pci_dev *dev) {return 0 ; }
drivers/pci/pci.h renamed from drivers/pci/pci.h~pci_block_user_config_io_during_bist_again +7 -0
@@ -11,6 +11,13 @@ extern int pci_bus_alloc_resource(structvoid (*alignf)(void *,struct resource *,unsigned long ,unsigned long ), void *alignf_data); +extern int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +extern int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +extern int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +extern int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +extern int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +extern int pci_user_write_config_dword (struct pci_dev *dev,int where,u32 val); +/* PCI /proc functions */ #ifdef CONFIG_PROC_FS extern int pci_proc_attach_device (struct pci_dev *dev);
_
Quoted 16 lines · Brian King
Quoted 14 lines · Greg KH
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword)
Quoted 10 lines · Brian King
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +10
-0+void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL (pci_block_user_cfg_access);
EXPORT_SYMBOL_GPL()please?
Quoted 1 line
Ok.
I'm not entirely convinced these should be GPL-only exports. Basically, this is saying that any driver for a device that has this problem must be GPLd. I think that's a firmer stance than Linus had in mind originally.
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 66
Quoted 9 lines
+++
linux-2.6.11-rc2-bk9-bjking1/drivers/pci/access.c2005-02-01 08:39:57.000000000 -0600drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +4
-0@@ -60,3 +60,78 @@ 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_USER_READ_CONFIG (size,type) \ +int pci_user_read_config_##size \ + (struct pci_dev *dev, int pos, type *val) \
{ \
unsigned long flags; \Could you line up the \ so they're uniform like the above PCI_OP_READ?
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 78
Quoted 10 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +10
-0+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, 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; \
Does this actually work? Surely if you're reading byte 14, you get the byte that was at address 12 or 15 in the config space, depending whether you're on a big or little endian machine?
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 136
Quoted 8 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +8
-0+void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +}
If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
Signature · 6 lines
"Next the statesmen will invent cheap lies, putting the blame upon the nation that is attacked, and every man will be glad of those conscience-soothing falsities, and will diligently study them, and refuse to examine any refutations of them; and thus he will by and by convince himself that the war is just, and will thank God for the better sleep he enjoys after this process of grotesque self-deception." -- Mark Twain
Brian King
maintainerQuoted 1 line · Matthew Wilcox
Quoted 1 line
+++
linux-2.6.11-rc2-bk9-bjking1/drivers/pci/access.c2005-02-01
08:39:57.000000000 -0600
Quoted 12 lines
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 66
Quoted 8 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +4
-0@@ -60,3 +60,78 @@ 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_USER_READ_CONFIG (size,type) \ +int pci_user_read_config_##size \ + (struct pci_dev *dev, int pos, type *val) \{ \ unsigned long flags; \
Quoted 1 line
Could you line up the \ so they're uniform like the above
PCI_OP_READ?
Ok.
Quoted 7 lines
Quoted 7 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +7
-0+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, pos,sizeof (type), &data); \ +else if (pos <sizeof (dev->saved_config_space)) \
Quoted 8 lines
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 78
Quoted 3 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +3
-0+ data = dev->saved_config_space[pos/sizeof (dev->saved_config_space[0 ])]; \ +spin_unlock_irqrestore (&pci_lock, flags); \ + *val = (type )data; \Does this actually work? Surely if you're reading byte 14, you get the byte that was at address 12 or 15 in the config space, depending whether you're on a big or little endian machine?
It actually only works for 4 byte accesses. I am fixing this and will be posting a patch later.
Quoted 4 lines
Quoted 4 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +4
-0+void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; +
Quoted 8 lines
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 136
Quoted 4 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +4
-0+spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +}If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
I did not think that was necessary. It will certainly make the patch more complicated. To implement it would really require we make the userspace writers wait, which gets ugly since the wait is based on a flag that can be updated at interrupt level so you end up with some fun spinlocking. Not a big deal, it just starts getting ugly. Keep in mind, one of the potential uses of this is for power management on PPC where a given device could have its config space blocked for a long time.
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
Quoted 4 lines · Brian King
Quoted 2 lines
If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
I did not think that was necessary.
We have to do *something*. We can't just throw away writes.
I see a few options:
- Log all pending writes to config space and replay the log when the device is unblocked.
- Fail writes to config space while the device is blocked.
- Write to the saved config space and then blat the saved config space back to the device upon unblocking.
Any other ideas?
BTW, you know things like XFree86 go completely around the kernel's PCI accessors and poke at config space directly?
Signature · 6 lines
"Next the statesmen will invent cheap lies, putting the blame upon the nation that is attacked, and every man will be glad of those conscience-soothing falsities, and will diligently study them, and refuse to examine any refutations of them; and thus he will by and by convince himself that the war is just, and will thank God for the better sleep he enjoys after this process of grotesque self-deception." -- Mark Twain
Brian King
maintainerQuoted 8 lines · Matthew Wilcox
Quoted 4 lines · Brian King
Quoted 2 lines
If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
I did not think that was necessary.
Quoted 6 lines
We have to do *something*. We can't just throw away writes.
I see a few options:
- Log all pending writes to config space and replay the log when the device is unblocked.
This would need to be dynamic in size, as a device could be blocked for a long time. In the scenario that this is used for power management and the device could be blocked for a long time, its unclear that we would still want all the writes accumulated to be written out when the device becomes unblocked.
Quoted 1 line
- Fail writes to config space while the device is blocked.
This would be nice and simple. I know Alan had some issue with returning failures on reads when blocked.
Alan - do you have the same issue on writes?
Quoted 2 lines
- Write to the saved config space and then blat the saved config space back to the device upon unblocking.
Would also be pretty simple to do and seems a little safer than potentially assaulting the recently unblocked device with who knows what values. The only problem I see with this is that we could end up returning strange values on cached reads if the writes update the cache. If userspace wrote to a read only register, we would end up returning that value on the read, which may not be the right thing to do.
Quoted 1 line
Any other ideas?
We could go back to Alan's idea of putting userspace reads/writes to sleep when the device is blocked, although this has additional complications as well...
Quoted 2 lines
BTW, you know things like XFree86 go completely around the kernel's PCI accessors and poke at config space directly?
The purpose of this API is to provide a way for the kernel to stop userspace from accessing PCI devices when the results of doing so would be catastrophic, such as a PCI bus error. The only users of this API so far are device drivers running BIST on an adapter (which shouldn't happen on a video card AFAIK) and for PPC power management (Ben - will this be an issue for you?)
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
Quoted 7 lines · Matthew Wilcox
Quoted 4 lines · Brian King
Quoted 2 lines
If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
I did not think that was necessary.
We have to do *something*. We can't just throw away writes.
I think we can in fact. Again, nobody outside of the driver has legitimacy to write to the config space of a device, especially if the device is "unreachable" (either doing a BIST or power managed).
Quoted 5 lines
I see a few options:
- Log all pending writes to config space and replay the log when the device is unblocked.
- Fail writes to config space while the device is blocked.
I agree that returning an error in this case would be a good idea.
Quoted 7 lines
- Write to the saved config space and then blat the saved config space back to the device upon unblocking.
Any other ideas?
BTW, you know things like XFree86 go completely around the kernel's PCI accessors and poke at config space directly?
Not anymore afaik. They use /proc/bus/pci
Ben.
Brian King
maintainerQuoted 10 lines · Matthew Wilcox
Quoted 4 lines · Brian King
Quoted 2 lines
If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
I did not think that was necessary.
We have to do *something*. We can't just throw away writes.
Quoted 7 lines
I see a few options:
- Log all pending writes to config space and replay the log when the device is unblocked.
- Fail writes to config space while the device is blocked.
- Write to the saved config space and then blat the saved config space back to the device upon unblocking.
Here is an updated patch which will now fail writes to config space while the device is blocked. I have also fixed up the caching to return the correct data and tested it on both little endian and big endian machines.
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
linux-2.6.11-rc2-bk9-bjking1/drivers/pci/access.c | 86 +++++++++++++++++++ linux-2.6.11-rc2-bk9-bjking1/drivers/pci/pci-sysfs.c | 10 +- linux-2.6.11-rc2-bk9-bjking1/drivers/pci/pci.h | 7 + linux-2.6.11-rc2-bk9-bjking1/drivers/pci/proc.c | 30 +++--- linux-2.6.11-rc2-bk9-bjking1/drivers/pci/syscall.c | 14 +-- linux-2.6.11-rc2-bk9-bjking1/include/linux/pci.h | 7 + 6 files changed, 127 insertions(+), 27 deletions(-)
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +86 -0
@@ -60,3 +60,89 @@ 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); + +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) \ +{ \ + 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, \ + 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); \ + *val = (type)data; \ + return ret; \ +} + +#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, \ + pos, sizeof(type), val); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +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) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL_GPL (pci_block_user_cfg_access); + +/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @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; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL_GPL (pci_unblock_user_cfg_access);
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist_again +5 -5
@@ -110,7 +110,7 @@ pci_read_config(struct kobject *kobj, chwhile (off &3 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++;if (--size ==0 )
@@ -119,7 +119,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {unsigned int val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); buf[off - init_off] = val &0xff ; buf[off - init_off +1 ] = (val >>8 ) &0xff ; buf[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -130,7 +130,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >0 ) {unsigned char val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); buf[off - init_off] = val; off++; --size;
@@ -154,7 +154,7 @@ pci_write_config(struct kobject *kobj, c }while (off &3 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++;if (--size ==0 )break ;
@@ -171,7 +171,7 @@ pci_write_config(struct kobject *kobj, c }while (size >0 ) { -pci_write_config_byte (dev, off, buf[off - init_off]); +pci_user_write_config_byte (dev, off, buf[off - init_off]); off++; --size; }
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist_again +16 -14
@@ -16,6 +16,8 @@#include <asm/uaccess.h> #include <asm/byteorder.h> +#include "pci.h" +static int proc_initialized;/* = 0 */ static loff_t
@@ -79,7 +81,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -88,7 +90,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -97,7 +99,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -106,7 +108,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -115,7 +117,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -150,7 +152,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -159,7 +161,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -168,7 +170,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -177,7 +179,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -186,7 +188,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -474,10 +476,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist_again +7 -7
@@ -13,7 +13,7 @@#include <linux/smp_lock.h> #include <linux/syscalls.h> #include <asm/uaccess.h> - +#include "pci.h" asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +6 -1
@@ -557,7 +557,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_ucfg_access:1; /* userspace 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? */
@@ -896,6 +897,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
@@ -947,6 +950,8 @@ static inline void pci_unregister_driverstatic inline int pci_find_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline int pci_find_ext_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline const struct pci_device_id *pci_match_device (const struct pci_device_id *ids,const struct pci_dev *dev) {return NULL ; } +static inline void pci_block_user_cfg_access (struct pci_dev *dev) { } +static inline void pci_unblock_user_cfg_access (struct pci_dev *dev) { }/* Power management related routines */ static inline int pci_save_state (struct pci_dev *dev) {return 0 ; }
drivers/pci/pci.h renamed from drivers/pci/pci.h~pci_block_user_config_io_during_bist_again +7 -0
@@ -11,6 +11,13 @@ extern int pci_bus_alloc_resource(structvoid (*alignf)(void *,struct resource *,unsigned long ,unsigned long ), void *alignf_data); +extern int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +extern int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +extern int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +extern int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +extern int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +extern int pci_user_write_config_dword (struct pci_dev *dev,int where,u32 val); +/* PCI /proc functions */ #ifdef CONFIG_PROC_FS extern int pci_proc_attach_device (struct pci_dev *dev);
_
Greg KH
AppliedQuoted 19 lines · Brian King
Quoted 10 lines · Matthew Wilcox
Quoted 4 lines · Brian King
Quoted 2 lines
If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
I did not think that was necessary.
We have to do *something*. We can't just throw away writes.
Quoted 7 lines
I see a few options:
- Log all pending writes to config space and replay the log when the device is unblocked.
- Fail writes to config space while the device is blocked.
- Write to the saved config space and then blat the saved config space back to the device upon unblocking.
Quoted 4 lines
Here is an updated patch which will now fail writes to config space while the device is blocked. I have also fixed up the caching to return the correct data and tested it on both little endian and big endian machines.
Applied, thanks.
greg k-h
Brian King
maintainerQuoted 29 lines · Greg KH
Quoted 21 lines · Brian King
Quoted 11 lines · Matthew Wilcox
Quoted 4 lines · Brian King
Quoted 2 lines
If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
I did not think that was necessary.
We have to do *something*. We can't just throw away writes.
Quoted 7 lines
I see a few options:
- Log all pending writes to config space and replay the log when the device is unblocked.
- Fail writes to config space while the device is blocked.
- Write to the saved config space and then blat the saved config space back to the device upon unblocking.Quoted 4 lines
Here is an updated patch which will now fail writes to config space while the device is blocked. I have also fixed up the caching to return the correct data and tested it on both little endian and big endian machines.
Quoted 4 lines
Applied, thanks.
greg k-h
Resending this now that 2.6.13 has opened up.
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
linux-2.6-bjking1/drivers/pci/access.c | 86 ++++++++++++++++++++++++++++++ linux-2.6-bjking1/drivers/pci/pci-sysfs.c | 10 +-- linux-2.6-bjking1/drivers/pci/pci.h | 7 ++ linux-2.6-bjking1/drivers/pci/proc.c | 28 ++++----- linux-2.6-bjking1/drivers/pci/syscall.c | 14 ++-- linux-2.6-bjking1/include/linux/pci.h | 7 ++ 6 files changed, 125 insertions(+), 27 deletions(-)
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +86 -0
@@ -60,3 +60,89 @@ 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); + +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) \ +{ \ + 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, \ + 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); \ + *val = (type)data; \ + return ret; \ +} + +#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, \ + pos, sizeof(type), val); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +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) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL_GPL (pci_block_user_cfg_access); + +/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @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; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL_GPL (pci_unblock_user_cfg_access);
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist_again +5 -5
@@ -123,7 +123,7 @@ pci_read_config(struct kobject *kobj, chif ((off &1 ) && size) {u8 val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); data[off - init_off] = val; off++; size--;
@@ -140,7 +140,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {u32 val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; data[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -160,7 +160,7 @@ pci_read_config(struct kobject *kobj, chif (size >0 ) {u8 val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); data[off - init_off] = val; off++; --size;
@@ -185,7 +185,7 @@ pci_write_config(struct kobject *kobj, c }if ((off &1 ) && size) { -pci_write_config_byte (dev, off, data[off - init_off]); +pci_user_write_config_byte (dev, off, data[off - init_off]); off++; size--; }
@@ -217,7 +217,7 @@ pci_write_config(struct kobject *kobj, c }if (size) { -pci_write_config_byte (dev, off, data[off - init_off]); +pci_user_write_config_byte (dev, off, data[off - init_off]); off++; --size; }
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist_again +14 -14
@@ -80,7 +80,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -89,7 +89,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -98,7 +98,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -107,7 +107,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -116,7 +116,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -151,7 +151,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -160,7 +160,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -169,7 +169,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -178,7 +178,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -187,7 +187,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -478,10 +478,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist_again +7 -7
@@ -13,7 +13,7 @@#include <linux/smp_lock.h> #include <linux/syscalls.h> #include <asm/uaccess.h> - +#include "pci.h" asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +6 -1
@@ -556,7 +556,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_ucfg_access:1; /* userspace 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? */
@@ -897,6 +898,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
@@ -947,6 +950,8 @@ static inline void pci_unregister_driverstatic inline int pci_find_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline int pci_find_ext_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline const struct pci_device_id *pci_match_device (const struct pci_device_id *ids,const struct pci_dev *dev) {return NULL ; } +static inline void pci_block_user_cfg_access (struct pci_dev *dev) { } +static inline void pci_unblock_user_cfg_access (struct pci_dev *dev) { }/* Power management related routines */ static inline int pci_save_state (struct pci_dev *dev) {return 0 ; }
drivers/pci/pci.h renamed from drivers/pci/pci.h~pci_block_user_config_io_during_bist_again +7 -0
@@ -11,6 +11,13 @@ extern int pci_bus_alloc_resource(structvoid (*alignf)(void *,struct resource *,unsigned long ,unsigned long ), void *alignf_data); +extern int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +extern int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +extern int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +extern int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +extern int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +extern int pci_user_write_config_dword (struct pci_dev *dev,int where,u32 val); +/* PCI /proc functions */ #ifdef CONFIG_PROC_FS extern int pci_proc_attach_device (struct pci_dev *dev);
_
Brian King
maintainerPATCH 1/2 pci: Block config access during BIST (resend)
Quoted 10 lines · Brian King
Quoted 9 lines · Greg KH
Quoted 4 lines
Here is an updated patch which will now fail writes to config space while the device is blocked. I have also fixed up the caching to return the correct data and tested it on both little endian and big endian machines.
Applied, thanks.
greg k-h
Greg,
This patch appears to have been dropped. Please apply.
Thanks
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
linux-2.6-bjking1/drivers/pci/access.c | 86 ++++++++++++++++++++++++++++++ linux-2.6-bjking1/drivers/pci/pci-sysfs.c | 20 +++--- linux-2.6-bjking1/drivers/pci/pci.h | 7 ++ linux-2.6-bjking1/drivers/pci/proc.c | 28 ++++----- linux-2.6-bjking1/drivers/pci/syscall.c | 14 ++-- linux-2.6-bjking1/include/linux/pci.h | 5 + 6 files changed, 129 insertions(+), 31 deletions(-)
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +86 -0
@@ -60,3 +60,89 @@ 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); + +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) \ +{ \ + 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, \ + 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); \ + *val = (type)data; \ + return ret; \ +} + +#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, \ + pos, sizeof(type), val); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +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) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL_GPL (pci_block_user_cfg_access); + +/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @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; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL_GPL (pci_unblock_user_cfg_access);
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist_again +10 -10
@@ -126,7 +126,7 @@ pci_read_config(struct kobject *kobj, chif ((off &1 ) && size) {u8 val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); data[off - init_off] = val; off++; size--;
@@ -134,7 +134,7 @@ pci_read_config(struct kobject *kobj, chif ((off &3 ) && size >2 ) {u16 val; -pci_read_config_word (dev, off, &val); +pci_user_read_config_word (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; off +=2 ;
@@ -143,7 +143,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {u32 val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; data[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -154,7 +154,7 @@ pci_read_config(struct kobject *kobj, chif (size >=2 ) {u16 val; -pci_read_config_word (dev, off, &val); +pci_user_read_config_word (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; off +=2 ;
@@ -163,7 +163,7 @@ pci_read_config(struct kobject *kobj, chif (size >0 ) {u8 val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); data[off - init_off] = val; off++; --size;
@@ -188,7 +188,7 @@ pci_write_config(struct kobject *kobj, c }if ((off &1 ) && size) { -pci_write_config_byte (dev, off, data[off - init_off]); +pci_user_write_config_byte (dev, off, data[off - init_off]); off++; size--; }
@@ -196,7 +196,7 @@ pci_write_config(struct kobject *kobj, cif ((off &3 ) && size >2 ) {u16 val = data[off - init_off]; val |= (u16 ) data[off - init_off +1 ] <<8 ; -pci_write_config_word (dev, off, val); +pci_user_write_config_word (dev, off, val); off +=2 ; size -=2 ; }
@@ -206,7 +206,7 @@ pci_write_config(struct kobject *kobj, c val |= (u32 ) data[off - init_off +1 ] <<8 ; val |= (u32 ) data[off - init_off +2 ] <<16 ; val |= (u32 ) data[off - init_off +3 ] <<24 ; -pci_write_config_dword (dev, off, val); +pci_user_write_config_dword (dev, off, val); off +=4 ; size -=4 ; }
@@ -214,13 +214,13 @@ pci_write_config(struct kobject *kobj, cif (size >=2 ) {u16 val = data[off - init_off]; val |= (u16 ) data[off - init_off +1 ] <<8 ; -pci_write_config_word (dev, off, val); +pci_user_write_config_word (dev, off, val); off +=2 ; size -=2 ; }if (size) { -pci_write_config_byte (dev, off, data[off - init_off]); +pci_user_write_config_byte (dev, off, data[off - init_off]); off++; --size; }
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist_again +14 -14
@@ -80,7 +80,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -89,7 +89,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -98,7 +98,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -107,7 +107,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -116,7 +116,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -151,7 +151,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -160,7 +160,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -169,7 +169,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -178,7 +178,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -187,7 +187,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -484,10 +484,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist_again +7 -7
@@ -13,7 +13,7 @@#include <linux/smp_lock.h> #include <linux/syscalls.h> #include <asm/uaccess.h> - +#include "pci.h" asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +5 -0
@@ -557,6 +557,7 @@ struct pci_dev { unsigned int is_enabled:1; /* pci_enable_device has been called */ 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 */u32 saved_config_space[16 ];/* config space saved at suspend time */ struct bin_attribute *rom_attr;/* attribute descriptor for sysfs ROM entry */
@@ -912,6 +913,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
@@ -962,6 +965,8 @@ static inline void pci_unregister_driverstatic inline int pci_find_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline int pci_find_ext_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline const struct pci_device_id *pci_match_device (const struct pci_device_id *ids,const struct pci_dev *dev) {return NULL ; } +static inline void pci_block_user_cfg_access (struct pci_dev *dev) { } +static inline void pci_unblock_user_cfg_access (struct pci_dev *dev) { }/* Power management related routines */ static inline int pci_save_state (struct pci_dev *dev) {return 0 ; }
drivers/pci/pci.h renamed from drivers/pci/pci.h~pci_block_user_config_io_during_bist_again +7 -0
@@ -15,6 +15,13 @@ extern int pci_bus_alloc_resource(structextern int (*platform_pci_choose_state)(struct pci_dev *dev,pm_message_t state);extern int (*platform_pci_set_power_state)(struct pci_dev *dev,pci_power_t state); +extern int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +extern int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +extern int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +extern int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +extern int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +extern int pci_user_write_config_dword (struct pci_dev *dev,int where,u32 val); +/* PCI /proc functions */ #ifdef CONFIG_PROC_FS extern int pci_proc_attach_device (struct pci_dev *dev);
_
Brian King
maintainerRe: [PATCH 2/2] ipr: Block config access during BIST (resend)
Greg,
Please apply along with the previous pci patch.
Thanks
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
IPR scsi adapter 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 makes use of some newly added PCI layer APIs that allow for protection from userspace accessing config space of a device in scenarios such as this.
---
linux-2.6-bjking1/drivers/scsi/ipr.c | 2 ++ 1 files changed, 2 insertions(+)
drivers/scsi/ipr.c renamed from drivers/scsi/ipr.c~ipr_block_user_config_io_during_bist +2 -0
@@ -4944,6 +4944,7 @@ static int ipr_reset_restore_cfg_space(sint rc;ENTER ; +pci_unblock_user_cfg_access (ioa_cfg->pdev); rc =pci_restore_state (ioa_cfg->pdev);if (rc !=PCIBIOS_SUCCESSFUL ) {
@@ -4998,6 +4999,7 @@ static int ipr_reset_start_bist(struct iint rc;ENTER ; +pci_block_user_cfg_access (ioa_cfg->pdev); rc =pci_write_config_byte (ioa_cfg->pdev,PCI_BIST ,PCI_BIST_START );if (rc !=PCIBIOS_SUCCESSFUL ) {
_
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 130
Quoted 9 lines · Brian King
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +8
-0+void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags);
Are you sure the locking in here is meaningful? All it will really do is give you a couple of barriers.
Brian King
maintainerQuoted 14 lines · Andrew Morton
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 130
Quoted 8 lines · Brian King
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +8
-0+void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags);Are you sure the locking in here is meaningful? All it will really do is give you a couple of barriers.
Actually, it is meaningful. It synchronizes the blocking of pci config accesses with other pci config accesses that may be going on when this function is called. Without the locking, the API cannot guarantee that no further user initiated PCI config accesses will be initiated after this function is called.
Brian
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
Quoted 16 lines · Brian King
Quoted 14 lines · Andrew Morton
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 130
Quoted 8 lines · Brian King
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +8
-0+void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags);Are you sure the locking in here is meaningful? All it will really do is give you a couple of barriers.
Quoted 5 lines
Actually, it is meaningful. It synchronizes the blocking of pci config accesses with other pci config accesses that may be going on when this function is called. Without the locking, the API cannot guarantee that no further user initiated PCI config accesses will be initiated after this function is called.
I don't have the impression you understood what Andrew wrote. dev->block_ucfg_access = 1 is essentially an atomic operation. AFAIK, Use of the pci_lock doesn't solve any race conditions that mb() wouldn't solve.
If you had:
spin_lock_irqsave(&pci_lock, flags); pci_save_state(dev); dev->block_ucfg_access = 1; spin_unlock_irqrestore(&pci_lock, flags);
Then I could buy your arguement since the flag now implies we need to atomically save state and set the flag.
grant
Quoted 23 lines · Grant Grundler
Quoted 16 lines · Brian King
Quoted 14 lines · Andrew Morton
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 130
Quoted 8 lines · Brian King
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +8
-0+void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags);Are you sure the locking in here is meaningful? All it will really do is give you a couple of barriers.
Quoted 5 lines
Actually, it is meaningful. It synchronizes the blocking of pci config accesses with other pci config accesses that may be going on when this function is called. Without the locking, the API cannot guarantee that no further user initiated PCI config accesses will be initiated after this function is called.
Quoted 4 lines
I don't have the impression you understood what Andrew wrote.
dev->block_ucfg_access= 1 is essentially an atomic operation. AFAIK, Use of thepci_lockdoesn't solve any race conditions thatmb()wouldn't solve.
Think about it. Taking the lock ensures that we don't do the assignment (dev->block_ucfg_access = 1) while any other cpu has the pci_lock. In other words, the reason for taking the lock is so that we wait until nobody else is doing an access, not to make others wait.
Quoted 8 lines
If you had:
spin_lock_irqsave(&pci_lock, flags); pci_save_state(dev); dev->block_ucfg_access = 1; spin_unlock_irqrestore(&pci_lock, flags);Then I could buy your arguement since the flag now implies we need to atomically save state and set the flag.
That's probably a good thing to do to.
Paul.
Quoted 4 lines · Paul Mackerras
Think about it. Taking the lock ensures that we don't do the assignment (
dev->block_ucfg_access= 1) while any other cpu has thepci_lock. In other words, the reason for taking the lock is so that we wait until nobody else is doing an access, not to make others wait.
The block_ucfg_access field is only used when making the choice to use saved state or call the PCI bus cfg accessor.
I don't what problem waiting solves here since any CPU already accessing real cfg space will finish what they are doing anyway. ie they already made the choice to access real cfg space.
We just need to make sure successive accesses to cfg space for this device only access the saved state. For that, a memory barrier around all uses of block_ucfg_access should be sufficient. Or do you think I'm still drinking the wrong color cool-aid?
Quoted 10 lines
Quoted 8 lines
If you had:
spin_lock_irqsave(&pci_lock, flags); pci_save_state(dev); dev->block_ucfg_access = 1; spin_unlock_irqrestore(&pci_lock, flags);Then I could buy your arguement since the flag now implies we need to atomically save state and set the flag.
That's probably a good thing to do to.
One needs to verify pci_lock isn't acquired in pci_save_state() (or some other obvious dead lock).
It would make sense to block pci cfg *writes* to that device while we save the state.
grant
Brian King
maintainerQuoted 8 lines · Grant Grundler
Quoted 4 lines · Paul Mackerras
Think about it. Taking the lock ensures that we don't do the assignment (
dev->block_ucfg_access= 1) while any other cpu has thepci_lock. In other words, the reason for taking the lock is so that we wait until nobody else is doing an access, not to make others wait.
Quoted 9 lines
The
block_ucfg_accessfield is only used when making the choice to use saved state or call the PCI bus cfg accessor.
I don't what problem waiting solves here since any CPU already accessing real cfg space will finish what they are doing anyway. ie they already made the choice to access real cfg space.
We just need to make sure successive accesses to cfg space for this device only access the saved state. For that, a memory barrier around all uses ofblock_ucfg_accessshould be sufficient. Or do you think I'm still drinking the wrong color cool-aid?
Without the locking, we introduce a race condition.
CPU 0 CPU 1
pci_block_user_cfg_access
pci_save_statepci_read_user_config_space
check block_ucfg_access
set block_ucfg_access
other code that puts the device in a state such that it cannot handle read config i/o, such as running BIST.
pci read config
In this scenario, If the real read on the left happens after the flag is set to block user config accesses, then the thread that set the flag could go off and start BIST or do something else to put the pci device in a state where it cannot accept real config I/O and we end up with a target abort, which is exactly what this patch is attempting to fix.
Granted, for the specific usage scenario in ipr, where I am using this to block config space over BIST, I use a pci config write to start BIST, which would end up being a point of synchronization, but that seems a bit non-obvious and limits how the patch can be used by others...
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
If you had:
spin_lock_irqsave(&pci_lock, flags); pci_save_state(dev); dev->block_ucfg_access = 1; spin_unlock_irqrestore(&pci_lock, flags);
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Then I could buy your arguement since the flag now implies we need to atomically save state and set the flag.
That's probably a good thing to do to.
One needs to verify
pci_lockisn't acquired inpci_save_state()(or some other obvious dead lock).
Unfortunately, it is... Every pci config access grabs the lock, so we would need to use some special code that did not acquire the lock in pci_save_state if we wanted to do such a thing.
Brian
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
On Sat, Sep 03, 2005 at 06:37:52PM -0500, Brian King wrote:
...
Quoted 14 lines
Without the locking, we introduce a race condition.
CPU 0 CPU 1
pci_block_user_cfg_access
pci_save_statepci_read_user_config_space
checkblock_ucfg_access
setblock_ucfg_access
other code that puts the device in a state such that it cannot handle read config i/o, such as running BIST.
Quoted 1 line
pci read config
Ok this is good example - I see what the problem is. You could use the following sequence too then:
pci_block_user_cfg_access pci_save_state block_ucfg_access = 1 mb() while (spin_is_locked(&pci_lock)) relax_cpu();
Think this is sufficient?
Quoted 4 lines
Granted, for the specific usage scenario in ipr, where I am using this to block config space over BIST, I use a pci config write to start BIST, which would end up being a point of synchronization, but that seems a bit non-obvious and limits how the patch can be used by others...
Yes, agreed.
grant
Brian King
maintainerQuoted 22 lines · Grant Grundler
On Sat, Sep 03, 2005 at 06:37:52PM -0500, Brian King wrote:
...Quoted 14 lines
Without the locking, we introduce a race condition.
CPU 0 CPU 1
pci_block_user_cfg_access
pci_save_statepci_read_user_config_space
checkblock_ucfg_access
setblock_ucfg_access
other code that puts the device in a state such that it cannot handle read config i/o, such as running BIST.Quoted 1 line
pci read config
Ok this is good example - I see what the problem is. You could use the following sequence too then:
Quoted 8 lines
pci_block_user_cfg_access pci_save_state block_ucfg_access = 1 mb() while (spin_is_locked(&pci_lock)) relax_cpu();Think this is sufficient?
That should work also. Here is an updated patch.
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
drivers/pci/access.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/pci-sysfs.c | 20 +++++------ drivers/pci/pci.h | 7 ++++ drivers/pci/proc.c | 28 ++++++++-------- drivers/pci/syscall.c | 14 ++++---- include/linux/pci.h | 5 ++ 6 files changed, 124 insertions(+), 31 deletions(-)
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +81 -0
@@ -60,3 +60,84 @@ 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); + +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) \ +{ \ + 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, \ + 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); \ + *val = (type)data; \ + return ret; \ +} + +#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, \ + pos, sizeof(type), val); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +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) +{ +pci_save_state (dev); + dev->block_ucfg_access =1 ; +mb (); +while (spin_is_locked (&pci_lock)) +cpu_relax (); +} +EXPORT_SYMBOL_GPL (pci_block_user_cfg_access); + +/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @dev: pci device struct +* +* This function allows userspace PCI config accesses to resume. +**/ +void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ + dev->block_ucfg_access =0 ; +} +EXPORT_SYMBOL_GPL (pci_unblock_user_cfg_access);
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist_again +10 -10
@@ -126,7 +126,7 @@ pci_read_config(struct kobject *kobj, chif ((off &1 ) && size) {u8 val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); data[off - init_off] = val; off++; size--;
@@ -134,7 +134,7 @@ pci_read_config(struct kobject *kobj, chif ((off &3 ) && size >2 ) {u16 val; -pci_read_config_word (dev, off, &val); +pci_user_read_config_word (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; off +=2 ;
@@ -143,7 +143,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {u32 val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; data[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -154,7 +154,7 @@ pci_read_config(struct kobject *kobj, chif (size >=2 ) {u16 val; -pci_read_config_word (dev, off, &val); +pci_user_read_config_word (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; off +=2 ;
@@ -163,7 +163,7 @@ pci_read_config(struct kobject *kobj, chif (size >0 ) {u8 val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); data[off - init_off] = val; off++; --size;
@@ -188,7 +188,7 @@ pci_write_config(struct kobject *kobj, c }if ((off &1 ) && size) { -pci_write_config_byte (dev, off, data[off - init_off]); +pci_user_write_config_byte (dev, off, data[off - init_off]); off++; size--; }
@@ -196,7 +196,7 @@ pci_write_config(struct kobject *kobj, cif ((off &3 ) && size >2 ) {u16 val = data[off - init_off]; val |= (u16 ) data[off - init_off +1 ] <<8 ; -pci_write_config_word (dev, off, val); +pci_user_write_config_word (dev, off, val); off +=2 ; size -=2 ; }
@@ -206,7 +206,7 @@ pci_write_config(struct kobject *kobj, c val |= (u32 ) data[off - init_off +1 ] <<8 ; val |= (u32 ) data[off - init_off +2 ] <<16 ; val |= (u32 ) data[off - init_off +3 ] <<24 ; -pci_write_config_dword (dev, off, val); +pci_user_write_config_dword (dev, off, val); off +=4 ; size -=4 ; }
@@ -214,13 +214,13 @@ pci_write_config(struct kobject *kobj, cif (size >=2 ) {u16 val = data[off - init_off]; val |= (u16 ) data[off - init_off +1 ] <<8 ; -pci_write_config_word (dev, off, val); +pci_user_write_config_word (dev, off, val); off +=2 ; size -=2 ; }if (size) { -pci_write_config_byte (dev, off, data[off - init_off]); +pci_user_write_config_byte (dev, off, data[off - init_off]); off++; --size; }
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist_again +14 -14
@@ -80,7 +80,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -89,7 +89,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -98,7 +98,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -107,7 +107,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -116,7 +116,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -151,7 +151,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -160,7 +160,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -169,7 +169,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -178,7 +178,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -187,7 +187,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -484,10 +484,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist_again +7 -7
@@ -13,7 +13,7 @@#include <linux/smp_lock.h> #include <linux/syscalls.h> #include <asm/uaccess.h> - +#include "pci.h" asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +5 -0
@@ -557,6 +557,7 @@ struct pci_dev { unsigned int is_enabled:1; /* pci_enable_device has been called */ 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 */u32 saved_config_space[16 ];/* config space saved at suspend time */ struct bin_attribute *rom_attr;/* attribute descriptor for sysfs ROM entry */
@@ -912,6 +913,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
@@ -962,6 +965,8 @@ static inline void pci_unregister_driverstatic inline int pci_find_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline int pci_find_ext_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline const struct pci_device_id *pci_match_device (const struct pci_device_id *ids,const struct pci_dev *dev) {return NULL ; } +static inline void pci_block_user_cfg_access (struct pci_dev *dev) { } +static inline void pci_unblock_user_cfg_access (struct pci_dev *dev) { }/* Power management related routines */ static inline int pci_save_state (struct pci_dev *dev) {return 0 ; }
drivers/pci/pci.h renamed from drivers/pci/pci.h~pci_block_user_config_io_during_bist_again +7 -0
@@ -15,6 +15,13 @@ extern int pci_bus_alloc_resource(structextern int (*platform_pci_choose_state)(struct pci_dev *dev,pm_message_t state);extern int (*platform_pci_set_power_state)(struct pci_dev *dev,pci_power_t state); +extern int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +extern int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +extern int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +extern int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +extern int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +extern int pci_user_write_config_dword (struct pci_dev *dev,int where,u32 val); +/* PCI /proc functions */ #ifdef CONFIG_PROC_FS extern int pci_proc_attach_device (struct pci_dev *dev);
_
Quoted 1 line · Brian King
That should work also. Here is an updated patch.
...
The code looks good...but it got me thinking.
...
Quoted 10 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +10
-0+void pci_block_user_cfg_access (struct pci_dev *dev) +{ +pci_save_state (dev); + dev->block_ucfg_access =1 ; +mb (); +while (spin_is_locked (&pci_lock)) +cpu_relax (); +} +EXPORT_SYMBOL_GPL (pci_block_user_cfg_access); +
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 142
Quoted 10 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +10
-0+/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @dev: pci device struct +* +* This function allows userspace PCI config accesses to resume. +**/ +void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ + dev->block_ucfg_access =0 ; +}
Shouldn't pci_unblock_user_cfg_access() have a similar construct as pci_block_user_cfg_access()?
I'm thinking we don't want to pull the rug out from under someone who is accessing the saved state, right?
Or does something else guarantee that?
It wasn't obvious from this diff alone.
thanks,
grant
Brian King
maintainerQuoted 26 lines · Grant Grundler
Quoted 10 lines · Brian King
(quoted diff) +10
-0+void pci_block_user_cfg_access(struct pci_dev *dev) +{ + pci_save_state(dev); + dev->block_ucfg_access = 1; + mb(); + while (spin_is_locked(&pci_lock)) + cpu_relax(); +} +EXPORT_SYMBOL_GPL(pci_block_user_cfg_access); +Review
Quoted 10 lines
(quoted diff) +10
-0+/** + * pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes + * @dev: pci device struct + * + * This function allows userspace PCI config accesses to resume. + **/ +void pci_unblock_user_cfg_access(struct pci_dev *dev) +{ + dev->block_ucfg_access = 0; +}Shouldn't
pci_unblock_user_cfg_access()have a similar construct aspci_block_user_cfg_access()?
Quoted 5 lines
I'm thinking we don't want to pull the rug out from under someone who is accessing the saved state, right?
Or does something else guarantee that?It wasn't obvious from this diff alone.
Sounds reasonable enough. Updated patch attached.
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
linux-2.6-bjking1/drivers/pci/access.c | 84 ++++++++++++++++++++++++++++++ linux-2.6-bjking1/drivers/pci/pci-sysfs.c | 20 +++---- linux-2.6-bjking1/drivers/pci/pci.h | 7 ++ linux-2.6-bjking1/drivers/pci/proc.c | 28 +++++----- linux-2.6-bjking1/drivers/pci/syscall.c | 14 ++--- linux-2.6-bjking1/include/linux/pci.h | 5 + 6 files changed, 127 insertions(+), 31 deletions(-)
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +84 -0
@@ -60,3 +60,87 @@ 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); + +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) \ +{ \ + 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, \ + 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); \ + *val = (type)data; \ + return ret; \ +} + +#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, \ + pos, sizeof(type), val); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +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) +{ +pci_save_state (dev); + dev->block_ucfg_access =1 ; +mb (); +while (spin_is_locked (&pci_lock)) +cpu_relax (); +} +EXPORT_SYMBOL_GPL (pci_block_user_cfg_access); + +/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @dev: pci device struct +* +* This function allows userspace PCI config accesses to resume. +**/ +void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ + dev->block_ucfg_access =0 ; +mb (); +while (spin_is_locked (&pci_lock)) +cpu_relax (); +} +EXPORT_SYMBOL_GPL (pci_unblock_user_cfg_access);
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist_again +10 -10
@@ -126,7 +126,7 @@ pci_read_config(struct kobject *kobj, chif ((off &1 ) && size) {u8 val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); data[off - init_off] = val; off++; size--;
@@ -134,7 +134,7 @@ pci_read_config(struct kobject *kobj, chif ((off &3 ) && size >2 ) {u16 val; -pci_read_config_word (dev, off, &val); +pci_user_read_config_word (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; off +=2 ;
@@ -143,7 +143,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {u32 val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; data[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -154,7 +154,7 @@ pci_read_config(struct kobject *kobj, chif (size >=2 ) {u16 val; -pci_read_config_word (dev, off, &val); +pci_user_read_config_word (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; off +=2 ;
@@ -163,7 +163,7 @@ pci_read_config(struct kobject *kobj, chif (size >0 ) {u8 val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); data[off - init_off] = val; off++; --size;
@@ -188,7 +188,7 @@ pci_write_config(struct kobject *kobj, c }if ((off &1 ) && size) { -pci_write_config_byte (dev, off, data[off - init_off]); +pci_user_write_config_byte (dev, off, data[off - init_off]); off++; size--; }
@@ -196,7 +196,7 @@ pci_write_config(struct kobject *kobj, cif ((off &3 ) && size >2 ) {u16 val = data[off - init_off]; val |= (u16 ) data[off - init_off +1 ] <<8 ; -pci_write_config_word (dev, off, val); +pci_user_write_config_word (dev, off, val); off +=2 ; size -=2 ; }
@@ -206,7 +206,7 @@ pci_write_config(struct kobject *kobj, c val |= (u32 ) data[off - init_off +1 ] <<8 ; val |= (u32 ) data[off - init_off +2 ] <<16 ; val |= (u32 ) data[off - init_off +3 ] <<24 ; -pci_write_config_dword (dev, off, val); +pci_user_write_config_dword (dev, off, val); off +=4 ; size -=4 ; }
@@ -214,13 +214,13 @@ pci_write_config(struct kobject *kobj, cif (size >=2 ) {u16 val = data[off - init_off]; val |= (u16 ) data[off - init_off +1 ] <<8 ; -pci_write_config_word (dev, off, val); +pci_user_write_config_word (dev, off, val); off +=2 ; size -=2 ; }if (size) { -pci_write_config_byte (dev, off, data[off - init_off]); +pci_user_write_config_byte (dev, off, data[off - init_off]); off++; --size; }
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist_again +14 -14
@@ -80,7 +80,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -89,7 +89,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -98,7 +98,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -107,7 +107,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -116,7 +116,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -151,7 +151,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -160,7 +160,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -169,7 +169,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -178,7 +178,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -187,7 +187,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -484,10 +484,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist_again +7 -7
@@ -13,7 +13,7 @@#include <linux/smp_lock.h> #include <linux/syscalls.h> #include <asm/uaccess.h> - +#include "pci.h" asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +5 -0
@@ -557,6 +557,7 @@ struct pci_dev { unsigned int is_enabled:1; /* pci_enable_device has been called */ 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 */u32 saved_config_space[16 ];/* config space saved at suspend time */ struct bin_attribute *rom_attr;/* attribute descriptor for sysfs ROM entry */
@@ -912,6 +913,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
@@ -962,6 +965,8 @@ static inline void pci_unregister_driverstatic inline int pci_find_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline int pci_find_ext_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline const struct pci_device_id *pci_match_device (const struct pci_device_id *ids,const struct pci_dev *dev) {return NULL ; } +static inline void pci_block_user_cfg_access (struct pci_dev *dev) { } +static inline void pci_unblock_user_cfg_access (struct pci_dev *dev) { }/* Power management related routines */ static inline int pci_save_state (struct pci_dev *dev) {return 0 ; }
drivers/pci/pci.h renamed from drivers/pci/pci.h~pci_block_user_config_io_during_bist_again +7 -0
@@ -15,6 +15,13 @@ extern int pci_bus_alloc_resource(structextern int (*platform_pci_choose_state)(struct pci_dev *dev,pm_message_t state);extern int (*platform_pci_set_power_state)(struct pci_dev *dev,pci_power_t state); +extern int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +extern int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +extern int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +extern int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +extern int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +extern int pci_user_write_config_dword (struct pci_dev *dev,int where,u32 val); +/* PCI /proc functions */ #ifdef CONFIG_PROC_FS extern int pci_proc_attach_device (struct pci_dev *dev);
_
Quoted 10 lines · Grant Grundler
Ok this is good example - I see what the problem is. You could use the following sequence too then:
pci_block_user_cfg_access pci_save_state block_ucfg_access = 1 mb() while (spin_is_locked(&pci_lock)) relax_cpu();Think this is sufficient?
Maybe, but it seems like a bad idea to me. It's longer, it's less obvious what's happening, and it precludes the sorts of optimization that we do on ppc64 where a cpu that is waiting for a lock can tell give its time slice to the cpu that is holding the lock (on systems where the hypervisor time-slices multiple virtual cpus on one physical cpu).
What's wrong with just doing spin_lock/spin_unlock?
Paul.
Quoted 2 lines · Paul Mackerras
Maybe, but it seems like a bad idea to me. It's longer, it's less obvious what's happening,
I would argue it more obvious. People looking at the code are immediately going to realize it was a deliberate choice to not use a spinlock.
Quoted 5 lines
and it precludes the sorts of optimization
that we do on ppc64 where a cpu that is waiting for a lock can tell give its time slice to the cpu that is holding the lock (on systems where the hypervisor time-slices multiple virtual cpus on one physical cpu).
relax_cpu() doesn't do that?
Quoted 1 line
What's wrong with just doing spin_lock/spin_unlock?
it's not wrong - just misleading IMHO. There is no "critical section" in that particular chunk of code.
If relax_cpu doesn't allow time-slice donation, then I guess spinlock/unlock with only a comment inside it explain why would be ok too.
thanks,
grant
Quoted 3 lines · Grant Grundler
I would argue it more obvious. People looking at the code are immediately going to realize it was a deliberate choice to not use a spinlock.
It achieves exactly the same effect as spin_lock/spin_unlock, just more verbosely. :)
Quoted 7 lines
Quoted 5 lines
and it precludes the sorts of optimization
that we do on ppc64 where a cpu that is waiting for a lock can tell give its time slice to the cpu that is holding the lock (on systems where the hypervisor time-slices multiple virtual cpus on one physical cpu).
relax_cpu()doesn't do that?
No, how can it, when it doesn't get told which virtual cpu to yield to? spin_lock knows which virtual cpu to yield to because we store that in the lock variable.
Quoted 4 lines
Quoted 1 line
What's wrong with just doing spin_lock/spin_unlock?
it's not wrong - just misleading IMHO. There is no "critical section" in that particular chunk of code.
Other code is using the lock to ensure the atomicity of a compound action, which involves the testing the flag and taking some action based on the value of the flag. We take the lock to preserve that atomicity. Locks are often used to make a set of compound actions atomic with respect to each other, which is what we're doing here.
Quoted 3 lines
If
relax_cpudoesn't allow time-slice donation, then I guess spinlock/unlock with only a comment inside it explain why would be ok too.
Preferable, in fact. :)
Paul.
Quoted 8 lines · Paul Mackerras
Quoted 3 lines · Grant Grundler
I would argue it more obvious. People looking at the code are immediately going to realize it was a deliberate choice to not use a spinlock.
It achieves exactly the same effect as spin_lock/spin_unlock, just more verbosely. :)
Not exactly identical. spin_try_lock() doesn't attempt to acquire the lock and thus force exclusive access to the cacheline. Other than that, I agree with you.
I don't see a problem with being verbose here since putting a spinlock around something that's already atomic (assignment) even caught Andrew's attention.
Quoted 5 lines
Quoted 1 line
relax_cpu()doesn't do that?No, how can it, when it doesn't get told which virtual cpu to yield to?
spin_lockknows which virtual cpu to yield to because we store that in the lock variable.
Ah ok. I was expecting relax_cpu() to just pick a "random" different one.
:^)
Quoted 8 lines
Quoted 2 lines
it's not wrong - just misleading IMHO. There is no "critical section" in that particular chunk of code.
Other code is using the lock to ensure the atomicity of a compound action, which involves the testing the flag and taking some action based on the value of the flag. We take the lock to preserve that atomicity. Locks are often used to make a set of compound actions atomic with respect to each other, which is what we're doing here.
Yes.
We agree this chunk only needs to wait until the lock is released. Other critical sections need to acquire/release the lock.
Quoted 5 lines
Quoted 3 lines
If
relax_cpudoesn't allow time-slice donation, then I guess spinlock/unlock with only a comment inside it explain why would be ok too.Preferable, in fact. :)
Ok. I was just suggesting the alternative since Andrew (correctly) questioned the use of a spinlock and it didn't look right to me either.
thanks,
grant
Brian King
maintainerQuoted 13 lines · Grant Grundler
Quoted 9 lines · Paul Mackerras
Quoted 3 lines · Grant Grundler
I would argue it more obvious. People looking at the code are immediately going to realize it was a deliberate choice to not use a spinlock.
It achieves exactly the same effect as spin_lock/spin_unlock, just more verbosely. :)
Quoted 7 lines
Not exactly identical.
spin_try_lock()doesn't attempt to acquire the lock and thus force exclusive access to the cacheline. Other than that, I agree with you.I don't see a problem with being verbose here since putting a spinlock around something that's already atomic (assignment) even caught Andrew's attention.
I reverted the patch to use a spinlock and added a comment. How does this look?
Signature · 3 lines
Brian King eServer Storage I/O IBM Linux Technology Center
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.
---
drivers/pci/access.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/pci-sysfs.c | 20 +++++----- drivers/pci/pci.h | 7 +++ drivers/pci/proc.c | 28 +++++++-------- drivers/pci/syscall.c | 14 +++---- include/linux/pci.h | 5 ++ 6 files changed, 132 insertions(+), 31 deletions(-)
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +89 -0
@@ -60,3 +60,92 @@ 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); + +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) \ +{ \ + 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, \ + 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); \ + *val = (type)data; \ + return ret; \ +} + +#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, \ + pos, sizeof(type), val); \ + spin_unlock_irqrestore(&pci_lock, flags); \ + return ret; \ +} + +PCI_USER_READ_CONFIG(byte, u8) +PCI_USER_READ_CONFIG(word, u16) +PCI_USER_READ_CONFIG(dword, u32) +PCI_USER_WRITE_CONFIG(byte, u8) +PCI_USER_WRITE_CONFIG(word, u16) +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) +{ +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); +} +EXPORT_SYMBOL_GPL (pci_block_user_cfg_access); + +/** +* pci_unblock_user_cfg_access - Unblock userspace PCI config reads/writes +* @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); +} +EXPORT_SYMBOL_GPL (pci_unblock_user_cfg_access);
drivers/pci/pci-sysfs.c renamed from drivers/pci/pci-sysfs.c~pci_block_user_config_io_during_bist_again +10 -10
@@ -126,7 +126,7 @@ pci_read_config(struct kobject *kobj, chif ((off &1 ) && size) {u8 val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); data[off - init_off] = val; off++; size--;
@@ -134,7 +134,7 @@ pci_read_config(struct kobject *kobj, chif ((off &3 ) && size >2 ) {u16 val; -pci_read_config_word (dev, off, &val); +pci_user_read_config_word (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; off +=2 ;
@@ -143,7 +143,7 @@ pci_read_config(struct kobject *kobj, chwhile (size >3 ) {u32 val; -pci_read_config_dword (dev, off, &val); +pci_user_read_config_dword (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; data[off - init_off +2 ] = (val >>16 ) &0xff ;
@@ -154,7 +154,7 @@ pci_read_config(struct kobject *kobj, chif (size >=2 ) {u16 val; -pci_read_config_word (dev, off, &val); +pci_user_read_config_word (dev, off, &val); data[off - init_off] = val &0xff ; data[off - init_off +1 ] = (val >>8 ) &0xff ; off +=2 ;
@@ -163,7 +163,7 @@ pci_read_config(struct kobject *kobj, chif (size >0 ) {u8 val; -pci_read_config_byte (dev, off, &val); +pci_user_read_config_byte (dev, off, &val); data[off - init_off] = val; off++; --size;
@@ -188,7 +188,7 @@ pci_write_config(struct kobject *kobj, c }if ((off &1 ) && size) { -pci_write_config_byte (dev, off, data[off - init_off]); +pci_user_write_config_byte (dev, off, data[off - init_off]); off++; size--; }
@@ -196,7 +196,7 @@ pci_write_config(struct kobject *kobj, cif ((off &3 ) && size >2 ) {u16 val = data[off - init_off]; val |= (u16 ) data[off - init_off +1 ] <<8 ; -pci_write_config_word (dev, off, val); +pci_user_write_config_word (dev, off, val); off +=2 ; size -=2 ; }
@@ -206,7 +206,7 @@ pci_write_config(struct kobject *kobj, c val |= (u32 ) data[off - init_off +1 ] <<8 ; val |= (u32 ) data[off - init_off +2 ] <<16 ; val |= (u32 ) data[off - init_off +3 ] <<24 ; -pci_write_config_dword (dev, off, val); +pci_user_write_config_dword (dev, off, val); off +=4 ; size -=4 ; }
@@ -214,13 +214,13 @@ pci_write_config(struct kobject *kobj, cif (size >=2 ) {u16 val = data[off - init_off]; val |= (u16 ) data[off - init_off +1 ] <<8 ; -pci_write_config_word (dev, off, val); +pci_user_write_config_word (dev, off, val); off +=2 ; size -=2 ; }if (size) { -pci_write_config_byte (dev, off, data[off - init_off]); +pci_user_write_config_byte (dev, off, data[off - init_off]); off++; --size; }
drivers/pci/proc.c renamed from drivers/pci/proc.c~pci_block_user_config_io_during_bist_again +14 -14
@@ -80,7 +80,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &1 ) && cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -89,7 +89,7 @@ proc_bus_pci_read(struct file *file, chaif ((pos &3 ) && cnt >2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -98,7 +98,7 @@ proc_bus_pci_read(struct file *file, chawhile (cnt >=4 ) {unsigned int val; -pci_read_config_dword (dev, pos, &val); +pci_user_read_config_dword (dev, pos, &val);__put_user (cpu_to_le32 (val), (unsigned int __user *) buf); buf +=4 ; pos +=4 ;
@@ -107,7 +107,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt >=2 ) {unsigned short val; -pci_read_config_word (dev, pos, &val); +pci_user_read_config_word (dev, pos, &val);__put_user (cpu_to_le16 (val), (unsigned short __user *) buf); buf +=2 ; pos +=2 ;
@@ -116,7 +116,7 @@ proc_bus_pci_read(struct file *file, chaif (cnt) {unsigned char val; -pci_read_config_byte (dev, pos, &val); +pci_user_read_config_byte (dev, pos, &val);__put_user (val, buf); buf++; pos++;
@@ -151,7 +151,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &1 ) && cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -160,7 +160,7 @@ proc_bus_pci_write(struct file *file, coif ((pos &3 ) && cnt >2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -169,7 +169,7 @@ proc_bus_pci_write(struct file *file, cowhile (cnt >=4 ) {unsigned int val;__get_user (val, (unsigned int __user *) buf); -pci_write_config_dword (dev, pos,le32_to_cpu (val)); +pci_user_write_config_dword (dev, pos,le32_to_cpu (val)); buf +=4 ; pos +=4 ; cnt -=4 ;
@@ -178,7 +178,7 @@ proc_bus_pci_write(struct file *file, coif (cnt >=2 ) {unsigned short val;__get_user (val, (unsigned short __user *) buf); -pci_write_config_word (dev, pos,le16_to_cpu (val)); +pci_user_write_config_word (dev, pos,le16_to_cpu (val)); buf +=2 ; pos +=2 ; cnt -=2 ;
@@ -187,7 +187,7 @@ proc_bus_pci_write(struct file *file, coif (cnt) {unsigned char val;__get_user (val, buf); -pci_write_config_byte (dev, pos, val); +pci_user_write_config_byte (dev, pos, val); buf++; pos++; cnt--;
@@ -484,10 +484,10 @@ static int show_dev_config(struct seq_fi drv =pci_dev_driver (dev); -pci_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); -pci_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); -pci_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); -pci_read_config_byte (dev,PCI_MAX_LAT , &max_lat); +pci_user_read_config_dword (dev,PCI_CLASS_REVISION , &class_rev); +pci_user_read_config_byte (dev,PCI_LATENCY_TIMER , &latency); +pci_user_read_config_byte (dev,PCI_MIN_GNT , &min_gnt); +pci_user_read_config_byte (dev,PCI_MAX_LAT , &max_lat);seq_printf (m," Bus %2d, device %3d, function %2d:\n" , dev->bus->number,PCI_SLOT (dev->devfn),PCI_FUNC (dev->devfn)); class =pci_class_name (class_rev >>16 );
drivers/pci/syscall.c renamed from drivers/pci/syscall.c~pci_block_user_config_io_during_bist_again +7 -7
@@ -13,7 +13,7 @@#include <linux/smp_lock.h> #include <linux/syscalls.h> #include <asm/uaccess.h> - +#include "pci.h" asmlinkage long sys_pciconfig_read(unsigned long bus, unsigned long dfn,
@@ -38,13 +38,13 @@ sys_pciconfig_read(unsigned long bus, unlock_kernel ();switch (len) {case 1 : - cfg_ret =pci_read_config_byte (dev, off, &byte); + cfg_ret =pci_user_read_config_byte (dev, off, &byte);break ;case 2 : - cfg_ret =pci_read_config_word (dev, off, &word); + cfg_ret =pci_user_read_config_word (dev, off, &word);break ;case 4 : - cfg_ret =pci_read_config_dword (dev, off, &dword); + cfg_ret =pci_user_read_config_dword (dev, off, &dword);break ;default : err = -EINVAL ;
@@ -112,7 +112,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (byte, (u8 __user *)buf);if (err)break ; - err =pci_write_config_byte (dev, off, byte); + err =pci_user_write_config_byte (dev, off, byte);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -121,7 +121,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (word, (u16 __user *)buf);if (err)break ; - err =pci_write_config_word (dev, off, word); + err =pci_user_write_config_word (dev, off, word);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
@@ -130,7 +130,7 @@ sys_pciconfig_write(unsigned long bus, u err =get_user (dword, (u32 __user *)buf);if (err)break ; - err =pci_write_config_dword (dev, off, dword); + err =pci_user_write_config_dword (dev, off, dword);if (err !=PCIBIOS_SUCCESSFUL ) err = -EIO ;break ;
include/linux/pci.h renamed from include/linux/pci.h~pci_block_user_config_io_during_bist_again +5 -0
@@ -557,6 +557,7 @@ struct pci_dev { unsigned int is_enabled:1; /* pci_enable_device has been called */ 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 */u32 saved_config_space[16 ];/* config space saved at suspend time */ struct bin_attribute *rom_attr;/* attribute descriptor for sysfs ROM entry */
@@ -912,6 +913,8 @@ extern void pci_disable_msix(struct pci_extern void msi_remove_pci_irq_vectors (struct pci_dev *dev);#endif +extern void pci_block_user_cfg_access (struct pci_dev *dev); +extern void pci_unblock_user_cfg_access (struct pci_dev *dev);#endif /* CONFIG_PCI */ /* Include architecture-dependent settings and functions */
@@ -962,6 +965,8 @@ static inline void pci_unregister_driverstatic inline int pci_find_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline int pci_find_ext_capability (struct pci_dev *dev,int cap) {return 0 ; }static inline const struct pci_device_id *pci_match_device (const struct pci_device_id *ids,const struct pci_dev *dev) {return NULL ; } +static inline void pci_block_user_cfg_access (struct pci_dev *dev) { } +static inline void pci_unblock_user_cfg_access (struct pci_dev *dev) { }/* Power management related routines */ static inline int pci_save_state (struct pci_dev *dev) {return 0 ; }
drivers/pci/pci.h renamed from drivers/pci/pci.h~pci_block_user_config_io_during_bist_again +7 -0
@@ -15,6 +15,13 @@ extern int pci_bus_alloc_resource(structextern int (*platform_pci_choose_state)(struct pci_dev *dev,pm_message_t state);extern int (*platform_pci_set_power_state)(struct pci_dev *dev,pci_power_t state); +extern int pci_user_read_config_byte (struct pci_dev *dev,int where,u8 *val); +extern int pci_user_read_config_word (struct pci_dev *dev,int where,u16 *val); +extern int pci_user_read_config_dword (struct pci_dev *dev,int where,u32 *val); +extern int pci_user_write_config_byte (struct pci_dev *dev,int where,u8 val); +extern int pci_user_write_config_word (struct pci_dev *dev,int where,u16 val); +extern int pci_user_write_config_dword (struct pci_dev *dev,int where,u32 val); +/* PCI /proc functions */ #ifdef CONFIG_PROC_FS extern int pci_proc_attach_device (struct pci_dev *dev);
_
Quoted 2 lines · Brian King
I reverted the patch to use a spinlock and added a comment. How does this look?
Fine with me. Ball is in akpm/Paul's court.
grant
Quoted 17 lines · Matthew Wilcox
Quoted 16 lines · Brian King
Quoted 14 lines · Greg KH
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword)
Quoted 10 lines · Brian King
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +10
-0+void pci_block_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +pci_save_state (dev); +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =1 ; +spin_unlock_irqrestore (&pci_lock, flags); +} +EXPORT_SYMBOL (pci_block_user_cfg_access);
EXPORT_SYMBOL_GPL()please?
Quoted 5 lines
Quoted 1 line
Ok.
I'm not entirely convinced these should be GPL-only exports. Basically, this is saying that any driver for a device that has this problem must be GPLd. I think that's a firmer stance than Linus had in mind originally.
"originally"? These are new functions added to the PCI core. I think that any driver that wants to use this functionality had better be released under the GPL as what they are wanting to do is a "new" thing.
That's why I prefer all new exports to be marked _GPL.
thanks,
greg k-h
Quoted 11 lines
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 136
Quoted 8 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +8
-0+void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +}If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
This has been discussed a lot already. I think we might as well let the thing fail in random and odd ways, as it's some pretty broken hardware anyway that wants this functionality :)
thanks,
greg k-h
Quoted 6 lines · Greg KH
Quoted 2 lines
If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
This has been discussed a lot already. I think we might as well let the thing fail in random and odd ways, as it's some pretty broken hardware anyway that wants this functionality :)
I don't agree that it's broken hardware, especially in the case where we use these for power managed devices (on pmac, or eventually embedded, where we can have PM be as drastic as completely removing power/clock from a device or that sort of thing).
But as I wrote earlier, I consider that in those cases, userland has no business writing to config space. If it does, then it has to be aware of the device beeing potentially offlined or that sort of thing and have proper interface to the kernel driver etc...
The reads coming from the cache, on the other hand, have a more legitimate use which are to let
- distro HW probing tool to actually see devices even when they
are power managed or BIST by their kernel driver
- crap like XFree86 to have a proper idea of what address ranges
are actually used on the bus, including devices that are power managed or BIST (*).
Ben.
(*) An unrelated note: It's not always legal to allocate thigns in PCI space or move devices around anyway. It's definitely not on logically partitionned PPC machines where HV won't let a partition access other IO ranges than the ones where the "allowed" devices for this partition were intially put by the firmware... Again, X should really be changed to just stop doing anything but "probing" on the bus at least with Linux. The only problem is the VGA enable story, but as we discussed earlier, that too should be dealt with in the kernel. I think we could integrate that cleanly in sysfs for PCI devices in the generic code btw.
Ben.
Quoted 12 lines · Matthew Wilcox
drivers/pci/access.c EXPORT_SYMBOL(pci_bus_read_config_dword) · line 136
Quoted 9 lines
drivers/pci/access.c renamed from drivers/pci/access.c~pci_block_user_config_io_during_bist_again +8
-0+void pci_unblock_user_cfg_access (struct pci_dev *dev) +{ +unsigned long flags; + +spin_lock_irqsave (&pci_lock, flags); + dev->block_ucfg_access =0 ; +spin_unlock_irqrestore (&pci_lock, flags); +}If we've done a write to config space while the adapter was blocked, shouldn't we replay those accesses at this point?
I think no. In fact, I would be ok returning errors on writes from userland. Need to do config space writes from userland is rare, must more than reads, and if whatever does it can't properly arbitrate with what's going on in the kernel, then it's broken.
Ben.
Quoted 4 lines · Andi Kleen
[email protected] writes:
I think it would be better to do this check higher level in the driver. IMHO pci_*_config should stay lean and fast low level functions without such baggage.
Userspace pci interfaces can also trigger this withour protection.