Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Rafael J. Wysocki]

On Friday, April 13, 2012, Mikko Vinni wrote:
> Bug report:
>     https://bugzilla.kernel.org/show_bug.cgi?id=43099
> 
> Briefly:
> 
> After s2ram the touchpad on a HP dv5 laptop doesn't work anymore.
> 
> dmesg contains this about the touchpad
> Apr 11 18:00:05 koni kernel: i8042: Can't write CTR while closing AUX port
> Apr 11 18:00:08 koni kernel: i8042: Can't reactivate AUX port
> 
> 
> Full log in bugzilla with PCI and i8042 debug turned on.
> 
> Bisected to:
> 
> commit 26f41062f28de65e11d3cf353e52d0be73442be1
> Author: Kay, Allen M <allen.m.kay@intel.com>
> Date:   Thu Jan 26 10:25:53 2012 -0800
> 
>     PCI: check for pci bar restore completion and retry
>     
>     On some OEM systems, pci_restore_state() is called while FLR has not yet
>     completed.  As a result, PCI BAR register restore is not successful.  This fix
>     reads back the restored value and compares it with saved value and re-tries 10
>     times before giving up.

Does reverting the above commit on top of the current Linus' tree fix the
problem for you?

Rafael

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Mikko Vinni]

From: Rafael J. Wysocki:

> On Friday, April 13, 2012, Mikko Vinni wrote:
>>  Bug report:
>>     https://bugzilla.kernel.org/show_bug.cgi?id=43099
>> 
>>  Briefly:
>> 
>>  After s2ram the touchpad on a HP dv5 laptop doesn't work anymore.
>> 
>>  dmesg contains this about the touchpad
>>  Apr 11 18:00:05 koni kernel: i8042: Can't write CTR while closing AUX 
> port
>>  Apr 11 18:00:08 koni kernel: i8042: Can't reactivate AUX port
>> 
>> 
>>  Full log in bugzilla with PCI and i8042 debug turned on.
>> 
>>  Bisected to:
>> 
>>  commit 26f41062f28de65e11d3cf353e52d0be73442be1
>>  Author: Kay, Allen M <allen.m.kay@intel.com>
>>  Date:   Thu Jan 26 10:25:53 2012 -0800
>> 
>>      PCI: check for pci bar restore completion and retry
>>     
>>      On some OEM systems, pci_restore_state() is called while FLR has not 
> yet
>>      completed.  As a result, PCI BAR register restore is not successful.  
> This fix
>>      reads back the restored value and compares it with saved value and 
> re-tries 10
>>      times before giving up.
> 
> Does reverting the above commit on top of the current Linus' tree fix the
> problem for you?


It does (at least with the tree from yesterday, 4166fb64593514ad920b7dbd290e0a934b37d24a).


Mikko

> 
> Rafael
> 

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Rafael J. Wysocki]

On Friday, April 13, 2012, Mikko Vinni wrote:
> From: Rafael J. Wysocki:
> 
> > On Friday, April 13, 2012, Mikko Vinni wrote:
> >>  Bug report:
> >>     https://bugzilla.kernel.org/show_bug.cgi?id=43099
> >> 
> >>  Briefly:
> >> 
> >>  After s2ram the touchpad on a HP dv5 laptop doesn't work anymore.
> >> 
> >>  dmesg contains this about the touchpad
> >>  Apr 11 18:00:05 koni kernel: i8042: Can't write CTR while closing AUX 
> > port
> >>  Apr 11 18:00:08 koni kernel: i8042: Can't reactivate AUX port
> >> 
> >> 
> >>  Full log in bugzilla with PCI and i8042 debug turned on.
> >> 
> >>  Bisected to:
> >> 
> >>  commit 26f41062f28de65e11d3cf353e52d0be73442be1
> >>  Author: Kay, Allen M <allen.m.kay@intel.com>
> >>  Date:   Thu Jan 26 10:25:53 2012 -0800
> >> 
> >>      PCI: check for pci bar restore completion and retry
> >>     
> >>      On some OEM systems, pci_restore_state() is called while FLR has not 
> > yet
> >>      completed.  As a result, PCI BAR register restore is not successful.  
> > This fix
> >>      reads back the restored value and compares it with saved value and 
> > re-tries 10
> >>      times before giving up.
> > 
> > Does reverting the above commit on top of the current Linus' tree fix the
> > problem for you?
> 
> 
> It does (at least with the tree from yesterday, 4166fb64593514ad920b7dbd290e0a934b37d24a).

OK, thanks.

Please change that dev_dbg() in pci_restore_state() into dev_info()
and see how many times it gets printed with the commit applied (not reverted).

[Margin note: I think it would make more sense to read the value from
the register _after_ and not before the waiting and I'm not exactly sure why
msleep() is not used.]

Thanks,
Rafael

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Mikko Vinni]

From: Rafael J. Wysocki:

> On Friday, April 13, 2012, Mikko Vinni wrote:
>>  From: Rafael J. Wysocki:
>> 
>>  > On Friday, April 13, 2012, Mikko Vinni wrote:
>>  >>  Bug report:
>>  >>    https://bugzilla.kernel.org/show_bug.cgi?id=43099
>>  >> 

^^^ some of the home-work is already there, including dmesg with and without the patch
with PCI debug on, which hopefully does the same as dev_info

> 
> Please change that dev_dbg() in pci_restore_state() into dev_info()
> and see how many times it gets printed with the commit applied (not reverted).


