[RFC][PATCH 6/6] PCI PM: pci_save/restore_state improvements

[RFC][PATCH 6/6] PCI PM: pci_save/restore_state improvements

[Adam Belay]

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 */

Re: [RFC][PATCH 6/6] PCI PM: pci_save/restore_state improvements

[Greg KH]

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

Re: [RFC][PATCH 6/6] PCI PM: pci_save/restore_state improvements

[Adam Belay]

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

Re: [RFC][PATCH 6/6] PCI PM: pci_save/restore_state improvements

[Greg KH]

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

Re: [linux-pm] Re: [RFC][PATCH 6/6] PCI PM: pci_save/restore_state improvements

[Patrick Mochel]

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

Re: [linux-pm] Re: [RFC][PATCH 6/6] PCI PM: pci_save/restore_state improvements

[Greg KH]

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

Re: [RFC][PATCH 6/6] PCI PM: pci_save/restore_state improvements

[Adam Belay]

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