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.
--- a/drivers/pci/pm.c 2005-11-13 20:32:24.000000000 -0500
+++ b/drivers/pci/pm.c 2005-11-13 20:29:32.000000000 -0500
@@ -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;
}
--- a/include/linux/pci.h 2005-11-08 17:10:22.000000000 -0500
+++ b/include/linux/pci.h 2005-11-13 20:21:20.000000000 -0500
@@ -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 */
On Tue, Nov 15, 2005 at 10:31:42PM -0500, Adam Belay wrote:
> 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.
>
>
> --- a/drivers/pci/pm.c 2005-11-13 20:32:24.000000000 -0500
> +++ b/drivers/pci/pm.c 2005-11-13 20:29:32.000000000 -0500
> @@ -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);
Why are we saving and restoring smaller ammounts of config space now?
thanks,
greg k-h
On Tue, 2005-11-15 at 22:31 -0800, Greg KH wrote:
> On Tue, Nov 15, 2005 at 10:31:42PM -0500, Adam Belay wrote:
> > 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.
> >
> >
> > --- a/drivers/pci/pm.c 2005-11-13 20:32:24.000000000 -0500
> > +++ b/drivers/pci/pm.c 2005-11-13 20:29:32.000000000 -0500
> > @@ -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);
>
> 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
On Wed, Nov 16, 2005 at 02:26:04AM -0500, Adam Belay wrote:
> On Tue, 2005-11-15 at 22:31 -0800, Greg KH wrote:
> > On Tue, Nov 15, 2005 at 10:31:42PM -0500, Adam Belay wrote:
> > > 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.
> > >
> > >
> > > --- a/drivers/pci/pm.c 2005-11-13 20:32:24.000000000 -0500
> > > +++ b/drivers/pci/pm.c 2005-11-13 20:29:32.000000000 -0500
> > > @@ -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);
> >
> > 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.
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
On Wed, 16 Nov 2005, Greg KH wrote:
> On Wed, Nov 16, 2005 at 02:26:04AM -0500, Adam Belay wrote:
> > On Tue, 2005-11-15 at 22:31 -0800, Greg KH wrote:
> > > On Tue, Nov 15, 2005 at 10:31:42PM -0500, Adam Belay wrote:
> > > > 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.
> > > >
> > > >
> > > > --- a/drivers/pci/pm.c 2005-11-13 20:32:24.000000000 -0500
> > > > +++ b/drivers/pci/pm.c 2005-11-13 20:29:32.000000000 -0500
> > > > @@ -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);
> > >
> > > 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.
>
> 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
On Wed, 2005-11-16 at 10:06 -0800, Greg KH wrote:
> On Wed, Nov 16, 2005 at 02:26:04AM -0500, Adam Belay wrote:
> > On Tue, 2005-11-15 at 22:31 -0800, Greg KH wrote:
> > > On Tue, Nov 15, 2005 at 10:31:42PM -0500, Adam Belay wrote:
> > > > 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.
> > > >
> > > >
> > > > --- a/drivers/pci/pm.c 2005-11-13 20:32:24.000000000 -0500
> > > > +++ b/drivers/pci/pm.c 2005-11-13 20:29:32.000000000 -0500
> > > > @@ -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);
> > >
> > > 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.
>
> 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
On Thu, Nov 17, 2005 at 06:50:30PM -0500, Adam Belay wrote:
>
> 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