I ran the dmesg through "uniq -c" and it looks like this:

      1 ACPI: Low-level resume complete
      1 PM: Restoring platform NVS memory
      1 Enabling non-boot CPUs ...
      1 Booting Node 0 Processor 1 APIC 0x1
      1 CPU1 is up
      1 ACPI: Waking up from system sleep state S3
     10 pcieport 0000:00:02.0: restoring config space at offset 0x7 (was 0x20005151, writing 0x5151)
      1 pcieport 0000:00:02.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:04.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:05.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
     10 pcieport 0000:00:06.0: restoring config space at offset 0x7 (was 0x20002121, writing 0x2121)
      1 pcieport 0000:00:06.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:0a.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
      1 ahci 0000:00:11.0: restoring config space at offset 0x1 (was 0x2300003, writing 0x2300007)
      1 ohci_hcd 0000:00:12.0: restoring config space at offset 0x1 (was 0x2a00002, writing 0x2a00013)
      1 ehci_hcd 0000:00:12.2: BAR 0: set to [mem 0xd2408500-0xd24085ff] (PCI address [0xd2408500-0xd24085ff])
      1 ehci_hcd 0000:00:12.2: restoring config space at offset 0x1 (was 0x2b00000, writing 0x2b00013)
      1 ehci_hcd 0000:00:12.2: PME# disabled
      1 ohci_hcd 0000:00:13.0: restoring config space at offset 0x1 (was 0x2a00002, writing 0x2a00013)
      1 ehci_hcd 0000:00:13.2: BAR 0: set to [mem 0xd2408400-0xd24084ff] (PCI address [0xd2408400-0xd24084ff])
      1 ehci_hcd 0000:00:13.2: restoring config space at offset 0x1 (was 0x2b00000, writing 0x2b00013)
      1 ehci_hcd 0000:00:13.2: PME# disabled
      1 pata_atiixp 0000:00:14.1: restoring config space at offset 0x1 (was 0x2300011, writing 0x2300015)
      1 radeon 0000:01:00.0: restoring config space at offset 0x1 (was 0x100003, writing 0x100407)
      1 r8169 0000:09:00.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
      1 firewire_ohci 0000:0a:00.0: Refused to change power state, currently in D3
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xf (was 0xffffffff, writing 0x10a)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xe (was 0xffffffff, writing 0x0)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xd (was 0xffffffff, writing 0x44)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xc (was 0xffffffff, writing 0x0)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xb (was 0xffffffff, writing 0x3600103c)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xa (was 0xffffffff, writing 0x0)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x9 (was 0xffffffff, writing 0xd1100c00)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x8 (was 0xffffffff, writing 0xd1100c80)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x7 (was 0xffffffff, writing 0x0)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x6 (was 0xffffffff, writing 0x0)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x5 (was 0xffffffff, writing 0xd1100d00)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x4 (was 0xffffffff, writing 0xd1100000)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x3 (was 0xffffffff, writing 0x800000)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x2 (was 0xffffffff, writing 0xc001000)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x1 (was 0xffffffff, writing 0x100007)
     10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2380197b)
      1 sdhci-pci 0000:0a:00.1: Refused to change power state, currently in D3
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xf (was 0xffffffff, writing 0x10a)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xe (was 0xffffffff, writing 0x0)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xd (was 0xffffffff, writing 0xa4)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xc (was 0xffffffff, writing 0x0)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xb (was 0xffffffff, writing 0x3600103c)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xa (was 0xffffffff, writing 0x0)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x9 (was 0xffffffff, writing 0x0)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x8 (was 0xffffffff, writing 0x0)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x7 (was 0xffffffff, writing 0x0)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x6 (was 0xffffffff, writing 0x0)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x5 (was 0xffffffff, writing 0x0)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x4 (was 0xffffffff, writing 0xd1100b00)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x3 (was 0xffffffff, writing 0x800000)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x2 (was 0xffffffff, writing 0x8800000)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x1 (was 0xffffffff, writing 0x100007)
     10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2382197b)
      1 pci 0000:0a:00.2: Refused to change power state, currently in D3
     10 pci 0000:0a:00.2: restoring config space at offset 0xf (was 0xffffffff, writing 0x10a)
     10 pci 0000:0a:00.2: restoring config space at offset 0xe (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.2: restoring config space at offset 0xd (was 0xffffffff, writing 0xa4)
     10 pci 0000:0a:00.2: restoring config space at offset 0xc (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.2: restoring config space at offset 0xb (was 0xffffffff, writing 0x3600103c)
     10 pci 0000:0a:00.2: restoring config space at offset 0xa (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.2: restoring config space at offset 0x9 (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.2: restoring config space at offset 0x8 (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.2: restoring config space at offset 0x7 (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.2: restoring config space at offset 0x6 (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.2: restoring config space at offset 0x5 (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.2: restoring config space at offset 0x4 (was 0xffffffff, writing 0xd1100a00)
     10 pci 0000:0a:00.2: restoring config space at offset 0x3 (was 0xffffffff, writing 0x800000)
     10 pci 0000:0a:00.2: restoring config space at offset 0x2 (was 0xffffffff, writing 0x8050100)
     10 pci 0000:0a:00.2: restoring config space at offset 0x1 (was 0xffffffff, writing 0x100003)
     10 pci 0000:0a:00.2: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2381197b)
      1 jmb38x_ms 0000:0a:00.3: Refused to change power state, currently in D3
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0xf (was 0xffffffff, writing 0x10a)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0xe (was 0xffffffff, writing 0x0)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0xd (was 0xffffffff, writing 0xa4)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0xc (was 0xffffffff, writing 0x0)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0xb (was 0xffffffff, writing 0x3600103c)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0xa (was 0xffffffff, writing 0x0)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x9 (was 0xffffffff, writing 0x0)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x8 (was 0xffffffff, writing 0x0)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x7 (was 0xffffffff, writing 0x0)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x6 (was 0xffffffff, writing 0x0)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x5 (was 0xffffffff, writing 0x0)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x4 (was 0xffffffff, writing 0xd1100900)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x3 (was 0xffffffff, writing 0x800000)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x2 (was 0xffffffff, writing 0x8800000)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x1 (was 0xffffffff, writing 0x100007)
     10 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2383197b)
      1 pci 0000:0a:00.4: Refused to change power state, currently in D3
     10 pci 0000:0a:00.4: restoring config space at offset 0xf (was 0xffffffff, writing 0x10a)
     10 pci 0000:0a:00.4: restoring config space at offset 0xe (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.4: restoring config space at offset 0xd (was 0xffffffff, writing 0xa4)
     10 pci 0000:0a:00.4: restoring config space at offset 0xc (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.4: restoring config space at offset 0xb (was 0xffffffff, writing 0x3600103c)
     10 pci 0000:0a:00.4: restoring config space at offset 0xa (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.4: restoring config space at offset 0x9 (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.4: restoring config space at offset 0x8 (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.4: restoring config space at offset 0x7 (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.4: restoring config space at offset 0x6 (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.4: restoring config space at offset 0x5 (was 0xffffffff, writing 0x0)
     10 pci 0000:0a:00.4: restoring config space at offset 0x4 (was 0xffffffff, writing 0xd1100800)
     10 pci 0000:0a:00.4: restoring config space at offset 0x3 (was 0xffffffff, writing 0x800000)
     10 pci 0000:0a:00.4: restoring config space at offset 0x2 (was 0xffffffff, writing 0x8800000)
     10 pci 0000:0a:00.4: restoring config space at offset 0x1 (was 0xffffffff, writing 0x100007)
     10 pci 0000:0a:00.4: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2384197b)
      1 PM: noirq resume of devices complete after 8474.806 msecs
      1 PM: early resume of devices complete after 0.194 msecs
      1 ohci_hcd 0000:00:12.0: enabling bus mastering


Interestingly, this only seems to happen after the first s2ram.
https://bugzilla.kernel.org/attachment.cgi?id=72904 contains the dmesg from
three s2ram's, and after the second and third one write seems to be enough.
Even the failing firewire/card reader (another bug) is more quiet.

... (after the second s2ram, touchpad works after this)
      1 CPU1 is up
      1 ACPI: Waking up from system sleep state S3
      1 pcieport 0000:00:02.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:04.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:05.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:06.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:0a.0: restoring config space at offset 0x1 (was 0x100007, writing 0x100407)
      1 ahci 0000:00:11.0: restoring config space at offset 0x1 (was 0x2300003, writing 0x2300007)
      1 ohci_hcd 0000:00:12.0: restoring config space at offset 0x1 (was 0x2a00002, writing 0x2a00013)
      1 ehci_hcd 0000:00:12.2: BAR 0: set to [mem 0xd2408500-0xd24085ff] (PCI address [0xd2408500-0xd24085ff])
      1 ehci_hcd 0000:00:12.2: restoring config space at offset 0x1 (was 0x2b00000, writing 0x2b00013)
      1 ehci_hcd 0000:00:12.2: PME# disabled
... 


Mikko


> 
> [Margin note: I think it would make more sense to read the value from
> the register _after_ and not before the waiting and I'm not exactly sure why
> msleep() is not used.]
> 
> Thanks,
> Rafael
> 

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Rafael J. Wysocki]

Hi,

On Friday, April 13, 2012, Mikko Vinni wrote:
> From: Rafael J. Wysocki:
> 
> > On Friday, April 13, 2012, Mikko Vinni wrote:
> >>  From: Rafael J. Wysocki:
> >> 
> >>  > On Friday, April 13, 2012, Mikko Vinni wrote:
> >>  >>  Bug report:
> >>  >>    https://bugzilla.kernel.org/show_bug.cgi?id=43099
> >>  >> 
> 
> ^^^ some of the home-work is already there, including dmesg with and without the patch
> with PCI debug on, which hopefully does the same as dev_info
> 
> > 
> > Please change that dev_dbg() in pci_restore_state() into dev_info()
> > and see how many times it gets printed with the commit applied (not reverted).
> 
> 
> I ran the dmesg through "uniq -c" and it looks like this:
> 
>       1 ACPI: Low-level resume complete
>       1 PM: Restoring platform NVS memory
>       1 Enabling non-boot CPUs ...
>       1 Booting Node 0 Processor 1 APIC 0x1
>       1 CPU1 is up
>       1 ACPI: Waking up from system sleep state S3
>      10 pcieport 0000:00:02.0: restoring config space at offset 0x7 (was 0x20005151, writing 0x5151)

Well, yeah.  So the commit you bisected it to is total and utter crap.
Most importantly, it retries the writes for all of the dwords in the device's
config space _including_ the status register (which by definition need not be
the same as the written value).

I wonder if the appended patch helps?

Rafael


---
 drivers/pci/pci.c |   50 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 18 deletions(-)

Index: linux/drivers/pci/pci.c
===================================================================
--- linux.orig/drivers/pci/pci.c
+++ linux/drivers/pci/pci.c
@@ -967,16 +967,40 @@ pci_save_state(struct pci_dev *dev)
 	return 0;
 }
 
+static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
+				     int retry)
+{
+	int index;
+
+	for (index = end; index >= start; index--) {
+		u32 val, saved;
+		int offset = 4 * index;
+		int count = retry;
+
+		saved = pdev->saved_config_space[index];
+		pci_read_config_dword(pdev, offset, &val);
+		while (saved != val) {
+			dev_dbg(&pdev->dev, "restoring config space at offset "
+				"%#x (was %#x, writing %#x)\n",
+				offset, val, saved);
+			pci_write_config_dword(pdev, offset, saved);
+			if (count-- > 0) {
+				pci_read_config_dword(pdev, offset, &val);
+				if (saved != val)
+					msleep(10);
+			} else {
+				break;
+			}
+		}
+	}
+}
+
 /** 
  * pci_restore_state - Restore the saved state of a PCI device
  * @dev: - PCI device that we're dealing with
  */
 void pci_restore_state(struct pci_dev *dev)
 {
-	int i;
-	u32 val;
-	int tries;
-
 	if (!dev->state_saved)
 		return;
 
@@ -984,24 +1008,14 @@ void pci_restore_state(struct pci_dev *d
 	pci_restore_pcie_state(dev);
 	pci_restore_ats_state(dev);
 
+	pci_restore_config_space(dev, 10, 15, 0);
 	/*
 	 * The Base Address register should be programmed before the command
 	 * register(s)
 	 */
-	for (i = 15; i >= 0; i--) {
-		pci_read_config_dword(dev, i * 4, &val);
-		tries = 10;		
-		while (tries && val != dev->saved_config_space[i]) {
-			dev_dbg(&dev->dev, "restoring config "
-				"space at offset %#x (was %#x, writing %#x)\n",
-				i, val, (int)dev->saved_config_space[i]);
-			pci_write_config_dword(dev,i * 4,
-				dev->saved_config_space[i]);
-			pci_read_config_dword(dev, i * 4, &val);
-			mdelay(10);
-			tries--;
-		}
-	}
+	pci_restore_config_space(dev, 4, 9, 10);
+	pci_restore_config_space(dev, 0, 3, 0);
+
 	pci_restore_pcix_state(dev);
 	pci_restore_msi_state(dev);
 	pci_restore_iov_state(dev);

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Mikko Vinni]

From: Rafael J. Wysocki:

>>  > Please change that dev_dbg() in pci_restore_state() into dev_info()
>>  > and see how many times it gets printed with the commit applied (not 
> reverted).
>> 
>> 
>>  I ran the dmesg through "uniq -c" and it looks like this:
>> 
>>        1 ACPI: Low-level resume complete
>>        1 PM: Restoring platform NVS memory
>>        1 Enabling non-boot CPUs ...
>>        1 Booting Node 0 Processor 1 APIC 0x1
>>        1 CPU1 is up
>>        1 ACPI: Waking up from system sleep state S3
>>       10 pcieport 0000:00:02.0: restoring config space at offset 0x7 (was 
> 0x20005151, writing 0x5151)
> 
> Well, yeah.  So the commit you bisected it to is total and utter crap.
> Most importantly, it retries the writes for all of the dwords in the 
> device's
> config space _including_ the status register (which by definition need not be
> the same as the written value).
> 
> I wonder if the appended patch helps?

It does. It seems there is a change only in the devices that refuse to wake up;
somehow writing too many times to them affects the rest of the machine.
So, for this machine your patch fixes the problem of the touchpad not working.

The write is now attempted (at most) 11 times, as shown below. Probably not
a huge deal.

      1 ACPI: Waking up from system sleep state S3
     11 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 0x20005151, writing 0x5151)
      1 pcieport 0000:00:02.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:04.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:05.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
     11 pcieport 0000:00:06.0: restoring config space at offset 0x1c (was 0x20002121, writing 0x2121)
      1 pcieport 0000:00:06.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:0a.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
      1 ahci 0000:00:11.0: restoring config space at offset 0x4 (was 0x2300003, writing 0x2300007)
      1 ohci_hcd 0000:00:12.0: restoring config space at offset 0x4 (was 0x2a00002, writing 0x2a00013)
      1 ehci_hcd 0000:00:12.2: BAR 0: set to [mem 0xd2408500-0xd24085ff] (PCI address [0xd2408500-0xd24085ff])
      1 ehci_hcd 0000:00:12.2: restoring config space at offset 0x4 (was 0x2b00000, writing 0x2b00013)
      1 ehci_hcd 0000:00:12.2: PME# disabled
      1 ohci_hcd 0000:00:13.0: restoring config space at offset 0x4 (was 0x2a00002, writing 0x2a00013)
      1 ehci_hcd 0000:00:13.2: BAR 0: set to [mem 0xd2408400-0xd24084ff] (PCI address [0xd2408400-0xd24084ff])
      1 ehci_hcd 0000:00:13.2: restoring config space at offset 0x4 (was 0x2b00000, writing 0x2b00013)
      1 ehci_hcd 0000:00:13.2: PME# disabled
      1 pata_atiixp 0000:00:14.1: restoring config space at offset 0x4 (was 0x2300011, writing 0x2300015)
      1 radeon 0000:01:00.0: restoring config space at offset 0x4 (was 0x100003, writing 0x20100407)
      1 pci 0000:09:00.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100003)
      1 firewire_ohci 0000:0a:00.0: Refused to change power state, currently in D3
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x3c (was 0xffffffff, writing 0x10a)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x34 (was 0xffffffff, writing 0x44)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x30 (was 0xffffffff, writing 0x0)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x2c (was 0xffffffff, writing 0x3600103c)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0xd1100c00)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x20 (was 0xffffffff, writing 0xd1100c80)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x1c (was 0xffffffff, writing 0x0)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x18 (was 0xffffffff, writing 0x0)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x14 (was 0xffffffff, writing 0xd1100d00)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x10 (was 0xffffffff, writing 0xd1100000)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xc (was 0xffffffff, writing 0x800000)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x8 (was 0xffffffff, writing 0xc001000)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100007)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2380197b)
      1 sdhci-pci 0000:0a:00.1: Refused to change power state, currently in D3
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x3c (was 0xffffffff, writing 0x10a)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x34 (was 0xffffffff, writing 0xa4)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x30 (was 0xffffffff, writing 0x0)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x2c (was 0xffffffff, writing 0x3600103c)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x20 (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x1c (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x18 (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x14 (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x10 (was 0xffffffff, writing 0xd1100b00)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xc (was 0xffffffff, writing 0x800000)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x8 (was 0xffffffff, writing 0x8800000)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100007)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2382197b)
      1 pci 0000:0a:00.2: Refused to change power state, currently in D3
      1 pci 0000:0a:00.2: restoring config space at offset 0x3c (was 0xffffffff, writing 0x10a)
      1 pci 0000:0a:00.2: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
      1 pci 0000:0a:00.2: restoring config space at offset 0x34 (was 0xffffffff, writing 0xa4)
      1 pci 0000:0a:00.2: restoring config space at offset 0x30 (was 0xffffffff, writing 0x0)
      1 pci 0000:0a:00.2: restoring config space at offset 0x2c (was 0xffffffff, writing 0x3600103c)
      1 pci 0000:0a:00.2: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x20 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x1c (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x18 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x14 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x10 (was 0xffffffff, writing 0xd1100a00)
      1 pci 0000:0a:00.2: restoring config space at offset 0xc (was 0xffffffff, writing 0x800000)
      1 pci 0000:0a:00.2: restoring config space at offset 0x8 (was 0xffffffff, writing 0x8050100)
      1 pci 0000:0a:00.2: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100003)
      1 pci 0000:0a:00.2: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2381197b)
      1 jmb38x_ms 0000:0a:00.3: Refused to change power state, currently in D3
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x3c (was 0xffffffff, writing 0x10a)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x34 (was 0xffffffff, writing 0xa4)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x30 (was 0xffffffff, writing 0x0)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x2c (was 0xffffffff, writing 0x3600103c)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x20 (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x1c (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x18 (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x14 (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x10 (was 0xffffffff, writing 0xd1100900)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0xc (was 0xffffffff, writing 0x800000)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x8 (was 0xffffffff, writing 0x8800000)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100007)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2383197b)
      1 pci 0000:0a:00.4: Refused to change power state, currently in D3
      1 pci 0000:0a:00.4: restoring config space at offset 0x3c (was 0xffffffff, writing 0x10a)
      1 pci 0000:0a:00.4: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
      1 pci 0000:0a:00.4: restoring config space at offset 0x34 (was 0xffffffff, writing 0xa4)
      1 pci 0000:0a:00.4: restoring config space at offset 0x30 (was 0xffffffff, writing 0x0)
      1 pci 0000:0a:00.4: restoring config space at offset 0x2c (was 0xffffffff, writing 0x3600103c)
      1 pci 0000:0a:00.4: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x20 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x1c (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x18 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x14 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x10 (was 0xffffffff, writing 0xd1100800)
      1 pci 0000:0a:00.4: restoring config space at offset 0xc (was 0xffffffff, writing 0x800000)
      1 pci 0000:0a:00.4: restoring config space at offset 0x8 (was 0xffffffff, writing 0x8800000)
      1 pci 0000:0a:00.4: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100007)
      1 pci 0000:0a:00.4: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2384197b)
      1 PM: noirq resume of devices complete after 6539.864 msecs
      1 PM: early resume of devices complete after 0.159 msecs
      1 ohci_hcd 0000:00:12.0: enabling bus mastering
...


Thanks,

Mikko


> 
> Rafael
> 
> 
> ---
> drivers/pci/pci.c |   50 ++++++++++++++++++++++++++++++++------------------
> 1 file changed, 32 insertions(+), 18 deletions(-)
> 
> Index: linux/drivers/pci/pci.c
> ===================================================================
> --- linux.orig/drivers/pci/pci.c
> +++ linux/drivers/pci/pci.c
> @@ -967,16 +967,40 @@ pci_save_state(struct pci_dev *dev)
>     return 0;
> }
> 
> +static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
> +                     int retry)
> +{
> +    int index;
> +
> +    for (index = end; index >= start; index--) {
> +        u32 val, saved;
> +        int offset = 4 * index;
> +        int count = retry;
> +
> +        saved = pdev->saved_config_space[index];
> +        pci_read_config_dword(pdev, offset, &val);
> +        while (saved != val) {
> +            dev_dbg(&pdev->dev, "restoring config space at offset 
> "
> +                "%#x (was %#x, writing %#x)\n",
> +                offset, val, saved);
> +            pci_write_config_dword(pdev, offset, saved);
> +            if (count-- > 0) {
> +                pci_read_config_dword(pdev, offset, &val);
> +                if (saved != val)
> +                    msleep(10);
> +            } else {
> +                break;
> +            }
> +        }
> +    }
> +}
> +
> /** 
>   * pci_restore_state - Restore the saved state of a PCI device
>   * @dev: - PCI device that we're dealing with
>   */
> void pci_restore_state(struct pci_dev *dev)
> {
> -    int i;
> -    u32 val;
> -    int tries;
> -
>     if (!dev->state_saved)
>         return;
> 
> @@ -984,24 +1008,14 @@ void pci_restore_state(struct pci_dev *d
>     pci_restore_pcie_state(dev);
>     pci_restore_ats_state(dev);
> 
> +    pci_restore_config_space(dev, 10, 15, 0);
>     /*
>      * The Base Address register should be programmed before the command
>      * register(s)
>      */
> -    for (i = 15; i >= 0; i--) {
> -        pci_read_config_dword(dev, i * 4, &val);
> -        tries = 10;        
> -        while (tries && val != dev->saved_config_space[i]) {
> -            dev_dbg(&dev->dev, "restoring config "
> -                "space at offset %#x (was %#x, writing %#x)\n",
> -                i, val, (int)dev->saved_config_space[i]);
> -            pci_write_config_dword(dev,i * 4,
> -                dev->saved_config_space[i]);
> -            pci_read_config_dword(dev, i * 4, &val);
> -            mdelay(10);
> -            tries--;
> -        }
> -    }
> +    pci_restore_config_space(dev, 4, 9, 10);
> +    pci_restore_config_space(dev, 0, 3, 0);
> +
>     pci_restore_pcix_state(dev);
>     pci_restore_msi_state(dev);
>     pci_restore_iov_state(dev);
> 

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Rafael J. Wysocki]

On Sunday, April 15, 2012, Mikko Vinni wrote:
> From: Rafael J. Wysocki:
> 
> >>  > Please change that dev_dbg() in pci_restore_state() into dev_info()
> >>  > and see how many times it gets printed with the commit applied (not 
> > reverted).
> >> 
> >> 
> >>  I ran the dmesg through "uniq -c" and it looks like this:
> >> 
> >>        1 ACPI: Low-level resume complete
> >>        1 PM: Restoring platform NVS memory
> >>        1 Enabling non-boot CPUs ...
> >>        1 Booting Node 0 Processor 1 APIC 0x1
> >>        1 CPU1 is up
> >>        1 ACPI: Waking up from system sleep state S3
> >>       10 pcieport 0000:00:02.0: restoring config space at offset 0x7 (was 
> > 0x20005151, writing 0x5151)
> > 
> > Well, yeah.  So the commit you bisected it to is total and utter crap.
> > Most importantly, it retries the writes for all of the dwords in the 
> > device's
> > config space _including_ the status register (which by definition need not be
> > the same as the written value).
> > 
> > I wonder if the appended patch helps?
> 
> It does. It seems there is a change only in the devices that refuse to wake up;
> somehow writing too many times to them affects the rest of the machine.
> So, for this machine your patch fixes the problem of the touchpad not working.

Good, thanks for the confirmation. :-)

> The write is now attempted (at most) 11 times, as shown below. Probably not
> a huge deal.
> 
>       1 ACPI: Waking up from system sleep state S3
>      11 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 0x20005151, writing 0x5151)

Well, here you see that the PCI Express port is refusing to accept a new BAR value.
It seems not to be operational and this probably is the reason why the device below
the port don't respond later.

I'm not sure why that happens, though.

>       1 pcieport 0000:00:02.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
>       1 pcieport 0000:00:04.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
>       1 pcieport 0000:00:05.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
>      11 pcieport 0000:00:06.0: restoring config space at offset 0x1c (was 0x20002121, writing 0x2121)

The same happens here, apparently.

Do you still have the kernel where all PCI devices worked correctly after system
resume?

Rafael

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Rafael J. Wysocki]

On Sunday, April 15, 2012, Rafael J. Wysocki wrote:
> On Sunday, April 15, 2012, Mikko Vinni wrote:
> > From: Rafael J. Wysocki:
> > 
> > >>  > Please change that dev_dbg() in pci_restore_state() into dev_info()
> > >>  > and see how many times it gets printed with the commit applied (not 
> > > reverted).
> > >> 
> > >> 
> > >>  I ran the dmesg through "uniq -c" and it looks like this:
> > >> 
> > >>        1 ACPI: Low-level resume complete
> > >>        1 PM: Restoring platform NVS memory
> > >>        1 Enabling non-boot CPUs ...
> > >>        1 Booting Node 0 Processor 1 APIC 0x1
> > >>        1 CPU1 is up
> > >>        1 ACPI: Waking up from system sleep state S3
> > >>       10 pcieport 0000:00:02.0: restoring config space at offset 0x7 (was 
> > > 0x20005151, writing 0x5151)
> > > 
> > > Well, yeah.  So the commit you bisected it to is total and utter crap.
> > > Most importantly, it retries the writes for all of the dwords in the 
> > > device's
> > > config space _including_ the status register (which by definition need not be
> > > the same as the written value).
> > > 
> > > I wonder if the appended patch helps?
> > 
> > It does. It seems there is a change only in the devices that refuse to wake up;
> > somehow writing too many times to them affects the rest of the machine.
> > So, for this machine your patch fixes the problem of the touchpad not working.
> 
> Good, thanks for the confirmation. :-)
> 
> > The write is now attempted (at most) 11 times, as shown below. Probably not
> > a huge deal.
> > 
> >       1 ACPI: Waking up from system sleep state S3
> >      11 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 0x20005151, writing 0x5151)
> 
> Well, here you see that the PCI Express port is refusing to accept a new BAR value.
> It seems not to be operational and this probably is the reason why the device below
> the port don't respond later.
> 
> I'm not sure why that happens, though.

OK, I know.  There simply are fewer BARs for bridges (and PCIe ports)
and here we're attempting to overwrite the secondary status register.  Sigh.

Well, I guess we should only retry the writes to BARs for Type 0 headers.

Thanks,
Rafael

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Rafael J. Wysocki]

On Sunday, April 15, 2012, Rafael J. Wysocki wrote:
> On Sunday, April 15, 2012, Rafael J. Wysocki wrote:
> > On Sunday, April 15, 2012, Mikko Vinni wrote:
> > > From: Rafael J. Wysocki:
> > > 
> > > >>  > Please change that dev_dbg() in pci_restore_state() into dev_info()
> > > >>  > and see how many times it gets printed with the commit applied (not 
> > > > reverted).
> > > >> 
> > > >> 
> > > >>  I ran the dmesg through "uniq -c" and it looks like this:
> > > >> 
> > > >>        1 ACPI: Low-level resume complete
> > > >>        1 PM: Restoring platform NVS memory
> > > >>        1 Enabling non-boot CPUs ...
> > > >>        1 Booting Node 0 Processor 1 APIC 0x1
> > > >>        1 CPU1 is up
> > > >>        1 ACPI: Waking up from system sleep state S3
> > > >>       10 pcieport 0000:00:02.0: restoring config space at offset 0x7 (was 
> > > > 0x20005151, writing 0x5151)
> > > > 
> > > > Well, yeah.  So the commit you bisected it to is total and utter crap.
> > > > Most importantly, it retries the writes for all of the dwords in the 
> > > > device's
> > > > config space _including_ the status register (which by definition need not be
> > > > the same as the written value).
> > > > 
> > > > I wonder if the appended patch helps?
> > > 
> > > It does. It seems there is a change only in the devices that refuse to wake up;
> > > somehow writing too many times to them affects the rest of the machine.
> > > So, for this machine your patch fixes the problem of the touchpad not working.
> > 
> > Good, thanks for the confirmation. :-)
> > 
> > > The write is now attempted (at most) 11 times, as shown below. Probably not
> > > a huge deal.
> > > 
> > >       1 ACPI: Waking up from system sleep state S3
> > >      11 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 0x20005151, writing 0x5151)
> > 
> > Well, here you see that the PCI Express port is refusing to accept a new BAR value.
> > It seems not to be operational and this probably is the reason why the device below
> > the port don't respond later.
> > 
> > I'm not sure why that happens, though.
> 
> OK, I know.  There simply are fewer BARs for bridges (and PCIe ports)
> and here we're attempting to overwrite the secondary status register.  Sigh.
> 
> Well, I guess we should only retry the writes to BARs for Type 0 headers.

Which is done by the appended updated patch.  Can you please give it a try?

Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PCI: Fix regression in pci_restore_state(), v4

Commit 26f41062f28de65e11d3cf353e52d0be73442be1

    PCI: check for pci bar restore completion and retry

attempted to address problems with PCI BAR restoration on systems
where FLR had not been completed before pci_restore_state() was
called, but it did that in an utterly wrong way.

First off, instead of retrying the writes for the BAR registers
only, it did that for all of the PCI config space of the device,
including the status registers (whose values after the write quite
obviously need not be the same as the written ones).  Second, it
added arbitrary delay to pci_restore_state() even for systems
where the PCI config space restoration was successful at first
attempt.  Finally, the mdelay(10) it added to every iteration of the
writing loop was way too much of a delay for any reasonable device.

All of this actually caused resume failures for some devices on
the Mikko's system.

To fix the regression, make pci_restore_state() only retry the
writes to BAR registers and only for devices with Type 0 headers.
Moreover, make it wait only if the next first read from the given
register doesn't return the value written to it.  Finally, make it
wait for 1 ms, instead of 10 ms, after every failing attempt to write
into the device's config space.

Reported-by: Mikko Vinni <mmvinni@yahoo.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c |   67 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 46 insertions(+), 21 deletions(-)

Index: linux/drivers/pci/pci.c
===================================================================
--- linux.orig/drivers/pci/pci.c
+++ linux/drivers/pci/pci.c
@@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
 	return 0;
 }
 
