pci_restore_state

pci_restore_state

[Ryan Lortie]

[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]

Hello.

Currently pci_restore_state() (drivers/pci/pci.c) restores the PCI state
by copying in the saved PCI configuration space, a dword at a time,
starting from 0 up to 15.

This causes a crash when my Macbook resumes from sleep (specifically,
when restoring the configuration space of the PCI bridge).

Reading the PCI specification, the register at dword 1 (ie: bytes 4-7)
is split half and half between status and command words.  The command
word effectively controls the way which the PCI device interacts with
the system.  If it is 0, the device is logically disconnected from the
bus (PCI Local Bus Specification Revision 2.2, 6.2.2 "Device
Control").  

When a device first powers up, the command register value is normally
zero (and is zero in my specific case).

The problem with the way that the PCI state is currently restored is
that the write to the command register logically reconnects the device
to the bus before the rest of the configuration space has been filled
in.

My Macbook crashes on resume.

If I reverse the for loop to start from 15 and count down to 0, then the
majority of the configuration space is filled in _before_ the command
word is modified.  No crash.

About dword 0 -- this dword contains only the vendor and product ID
codes which will always be set by the device (as far as I know) so it's
not a problem to not have written these before restoring the value of
the command register.  I'm not sure they even ever need to be written.

I am essentially requesting that this piece of code be changed in the
stock kernel as I changed it (described above).  It makes sleep work on
my Macbook and may fix some others too (and/or make more reliable some
which already work).

Cheers

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 703 bytes --]

Re: pci_restore_state

[Andrew Morton]

On Sun, 04 Jun 2006 06:13:30 -0400
Ryan Lortie <desrt@desrt.ca> wrote:

> Currently pci_restore_state() (drivers/pci/pci.c) restores the PCI state
> by copying in the saved PCI configuration space, a dword at a time,
> starting from 0 up to 15.
> 
> This causes a crash when my Macbook resumes from sleep (specifically,
> when restoring the configuration space of the PCI bridge).
> 
> Reading the PCI specification, the register at dword 1 (ie: bytes 4-7)
> is split half and half between status and command words.  The command
> word effectively controls the way which the PCI device interacts with
> the system.  If it is 0, the device is logically disconnected from the
> bus (PCI Local Bus Specification Revision 2.2, 6.2.2 "Device
> Control").  
> 
> When a device first powers up, the command register value is normally
> zero (and is zero in my specific case).
> 
> The problem with the way that the PCI state is currently restored is
> that the write to the command register logically reconnects the device
> to the bus before the rest of the configuration space has been filled
> in.
> 
> My Macbook crashes on resume.
> 
> If I reverse the for loop to start from 15 and count down to 0, then the
> majority of the configuration space is filled in _before_ the command
> word is modified.  No crash.

We have a patch pending which will do that.

http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-03-pci/pci-reverse-pci-config-space-restore-order.patch

The present plan will be to get this into 2.6.18.

Re: pci_restore_state

[Paul Mackerras]

Andrew Morton writes:

> On Sun, 04 Jun 2006 06:13:30 -0400
> Ryan Lortie <desrt@desrt.ca> wrote:
> > If I reverse the for loop to start from 15 and count down to 0, then the
> > majority of the configuration space is filled in _before_ the command
> > word is modified.  No crash.
> 
> We have a patch pending which will do that.
> 
> http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-03-pci/pci-reverse-pci-config-space-restore-order.patch

We really shouldn't be writing to the BIST register, at least...

Also, I don't quite see the point of writing to the read-only
registers such as vendor and device ID.

Paul.

Re: pci_restore_state

[Adam Belay]

On Sun, Jun 04, 2006 at 09:20:24PM +1000, Paul Mackerras wrote:
> Andrew Morton writes:
> 
> > On Sun, 04 Jun 2006 06:13:30 -0400
> > Ryan Lortie <desrt@desrt.ca> wrote:
> > > If I reverse the for loop to start from 15 and count down to 0, then the
> > > majority of the configuration space is filled in _before_ the command
> > > word is modified.  No crash.
> > 
> > We have a patch pending which will do that.
> > 
> > http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-03-pci/pci-reverse-pci-config-space-restore-order.patch
> 
> We really shouldn't be writing to the BIST register, at least...
> 
> Also, I don't quite see the point of writing to the read-only
> registers such as vendor and device ID.
> 
> Paul.

Any comments on this patch as an alternative solution?

http://marc.theaimsgroup.com/?l=linux-kernel&m=114949711413176&w=2

Thanks,
Adam