PCI PM: pci_save/restore_state improvements
RFC PATCH 6/6 PCI PM: pci_save/restore_state improvements
This patch makes some improvements to pci_save_state and pci_restore_state. Instead of saving and restoring all standard registers (even read-only ones), it only restores necessary registers. Also, the command register is handled more carefully. Let me know if I'm missing anything important.
drivers/pci/pm.c +20 -7
@@ -53,10 +53,13 @@*/ int pci_save_state (struct pci_dev *dev) { -int i; -/* XXX: 100% dword access ok here? */ -for (i =0 ; i <16 ; i++) -pci_read_config_dword (dev, i *4 ,&dev->saved_config_space[i]); +struct pci_dev_config * conf = &dev->saved_config; + +pci_read_config_word (dev,PCI_COMMAND , &conf->command); +pci_read_config_byte (dev,PCI_CACHE_LINE_SIZE , &conf->cacheline_size); +pci_read_config_byte (dev,PCI_LATENCY_TIMER , &conf->latency_timer); +pci_read_config_byte (dev,PCI_INTERRUPT_PIN , &conf->interrupt_line); +return 0 ; }
@@ -68,10 +71,20 @@*/ int pci_restore_state (struct pci_dev *dev) { -int i; +u16 command; +struct pci_dev_config * conf = &dev->saved_config; + + command = conf->command & ~(PCI_COMMAND_IO |PCI_COMMAND_MEMORY | +PCI_COMMAND_MASTER ); + +pci_write_config_word (dev,PCI_COMMAND , command); +pci_write_config_byte (dev,PCI_CACHE_LINE_SIZE , conf->cacheline_size); +pci_write_config_byte (dev,PCI_LATENCY_TIMER , conf->latency_timer); +pci_write_config_byte (dev,PCI_INTERRUPT_PIN , conf->interrupt_line); + +pci_restore_bars (dev); + -for (i =0 ; i <16 ; i++) -pci_write_config_dword (dev,i *4 , dev->saved_config_space[i]);return 0 ; }
include/linux/pci.h +8 -1
@@ -68,6 +68,13 @@#define DEVICE_COUNT_COMPATIBLE 4#define DEVICE_COUNT_RESOURCE 12 +struct pci_dev_config { +unsigned short command; +unsigned char cacheline_size; +unsigned char latency_timer; +unsigned char interrupt_line; +}; +struct pci_dev_pm {unsigned int pm_offset;/* the PCI PM capability offset */
@@ -152,7 +159,7 @@ unsigned int is_busmaster:1; /* device is busmaster */ unsigned int no_msi:1; /* device may not use msi */ -u32 saved_config_space[16 ];/* config space saved at suspend time */ +struct pci_dev_config saved_config;/* config space saved for power management */ struct bin_attribute *rom_attr;/* attribute descriptor for sysfs ROM entry */ int rom_attr_enabled;/* has display of the rom attribute been enabled? */ struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE ];/* sysfs file for resources */
Quoted 13 lines · Adam Belay
This patch makes some improvements to
pci_save_stateandpci_restore_state. Instead of saving and restoring all standard registers (even read-only ones), it only restores necessary registers. Also, the command register is handled more carefully. Let me know if I'm missing anything important.drivers/pci/pm.c +0
-0@@ -53,10 +53,13 @@*/ int pci_save_state (struct pci_dev *dev) {
drivers/pci/pm.c line 61
Quoted 10 lines
drivers/pci/pm.c +6
-4-int i; -/* XXX: 100% dword access ok here? */ -for (i =0 ; i <16 ; i++) -pci_read_config_dword (dev, i *4 ,&dev->saved_config_space[i]); +struct pci_dev_config * conf = &dev->saved_config; + +pci_read_config_word (dev,PCI_COMMAND , &conf->command); +pci_read_config_byte (dev,PCI_CACHE_LINE_SIZE , &conf->cacheline_size); +pci_read_config_byte (dev,PCI_LATENCY_TIMER , &conf->latency_timer); +pci_read_config_byte (dev,PCI_INTERRUPT_PIN , &conf->interrupt_line);
Why are we saving and restoring smaller ammounts of config space now?
thanks,
greg k-h
Quoted 18 lines · Greg KH
Quoted 17 lines · Adam Belay
This patch makes some improvements to
pci_save_stateandpci_restore_state. Instead of saving and restoring all standard registers (even read-only ones), it only restores necessary registers. Also, the command register is handled more carefully. Let me know if I'm missing anything important.drivers/pci/pm.c +0
-4@@ -53,10 +53,13 @@*/ int pci_save_state (struct pci_dev *dev) { -int i; -/* XXX: 100% dword access ok here? */ -for (i =0 ; i <16 ; i++) -pci_read_config_dword (dev, i *4 ,&dev->saved_config_space[i]);
Quoted 8 lines
drivers/pci/pm.c line 61
Quoted 6 lines
drivers/pci/pm.c +6
-0+struct pci_dev_config * conf = &dev->saved_config; + +pci_read_config_word (dev,PCI_COMMAND , &conf->command); +pci_read_config_byte (dev,PCI_CACHE_LINE_SIZE , &conf->cacheline_size); +pci_read_config_byte (dev,PCI_LATENCY_TIMER , &conf->latency_timer); +pci_read_config_byte (dev,PCI_INTERRUPT_PIN , &conf->interrupt_line);Why are we saving and restoring smaller ammounts of config space now?
After looking at the spec, it seems that most of the registers we were restoring were read-only and couldn't possibly need to be restored. Also, the PCI PM spec suggests that only a subset of the registers should be restored. Finally, things like BIST should probably never be touched.
This patch is one of the main reasons I'm looking for comments. I wanted to see if there are any other necessary registers. I'm also thinking we might want to restore some capability structures (which we don't do now).
Thanks,
Adam
Quoted 28 lines · Adam Belay
Quoted 18 lines · Greg KH
Quoted 17 lines · Adam Belay
This patch makes some improvements to
pci_save_stateandpci_restore_state. Instead of saving and restoring all standard registers (even read-only ones), it only restores necessary registers. Also, the command register is handled more carefully. Let me know if I'm missing anything important.drivers/pci/pm.c +0
-4@@ -53,10 +53,13 @@*/ int pci_save_state (struct pci_dev *dev) { -int i; -/* XXX: 100% dword access ok here? */ -for (i =0 ; i <16 ; i++) -pci_read_config_dword (dev, i *4 ,&dev->saved_config_space[i]);Quoted 8 lines
drivers/pci/pm.c line 61
Quoted 6 lines
drivers/pci/pm.c +6
-0+struct pci_dev_config * conf = &dev->saved_config; + +pci_read_config_word (dev,PCI_COMMAND , &conf->command); +pci_read_config_byte (dev,PCI_CACHE_LINE_SIZE , &conf->cacheline_size); +pci_read_config_byte (dev,PCI_LATENCY_TIMER , &conf->latency_timer); +pci_read_config_byte (dev,PCI_INTERRUPT_PIN , &conf->interrupt_line);Why are we saving and restoring smaller ammounts of config space now?
Quoted 5 lines
After looking at the spec, it seems that most of the registers we were restoring were read-only and couldn't possibly need to be restored. Also, the PCI PM spec suggests that only a subset of the registers should be restored. Finally, things like BIST should probably never be touched.
Ok, but be aware that this _might_ cause problems for some cards/drivers that were relying on the old way... As long as you don't mind me assigning those bugs to you, I don't have a problem with this :)
thanks,
greg k-h
Quoted 35 lines · Greg KH
Quoted 28 lines · Adam Belay
Quoted 18 lines · Greg KH
Quoted 17 lines · Adam Belay
This patch makes some improvements to
pci_save_stateandpci_restore_state. Instead of saving and restoring all standard registers (even read-only ones), it only restores necessary registers. Also, the command register is handled more carefully. Let me know if I'm missing anything important.drivers/pci/pm.c +0
-4@@ -53,10 +53,13 @@*/ int pci_save_state (struct pci_dev *dev) { -int i; -/* XXX: 100% dword access ok here? */ -for (i =0 ; i <16 ; i++) -pci_read_config_dword (dev, i *4 ,&dev->saved_config_space[i]);Quoted 8 lines
drivers/pci/pm.c line 61
Quoted 6 lines
drivers/pci/pm.c +6
-0+struct pci_dev_config * conf = &dev->saved_config; + +pci_read_config_word (dev,PCI_COMMAND , &conf->command); +pci_read_config_byte (dev,PCI_CACHE_LINE_SIZE , &conf->cacheline_size); +pci_read_config_byte (dev,PCI_LATENCY_TIMER , &conf->latency_timer); +pci_read_config_byte (dev,PCI_INTERRUPT_PIN , &conf->interrupt_line);Why are we saving and restoring smaller ammounts of config space now?
Quoted 5 lines
After looking at the spec, it seems that most of the registers we were restoring were read-only and couldn't possibly need to be restored. Also, the PCI PM spec suggests that only a subset of the registers should be restored. Finally, things like BIST should probably never be touched.
Quoted 3 lines
Ok, but be aware that this _might_ cause problems for some cards/drivers that were relying on the old way... As long as you don't mind me assigning those bugs to you, I don't have a problem with this :)
If any drivers have bugs in that area, then they most likely only worked by accident in the first place. The confg space is mostly read-only anyway, and there could be things we don't want to blindly overwrite. If any drivers need more, they should save it themselves.
Pat
Quoted 35 lines · Greg KH
Quoted 28 lines · Adam Belay
Quoted 18 lines · Greg KH
Quoted 17 lines · Adam Belay
This patch makes some improvements to
pci_save_stateandpci_restore_state. Instead of saving and restoring all standard registers (even read-only ones), it only restores necessary registers. Also, the command register is handled more carefully. Let me know if I'm missing anything important.drivers/pci/pm.c +0
-4@@ -53,10 +53,13 @@*/ int pci_save_state (struct pci_dev *dev) { -int i; -/* XXX: 100% dword access ok here? */ -for (i =0 ; i <16 ; i++) -pci_read_config_dword (dev, i *4 ,&dev->saved_config_space[i]);Quoted 8 lines
drivers/pci/pm.c line 61
Quoted 6 lines
drivers/pci/pm.c +6
-0+struct pci_dev_config * conf = &dev->saved_config; + +pci_read_config_word (dev,PCI_COMMAND , &conf->command); +pci_read_config_byte (dev,PCI_CACHE_LINE_SIZE , &conf->cacheline_size); +pci_read_config_byte (dev,PCI_LATENCY_TIMER , &conf->latency_timer); +pci_read_config_byte (dev,PCI_INTERRUPT_PIN , &conf->interrupt_line);Why are we saving and restoring smaller ammounts of config space now?
Quoted 5 lines
After looking at the spec, it seems that most of the registers we were restoring were read-only and couldn't possibly need to be restored. Also, the PCI PM spec suggests that only a subset of the registers should be restored. Finally, things like BIST should probably never be touched.
Quoted 3 lines
Ok, but be aware that this _might_ cause problems for some cards/drivers that were relying on the old way... As long as you don't mind me assigning those bugs to you, I don't have a problem with this :)
I'm probably going to regret this, but I'd be happy to take on any PCI PM subsystem bug reports. Unless I forgot a register we need to restore, I'm not expecting this to cause too many problems. A little time in -mm should shake out any issues out rather quickly.
Thanks,
Adam
Quoted 5 lines · Adam Belay
I'm probably going to regret this, but I'd be happy to take on any PCI PM subsystem bug reports. Unless I forgot a register we need to restore, I'm not expecting this to cause too many problems. A little time in -mm should shake out any issues out rather quickly.
Ok, respin them and I'll be glad to add them to my tree.
thanks,
greg k-h