+static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
+				     u32 saved_val, int retry)
+{
+	u32 val;
+
+	pci_read_config_dword(pdev, offset, &val);
+	if (val == saved_val)
+		return;
+
+	for (;;) {
+		dev_dbg(&pdev->dev, "restoring config space at offset "
+			"%#x (was %#x, writing %#x)\n", offset, val, saved_val);
+		pci_write_config_dword(pdev, offset, saved_val);
+		if (retry-- <= 0)
+			return;
+
+		pci_read_config_dword(pdev, offset, &val);
+		if (val == saved_val)
+			return;
+
+		mdelay(1);
+	}
+}
+
+static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
+				     int retry)
+{
+	int index;
+
+	for (index = end; index >= start; index--)
+		pci_restore_config_dword(pdev, 4 * index,
+					 pdev->saved_config_space[index],
+					 retry);
+}
+
 /** 
  * pci_restore_state - Restore the saved state of a PCI device
  * @dev: - PCI device that we're dealing with
  */
 void pci_restore_state(struct pci_dev *dev)
 {
-	int i;
-	u32 val;
-	int tries;
-
 	if (!dev->state_saved)
 		return;
 
@@ -984,24 +1015,18 @@ void pci_restore_state(struct pci_dev *d
 	pci_restore_pcie_state(dev);
 	pci_restore_ats_state(dev);
 
-	/*
-	 * The Base Address register should be programmed before the command
-	 * register(s)
-	 */
-	for (i = 15; i >= 0; i--) {
-		pci_read_config_dword(dev, i * 4, &val);
-		tries = 10;		
-		while (tries && val != dev->saved_config_space[i]) {
-			dev_dbg(&dev->dev, "restoring config "
-				"space at offset %#x (was %#x, writing %#x)\n",
-				i, val, (int)dev->saved_config_space[i]);
-			pci_write_config_dword(dev,i * 4,
-				dev->saved_config_space[i]);
-			pci_read_config_dword(dev, i * 4, &val);
-			mdelay(10);
-			tries--;
-		}
+	if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
+		pci_restore_config_space(dev, 10, 15, 0);
+		/*
+		 * The Base Address register should be programmed before the
+		 * command register(s)
+		 */
+		pci_restore_config_space(dev, 4, 9, 10);
+		pci_restore_config_space(dev, 0, 3, 0);
+	} else {
+		pci_restore_config_space(dev, 0, 15, 0);
 	}
+
 	pci_restore_pcix_state(dev);
 	pci_restore_msi_state(dev);
 	pci_restore_iov_state(dev);

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Mikko Vinni]

From: Rafael J. Wysocki

>>  OK, I know.  There simply are fewer BARs for bridges (and PCIe ports)
>>  and here we're attempting to overwrite the secondary status register.  
> Sigh.
>> 
>>  Well, I guess we should only retry the writes to BARs for Type 0 headers.
> 
> Which is done by the appended updated patch.  Can you please give it a try?


Ok, I tested v4 and also v3, which is in 3.4-rc3. On this laptop both versions
work equally: touchpad works after resume (no help for the card reader
problem, though).

Here is part of the resume log with version 4:

      1 ACPI: Waking up from system sleep state S3
      1 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 0x20005151, writing 0x5151)
      1 pcieport 0000:00:02.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:04.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:05.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:06.0: restoring config space at offset 0x1c (was 0x20002121, writing 0x2121)
      1 pcieport 0000:00:06.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
      1 pcieport 0000:00:0a.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
      1 ahci 0000:00:11.0: restoring config space at offset 0x4 (was 0x2300003, writing 0x2300007)
      1 ohci_hcd 0000:00:12.0: restoring config space at offset 0x4 (was 0x2a00002, writing 0x2a00013)
      1 ehci_hcd 0000:00:12.2: BAR 0: set to [mem 0xd2408500-0xd24085ff] (PCI address [0xd2408500-0xd24085ff])
      1 ehci_hcd 0000:00:12.2: restoring config space at offset 0x4 (was 0x2b00000, writing 0x2b00013)
      1 ehci_hcd 0000:00:12.2: PME# disabled
      1 ohci_hcd 0000:00:13.0: restoring config space at offset 0x4 (was 0x2a00002, writing 0x2a00013)
      1 ehci_hcd 0000:00:13.2: BAR 0: set to [mem 0xd2408400-0xd24084ff] (PCI address [0xd2408400-0xd24084ff])
      1 ehci_hcd 0000:00:13.2: restoring config space at offset 0x4 (was 0x2b00000, writing 0x2b00013)
      1 ehci_hcd 0000:00:13.2: PME# disabled
      1 pata_atiixp 0000:00:14.1: restoring config space at offset 0x4 (was 0x2300011, writing 0x2300015)
      1 radeon 0000:01:00.0: restoring config space at offset 0x4 (was 0x100003, writing 0x100407)
      1 pci 0000:09:00.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100003)
      1 firewire_ohci 0000:0a:00.0: Refused to change power state, currently in D3
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x3c (was 0xffffffff, writing 0x10a)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x34 (was 0xffffffff, writing 0x44)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x30 (was 0xffffffff, writing 0x0)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x2c (was 0xffffffff, writing 0x3600103c)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x24 (was 0xffffffff, writing 0xd1100c00)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x20 (was 0xffffffff, writing 0xd1100c80)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x1c (was 0xffffffff, writing 0x0)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x18 (was 0xffffffff, writing 0x0)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x14 (was 0xffffffff, writing 0xd1100d00)
     11 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x10 (was 0xffffffff, writing 0xd1100000)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xc (was 0xffffffff, writing 0x800000)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x8 (was 0xffffffff, writing 0xc001000)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100007)
      1 firewire_ohci 0000:0a:00.0: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2380197b)
      1 sdhci-pci 0000:0a:00.1: Refused to change power state, currently in D3
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x3c (was 0xffffffff, writing 0x10a)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x34 (was 0xffffffff, writing 0xa4)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x30 (was 0xffffffff, writing 0x0)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x2c (was 0xffffffff, writing 0x3600103c)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x20 (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x1c (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x18 (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x14 (was 0xffffffff, writing 0x0)
     11 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x10 (was 0xffffffff, writing 0xd1100b00)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xc (was 0xffffffff, writing 0x800000)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x8 (was 0xffffffff, writing 0x8800000)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100007)
      1 sdhci-pci 0000:0a:00.1: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2382197b)
      1 pci 0000:0a:00.2: Refused to change power state, currently in D3
      1 pci 0000:0a:00.2: restoring config space at offset 0x3c (was 0xffffffff, writing 0x10a)
      1 pci 0000:0a:00.2: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
      1 pci 0000:0a:00.2: restoring config space at offset 0x34 (was 0xffffffff, writing 0xa4)
      1 pci 0000:0a:00.2: restoring config space at offset 0x30 (was 0xffffffff, writing 0x0)
      1 pci 0000:0a:00.2: restoring config space at offset 0x2c (was 0xffffffff, writing 0x3600103c)
      1 pci 0000:0a:00.2: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x20 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x1c (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x18 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x14 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.2: restoring config space at offset 0x10 (was 0xffffffff, writing 0xd1100a00)
      1 pci 0000:0a:00.2: restoring config space at offset 0xc (was 0xffffffff, writing 0x800000)
      1 pci 0000:0a:00.2: restoring config space at offset 0x8 (was 0xffffffff, writing 0x8050100)
      1 pci 0000:0a:00.2: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100003)
      1 pci 0000:0a:00.2: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2381197b)
      1 jmb38x_ms 0000:0a:00.3: Refused to change power state, currently in D3
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x3c (was 0xffffffff, writing 0x10a)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x34 (was 0xffffffff, writing 0xa4)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x30 (was 0xffffffff, writing 0x0)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x2c (was 0xffffffff, writing 0x3600103c)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x20 (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x1c (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x18 (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x14 (was 0xffffffff, writing 0x0)
     11 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x10 (was 0xffffffff, writing 0xd1100900)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0xc (was 0xffffffff, writing 0x800000)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x8 (was 0xffffffff, writing 0x8800000)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100007)
      1 jmb38x_ms 0000:0a:00.3: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2383197b)
      1 pci 0000:0a:00.4: Refused to change power state, currently in D3
      1 pci 0000:0a:00.4: restoring config space at offset 0x3c (was 0xffffffff, writing 0x10a)
      1 pci 0000:0a:00.4: restoring config space at offset 0x38 (was 0xffffffff, writing 0x0)
      1 pci 0000:0a:00.4: restoring config space at offset 0x34 (was 0xffffffff, writing 0xa4)
      1 pci 0000:0a:00.4: restoring config space at offset 0x30 (was 0xffffffff, writing 0x0)
      1 pci 0000:0a:00.4: restoring config space at offset 0x2c (was 0xffffffff, writing 0x3600103c)
      1 pci 0000:0a:00.4: restoring config space at offset 0x28 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x24 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x20 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x1c (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x18 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x14 (was 0xffffffff, writing 0x0)
     11 pci 0000:0a:00.4: restoring config space at offset 0x10 (was 0xffffffff, writing 0xd1100800)
      1 pci 0000:0a:00.4: restoring config space at offset 0xc (was 0xffffffff, writing 0x800000)
      1 pci 0000:0a:00.4: restoring config space at offset 0x8 (was 0xffffffff, writing 0x8800000)
      1 pci 0000:0a:00.4: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100007)
      1 pci 0000:0a:00.4: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2384197b)
      1 PM: noirq resume of devices complete after 440.260 msecs
      1 PM: early resume of devices complete after 0.152 msecs
      1 ohci_hcd 0000:00:12.0: enabling bus mastering
      1 ohci_hcd 0000:00:12.1: enabling bus mastering


Difference from v3:
--- /tmp/v3-uniq        2012-04-16 09:57:42.880521878 +0300
+++ /tmp/v4-uniq        2012-04-16 09:58:03.880545386 +0300
@@ -1019,11 +1018,11 @@
       1 Booting Node 0 Processor 1 APIC 0x1
       1 CPU1 is up
       1 ACPI: Waking up from system sleep state S3
-     11 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 0x20005151, writing 0x5151)
+      1 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 0x20005151, writing 0x5151)
       1 pcieport 0000:00:02.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
       1 pcieport 0000:00:04.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
       1 pcieport 0000:00:05.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
-     11 pcieport 0000:00:06.0: restoring config space at offset 0x1c (was 0x20002121, writing 0x2121)
+      1 pcieport 0000:00:06.0: restoring config space at offset 0x1c (was 0x20002121, writing 0x2121)
       1 pcieport 0000:00:06.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
       1 pcieport 0000:00:0a.0: restoring config space at offset 0x4 (was 0x100007, writing 0x100407)
       1 ahci 0000:00:11.0: restoring config space at offset 0x4 (was 0x2300003, writing 0x2300007)
@@ -1123,8 +1122,8 @@
       1 pci 0000:0a:00.4: restoring config space at offset 0x8 (was 0xffffffff, writing 0x8800000)
       1 pci 0000:0a:00.4: restoring config space at offset 0x4 (was 0xffffffff, writing 0x100007)
       1 pci 0000:0a:00.4: restoring config space at offset 0x0 (was 0xffffffff, writing 0x2384197b)
-      1 PM: noirq resume of devices complete after 460.315 msecs
-      1 PM: early resume of devices complete after 0.168 msecs
+      1 PM: noirq resume of devices complete after 440.260 msecs
+      1 PM: early resume of devices complete after 0.152 msecs
       1 ohci_hcd 0000:00:12.0: enabling bus mastering
       1 ohci_hcd 0000:00:12.1: enabling bus mastering
       1 ehci_hcd 0000:00:12.2: enabling bus mastering


(...if only radeon didn't add 10 seconds to resume time, that improvement of 20 msecs
would be very nice; that is another issue, of course, and not a new problem for this
kernel version:

[drm:atom_op_jump] *ERROR* atombios stuck in loop for more than 5secs aborting
[drm:atom_execute_table_locked] *ERROR* atombios stuck executing F8B6 (len 485, WS 0, PS 4) @ 0xF8F7
[drm:atom_op_jump] *ERROR* atombios stuck in loop for more than 5secs aborting
[drm:atom_execute_table_locked] *ERROR* atombios stuck executing F8B6 (len 485, WS 0, PS 4) @ 0xF8F7
PM: resume of devices complete after 12741.575 msecs
)


Thanks,

Mikko


> 
> Rafael
> 
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PCI: Fix regression in pci_restore_state(), v4
> 
> Commit 26f41062f28de65e11d3cf353e52d0be73442be1
> 
>     PCI: check for pci bar restore completion and retry
> 
> attempted to address problems with PCI BAR restoration on systems
> where FLR had not been completed before pci_restore_state() was
> called, but it did that in an utterly wrong way.
> 
> First off, instead of retrying the writes for the BAR registers
> only, it did that for all of the PCI config space of the device,
> including the status registers (whose values after the write quite
> obviously need not be the same as the written ones).  Second, it
> added arbitrary delay to pci_restore_state() even for systems
> where the PCI config space restoration was successful at first
> attempt.  Finally, the mdelay(10) it added to every iteration of the
> writing loop was way too much of a delay for any reasonable device.
> 
> All of this actually caused resume failures for some devices on
> the Mikko's system.
> 
> To fix the regression, make pci_restore_state() only retry the
> writes to BAR registers and only for devices with Type 0 headers.
> Moreover, make it wait only if the next first read from the given
> register doesn't return the value written to it.  Finally, make it
> wait for 1 ms, instead of 10 ms, after every failing attempt to write
> into the device's config space.
> 
> Reported-by: Mikko Vinni <mmvinni@yahoo.com>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
> drivers/pci/pci.c |   67 +++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 46 insertions(+), 21 deletions(-)
> 
> Index: linux/drivers/pci/pci.c
> ===================================================================
> --- linux.orig/drivers/pci/pci.c
> +++ linux/drivers/pci/pci.c
> @@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
>     return 0;
> }
> 
> +static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
> +                     u32 saved_val, int retry)
> +{
> +    u32 val;
> +
> +    pci_read_config_dword(pdev, offset, &val);
> +    if (val == saved_val)
> +        return;
> +
> +    for (;;) {
> +        dev_dbg(&pdev->dev, "restoring config space at offset 
> "
> +            "%#x (was %#x, writing %#x)\n", offset, val, 
> saved_val);
> +        pci_write_config_dword(pdev, offset, saved_val);
> +        if (retry-- <= 0)
> +            return;
> +
> +        pci_read_config_dword(pdev, offset, &val);
> +        if (val == saved_val)
> +            return;
> +
> +        mdelay(1);
> +    }
> +}
> +
> +static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
> +                     int retry)
> +{
> +    int index;
> +
> +    for (index = end; index >= start; index--)
> +        pci_restore_config_dword(pdev, 4 * index,
> +                     pdev->saved_config_space[index],
> +                     retry);
> +}
> +
> /** 
>   * pci_restore_state - Restore the saved state of a PCI device
>   * @dev: - PCI device that we're dealing with
>   */
> void pci_restore_state(struct pci_dev *dev)
> {
> -    int i;
> -    u32 val;
> -    int tries;
> -
>     if (!dev->state_saved)
>         return;
> 
> @@ -984,24 +1015,18 @@ void pci_restore_state(struct pci_dev *d
>     pci_restore_pcie_state(dev);
>     pci_restore_ats_state(dev);
> 
> -    /*
> -     * The Base Address register should be programmed before the command
> -     * register(s)
> -     */
> -    for (i = 15; i >= 0; i--) {
> -        pci_read_config_dword(dev, i * 4, &val);
> -        tries = 10;        
> -        while (tries && val != dev->saved_config_space[i]) {
> -            dev_dbg(&dev->dev, "restoring config "
> -                "space at offset %#x (was %#x, writing %#x)\n",
> -                i, val, (int)dev->saved_config_space[i]);
> -            pci_write_config_dword(dev,i * 4,
> -                dev->saved_config_space[i]);
> -            pci_read_config_dword(dev, i * 4, &val);
> -            mdelay(10);
> -            tries--;
> -        }
> +    if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
> +        pci_restore_config_space(dev, 10, 15, 0);
> +        /*
> +         * The Base Address register should be programmed before the
> +         * command register(s)
> +         */
> +        pci_restore_config_space(dev, 4, 9, 10);
> +        pci_restore_config_space(dev, 0, 3, 0);
> +    } else {
> +        pci_restore_config_space(dev, 0, 15, 0);
>     }
> +
>     pci_restore_pcix_state(dev);
>     pci_restore_msi_state(dev);
>     pci_restore_iov_state(dev);
> 

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Rafael J. Wysocki]

On Monday, April 16, 2012, Mikko Vinni wrote:
> From: Rafael J. Wysocki
> 
> >>  OK, I know.  There simply are fewer BARs for bridges (and PCIe ports)
> >>  and here we're attempting to overwrite the secondary status register.  
> > Sigh.
> >> 
> >>  Well, I guess we should only retry the writes to BARs for Type 0 headers.
> > 
> > Which is done by the appended updated patch.  Can you please give it a try?
> 
> 
> Ok, I tested v4 and also v3, which is in 3.4-rc3. On this laptop both versions
> work equally: touchpad works after resume (no help for the card reader
> problem, though).

[...]

> Difference from v3:
> --- /tmp/v3-uniq        2012-04-16 09:57:42.880521878 +0300
> +++ /tmp/v4-uniq        2012-04-16 09:58:03.880545386 +0300
> @@ -1019,11 +1018,11 @@
>        1 Booting Node 0 Processor 1 APIC 0x1
>        1 CPU1 is up
>        1 ACPI: Waking up from system sleep state S3
> -     11 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 0x20005151, writing 0x5151)
> +      1 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 0x20005151, writing 0x5151)

OK, so clearly we need to prevent the writes into the secondary status register
of the PCIe port from being repreated pointlessly.

Can you please test the appended patch from completness on top of 3.4-rc3?

Rafael


---
 drivers/pci/pci.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Index: linux/drivers/pci/pci.c
===================================================================
--- linux.orig/drivers/pci/pci.c
+++ linux/drivers/pci/pci.c
@@ -1015,13 +1015,17 @@ void pci_restore_state(struct pci_dev *d
 	pci_restore_pcie_state(dev);
 	pci_restore_ats_state(dev);
 
-	pci_restore_config_space(dev, 10, 15, 0);
-	/*
-	 * The Base Address register should be programmed before the command
-	 * register(s)
-	 */
-	pci_restore_config_space(dev, 4, 9, 10);
-	pci_restore_config_space(dev, 0, 3, 0);
+	if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
+		pci_restore_config_space(dev, 10, 15, 0);
+		/*
+		 * The Base Address register should be programmed before the
+		 * command register(s)
+		 */
+		pci_restore_config_space(dev, 4, 9, 10);
+		pci_restore_config_space(dev, 0, 3, 0);
+	} else {
+		pci_restore_config_space(dev, 0, 15, 0);
+	}
 
 	pci_restore_pcix_state(dev);
 	pci_restore_msi_state(dev);

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Mikko Vinni]

From: Rafael J. Wysocki:

>>  Ok, I tested v4 and also v3, which is in 3.4-rc3. On this laptop both 
> versions
>>  work equally: touchpad works after resume (no help for the card reader
>>  problem, though).
> 
> [...]
> 
>>  Difference from v3:
>>  --- /tmp/v3-uniq        2012-04-16 09:57:42.880521878 +0300
>>  +++ /tmp/v4-uniq        2012-04-16 09:58:03.880545386 +0300
>>  @@ -1019,11 +1018,11 @@
>>         1 Booting Node 0 Processor 1 APIC 0x1
>>         1 CPU1 is up
>>         1 ACPI: Waking up from system sleep state S3
>>  -     11 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 
> 0x20005151, writing 0x5151)
>>  +      1 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 
> 0x20005151, writing 0x5151)
> 
> OK, so clearly we need to prevent the writes into the secondary status register
> of the PCIe port from being repreated pointlessly.
> 
> Can you please test the appended patch from completness on top of 3.4-rc3?

I think that's what I did already :)

git pull to 3.4-rc3
(test, result as v3 output/diff in the previous email)
Revert "PCI: Fix regression in pci_restore_state(), v3"
apply v4 of the patch
(test, result as v4 output in the previous email)
git diff v3.4-rc3 -- drivers/pci/pci.c --> matches the patch below


Mikko


> 
> Rafael
> 
> 
> ---
> drivers/pci/pci.c |   18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
> 
> Index: linux/drivers/pci/pci.c
> ===================================================================
> --- linux.orig/drivers/pci/pci.c
> +++ linux/drivers/pci/pci.c
> @@ -1015,13 +1015,17 @@ void pci_restore_state(struct pci_dev *d
>     pci_restore_pcie_state(dev);
>     pci_restore_ats_state(dev);
> 
> -    pci_restore_config_space(dev, 10, 15, 0);
> -    /*
> -     * The Base Address register should be programmed before the command
> -     * register(s)
> -     */
> -    pci_restore_config_space(dev, 4, 9, 10);
> -    pci_restore_config_space(dev, 0, 3, 0);
> +    if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
> +        pci_restore_config_space(dev, 10, 15, 0);
> +        /*
> +         * The Base Address register should be programmed before the
> +         * command register(s)
> +         */
> +        pci_restore_config_space(dev, 4, 9, 10);
> +        pci_restore_config_space(dev, 0, 3, 0);
> +    } else {
> +        pci_restore_config_space(dev, 0, 15, 0);
> +    }
> 
>     pci_restore_pcix_state(dev);
>     pci_restore_msi_state(dev);
> 

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Rafael J. Wysocki]

On Monday, April 16, 2012, Mikko Vinni wrote:
> From: Rafael J. Wysocki:
> 
> >>  Ok, I tested v4 and also v3, which is in 3.4-rc3. On this laptop both 
> > versions
> >>  work equally: touchpad works after resume (no help for the card reader
> >>  problem, though).
> > 
> > [...]
> > 
> >>  Difference from v3:
> >>  --- /tmp/v3-uniq        2012-04-16 09:57:42.880521878 +0300
> >>  +++ /tmp/v4-uniq        2012-04-16 09:58:03.880545386 +0300
> >>  @@ -1019,11 +1018,11 @@
> >>         1 Booting Node 0 Processor 1 APIC 0x1
> >>         1 CPU1 is up
> >>         1 ACPI: Waking up from system sleep state S3
> >>  -     11 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 
> > 0x20005151, writing 0x5151)
> >>  +      1 pcieport 0000:00:02.0: restoring config space at offset 0x1c (was 
> > 0x20005151, writing 0x5151)
> > 
> > OK, so clearly we need to prevent the writes into the secondary status register
> > of the PCIe port from being repreated pointlessly.
> > 
> > Can you please test the appended patch from completness on top of 3.4-rc3?
> 
> I think that's what I did already :)
> 
> git pull to 3.4-rc3
> (test, result as v3 output/diff in the previous email)
> Revert "PCI: Fix regression in pci_restore_state(), v3"
> apply v4 of the patch
> (test, result as v4 output in the previous email)
> git diff v3.4-rc3 -- drivers/pci/pci.c --> matches the patch below

Well, that's correct. :-)

Thanks a lot anyway,
Rafael

[PATCH] PCI: Retry BARs restoration for Type 0 headers only

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Some shortcomings introduced into pci_restore_state() by commit
26f41062f28d ("PCI: check for pci bar restore completion and
retry") have been fixed by recent commit ebfc5b802fa76 ("PCI: Fix
regression in pci_restore_state(), v3"), but that commit treats
all PCI devices as those with Type 0 configuration headers.  That
is not entirely correct, because Type 1 and Type 2 headers have
different layouts.  In particular, the area occupied by BARs in
Type 0 config headers contains the secondary status register in
Type 1 ones and it doesn't make sense to retry the restoration of
that register even if the value read back from it after a write is
not the same as the written one (it very well may be different).

For this reason, make pci_restore_state() only retry the restoration
of BARs for Type 0 config headers.  This effectively makes it behave
as before commit 26f41062f28d for all header types except for Type 0.

Tested-by: Mikko Vinni <mmvinni@yahoo.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Index: linux/drivers/pci/pci.c
===================================================================
--- linux.orig/drivers/pci/pci.c
+++ linux/drivers/pci/pci.c
@@ -1015,13 +1015,17 @@ void pci_restore_state(struct pci_dev *d
 	pci_restore_pcie_state(dev);
 	pci_restore_ats_state(dev);
 
-	pci_restore_config_space(dev, 10, 15, 0);
-	/*
-	 * The Base Address register should be programmed before the command
-	 * register(s)
-	 */
-	pci_restore_config_space(dev, 4, 9, 10);
-	pci_restore_config_space(dev, 0, 3, 0);
+	if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
+		pci_restore_config_space(dev, 10, 15, 0);
+		/*
+		 * The Base Address register should be programmed before the
+		 * command register(s)
+		 */
+		pci_restore_config_space(dev, 4, 9, 10);
+		pci_restore_config_space(dev, 0, 3, 0);
+	} else {
+		pci_restore_config_space(dev, 0, 15, 0);
+	}
 
 	pci_restore_pcix_state(dev);
 	pci_restore_msi_state(dev);

Re: [PATCH] PCI: Retry BARs restoration for Type 0 headers only

[Linus Torvalds]

On Mon, Apr 16, 2012 at 1:35 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> Some shortcomings introduced into pci_restore_state() by commit
> 26f41062f28d ("PCI: check for pci bar restore completion and
> retry") have been fixed by recent commit ebfc5b802fa76 ("PCI: Fix
> regression in pci_restore_state(), v3"), but that commit treats
> all PCI devices as those with Type 0 configuration headers.

Make me happier and just make all of this a helper function again.
Call that helper function pci_restore_config_space(), and make the
existing "pci_restore_config_space()" be called
"pci_restore_config_space_range()" or something. Ok?

                   Linus

Re: [PATCH] PCI: Retry BARs restoration for Type 0 headers only

[Rafael J. Wysocki]

On Monday, April 16, 2012, Linus Torvalds wrote:
> On Mon, Apr 16, 2012 at 1:35 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >
> > Some shortcomings introduced into pci_restore_state() by commit
> > 26f41062f28d ("PCI: check for pci bar restore completion and
> > retry") have been fixed by recent commit ebfc5b802fa76 ("PCI: Fix
> > regression in pci_restore_state(), v3"), but that commit treats
> > all PCI devices as those with Type 0 configuration headers.
> 
> Make me happier and just make all of this a helper function again.
> Call that helper function pci_restore_config_space(), and make the
> existing "pci_restore_config_space()" be called
> "pci_restore_config_space_range()" or something. Ok?

Something like this, you mean?

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PCI: Retry BARs restoration for Type 0 headers only

Some shortcomings introduced into pci_restore_state() by commit
26f41062f28d ("PCI: check for pci bar restore completion and
retry") have been fixed by recent commit ebfc5b802fa76 ("PCI: Fix
regression in pci_restore_state(), v3"), but that commit treats
all PCI devices as those with Type 0 configuration headers.  That
is not entirely correct, because Type 1 and Type 2 headers have
different layouts.  In particular, the area occupied by BARs in
Type 0 config headers contains the secondary status register in
Type 1 ones and it doesn't make sense to retry the restoration of
that register even if the value read back from it after a write is
not the same as the written one (it very well may be different).

For this reason, make pci_restore_state() only retry the restoration
of BARs for Type 0 config headers.  This effectively makes it behave
as before commit 26f41062f28d for all header types except for Type 0.

Tested-by: Mikko Vinni <mmvinni@yahoo.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Index: linux/drivers/pci/pci.c
===================================================================
--- linux.orig/drivers/pci/pci.c
+++ linux/drivers/pci/pci.c
@@ -991,8 +991,8 @@ static void pci_restore_config_dword(str
 	}
 }
 
-static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
-				     int retry)
+static void pci_restore_config_space_range(struct pci_dev *pdev,
+					   int start, int end, int retry)
 {
 	int index;
 
@@ -1002,6 +1002,18 @@ static void pci_restore_config_space(str
 					 retry);
 }
 
+static void pci_restore_config_space(struct pci_dev *pdev)
+{
+	if (pdev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
+		pci_restore_config_space_range(pdev, 10, 15, 0);
+		/* Restore BARs before the command register. */
+		pci_restore_config_space_range(pdev, 4, 9, 10);
+		pci_restore_config_space_range(pdev, 0, 3, 0);
+	} else {
+		pci_restore_config_space_range(pdev, 0, 15, 0);
+	}
+}
+
 /** 
  * pci_restore_state - Restore the saved state of a PCI device
  * @dev: - PCI device that we're dealing with
@@ -1015,13 +1027,7 @@ void pci_restore_state(struct pci_dev *d
 	pci_restore_pcie_state(dev);
 	pci_restore_ats_state(dev);
 
-	pci_restore_config_space(dev, 10, 15, 0);
-	/*
-	 * The Base Address register should be programmed before the command
-	 * register(s)
-	 */
-	pci_restore_config_space(dev, 4, 9, 10);
-	pci_restore_config_space(dev, 0, 3, 0);
+	pci_restore_config_space(dev);
 
 	pci_restore_pcix_state(dev);
 	pci_restore_msi_state(dev);

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Mikko Vinni]

From: Rafael J. Wysocki

>
> Do you still have the kernel where all PCI devices worked correctly after system
> resume?

If you are talking about https://bugzilla.kernel.org/show_bug.cgi?id=35452,
I am not aware of such a kernel. That bug has existed for as long as I have
cared to test.

Testing the patch v4 soon.


Mikko

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Rafael J. Wysocki]

On Monday, April 16, 2012, Mikko Vinni wrote:
> From: Rafael J. Wysocki
> 
> >
> > Do you still have the kernel where all PCI devices worked correctly after system
> > resume?
> 
> If you are talking about https://bugzilla.kernel.org/show_bug.cgi?id=35452,
> I am not aware of such a kernel. That bug has existed for as long as I have
> cared to test.

OK, let's try to debug it a bit more in the Bugzilla entry.

> Testing the patch v4 soon.

Thanks!

[PATCH] PCI: Fix regression in pci_restore_state()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Commit 26f41062f28de65e11d3cf353e52d0be73442be1

    PCI: check for pci bar restore completion and retry

attempted to address problems with PCI BAR restoration on systems
where FLR had not been completed before pci_restore_state() was
called, but it did that in an utterly wrong way.

First off, instead of retrying the writes for the BAR registers
only, it did that for all of the PCI config space of the device,
including the status register (whose value after the write quite
obviously need not be the same as the written one).  Second, it
added arbitrary delay to pci_restore_state() even for systems
where the PCI config space restoration was successful at first
attempt.  Finally, it used mdelay() for that, although there's
no rule stating that pci_restore_state() could not sleep (at least
I'm not aware of such a rule).

All of this actually caused resume failures for some devices on
the Mikko's system.

To fix the regression, make pci_restore_state() only retry the
writes for BAR registers and only wait if the first read from
the register doesn't return the written value.  Additionally, make
it use msleep() instead of mdelay() for waiting.

Reported-and-tested-by: Mikko Vinni <mmvinni@yahoo.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c |   50 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 18 deletions(-)

Index: linux/drivers/pci/pci.c
===================================================================
--- linux.orig/drivers/pci/pci.c
+++ linux/drivers/pci/pci.c
@@ -967,16 +967,40 @@ pci_save_state(struct pci_dev *dev)
 	return 0;
 }
 
+static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
+				     int retry)
+{
+	int index;
+
+	for (index = end; index >= start; index--) {
+		u32 val, saved;
+		int offset = 4 * index;
+		int count = retry;
+
+		saved = pdev->saved_config_space[index];
+		pci_read_config_dword(pdev, offset, &val);
+		while (saved != val) {
+			dev_dbg(&pdev->dev, "restoring config space at offset "
+				"%#x (was %#x, writing %#x)\n",
+				offset, val, saved);
+			pci_write_config_dword(pdev, offset, saved);
+			if (count-- > 0) {
+				pci_read_config_dword(pdev, offset, &val);
+				if (saved != val)
+					msleep(10);
+			} else {
+				break;
+			}
+		}
+	}
+}
+
 /** 
  * pci_restore_state - Restore the saved state of a PCI device
  * @dev: - PCI device that we're dealing with
  */
 void pci_restore_state(struct pci_dev *dev)
 {
-	int i;
-	u32 val;
-	int tries;
-
 	if (!dev->state_saved)
 		return;
 
@@ -984,24 +1008,14 @@ void pci_restore_state(struct pci_dev *d
 	pci_restore_pcie_state(dev);
 	pci_restore_ats_state(dev);
 
+	pci_restore_config_space(dev, 10, 15, 0);
 	/*
 	 * The Base Address register should be programmed before the command
 	 * register(s)
 	 */
-	for (i = 15; i >= 0; i--) {
-		pci_read_config_dword(dev, i * 4, &val);
-		tries = 10;		
-		while (tries && val != dev->saved_config_space[i]) {
-			dev_dbg(&dev->dev, "restoring config "
-				"space at offset %#x (was %#x, writing %#x)\n",
-				i, val, (int)dev->saved_config_space[i]);
-			pci_write_config_dword(dev,i * 4,
-				dev->saved_config_space[i]);
-			pci_read_config_dword(dev, i * 4, &val);
-			mdelay(10);
-			tries--;
-		}
-	}
+	pci_restore_config_space(dev, 4, 9, 10);
+	pci_restore_config_space(dev, 0, 3, 0);
+
 	pci_restore_pcix_state(dev);
 	pci_restore_msi_state(dev);
 	pci_restore_iov_state(dev);

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Linus Torvalds]

On Sun, Apr 15, 2012 at 11:32 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>
> To fix the regression, make pci_restore_state() only retry the
> writes for BAR registers and only wait if the first read from
> the register doesn't return the written value.  Additionally, make
> it use msleep() instead of mdelay() for waiting.

WHAT?

msleep() is the one that can sleep. So that part of the change is just
crazy. There's no way you can sleep during PCI register restore.

                       Linus

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Rafael J. Wysocki]

On Sunday, April 15, 2012, Linus Torvalds wrote:
> On Sun, Apr 15, 2012 at 11:32 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >
> > To fix the regression, make pci_restore_state() only retry the
> > writes for BAR registers and only wait if the first read from
> > the register doesn't return the written value.  Additionally, make
> > it use msleep() instead of mdelay() for waiting.
> 
> WHAT?
> 
> msleep() is the one that can sleep. So that part of the change is just
> crazy. There's no way you can sleep during PCI register restore.

OK, point taken.

mdelay(10) doesn't really look good either to me in this case, though.

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PCI: Fix regression in pci_restore_state(), v2

Commit 26f41062f28de65e11d3cf353e52d0be73442be1

    PCI: check for pci bar restore completion and retry

attempted to address problems with PCI BAR restoration on systems
where FLR had not been completed before pci_restore_state() was
called, but it did that in an utterly wrong way.

First off, instead of retrying the writes for the BAR registers
only, it did that for all of the PCI config space of the device,
including the status register (whose value after the write quite
obviously need not be the same as the written one).  Second, it
added arbitrary delay to pci_restore_state() even for systems
where the PCI config space restoration was successful at first
attempt.

All of this actually caused resume failures for some devices on
the Mikko's system.

To fix the regression, make pci_restore_state() only retry the
writes for BAR registers and only wait if the first read from
the register doesn't return the written value.

Reported-by: Mikko Vinni <mmvinni@yahoo.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c |   50 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 18 deletions(-)

Index: linux/drivers/pci/pci.c
===================================================================
--- linux.orig/drivers/pci/pci.c
+++ linux/drivers/pci/pci.c
@@ -967,16 +967,40 @@ pci_save_state(struct pci_dev *dev)
 	return 0;
 }
 
+static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
+				     int retry)
+{
+	int index;
+
+	for (index = end; index >= start; index--) {
+		u32 val, saved;
+		int offset = 4 * index;
+		int count = retry;
+
+		saved = pdev->saved_config_space[index];
+		pci_read_config_dword(pdev, offset, &val);
+		while (saved != val) {
+			dev_dbg(&pdev->dev, "restoring config space at offset "
+				"%#x (was %#x, writing %#x)\n",
+				offset, val, saved);
+			pci_write_config_dword(pdev, offset, saved);
+			if (count-- > 0) {
+				pci_read_config_dword(pdev, offset, &val);
+				if (saved != val)
+					mdelay(10);
+			} else {
+				break;
+			}
+		}
+	}
+}
+
 /** 
  * pci_restore_state - Restore the saved state of a PCI device
  * @dev: - PCI device that we're dealing with
  */
 void pci_restore_state(struct pci_dev *dev)
 {
-	int i;
-	u32 val;
-	int tries;
-
 	if (!dev->state_saved)
 		return;
 
@@ -984,24 +1008,14 @@ void pci_restore_state(struct pci_dev *d
 	pci_restore_pcie_state(dev);
 	pci_restore_ats_state(dev);
 
+	pci_restore_config_space(dev, 10, 15, 0);
 	/*
 	 * The Base Address register should be programmed before the command
 	 * register(s)
 	 */
-	for (i = 15; i >= 0; i--) {
-		pci_read_config_dword(dev, i * 4, &val);
-		tries = 10;		
-		while (tries && val != dev->saved_config_space[i]) {
-			dev_dbg(&dev->dev, "restoring config "
-				"space at offset %#x (was %#x, writing %#x)\n",
-				i, val, (int)dev->saved_config_space[i]);
-			pci_write_config_dword(dev,i * 4,
-				dev->saved_config_space[i]);
-			pci_read_config_dword(dev, i * 4, &val);
-			mdelay(10);
-			tries--;
-		}
-	}
+	pci_restore_config_space(dev, 4, 9, 10);
+	pci_restore_config_space(dev, 0, 3, 0);
+
 	pci_restore_pcix_state(dev);
 	pci_restore_msi_state(dev);
 	pci_restore_iov_state(dev);

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Linus Torvalds]

On Sun, Apr 15, 2012 at 11:47 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>
> mdelay(10) doesn't really look good either to me in this case, though.

Oh, I agree. What kind of ass-backwards device actually needs that
kind of crazy delays? It is almost certainly buggy.

With retries, 10ms delays are totally unacceptable. There's something wrong.

A single ms *may* be ok.

Anyway, can you also split the actual "write _one_ register with
retry" into a function of its own? The code looks like crap with those
multiple levels of looping, with conditionals inside them etc. With a
simple helper function, you could change the break into return, and it
would look much better, I bet.

                    Linus

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Rafael J. Wysocki]

On Sunday, April 15, 2012, Linus Torvalds wrote:
> On Sun, Apr 15, 2012 at 11:47 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >
> > mdelay(10) doesn't really look good either to me in this case, though.
> 
> Oh, I agree. What kind of ass-backwards device actually needs that
> kind of crazy delays? It is almost certainly buggy.
> 
> With retries, 10ms delays are totally unacceptable. There's something wrong.
> 
> A single ms *may* be ok.
> 
> Anyway, can you also split the actual "write _one_ register with
> retry" into a function of its own? The code looks like crap with those
> multiple levels of looping, with conditionals inside them etc. With a
> simple helper function, you could change the break into return, and it
> would look much better, I bet.

Sure.  It appears cleaner this way.

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PCI: Fix regression in pci_restore_state(), v3

Commit 26f41062f28de65e11d3cf353e52d0be73442be1

    PCI: check for pci bar restore completion and retry

attempted to address problems with PCI BAR restoration on systems
where FLR had not been completed before pci_restore_state() was
called, but it did that in an utterly wrong way.

First off, instead of retrying the writes for the BAR registers
only, it did that for all of the PCI config space of the device,
including the status register (whose value after the write quite
obviously need not be the same as the written one).  Second, it
added arbitrary delay to pci_restore_state() even for systems
where the PCI config space restoration was successful at first
attempt.  Finally, the mdelay(10) it added to every iteration of the
writing loop was way too much of a delay for any reasonable device.

All of this actually caused resume failures for some devices on
the Mikko's system.

To fix the regression, make pci_restore_state() only retry the
writes for BAR registers and only wait if the first read from
the register doesn't return the written value.  Additionaly, make
it wait for 1 ms, instead of 10 ms, after every failing attempt
to write into config space.

Reported-by: Mikko Vinni <mmvinni@yahoo.com>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/pci/pci.c |   57 ++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 18 deletions(-)

Index: linux/drivers/pci/pci.c
===================================================================
--- linux.orig/drivers/pci/pci.c
+++ linux/drivers/pci/pci.c
@@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
 	return 0;
 }
 
+static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
+				     u32 saved_val, int retry)
+{
+	u32 val;
+
+	pci_read_config_dword(pdev, offset, &val);
+	if (val == saved_val)
+		return;
+
+	for (;;) {
+		dev_dbg(&pdev->dev, "restoring config space at offset "
+			"%#x (was %#x, writing %#x)\n", offset, val, saved_val);
+		pci_write_config_dword(pdev, offset, saved_val);
+		if (retry-- <= 0)
+			return;
+
+		pci_read_config_dword(pdev, offset, &val);
+		if (val == saved_val)
+			return;
+
+		mdelay(1);
+	}
+}
+
+static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
+				     int retry)
+{
+	int index;
+
+	for (index = end; index >= start; index--)
+		pci_restore_config_dword(pdev, 4 * index,
+					 pdev->saved_config_space[index],
+					 retry);
+}
+
 /** 
  * pci_restore_state - Restore the saved state of a PCI device
  * @dev: - PCI device that we're dealing with
  */
 void pci_restore_state(struct pci_dev *dev)
 {
-	int i;
-	u32 val;
-	int tries;
-
 	if (!dev->state_saved)
 		return;
 
@@ -984,24 +1015,14 @@ void pci_restore_state(struct pci_dev *d
 	pci_restore_pcie_state(dev);
 	pci_restore_ats_state(dev);
 
+	pci_restore_config_space(dev, 10, 15, 0);
 	/*
 	 * The Base Address register should be programmed before the command
 	 * register(s)
 	 */
-	for (i = 15; i >= 0; i--) {
-		pci_read_config_dword(dev, i * 4, &val);
-		tries = 10;		
-		while (tries && val != dev->saved_config_space[i]) {
-			dev_dbg(&dev->dev, "restoring config "
-				"space at offset %#x (was %#x, writing %#x)\n",
-				i, val, (int)dev->saved_config_space[i]);
-			pci_write_config_dword(dev,i * 4,
-				dev->saved_config_space[i]);
-			pci_read_config_dword(dev, i * 4, &val);
-			mdelay(10);
-			tries--;
-		}
-	}
+	pci_restore_config_space(dev, 4, 9, 10);
+	pci_restore_config_space(dev, 0, 3, 0);
+
 	pci_restore_pcix_state(dev);
 	pci_restore_msi_state(dev);
 	pci_restore_iov_state(dev);

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Bjorn Helgaas]

On Sun, Apr 15, 2012 at 1:40 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday, April 15, 2012, Linus Torvalds wrote:
>> On Sun, Apr 15, 2012 at 11:47 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >
>> > mdelay(10) doesn't really look good either to me in this case, though.
>>
>> Oh, I agree. What kind of ass-backwards device actually needs that
>> kind of crazy delays? It is almost certainly buggy.
>>
>> With retries, 10ms delays are totally unacceptable. There's something wrong.
>>
>> A single ms *may* be ok.
>>
>> Anyway, can you also split the actual "write _one_ register with
>> retry" into a function of its own? The code looks like crap with those
>> multiple levels of looping, with conditionals inside them etc. With a
>> simple helper function, you could change the break into return, and it
>> would look much better, I bet.
>
> Sure.  It appears cleaner this way.
>
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PCI: Fix regression in pci_restore_state(), v3
>
> Commit 26f41062f28de65e11d3cf353e52d0be73442be1
>
>    PCI: check for pci bar restore completion and retry
>
> attempted to address problems with PCI BAR restoration on systems
> where FLR had not been completed before pci_restore_state() was
> called, but it did that in an utterly wrong way.
>
> First off, instead of retrying the writes for the BAR registers
> only, it did that for all of the PCI config space of the device,
> including the status register (whose value after the write quite
> obviously need not be the same as the written one).  Second, it
> added arbitrary delay to pci_restore_state() even for systems
> where the PCI config space restoration was successful at first
> attempt.  Finally, the mdelay(10) it added to every iteration of the
> writing loop was way too much of a delay for any reasonable device.
>
> All of this actually caused resume failures for some devices on
> the Mikko's system.
>
> To fix the regression, make pci_restore_state() only retry the
> writes for BAR registers and only wait if the first read from
> the register doesn't return the written value.  Additionaly, make
> it wait for 1 ms, instead of 10 ms, after every failing attempt
> to write into config space.
>
> Reported-by: Mikko Vinni <mmvinni@yahoo.com>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/pci/pci.c |   57 ++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 39 insertions(+), 18 deletions(-)
>
> Index: linux/drivers/pci/pci.c
> ===================================================================
> --- linux.orig/drivers/pci/pci.c
> +++ linux/drivers/pci/pci.c
> @@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
>        return 0;
>  }
>
> +static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
> +                                    u32 saved_val, int retry)
> +{
> +       u32 val;
> +
> +       pci_read_config_dword(pdev, offset, &val);
> +       if (val == saved_val)
> +               return;
> +
> +       for (;;) {
> +               dev_dbg(&pdev->dev, "restoring config space at offset "
> +                       "%#x (was %#x, writing %#x)\n", offset, val, saved_val);
> +               pci_write_config_dword(pdev, offset, saved_val);
> +               if (retry-- <= 0)
> +                       return;
> +
> +               pci_read_config_dword(pdev, offset, &val);
> +               if (val == saved_val)
> +                       return;
> +
> +               mdelay(1);
> +       }
> +}
> +
> +static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
> +                                    int retry)
> +{
> +       int index;
> +
> +       for (index = end; index >= start; index--)
> +               pci_restore_config_dword(pdev, 4 * index,
> +                                        pdev->saved_config_space[index],
> +                                        retry);
> +}
> +
>  /**
>  * pci_restore_state - Restore the saved state of a PCI device
>  * @dev: - PCI device that we're dealing with
>  */
>  void pci_restore_state(struct pci_dev *dev)
>  {
> -       int i;
> -       u32 val;
> -       int tries;
> -
>        if (!dev->state_saved)
>                return;
>
> @@ -984,24 +1015,14 @@ void pci_restore_state(struct pci_dev *d
>        pci_restore_pcie_state(dev);
>        pci_restore_ats_state(dev);
>
> +       pci_restore_config_space(dev, 10, 15, 0);
>        /*
>         * The Base Address register should be programmed before the command
>         * register(s)
>         */
> -       for (i = 15; i >= 0; i--) {
> -               pci_read_config_dword(dev, i * 4, &val);
> -               tries = 10;
> -               while (tries && val != dev->saved_config_space[i]) {
> -                       dev_dbg(&dev->dev, "restoring config "
> -                               "space at offset %#x (was %#x, writing %#x)\n",
> -                               i, val, (int)dev->saved_config_space[i]);
> -                       pci_write_config_dword(dev,i * 4,
> -                               dev->saved_config_space[i]);
> -                       pci_read_config_dword(dev, i * 4, &val);
> -                       mdelay(10);
> -                       tries--;
> -               }
> -       }
> +       pci_restore_config_space(dev, 4, 9, 10);
> +       pci_restore_config_space(dev, 0, 3, 0);
> +
>        pci_restore_pcix_state(dev);
>        pci_restore_msi_state(dev);
>        pci_restore_iov_state(dev);

I'd feel better about this if there were a way to delay in the FLR
path instead.  If we delay in the restore path, we're only fixing one
of the many ways config space can be written.  Other paths that write
config space will just silently fail.

The PCIe spec (r3.0, sec 6.6.2) mentions waiting for the "pre-FLR
value for Completion Timeout," but I don't see anything that looks
like that in pcie_flr() or pci_af_flr().  Are there any other direct
ways we can detect when the FLR is complete?

Bjorn

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Rafael J. Wysocki]

On Monday, April 23, 2012, Bjorn Helgaas wrote:
> On Sun, Apr 15, 2012 at 1:40 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday, April 15, 2012, Linus Torvalds wrote:
> >> On Sun, Apr 15, 2012 at 11:47 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >
> >> > mdelay(10) doesn't really look good either to me in this case, though.
> >>
> >> Oh, I agree. What kind of ass-backwards device actually needs that
> >> kind of crazy delays? It is almost certainly buggy.
> >>
> >> With retries, 10ms delays are totally unacceptable. There's something wrong.
> >>
> >> A single ms *may* be ok.
> >>
> >> Anyway, can you also split the actual "write _one_ register with
> >> retry" into a function of its own? The code looks like crap with those
> >> multiple levels of looping, with conditionals inside them etc. With a
> >> simple helper function, you could change the break into return, and it
> >> would look much better, I bet.
> >
> > Sure.  It appears cleaner this way.
> >
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: PCI: Fix regression in pci_restore_state(), v3
> >
> > Commit 26f41062f28de65e11d3cf353e52d0be73442be1
> >
> >    PCI: check for pci bar restore completion and retry
> >
> > attempted to address problems with PCI BAR restoration on systems
> > where FLR had not been completed before pci_restore_state() was
> > called, but it did that in an utterly wrong way.
> >
> > First off, instead of retrying the writes for the BAR registers
> > only, it did that for all of the PCI config space of the device,
> > including the status register (whose value after the write quite
> > obviously need not be the same as the written one).  Second, it
> > added arbitrary delay to pci_restore_state() even for systems
> > where the PCI config space restoration was successful at first
> > attempt.  Finally, the mdelay(10) it added to every iteration of the
> > writing loop was way too much of a delay for any reasonable device.
> >
> > All of this actually caused resume failures for some devices on
> > the Mikko's system.
> >
> > To fix the regression, make pci_restore_state() only retry the
> > writes for BAR registers and only wait if the first read from
> > the register doesn't return the written value.  Additionaly, make
> > it wait for 1 ms, instead of 10 ms, after every failing attempt
> > to write into config space.
> >
> > Reported-by: Mikko Vinni <mmvinni@yahoo.com>
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/pci/pci.c |   57 ++++++++++++++++++++++++++++++++++++------------------
> >  1 file changed, 39 insertions(+), 18 deletions(-)
> >
> > Index: linux/drivers/pci/pci.c
> > ===================================================================
> > --- linux.orig/drivers/pci/pci.c
> > +++ linux/drivers/pci/pci.c
> > @@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
> >        return 0;
> >  }
> >
> > +static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
> > +                                    u32 saved_val, int retry)
> > +{
> > +       u32 val;
> > +
> > +       pci_read_config_dword(pdev, offset, &val);
> > +       if (val == saved_val)
> > +               return;
> > +
> > +       for (;;) {
> > +               dev_dbg(&pdev->dev, "restoring config space at offset "
> > +                       "%#x (was %#x, writing %#x)\n", offset, val, saved_val);
> > +               pci_write_config_dword(pdev, offset, saved_val);
> > +               if (retry-- <= 0)
> > +                       return;
> > +
> > +               pci_read_config_dword(pdev, offset, &val);
> > +               if (val == saved_val)
> > +                       return;
> > +
> > +               mdelay(1);
> > +       }
> > +}
> > +
> > +static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
> > +                                    int retry)
> > +{
> > +       int index;
> > +
> > +       for (index = end; index >= start; index--)
> > +               pci_restore_config_dword(pdev, 4 * index,
> > +                                        pdev->saved_config_space[index],
> > +                                        retry);
> > +}
> > +
> >  /**
> >  * pci_restore_state - Restore the saved state of a PCI device
> >  * @dev: - PCI device that we're dealing with
> >  */
> >  void pci_restore_state(struct pci_dev *dev)
> >  {
> > -       int i;
> > -       u32 val;
> > -       int tries;
> > -
> >        if (!dev->state_saved)
> >                return;
> >
> > @@ -984,24 +1015,14 @@ void pci_restore_state(struct pci_dev *d
> >        pci_restore_pcie_state(dev);
> >        pci_restore_ats_state(dev);
> >
> > +       pci_restore_config_space(dev, 10, 15, 0);
> >        /*
> >         * The Base Address register should be programmed before the command
> >         * register(s)
> >         */
> > -       for (i = 15; i >= 0; i--) {
> > -               pci_read_config_dword(dev, i * 4, &val);
> > -               tries = 10;
> > -               while (tries && val != dev->saved_config_space[i]) {
> > -                       dev_dbg(&dev->dev, "restoring config "
> > -                               "space at offset %#x (was %#x, writing %#x)\n",
> > -                               i, val, (int)dev->saved_config_space[i]);
> > -                       pci_write_config_dword(dev,i * 4,
> > -                               dev->saved_config_space[i]);
> > -                       pci_read_config_dword(dev, i * 4, &val);
> > -                       mdelay(10);
> > -                       tries--;
> > -               }
> > -       }
> > +       pci_restore_config_space(dev, 4, 9, 10);
> > +       pci_restore_config_space(dev, 0, 3, 0);
> > +
> >        pci_restore_pcix_state(dev);
> >        pci_restore_msi_state(dev);
> >        pci_restore_iov_state(dev);
> 
> I'd feel better about this if there were a way to delay in the FLR
> path instead.  If we delay in the restore path, we're only fixing one
> of the many ways config space can be written.  Other paths that write
> config space will just silently fail.
> 
> The PCIe spec (r3.0, sec 6.6.2) mentions waiting for the "pre-FLR
> value for Completion Timeout," but I don't see anything that looks
> like that in pcie_flr() or pci_af_flr().  Are there any other direct
> ways we can detect when the FLR is complete?

I'm not aware of any.

Thanks,
Rafael

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Don Dutile]

On 04/23/2012 03:53 PM, Rafael J. Wysocki wrote:
> On Monday, April 23, 2012, Bjorn Helgaas wrote:
>> On Sun, Apr 15, 2012 at 1:40 PM, Rafael J. Wysocki<rjw@sisk.pl>  wrote:
>>> On Sunday, April 15, 2012, Linus Torvalds wrote:
>>>> On Sun, Apr 15, 2012 at 11:47 AM, Rafael J. Wysocki<rjw@sisk.pl>  wrote:
>>>>>
>>>>> mdelay(10) doesn't really look good either to me in this case, though.
>>>>
>>>> Oh, I agree. What kind of ass-backwards device actually needs that
>>>> kind of crazy delays? It is almost certainly buggy.
>>>>
>>>> With retries, 10ms delays are totally unacceptable. There's something wrong.
>>>>
>>>> A single ms *may* be ok.
>>>>
>>>> Anyway, can you also split the actual "write _one_ register with
>>>> retry" into a function of its own? The code looks like crap with those
>>>> multiple levels of looping, with conditionals inside them etc. With a
>>>> simple helper function, you could change the break into return, and it
>>>> would look much better, I bet.
>>>
>>> Sure.  It appears cleaner this way.
>>>
>>> ---
>>> From: Rafael J. Wysocki<rjw@sisk.pl>
>>> Subject: PCI: Fix regression in pci_restore_state(), v3
>>>
>>> Commit 26f41062f28de65e11d3cf353e52d0be73442be1
>>>
>>>     PCI: check for pci bar restore completion and retry
>>>
>>> attempted to address problems with PCI BAR restoration on systems
>>> where FLR had not been completed before pci_restore_state() was
>>> called, but it did that in an utterly wrong way.
>>>
>>> First off, instead of retrying the writes for the BAR registers
>>> only, it did that for all of the PCI config space of the device,
>>> including the status register (whose value after the write quite
>>> obviously need not be the same as the written one).  Second, it
>>> added arbitrary delay to pci_restore_state() even for systems
>>> where the PCI config space restoration was successful at first
>>> attempt.  Finally, the mdelay(10) it added to every iteration of the
>>> writing loop was way too much of a delay for any reasonable device.
>>>
>>> All of this actually caused resume failures for some devices on
>>> the Mikko's system.
>>>
>>> To fix the regression, make pci_restore_state() only retry the
>>> writes for BAR registers and only wait if the first read from
>>> the register doesn't return the written value.  Additionaly, make
>>> it wait for 1 ms, instead of 10 ms, after every failing attempt
>>> to write into config space.
>>>
>>> Reported-by: Mikko Vinni<mmvinni@yahoo.com>
>>> Signed-off-by: Rafael J. Wysocki<rjw@sisk.pl>
>>> ---
>>>   drivers/pci/pci.c |   57 ++++++++++++++++++++++++++++++++++++------------------
>>>   1 file changed, 39 insertions(+), 18 deletions(-)
>>>
>>> Index: linux/drivers/pci/pci.c
>>> ===================================================================
>>> --- linux.orig/drivers/pci/pci.c
>>> +++ linux/drivers/pci/pci.c
>>> @@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
>>>         return 0;
>>>   }
>>>
>>> +static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
>>> +                                    u32 saved_val, int retry)
>>> +{
>>> +       u32 val;
>>> +
>>> +       pci_read_config_dword(pdev, offset,&val);
>>> +       if (val == saved_val)
>>> +               return;
>>> +
>>> +       for (;;) {
>>> +               dev_dbg(&pdev->dev, "restoring config space at offset "
>>> +                       "%#x (was %#x, writing %#x)\n", offset, val, saved_val);
>>> +               pci_write_config_dword(pdev, offset, saved_val);
>>> +               if (retry--<= 0)
>>> +                       return;
>>> +
>>> +               pci_read_config_dword(pdev, offset,&val);
>>> +               if (val == saved_val)
>>> +                       return;
>>> +
>>> +               mdelay(1);
>>> +       }
>>> +}
>>> +
>>> +static void pci_restore_config_space(struct pci_dev *pdev, int start, int end,
>>> +                                    int retry)
>>> +{
>>> +       int index;
>>> +
>>> +       for (index = end; index>= start; index--)
>>> +               pci_restore_config_dword(pdev, 4 * index,
>>> +                                        pdev->saved_config_space[index],
>>> +                                        retry);
>>> +}
>>> +
>>>   /**
>>>   * pci_restore_state - Restore the saved state of a PCI device
>>>   * @dev: - PCI device that we're dealing with
>>>   */
>>>   void pci_restore_state(struct pci_dev *dev)
>>>   {
>>> -       int i;
>>> -       u32 val;
>>> -       int tries;
>>> -
>>>         if (!dev->state_saved)
>>>                 return;
>>>
>>> @@ -984,24 +1015,14 @@ void pci_restore_state(struct pci_dev *d
>>>         pci_restore_pcie_state(dev);
>>>         pci_restore_ats_state(dev);
>>>
>>> +       pci_restore_config_space(dev, 10, 15, 0);
>>>         /*
>>>          * The Base Address register should be programmed before the command
>>>          * register(s)
>>>          */
>>> -       for (i = 15; i>= 0; i--) {
>>> -               pci_read_config_dword(dev, i * 4,&val);
>>> -               tries = 10;
>>> -               while (tries&&  val != dev->saved_config_space[i]) {
>>> -                       dev_dbg(&dev->dev, "restoring config "
>>> -                               "space at offset %#x (was %#x, writing %#x)\n",
>>> -                               i, val, (int)dev->saved_config_space[i]);
>>> -                       pci_write_config_dword(dev,i * 4,
>>> -                               dev->saved_config_space[i]);
>>> -                       pci_read_config_dword(dev, i * 4,&val);
>>> -                       mdelay(10);
>>> -                       tries--;
>>> -               }
>>> -       }
>>> +       pci_restore_config_space(dev, 4, 9, 10);
>>> +       pci_restore_config_space(dev, 0, 3, 0);
>>> +
>>>         pci_restore_pcix_state(dev);
>>>         pci_restore_msi_state(dev);
>>>         pci_restore_iov_state(dev);
>>
>> I'd feel better about this if there were a way to delay in the FLR
>> path instead.  If we delay in the restore path, we're only fixing one
>> of the many ways config space can be written.  Other paths that write
>> config space will just silently fail.
>>
>> The PCIe spec (r3.0, sec 6.6.2) mentions waiting for the "pre-FLR
>> value for Completion Timeout," but I don't see anything that looks
>> like that in pcie_flr() or pci_af_flr().  Are there any other direct
>> ways we can detect when the FLR is complete?
>
> I'm not aware of any.
>
> Thanks,
> Rafael
I don't think so, either.
I believe an ECN is being worked in the PCI-SIG
to add such a notification, though.
Even if adopted, need to wait for another crank of the hw before
the notification can be used.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Bjorn Helgaas]

On Mon, Apr 23, 2012 at 2:07 PM, Don Dutile <ddutile@redhat.com> wrote:
> On 04/23/2012 03:53 PM, Rafael J. Wysocki wrote:
>>
>> On Monday, April 23, 2012, Bjorn Helgaas wrote:
>>>
>>> On Sun, Apr 15, 2012 at 1:40 PM, Rafael J. Wysocki<rjw@sisk.pl>  wrote:
>>>>
>>>> On Sunday, April 15, 2012, Linus Torvalds wrote:
>>>>>
>>>>> On Sun, Apr 15, 2012 at 11:47 AM, Rafael J. Wysocki<rjw@sisk.pl>
>>>>>  wrote:
>>>>>>
>>>>>>
>>>>>> mdelay(10) doesn't really look good either to me in this case, though.
>>>>>
>>>>>
>>>>> Oh, I agree. What kind of ass-backwards device actually needs that
>>>>> kind of crazy delays? It is almost certainly buggy.
>>>>>
>>>>> With retries, 10ms delays are totally unacceptable. There's something
>>>>> wrong.
>>>>>
>>>>> A single ms *may* be ok.
>>>>>
>>>>> Anyway, can you also split the actual "write _one_ register with
>>>>> retry" into a function of its own? The code looks like crap with those
>>>>> multiple levels of looping, with conditionals inside them etc. With a
>>>>> simple helper function, you could change the break into return, and it
>>>>> would look much better, I bet.
>>>>
>>>>
>>>> Sure.  It appears cleaner this way.
>>>>
>>>> ---
>>>> From: Rafael J. Wysocki<rjw@sisk.pl>
>>>> Subject: PCI: Fix regression in pci_restore_state(), v3
>>>>
>>>> Commit 26f41062f28de65e11d3cf353e52d0be73442be1
>>>>
>>>>    PCI: check for pci bar restore completion and retry
>>>>
>>>> attempted to address problems with PCI BAR restoration on systems
>>>> where FLR had not been completed before pci_restore_state() was
>>>> called, but it did that in an utterly wrong way.
>>>>
>>>> First off, instead of retrying the writes for the BAR registers
>>>> only, it did that for all of the PCI config space of the device,
>>>> including the status register (whose value after the write quite
>>>> obviously need not be the same as the written one).  Second, it
>>>> added arbitrary delay to pci_restore_state() even for systems
>>>> where the PCI config space restoration was successful at first
>>>> attempt.  Finally, the mdelay(10) it added to every iteration of the
>>>> writing loop was way too much of a delay for any reasonable device.
>>>>
>>>> All of this actually caused resume failures for some devices on
>>>> the Mikko's system.
>>>>
>>>> To fix the regression, make pci_restore_state() only retry the
>>>> writes for BAR registers and only wait if the first read from
>>>> the register doesn't return the written value.  Additionaly, make
>>>> it wait for 1 ms, instead of 10 ms, after every failing attempt
>>>> to write into config space.
>>>>
>>>> Reported-by: Mikko Vinni<mmvinni@yahoo.com>
>>>> Signed-off-by: Rafael J. Wysocki<rjw@sisk.pl>
>>>> ---
>>>>  drivers/pci/pci.c |   57
>>>> ++++++++++++++++++++++++++++++++++++------------------
>>>>  1 file changed, 39 insertions(+), 18 deletions(-)
>>>>
>>>> Index: linux/drivers/pci/pci.c
>>>> ===================================================================
>>>> --- linux.orig/drivers/pci/pci.c
>>>> +++ linux/drivers/pci/pci.c
>>>> @@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
>>>>        return 0;
>>>>  }
>>>>
>>>> +static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
>>>> +                                    u32 saved_val, int retry)
>>>> +{
>>>> +       u32 val;
>>>> +
>>>> +       pci_read_config_dword(pdev, offset,&val);
>>>> +       if (val == saved_val)
>>>> +               return;
>>>> +
>>>> +       for (;;) {
>>>> +               dev_dbg(&pdev->dev, "restoring config space at offset "
>>>> +                       "%#x (was %#x, writing %#x)\n", offset, val,
>>>> saved_val);
>>>> +               pci_write_config_dword(pdev, offset, saved_val);
>>>> +               if (retry--<= 0)
>>>> +                       return;
>>>> +
>>>> +               pci_read_config_dword(pdev, offset,&val);
>>>> +               if (val == saved_val)
>>>> +                       return;
>>>> +
>>>> +               mdelay(1);
>>>> +       }
>>>> +}
>>>> +
>>>> +static void pci_restore_config_space(struct pci_dev *pdev, int start,
>>>> int end,
>>>> +                                    int retry)
>>>> +{
>>>> +       int index;
>>>> +
>>>> +       for (index = end; index>= start; index--)
>>>> +               pci_restore_config_dword(pdev, 4 * index,
>>>> +
>>>>  pdev->saved_config_space[index],
>>>> +                                        retry);
>>>> +}
>>>> +
>>>>  /**
>>>>  * pci_restore_state - Restore the saved state of a PCI device
>>>>  * @dev: - PCI device that we're dealing with
>>>>  */
>>>>  void pci_restore_state(struct pci_dev *dev)
>>>>  {
>>>> -       int i;
>>>> -       u32 val;
>>>> -       int tries;
>>>> -
>>>>        if (!dev->state_saved)
>>>>                return;
>>>>
>>>> @@ -984,24 +1015,14 @@ void pci_restore_state(struct pci_dev *d
>>>>        pci_restore_pcie_state(dev);
>>>>        pci_restore_ats_state(dev);
>>>>
>>>> +       pci_restore_config_space(dev, 10, 15, 0);
>>>>        /*
>>>>         * The Base Address register should be programmed before the
>>>> command
>>>>         * register(s)
>>>>         */
>>>> -       for (i = 15; i>= 0; i--) {
>>>> -               pci_read_config_dword(dev, i * 4,&val);
>>>> -               tries = 10;
>>>> -               while (tries&&  val != dev->saved_config_space[i]) {
>>>>
>>>> -                       dev_dbg(&dev->dev, "restoring config "
>>>> -                               "space at offset %#x (was %#x, writing
>>>> %#x)\n",
>>>> -                               i, val,
>>>> (int)dev->saved_config_space[i]);
>>>> -                       pci_write_config_dword(dev,i * 4,
>>>> -                               dev->saved_config_space[i]);
>>>> -                       pci_read_config_dword(dev, i * 4,&val);
>>>> -                       mdelay(10);
>>>> -                       tries--;
>>>> -               }
>>>> -       }
>>>> +       pci_restore_config_space(dev, 4, 9, 10);
>>>> +       pci_restore_config_space(dev, 0, 3, 0);
>>>> +
>>>>        pci_restore_pcix_state(dev);
>>>>        pci_restore_msi_state(dev);
>>>>        pci_restore_iov_state(dev);
>>>
>>>
>>> I'd feel better about this if there were a way to delay in the FLR
>>> path instead.  If we delay in the restore path, we're only fixing one
>>> of the many ways config space can be written.  Other paths that write
>>> config space will just silently fail.
>>>
>>> The PCIe spec (r3.0, sec 6.6.2) mentions waiting for the "pre-FLR
>>> value for Completion Timeout," but I don't see anything that looks
>>> like that in pcie_flr() or pci_af_flr().  Are there any other direct
>>> ways we can detect when the FLR is complete?
>>
>>
>> I'm not aware of any.
>>
>> Thanks,
>> Rafael
>
> I don't think so, either.
> I believe an ECN is being worked in the PCI-SIG
> to add such a notification, though.
> Even if adopted, need to wait for another crank of the hw before
> the notification can be used.

I agree, we can't do something that works only on new hardware -- we
have to make the existing hardware in the field work.

What about the "waiting for as much time as the pre-FLR value for
Completion Timeout" part?

Or can we do something like asserting FLR, sleeping 100ms, then
attempting a write to something in config space and retrying until it
sticks?  It's kludgy, but I'm not sure it's any worse than putting the
retries in the restore path, and it would have the advantage that
other writers of config space wouldn't have to worry.

Bjorn

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Don Dutile]

On 04/23/2012 06:33 PM, Bjorn Helgaas wrote:
> On Mon, Apr 23, 2012 at 2:07 PM, Don Dutile<ddutile@redhat.com>  wrote:
>> On 04/23/2012 03:53 PM, Rafael J. Wysocki wrote:
>>>
>>> On Monday, April 23, 2012, Bjorn Helgaas wrote:
>>>>
>>>> On Sun, Apr 15, 2012 at 1:40 PM, Rafael J. Wysocki<rjw@sisk.pl>    wrote:
>>>>>
>>>>> On Sunday, April 15, 2012, Linus Torvalds wrote:
>>>>>>
>>>>>> On Sun, Apr 15, 2012 at 11:47 AM, Rafael J. Wysocki<rjw@sisk.pl>
>>>>>>   wrote:
>>>>>>>
>>>>>>>
>>>>>>> mdelay(10) doesn't really look good either to me in this case, though.
>>>>>>
>>>>>>
>>>>>> Oh, I agree. What kind of ass-backwards device actually needs that
>>>>>> kind of crazy delays? It is almost certainly buggy.
>>>>>>
>>>>>> With retries, 10ms delays are totally unacceptable. There's something
>>>>>> wrong.
>>>>>>
>>>>>> A single ms *may* be ok.
>>>>>>
>>>>>> Anyway, can you also split the actual "write _one_ register with
>>>>>> retry" into a function of its own? The code looks like crap with those
>>>>>> multiple levels of looping, with conditionals inside them etc. With a
>>>>>> simple helper function, you could change the break into return, and it
>>>>>> would look much better, I bet.
>>>>>
>>>>>
>>>>> Sure.  It appears cleaner this way.
>>>>>
>>>>> ---
>>>>> From: Rafael J. Wysocki<rjw@sisk.pl>
>>>>> Subject: PCI: Fix regression in pci_restore_state(), v3
>>>>>
>>>>> Commit 26f41062f28de65e11d3cf353e52d0be73442be1
>>>>>
>>>>>     PCI: check for pci bar restore completion and retry
>>>>>
>>>>> attempted to address problems with PCI BAR restoration on systems
>>>>> where FLR had not been completed before pci_restore_state() was
>>>>> called, but it did that in an utterly wrong way.
>>>>>
>>>>> First off, instead of retrying the writes for the BAR registers
>>>>> only, it did that for all of the PCI config space of the device,
>>>>> including the status register (whose value after the write quite
>>>>> obviously need not be the same as the written one).  Second, it
>>>>> added arbitrary delay to pci_restore_state() even for systems
>>>>> where the PCI config space restoration was successful at first
>>>>> attempt.  Finally, the mdelay(10) it added to every iteration of the
>>>>> writing loop was way too much of a delay for any reasonable device.
>>>>>
>>>>> All of this actually caused resume failures for some devices on
>>>>> the Mikko's system.
>>>>>
>>>>> To fix the regression, make pci_restore_state() only retry the
>>>>> writes for BAR registers and only wait if the first read from
>>>>> the register doesn't return the written value.  Additionaly, make
>>>>> it wait for 1 ms, instead of 10 ms, after every failing attempt
>>>>> to write into config space.
>>>>>
>>>>> Reported-by: Mikko Vinni<mmvinni@yahoo.com>
>>>>> Signed-off-by: Rafael J. Wysocki<rjw@sisk.pl>
>>>>> ---
>>>>>   drivers/pci/pci.c |   57
>>>>> ++++++++++++++++++++++++++++++++++++------------------
>>>>>   1 file changed, 39 insertions(+), 18 deletions(-)
>>>>>
>>>>> Index: linux/drivers/pci/pci.c
>>>>> ===================================================================
>>>>> --- linux.orig/drivers/pci/pci.c
>>>>> +++ linux/drivers/pci/pci.c
>>>>> @@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
>>>>>         return 0;
>>>>>   }
>>>>>
>>>>> +static void pci_restore_config_dword(struct pci_dev *pdev, int offset,
>>>>> +                                    u32 saved_val, int retry)
>>>>> +{
>>>>> +       u32 val;
>>>>> +
>>>>> +       pci_read_config_dword(pdev, offset,&val);
>>>>> +       if (val == saved_val)
>>>>> +               return;
>>>>> +
>>>>> +       for (;;) {
>>>>> +               dev_dbg(&pdev->dev, "restoring config space at offset "
>>>>> +                       "%#x (was %#x, writing %#x)\n", offset, val,
>>>>> saved_val);
>>>>> +               pci_write_config_dword(pdev, offset, saved_val);
>>>>> +               if (retry--<= 0)
>>>>> +                       return;
>>>>> +
>>>>> +               pci_read_config_dword(pdev, offset,&val);
>>>>> +               if (val == saved_val)
>>>>> +                       return;
>>>>> +
>>>>> +               mdelay(1);
>>>>> +       }
>>>>> +}
>>>>> +
>>>>> +static void pci_restore_config_space(struct pci_dev *pdev, int start,
>>>>> int end,
>>>>> +                                    int retry)
>>>>> +{
>>>>> +       int index;
>>>>> +
>>>>> +       for (index = end; index>= start; index--)
>>>>> +               pci_restore_config_dword(pdev, 4 * index,
>>>>> +
>>>>>   pdev->saved_config_space[index],
>>>>> +                                        retry);
>>>>> +}
>>>>> +
>>>>>   /**
>>>>>   * pci_restore_state - Restore the saved state of a PCI device
>>>>>   * @dev: - PCI device that we're dealing with
>>>>>   */
>>>>>   void pci_restore_state(struct pci_dev *dev)
>>>>>   {
>>>>> -       int i;
>>>>> -       u32 val;
>>>>> -       int tries;
>>>>> -
>>>>>         if (!dev->state_saved)
>>>>>                 return;
>>>>>
>>>>> @@ -984,24 +1015,14 @@ void pci_restore_state(struct pci_dev *d
>>>>>         pci_restore_pcie_state(dev);
>>>>>         pci_restore_ats_state(dev);
>>>>>
>>>>> +       pci_restore_config_space(dev, 10, 15, 0);
>>>>>         /*
>>>>>          * The Base Address register should be programmed before the
>>>>> command
>>>>>          * register(s)
>>>>>          */
>>>>> -       for (i = 15; i>= 0; i--) {
>>>>> -               pci_read_config_dword(dev, i * 4,&val);
>>>>> -               tries = 10;
>>>>> -               while (tries&&    val != dev->saved_config_space[i]) {
>>>>>
>>>>> -                       dev_dbg(&dev->dev, "restoring config "
>>>>> -                               "space at offset %#x (was %#x, writing
>>>>> %#x)\n",
>>>>> -                               i, val,
>>>>> (int)dev->saved_config_space[i]);
>>>>> -                       pci_write_config_dword(dev,i * 4,
>>>>> -                               dev->saved_config_space[i]);
>>>>> -                       pci_read_config_dword(dev, i * 4,&val);
>>>>> -                       mdelay(10);
>>>>> -                       tries--;
>>>>> -               }
>>>>> -       }
>>>>> +       pci_restore_config_space(dev, 4, 9, 10);
>>>>> +       pci_restore_config_space(dev, 0, 3, 0);
>>>>> +
>>>>>         pci_restore_pcix_state(dev);
>>>>>         pci_restore_msi_state(dev);
>>>>>         pci_restore_iov_state(dev);
>>>>
>>>>
>>>> I'd feel better about this if there were a way to delay in the FLR
>>>> path instead.  If we delay in the restore path, we're only fixing one
>>>> of the many ways config space can be written.  Other paths that write
>>>> config space will just silently fail.
>>>>
>>>> The PCIe spec (r3.0, sec 6.6.2) mentions waiting for the "pre-FLR
>>>> value for Completion Timeout," but I don't see anything that looks
>>>> like that in pcie_flr() or pci_af_flr().  Are there any other direct
>>>> ways we can detect when the FLR is complete?
>>>
>>>
>>> I'm not aware of any.
>>>
>>> Thanks,
>>> Rafael
>>
>> I don't think so, either.
>> I believe an ECN is being worked in the PCI-SIG
>> to add such a notification, though.
>> Even if adopted, need to wait for another crank of the hw before
>> the notification can be used.
>
> I agree, we can't do something that works only on new hardware -- we
> have to make the existing hardware in the field work.
>
> What about the "waiting for as much time as the pre-FLR value for
> Completion Timeout" part?
>
> Or can we do something like asserting FLR, sleeping 100ms, then
> attempting a write to something in config space and retrying until it
> sticks?  It's kludgy, but I'm not sure it's any worse than putting the
> retries in the restore path, and it would have the advantage that
> other writers of config space wouldn't have to worry.
>
> Bjorn

Depending on system config, reading a port that is being FLR'd
can cause AERs, which if a driver is registered for the endpoint,
it will get AERs reported to the driver and potentially complicate the
FLRhandling.

This implies a hook to temp-disable AER during FLR, then turning it
back on (hw &/or sw).

- Don

ps -- and there was a deadly embrace where an AER induced during
       an FLR that was initiated by userspace (libvirt writing to device
       reset file in sysfs) would lock up the system b/c the AER
       handler did a config space access, which used the same mutex.
       Not sure if that was cleaned up finally.... very corner-case-ish,
       but it shows how subtle multiple PCIe events can become complicated.

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Bjorn Helgaas]

On Tue, Apr 24, 2012 at 10:03 AM, Don Dutile <ddutile@redhat.com> wrote:
> On 04/23/2012 06:33 PM, Bjorn Helgaas wrote:
>>
>> On Mon, Apr 23, 2012 at 2:07 PM, Don Dutile<ddutile@redhat.com>  wrote:
>>>
>>> On 04/23/2012 03:53 PM, Rafael J. Wysocki wrote:
>>>>
>>>>
>>>> On Monday, April 23, 2012, Bjorn Helgaas wrote:
>>>>>
>>>>>
>>>>> On Sun, Apr 15, 2012 at 1:40 PM, Rafael J. Wysocki<rjw@sisk.pl>
>>>>>  wrote:
>>>>>>
>>>>>>
>>>>>> On Sunday, April 15, 2012, Linus Torvalds wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Sun, Apr 15, 2012 at 11:47 AM, Rafael J. Wysocki<rjw@sisk.pl>
>>>>>>>  wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> mdelay(10) doesn't really look good either to me in this case,
>>>>>>>> though.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Oh, I agree. What kind of ass-backwards device actually needs that
>>>>>>> kind of crazy delays? It is almost certainly buggy.
>>>>>>>
>>>>>>> With retries, 10ms delays are totally unacceptable. There's something
>>>>>>> wrong.
>>>>>>>
>>>>>>> A single ms *may* be ok.
>>>>>>>
>>>>>>> Anyway, can you also split the actual "write _one_ register with
>>>>>>> retry" into a function of its own? The code looks like crap with
>>>>>>> those
>>>>>>> multiple levels of looping, with conditionals inside them etc. With a
>>>>>>> simple helper function, you could change the break into return, and
>>>>>>> it
>>>>>>> would look much better, I bet.
>>>>>>
>>>>>>
>>>>>>
>>>>>> Sure.  It appears cleaner this way.
>>>>>>
>>>>>> ---
>>>>>> From: Rafael J. Wysocki<rjw@sisk.pl>
>>>>>> Subject: PCI: Fix regression in pci_restore_state(), v3
>>>>>>
>>>>>> Commit 26f41062f28de65e11d3cf353e52d0be73442be1
>>>>>>
>>>>>>    PCI: check for pci bar restore completion and retry
>>>>>>
>>>>>> attempted to address problems with PCI BAR restoration on systems
>>>>>> where FLR had not been completed before pci_restore_state() was
>>>>>> called, but it did that in an utterly wrong way.
>>>>>>
>>>>>> First off, instead of retrying the writes for the BAR registers
>>>>>> only, it did that for all of the PCI config space of the device,
>>>>>> including the status register (whose value after the write quite
>>>>>> obviously need not be the same as the written one).  Second, it
>>>>>> added arbitrary delay to pci_restore_state() even for systems
>>>>>> where the PCI config space restoration was successful at first
>>>>>> attempt.  Finally, the mdelay(10) it added to every iteration of the
>>>>>> writing loop was way too much of a delay for any reasonable device.
>>>>>>
>>>>>> All of this actually caused resume failures for some devices on
>>>>>> the Mikko's system.
>>>>>>
>>>>>> To fix the regression, make pci_restore_state() only retry the
>>>>>> writes for BAR registers and only wait if the first read from
>>>>>> the register doesn't return the written value.  Additionaly, make
>>>>>> it wait for 1 ms, instead of 10 ms, after every failing attempt
>>>>>> to write into config space.
>>>>>>
>>>>>> Reported-by: Mikko Vinni<mmvinni@yahoo.com>
>>>>>> Signed-off-by: Rafael J. Wysocki<rjw@sisk.pl>
>>>>>> ---
>>>>>>  drivers/pci/pci.c |   57
>>>>>> ++++++++++++++++++++++++++++++++++++------------------
>>>>>>  1 file changed, 39 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> Index: linux/drivers/pci/pci.c
>>>>>> ===================================================================
>>>>>> --- linux.orig/drivers/pci/pci.c
>>>>>> +++ linux/drivers/pci/pci.c
>>>>>> @@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
>>>>>>        return 0;
>>>>>>  }
>>>>>>
>>>>>> +static void pci_restore_config_dword(struct pci_dev *pdev, int
>>>>>> offset,
>>>>>> +                                    u32 saved_val, int retry)
>>>>>> +{
>>>>>> +       u32 val;
>>>>>> +
>>>>>> +       pci_read_config_dword(pdev, offset,&val);
>>>>>> +       if (val == saved_val)
>>>>>> +               return;
>>>>>> +
>>>>>> +       for (;;) {
>>>>>> +               dev_dbg(&pdev->dev, "restoring config space at offset
>>>>>> "
>>>>>> +                       "%#x (was %#x, writing %#x)\n", offset, val,
>>>>>> saved_val);
>>>>>> +               pci_write_config_dword(pdev, offset, saved_val);
>>>>>> +               if (retry--<= 0)
>>>>>> +                       return;
>>>>>> +
>>>>>> +               pci_read_config_dword(pdev, offset,&val);
>>>>>> +               if (val == saved_val)
>>>>>> +                       return;
>>>>>> +
>>>>>> +               mdelay(1);
>>>>>> +       }
>>>>>> +}
>>>>>> +
>>>>>> +static void pci_restore_config_space(struct pci_dev *pdev, int start,
>>>>>> int end,
>>>>>> +                                    int retry)
>>>>>> +{
>>>>>> +       int index;
>>>>>> +
>>>>>> +       for (index = end; index>= start; index--)
>>>>>> +               pci_restore_config_dword(pdev, 4 * index,
>>>>>> +
>>>>>>  pdev->saved_config_space[index],
>>>>>> +                                        retry);
>>>>>> +}
>>>>>> +
>>>>>>  /**
>>>>>>  * pci_restore_state - Restore the saved state of a PCI device
>>>>>>  * @dev: - PCI device that we're dealing with
>>>>>>  */
>>>>>>  void pci_restore_state(struct pci_dev *dev)
>>>>>>  {
>>>>>> -       int i;
>>>>>> -       u32 val;
>>>>>> -       int tries;
>>>>>> -
>>>>>>        if (!dev->state_saved)
>>>>>>                return;
>>>>>>
>>>>>> @@ -984,24 +1015,14 @@ void pci_restore_state(struct pci_dev *d
>>>>>>        pci_restore_pcie_state(dev);
>>>>>>        pci_restore_ats_state(dev);
>>>>>>
>>>>>> +       pci_restore_config_space(dev, 10, 15, 0);
>>>>>>        /*
>>>>>>         * The Base Address register should be programmed before the
>>>>>> command
>>>>>>         * register(s)
>>>>>>         */
>>>>>> -       for (i = 15; i>= 0; i--) {
>>>>>> -               pci_read_config_dword(dev, i * 4,&val);
>>>>>> -               tries = 10;
>>>>>> -               while (tries&&    val != dev->saved_config_space[i]) {
>>>>>>
>>>>>> -                       dev_dbg(&dev->dev, "restoring config "
>>>>>> -                               "space at offset %#x (was %#x, writing
>>>>>> %#x)\n",
>>>>>> -                               i, val,
>>>>>> (int)dev->saved_config_space[i]);
>>>>>> -                       pci_write_config_dword(dev,i * 4,
>>>>>> -                               dev->saved_config_space[i]);
>>>>>> -                       pci_read_config_dword(dev, i * 4,&val);
>>>>>> -                       mdelay(10);
>>>>>> -                       tries--;
>>>>>> -               }
>>>>>> -       }
>>>>>> +       pci_restore_config_space(dev, 4, 9, 10);
>>>>>> +       pci_restore_config_space(dev, 0, 3, 0);
>>>>>> +
>>>>>>        pci_restore_pcix_state(dev);
>>>>>>        pci_restore_msi_state(dev);
>>>>>>        pci_restore_iov_state(dev);
>>>>>
>>>>>
>>>>>
>>>>> I'd feel better about this if there were a way to delay in the FLR
>>>>> path instead.  If we delay in the restore path, we're only fixing one
>>>>> of the many ways config space can be written.  Other paths that write
>>>>> config space will just silently fail.
>>>>>
>>>>> The PCIe spec (r3.0, sec 6.6.2) mentions waiting for the "pre-FLR
>>>>> value for Completion Timeout," but I don't see anything that looks
>>>>> like that in pcie_flr() or pci_af_flr().  Are there any other direct
>>>>> ways we can detect when the FLR is complete?
>>>>
>>>>
>>>>
>>>> I'm not aware of any.
>>>>
>>>> Thanks,
>>>> Rafael
>>>
>>>
>>> I don't think so, either.
>>> I believe an ECN is being worked in the PCI-SIG
>>> to add such a notification, though.
>>> Even if adopted, need to wait for another crank of the hw before
>>> the notification can be used.
>>
>>
>> I agree, we can't do something that works only on new hardware -- we
>> have to make the existing hardware in the field work.
>>
>> What about the "waiting for as much time as the pre-FLR value for
>> Completion Timeout" part?
>>
>> Or can we do something like asserting FLR, sleeping 100ms, then
>> attempting a write to something in config space and retrying until it
>> sticks?  It's kludgy, but I'm not sure it's any worse than putting the
>> retries in the restore path, and it would have the advantage that
>> other writers of config space wouldn't have to worry.
>
> Depending on system config, reading a port that is being FLR'd
> can cause AERs, which if a driver is registered for the endpoint,
> it will get AERs reported to the driver and potentially complicate the
> FLRhandling.
>
> This implies a hook to temp-disable AER during FLR, then turning it
> back on (hw &/or sw).

Don't we have the same possibility of causing AERs if we add this
retry stuff in pci_restore_state()?  It sounds like the same
possibility exists regardless of whether the config access happens in
the FLR path or the restore path.

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Don Dutile]

On 04/24/2012 01:01 PM, Bjorn Helgaas wrote:
> On Tue, Apr 24, 2012 at 10:03 AM, Don Dutile<ddutile@redhat.com>  wrote:
>> On 04/23/2012 06:33 PM, Bjorn Helgaas wrote:
>>>
>>> On Mon, Apr 23, 2012 at 2:07 PM, Don Dutile<ddutile@redhat.com>    wrote:
>>>>
>>>> On 04/23/2012 03:53 PM, Rafael J. Wysocki wrote:
>>>>>
>>>>>
>>>>> On Monday, April 23, 2012, Bjorn Helgaas wrote:
>>>>>>
>>>>>>
>>>>>> On Sun, Apr 15, 2012 at 1:40 PM, Rafael J. Wysocki<rjw@sisk.pl>
>>>>>>   wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Sunday, April 15, 2012, Linus Torvalds wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sun, Apr 15, 2012 at 11:47 AM, Rafael J. Wysocki<rjw@sisk.pl>
>>>>>>>>   wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> mdelay(10) doesn't really look good either to me in this case,
>>>>>>>>> though.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Oh, I agree. What kind of ass-backwards device actually needs that
>>>>>>>> kind of crazy delays? It is almost certainly buggy.
>>>>>>>>
>>>>>>>> With retries, 10ms delays are totally unacceptable. There's something
>>>>>>>> wrong.
>>>>>>>>
>>>>>>>> A single ms *may* be ok.
>>>>>>>>
>>>>>>>> Anyway, can you also split the actual "write _one_ register with
>>>>>>>> retry" into a function of its own? The code looks like crap with
>>>>>>>> those
>>>>>>>> multiple levels of looping, with conditionals inside them etc. With a
>>>>>>>> simple helper function, you could change the break into return, and
>>>>>>>> it
>>>>>>>> would look much better, I bet.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Sure.  It appears cleaner this way.
>>>>>>>
>>>>>>> ---
>>>>>>> From: Rafael J. Wysocki<rjw@sisk.pl>
>>>>>>> Subject: PCI: Fix regression in pci_restore_state(), v3
>>>>>>>
>>>>>>> Commit 26f41062f28de65e11d3cf353e52d0be73442be1
>>>>>>>
>>>>>>>     PCI: check for pci bar restore completion and retry
>>>>>>>
>>>>>>> attempted to address problems with PCI BAR restoration on systems
>>>>>>> where FLR had not been completed before pci_restore_state() was
>>>>>>> called, but it did that in an utterly wrong way.
>>>>>>>
>>>>>>> First off, instead of retrying the writes for the BAR registers
>>>>>>> only, it did that for all of the PCI config space of the device,
>>>>>>> including the status register (whose value after the write quite
>>>>>>> obviously need not be the same as the written one).  Second, it
>>>>>>> added arbitrary delay to pci_restore_state() even for systems
>>>>>>> where the PCI config space restoration was successful at first
>>>>>>> attempt.  Finally, the mdelay(10) it added to every iteration of the
>>>>>>> writing loop was way too much of a delay for any reasonable device.
>>>>>>>
>>>>>>> All of this actually caused resume failures for some devices on
>>>>>>> the Mikko's system.
>>>>>>>
>>>>>>> To fix the regression, make pci_restore_state() only retry the
>>>>>>> writes for BAR registers and only wait if the first read from
>>>>>>> the register doesn't return the written value.  Additionaly, make
>>>>>>> it wait for 1 ms, instead of 10 ms, after every failing attempt
>>>>>>> to write into config space.
>>>>>>>
>>>>>>> Reported-by: Mikko Vinni<mmvinni@yahoo.com>
>>>>>>> Signed-off-by: Rafael J. Wysocki<rjw@sisk.pl>
>>>>>>> ---
>>>>>>>   drivers/pci/pci.c |   57
>>>>>>> ++++++++++++++++++++++++++++++++++++------------------
>>>>>>>   1 file changed, 39 insertions(+), 18 deletions(-)
>>>>>>>
>>>>>>> Index: linux/drivers/pci/pci.c
>>>>>>> ===================================================================
>>>>>>> --- linux.orig/drivers/pci/pci.c
>>>>>>> +++ linux/drivers/pci/pci.c
>>>>>>> @@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
>>>>>>>         return 0;
>>>>>>>   }
>>>>>>>
>>>>>>> +static void pci_restore_config_dword(struct pci_dev *pdev, int
>>>>>>> offset,
>>>>>>> +                                    u32 saved_val, int retry)
>>>>>>> +{
>>>>>>> +       u32 val;
>>>>>>> +
>>>>>>> +       pci_read_config_dword(pdev, offset,&val);
>>>>>>> +       if (val == saved_val)
>>>>>>> +               return;
>>>>>>> +
>>>>>>> +       for (;;) {
>>>>>>> +               dev_dbg(&pdev->dev, "restoring config space at offset
>>>>>>> "
>>>>>>> +                       "%#x (was %#x, writing %#x)\n", offset, val,
>>>>>>> saved_val);
>>>>>>> +               pci_write_config_dword(pdev, offset, saved_val);
>>>>>>> +               if (retry--<= 0)
>>>>>>> +                       return;
>>>>>>> +
>>>>>>> +               pci_read_config_dword(pdev, offset,&val);
>>>>>>> +               if (val == saved_val)
>>>>>>> +                       return;
>>>>>>> +
>>>>>>> +               mdelay(1);
>>>>>>> +       }
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void pci_restore_config_space(struct pci_dev *pdev, int start,
>>>>>>> int end,
>>>>>>> +                                    int retry)
>>>>>>> +{
>>>>>>> +       int index;
>>>>>>> +
>>>>>>> +       for (index = end; index>= start; index--)
>>>>>>> +               pci_restore_config_dword(pdev, 4 * index,
>>>>>>> +
>>>>>>>   pdev->saved_config_space[index],
>>>>>>> +                                        retry);
>>>>>>> +}
>>>>>>> +
>>>>>>>   /**
>>>>>>>   * pci_restore_state - Restore the saved state of a PCI device
>>>>>>>   * @dev: - PCI device that we're dealing with
>>>>>>>   */
>>>>>>>   void pci_restore_state(struct pci_dev *dev)
>>>>>>>   {
>>>>>>> -       int i;
>>>>>>> -       u32 val;
>>>>>>> -       int tries;
>>>>>>> -
>>>>>>>         if (!dev->state_saved)
>>>>>>>                 return;
>>>>>>>
>>>>>>> @@ -984,24 +1015,14 @@ void pci_restore_state(struct pci_dev *d
>>>>>>>         pci_restore_pcie_state(dev);
>>>>>>>         pci_restore_ats_state(dev);
>>>>>>>
>>>>>>> +       pci_restore_config_space(dev, 10, 15, 0);
>>>>>>>         /*
>>>>>>>          * The Base Address register should be programmed before the
>>>>>>> command
>>>>>>>          * register(s)
>>>>>>>          */
>>>>>>> -       for (i = 15; i>= 0; i--) {
>>>>>>> -               pci_read_config_dword(dev, i * 4,&val);
>>>>>>> -               tries = 10;
>>>>>>> -               while (tries&&      val != dev->saved_config_space[i]) {
>>>>>>>
>>>>>>> -                       dev_dbg(&dev->dev, "restoring config "
>>>>>>> -                               "space at offset %#x (was %#x, writing
>>>>>>> %#x)\n",
>>>>>>> -                               i, val,
>>>>>>> (int)dev->saved_config_space[i]);
>>>>>>> -                       pci_write_config_dword(dev,i * 4,
>>>>>>> -                               dev->saved_config_space[i]);
>>>>>>> -                       pci_read_config_dword(dev, i * 4,&val);
>>>>>>> -                       mdelay(10);
>>>>>>> -                       tries--;
>>>>>>> -               }
>>>>>>> -       }
>>>>>>> +       pci_restore_config_space(dev, 4, 9, 10);
>>>>>>> +       pci_restore_config_space(dev, 0, 3, 0);
>>>>>>> +
>>>>>>>         pci_restore_pcix_state(dev);
>>>>>>>         pci_restore_msi_state(dev);
>>>>>>>         pci_restore_iov_state(dev);
>>>>>>
>>>>>>
>>>>>>
>>>>>> I'd feel better about this if there were a way to delay in the FLR
>>>>>> path instead.  If we delay in the restore path, we're only fixing one
>>>>>> of the many ways config space can be written.  Other paths that write
>>>>>> config space will just silently fail.
>>>>>>
>>>>>> The PCIe spec (r3.0, sec 6.6.2) mentions waiting for the "pre-FLR
>>>>>> value for Completion Timeout," but I don't see anything that looks
>>>>>> like that in pcie_flr() or pci_af_flr().  Are there any other direct
>>>>>> ways we can detect when the FLR is complete?
>>>>>
>>>>>
>>>>>
>>>>> I'm not aware of any.
>>>>>
>>>>> Thanks,
>>>>> Rafael
>>>>
>>>>
>>>> I don't think so, either.
>>>> I believe an ECN is being worked in the PCI-SIG
>>>> to add such a notification, though.
>>>> Even if adopted, need to wait for another crank of the hw before
>>>> the notification can be used.
>>>
>>>
>>> I agree, we can't do something that works only on new hardware -- we
>>> have to make the existing hardware in the field work.
>>>
>>> What about the "waiting for as much time as the pre-FLR value for
>>> Completion Timeout" part?
>>>
>>> Or can we do something like asserting FLR, sleeping 100ms, then
>>> attempting a write to something in config space and retrying until it
>>> sticks?  It's kludgy, but I'm not sure it's any worse than putting the
>>> retries in the restore path, and it would have the advantage that
>>> other writers of config space wouldn't have to worry.
>>
>> Depending on system config, reading a port that is being FLR'd
>> can cause AERs, which if a driver is registered for the endpoint,
>> it will get AERs reported to the driver and potentially complicate the
>> FLRhandling.
>>
>> This implies a hook to temp-disable AER during FLR, then turning it
>> back on (hw&/or sw).
>
> Don't we have the same possibility of causing AERs if we add this
> retry stuff in pci_restore_state()?  It sounds like the same
> possibility exists regardless of whether the config access happens in
> the FLR path or the restore path.


Dont know; have to re-read thread on where/when pci_restore_state() is recalled
wrt link synch/enable state(s).

Re: [PATCH] PCI: Fix regression in pci_restore_state()

[Bjorn Helgaas]

On Tue, Apr 24, 2012 at 11:35 AM, Don Dutile <ddutile@redhat.com> wrote:
> On 04/24/2012 01:01 PM, Bjorn Helgaas wrote:
>>
>> On Tue, Apr 24, 2012 at 10:03 AM, Don Dutile<ddutile@redhat.com>  wrote:
>>>
>>> On 04/23/2012 06:33 PM, Bjorn Helgaas wrote:
>>>>
>>>>
>>>> On Mon, Apr 23, 2012 at 2:07 PM, Don Dutile<ddutile@redhat.com>
>>>>  wrote:
>>>>>
>>>>>
>>>>> On 04/23/2012 03:53 PM, Rafael J. Wysocki wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Monday, April 23, 2012, Bjorn Helgaas wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Sun, Apr 15, 2012 at 1:40 PM, Rafael J. Wysocki<rjw@sisk.pl>
>>>>>>>  wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On Sunday, April 15, 2012, Linus Torvalds wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Sun, Apr 15, 2012 at 11:47 AM, Rafael J. Wysocki<rjw@sisk.pl>
>>>>>>>>>  wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> mdelay(10) doesn't really look good either to me in this case,
>>>>>>>>>> though.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Oh, I agree. What kind of ass-backwards device actually needs that
>>>>>>>>> kind of crazy delays? It is almost certainly buggy.
>>>>>>>>>
>>>>>>>>> With retries, 10ms delays are totally unacceptable. There's
>>>>>>>>> something
>>>>>>>>> wrong.
>>>>>>>>>
>>>>>>>>> A single ms *may* be ok.
>>>>>>>>>
>>>>>>>>> Anyway, can you also split the actual "write _one_ register with
>>>>>>>>> retry" into a function of its own? The code looks like crap with
>>>>>>>>> those
>>>>>>>>> multiple levels of looping, with conditionals inside them etc. With
>>>>>>>>> a
>>>>>>>>> simple helper function, you could change the break into return, and
>>>>>>>>> it
>>>>>>>>> would look much better, I bet.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Sure.  It appears cleaner this way.
>>>>>>>>
>>>>>>>> ---
>>>>>>>> From: Rafael J. Wysocki<rjw@sisk.pl>
>>>>>>>> Subject: PCI: Fix regression in pci_restore_state(), v3
>>>>>>>>
>>>>>>>> Commit 26f41062f28de65e11d3cf353e52d0be73442be1
>>>>>>>>
>>>>>>>>    PCI: check for pci bar restore completion and retry
>>>>>>>>
>>>>>>>> attempted to address problems with PCI BAR restoration on systems
>>>>>>>> where FLR had not been completed before pci_restore_state() was
>>>>>>>> called, but it did that in an utterly wrong way.
>>>>>>>>
>>>>>>>> First off, instead of retrying the writes for the BAR registers
>>>>>>>> only, it did that for all of the PCI config space of the device,
>>>>>>>> including the status register (whose value after the write quite
>>>>>>>> obviously need not be the same as the written one).  Second, it
>>>>>>>> added arbitrary delay to pci_restore_state() even for systems
>>>>>>>> where the PCI config space restoration was successful at first
>>>>>>>> attempt.  Finally, the mdelay(10) it added to every iteration of the
>>>>>>>> writing loop was way too much of a delay for any reasonable device.
>>>>>>>>
>>>>>>>> All of this actually caused resume failures for some devices on
>>>>>>>> the Mikko's system.
>>>>>>>>
>>>>>>>> To fix the regression, make pci_restore_state() only retry the
>>>>>>>> writes for BAR registers and only wait if the first read from
>>>>>>>> the register doesn't return the written value.  Additionaly, make
>>>>>>>> it wait for 1 ms, instead of 10 ms, after every failing attempt
>>>>>>>> to write into config space.
>>>>>>>>
>>>>>>>> Reported-by: Mikko Vinni<mmvinni@yahoo.com>
>>>>>>>> Signed-off-by: Rafael J. Wysocki<rjw@sisk.pl>
>>>>>>>> ---
>>>>>>>>  drivers/pci/pci.c |   57
>>>>>>>> ++++++++++++++++++++++++++++++++++++------------------
>>>>>>>>  1 file changed, 39 insertions(+), 18 deletions(-)
>>>>>>>>
>>>>>>>> Index: linux/drivers/pci/pci.c
>>>>>>>> ===================================================================
>>>>>>>> --- linux.orig/drivers/pci/pci.c
>>>>>>>> +++ linux/drivers/pci/pci.c
>>>>>>>> @@ -967,16 +967,47 @@ pci_save_state(struct pci_dev *dev)
>>>>>>>>        return 0;
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +static void pci_restore_config_dword(struct pci_dev *pdev, int
>>>>>>>> offset,
>>>>>>>> +                                    u32 saved_val, int retry)
>>>>>>>> +{
>>>>>>>> +       u32 val;
>>>>>>>> +
>>>>>>>> +       pci_read_config_dword(pdev, offset,&val);
>>>>>>>> +       if (val == saved_val)
>>>>>>>> +               return;
>>>>>>>> +
>>>>>>>> +       for (;;) {
>>>>>>>> +               dev_dbg(&pdev->dev, "restoring config space at
>>>>>>>> offset
>>>>>>>> "
>>>>>>>> +                       "%#x (was %#x, writing %#x)\n", offset, val,
>>>>>>>> saved_val);
>>>>>>>> +               pci_write_config_dword(pdev, offset, saved_val);
>>>>>>>> +               if (retry--<= 0)
>>>>>>>> +                       return;
>>>>>>>> +
>>>>>>>> +               pci_read_config_dword(pdev, offset,&val);
>>>>>>>> +               if (val == saved_val)
>>>>>>>> +                       return;
>>>>>>>> +
>>>>>>>> +               mdelay(1);
>>>>>>>> +       }
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void pci_restore_config_space(struct pci_dev *pdev, int
>>>>>>>> start,
>>>>>>>> int end,
>>>>>>>> +                                    int retry)
>>>>>>>> +{
>>>>>>>> +       int index;
>>>>>>>> +
>>>>>>>> +       for (index = end; index>= start; index--)
>>>>>>>> +               pci_restore_config_dword(pdev, 4 * index,
>>>>>>>> +
>>>>>>>>  pdev->saved_config_space[index],
>>>>>>>> +                                        retry);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  /**
>>>>>>>>  * pci_restore_state - Restore the saved state of a PCI device
>>>>>>>>  * @dev: - PCI device that we're dealing with
>>>>>>>>  */
>>>>>>>>  void pci_restore_state(struct pci_dev *dev)
>>>>>>>>  {
>>>>>>>> -       int i;
>>>>>>>> -       u32 val;
>>>>>>>> -       int tries;
>>>>>>>> -
>>>>>>>>        if (!dev->state_saved)
>>>>>>>>                return;
>>>>>>>>
>>>>>>>> @@ -984,24 +1015,14 @@ void pci_restore_state(struct pci_dev *d
>>>>>>>>        pci_restore_pcie_state(dev);
>>>>>>>>        pci_restore_ats_state(dev);
>>>>>>>>
>>>>>>>> +       pci_restore_config_space(dev, 10, 15, 0);
>>>>>>>>        /*
>>>>>>>>         * The Base Address register should be programmed before the
>>>>>>>> command
>>>>>>>>         * register(s)
>>>>>>>>         */
>>>>>>>> -       for (i = 15; i>= 0; i--) {
>>>>>>>> -               pci_read_config_dword(dev, i * 4,&val);
>>>>>>>> -               tries = 10;
>>>>>>>> -               while (tries&&      val !=
>>>>>>>> dev->saved_config_space[i]) {
>>>>>>>>
>>>>>>>> -                       dev_dbg(&dev->dev, "restoring config "
>>>>>>>> -                               "space at offset %#x (was %#x,
>>>>>>>> writing
>>>>>>>> %#x)\n",
>>>>>>>> -                               i, val,
>>>>>>>> (int)dev->saved_config_space[i]);
>>>>>>>> -                       pci_write_config_dword(dev,i * 4,
>>>>>>>> -                               dev->saved_config_space[i]);
>>>>>>>> -                       pci_read_config_dword(dev, i * 4,&val);
>>>>>>>> -                       mdelay(10);
>>>>>>>> -                       tries--;
>>>>>>>> -               }
>>>>>>>> -       }
>>>>>>>> +       pci_restore_config_space(dev, 4, 9, 10);
>>>>>>>> +       pci_restore_config_space(dev, 0, 3, 0);
>>>>>>>> +
>>>>>>>>        pci_restore_pcix_state(dev);
>>>>>>>>        pci_restore_msi_state(dev);
>>>>>>>>        pci_restore_iov_state(dev);
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> I'd feel better about this if there were a way to delay in the FLR
>>>>>>> path instead.  If we delay in the restore path, we're only fixing one
>>>>>>> of the many ways config space can be written.  Other paths that write
>>>>>>> config space will just silently fail.
>>>>>>>
>>>>>>> The PCIe spec (r3.0, sec 6.6.2) mentions waiting for the "pre-FLR
>>>>>>> value for Completion Timeout," but I don't see anything that looks
>>>>>>> like that in pcie_flr() or pci_af_flr().  Are there any other direct
>>>>>>> ways we can detect when the FLR is complete?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> I'm not aware of any.
>>>>>>
>>>>>> Thanks,
>>>>>> Rafael
>>>>>
>>>>>
>>>>>
>>>>> I don't think so, either.
>>>>> I believe an ECN is being worked in the PCI-SIG
>>>>> to add such a notification, though.
>>>>> Even if adopted, need to wait for another crank of the hw before
>>>>> the notification can be used.
>>>>
>>>>
>>>>
>>>> I agree, we can't do something that works only on new hardware -- we
>>>> have to make the existing hardware in the field work.
>>>>
>>>> What about the "waiting for as much time as the pre-FLR value for
>>>> Completion Timeout" part?
>>>>
>>>> Or can we do something like asserting FLR, sleeping 100ms, then
>>>> attempting a write to something in config space and retrying until it
>>>> sticks?  It's kludgy, but I'm not sure it's any worse than putting the
>>>> retries in the restore path, and it would have the advantage that
>>>> other writers of config space wouldn't have to worry.
>>>
>>>
>>> Depending on system config, reading a port that is being FLR'd
>>> can cause AERs, which if a driver is registered for the endpoint,
>>> it will get AERs reported to the driver and potentially complicate the
>>> FLRhandling.
>>>
>>> This implies a hook to temp-disable AER during FLR, then turning it
>>> back on (hw&/or sw).
>>
>> Don't we have the same possibility of causing AERs if we add this
>> retry stuff in pci_restore_state()?  It sounds like the same
>> possibility exists regardless of whether the config access happens in
>> the FLR path or the restore path.
>
> Dont know; have to re-read thread on where/when pci_restore_state() is
> recalled
> wrt link synch/enable state(s).

I don't know what to do with this patch yet.  Here's what I think
happens with this patch applied, in a case where we have to do some
retries:

    pcie_flr
      pci_write_config_word(dev, PCI_EXP_DEVCTL)  /* start FLR */
      msleep(100)
    ...
    pci_restore_state
      pci_read_config_dword(dev, 0x28)  /* CardBus CIS pointer */
      pci_write_config_dword(dev, 0x28)
      pci_read_config_dword(dev, 0x30)  /* Subsystem ID/vendor ID */
      pci_write_config_dword(dev, 0x30)
      ...
      pci_read_config_dword(dev, 0x10)  /* BAR 0 */
      for (;;) {
        pci_write_config_dword(dev, 0x10, saved_val)
        pci_read_config_dword(dev, 0x10, &val)
        if (val == saved_val)
          break;
      }

1) What happens if another code path reads or writes config space
before pci_restore_path()?  As far as I can tell, it fails silently.

2) Apparently (per Don), config access to a device in the middle of
FLR can cause AER.  If that's true, it seems like even this patch
could cause an AER, and the only possible thing is to rely on a delay
in pcie_flr() or temporarily disable AER.

3) If a config space write being accepted is a signal that the FLR is
complete, why don't we first restore a register where we *check*
whether the write is accepted?  The patch restores dwords 10-15 first,
and we don't retry those, so it seems like those updates could be
lost.  In fact, we do lots of other config writes in
pci_restore_pcie_state(), etc., and presumably all those could be
being dropped.

4) Why doesn't pcie_flr() do anything with the Completion Timeout
(PCIe spec r3.0, sec 6.6.2)?  It seems like using this could cause us
to wait longer than the current 100ms in pcie_flr().

5) If we're not retrying, it seems pointless to even read the register
first.  A nit, I know.

I don't feel comfortable that we understand the real problem here, so
I'm reluctant to apply the patch.  Obviously I know little about FLR
and the restore path, so possibly I just need to be educated :)

Bjorn

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[James Courtier-Dutton]

On 13 April 2012 21:47, Mikko Vinni <mmvinni@yahoo.com> wrote:
>       1 firewire_ohci 0000:0a:00.0: Refused to change power state, currently in D3
>      10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xf (was 0xffffffff, writing 0x10a)
>      10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xe (was 0xffffffff, writing 0x0)
<snip>
>       1 sdhci-pci 0000:0a:00.1: Refused to change power state, currently in D3
>      10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xf (was 0xffffffff, writing 0x10a)
>      10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xe (was 0xffffffff, writing 0x0)
<snip>

Could the problem be more due to "Refused to change power state,
currently in D3".
the "was 0xffffffff" would be returned if their was no power to the device.
Don't you need power, before you can write to the config space?

Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2

[Mikko Vinni]

From: James Courtier-Dutton:

> 
> On 13 April 2012 21:47, Mikko Vinni <mmvinni@yahoo.com> wrote:
>>        1 firewire_ohci 0000:0a:00.0: Refused to change power state, 
> currently in D3
>>       10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xf 
> (was 0xffffffff, writing 0x10a)
>>       10 firewire_ohci 0000:0a:00.0: restoring config space at offset 0xe 
> (was 0xffffffff, writing 0x0)
> <snip>
>>        1 sdhci-pci 0000:0a:00.1: Refused to change power state, currently in 
> D3
>>       10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xf (was 
> 0xffffffff, writing 0x10a)
>>       10 sdhci-pci 0000:0a:00.1: restoring config space at offset 0xe (was 
> 0xffffffff, writing 0x0)
> <snip>
> 
> Could the problem be more due to "Refused to change power state,
> currently in D3".
> the "was 0xffffffff" would be returned if their was no power to the 
> device.
> Don't you need power, before you can write to the config space?
>

Those messages and the problem of the combo card reader and firewire
not working after s2ram has been present for long
(https://bugzilla.kernel.org/show_bug.cgi?id=35452
dmesg here: https://lkml.org/lkml/2011/5/15/55).

Before 3.4-rc1 there was nothing wrong with the touchpad (and keyboard
and reboot) after s2ram, and the problems are somehow separate.
Rafael's patch fixes the new problem, any suggestions for the old
one would be warmly appreciated.


Mikko