* "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 @ 2012-04-13 9:52 ` Mikko Vinni 0 siblings, 0 replies; 40+ messages in thread From: Mikko Vinni @ 2012-04-13 9:52 UTC (permalink / raw) To: linux-pm, linux-input, linux-kernel Cc: Jean Guyader, Jesse Barnes, Allen Kay, Dmitry Torokhov 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. I will try to update the BIOS on this laptop next week, hoping that it might fix some of the s2ram issues, but other than that, maybe somebody has an idea? Thanks, Mikko ^ permalink raw reply [flat|nested] 40+ messages in thread
* "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 @ 2012-04-13 9:52 ` Mikko Vinni 0 siblings, 0 replies; 40+ messages in thread From: Mikko Vinni @ 2012-04-13 9:52 UTC (permalink / raw) To: linux-pm, linux-input, linux-kernel Cc: Dmitry Torokhov, Allen Kay, Jean Guyader, Jesse Barnes 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. I will try to update the BIOS on this laptop next week, hoping that it might fix some of the s2ram issues, but other than that, maybe somebody has an idea? Thanks, Mikko ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-13 9:52 ` Mikko Vinni (?) @ 2012-04-13 19:18 ` Rafael J. Wysocki 2012-04-13 19:49 ` Mikko Vinni -1 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-13 19:18 UTC (permalink / raw) To: Mikko Vinni Cc: linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jean Guyader, Jesse Barnes, Bjorn Helgaas, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-13 19:18 ` [linux-pm] " Rafael J. Wysocki @ 2012-04-13 19:49 ` Mikko Vinni 0 siblings, 0 replies; 40+ messages in thread From: Mikko Vinni @ 2012-04-13 19:49 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jean Guyader, Jesse Barnes, Bjorn Helgaas, linux-pci 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 > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 @ 2012-04-13 19:49 ` Mikko Vinni 0 siblings, 0 replies; 40+ messages in thread From: Mikko Vinni @ 2012-04-13 19:49 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jean Guyader, Jesse Barnes, Bjorn Helgaas, linux-pci 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 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-13 19:49 ` Mikko Vinni (?) @ 2012-04-13 20:19 ` Rafael J. Wysocki 2012-04-13 20:47 ` Mikko Vinni -1 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-13 20:19 UTC (permalink / raw) To: Mikko Vinni, Bjorn Helgaas Cc: linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jean Guyader, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-13 20:19 ` Rafael J. Wysocki @ 2012-04-13 20:47 ` Mikko Vinni 2012-04-14 22:11 ` Rafael J. Wysocki 2012-04-15 8:12 ` James Courtier-Dutton 0 siblings, 2 replies; 40+ messages in thread From: Mikko Vinni @ 2012-04-13 20:47 UTC (permalink / raw) To: Rafael J. Wysocki, Bjorn Helgaas Cc: linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-13 20:47 ` Mikko Vinni @ 2012-04-14 22:11 ` Rafael J. Wysocki 2012-04-15 10:39 ` Mikko Vinni 2012-04-15 8:12 ` James Courtier-Dutton 1 sibling, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-14 22:11 UTC (permalink / raw) To: Mikko Vinni Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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); ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-14 22:11 ` Rafael J. Wysocki @ 2012-04-15 10:39 ` Mikko Vinni 0 siblings, 0 replies; 40+ messages in thread From: Mikko Vinni @ 2012-04-15 10:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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); > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 @ 2012-04-15 10:39 ` Mikko Vinni 0 siblings, 0 replies; 40+ messages in thread From: Mikko Vinni @ 2012-04-15 10:39 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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); > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-15 10:39 ` Mikko Vinni (?) @ 2012-04-15 18:30 ` Rafael J. Wysocki 2012-04-15 19:52 ` Rafael J. Wysocki 2012-04-16 5:15 ` [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 Mikko Vinni -1 siblings, 2 replies; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-15 18:30 UTC (permalink / raw) To: Mikko Vinni Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-15 18:30 ` Rafael J. Wysocki @ 2012-04-15 19:52 ` Rafael J. Wysocki 2012-04-15 20:56 ` Rafael J. Wysocki 2012-04-16 5:15 ` [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 Mikko Vinni 1 sibling, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-15 19:52 UTC (permalink / raw) To: Mikko Vinni Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-15 19:52 ` Rafael J. Wysocki @ 2012-04-15 20:56 ` Rafael J. Wysocki 2012-04-16 7:23 ` Mikko Vinni 0 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-15 20:56 UTC (permalink / raw) To: Mikko Vinni Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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); ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-15 20:56 ` Rafael J. Wysocki @ 2012-04-16 7:23 ` Mikko Vinni 2012-04-16 16:17 ` Rafael J. Wysocki 0 siblings, 1 reply; 40+ messages in thread From: Mikko Vinni @ 2012-04-16 7:23 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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); > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-16 7:23 ` Mikko Vinni @ 2012-04-16 16:17 ` Rafael J. Wysocki 2012-04-16 18:57 ` Mikko Vinni 0 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-16 16:17 UTC (permalink / raw) To: Mikko Vinni Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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); ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-16 16:17 ` Rafael J. Wysocki @ 2012-04-16 18:57 ` Mikko Vinni 2012-04-16 19:59 ` Rafael J. Wysocki 2012-04-16 20:35 ` [PATCH] PCI: Retry BARs restoration for Type 0 headers only Rafael J. Wysocki 0 siblings, 2 replies; 40+ messages in thread From: Mikko Vinni @ 2012-04-16 18:57 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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); > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-16 18:57 ` Mikko Vinni @ 2012-04-16 19:59 ` Rafael J. Wysocki 2012-04-16 20:35 ` [PATCH] PCI: Retry BARs restoration for Type 0 headers only Rafael J. Wysocki 1 sibling, 0 replies; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-16 19:59 UTC (permalink / raw) To: Mikko Vinni Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] PCI: Retry BARs restoration for Type 0 headers only 2012-04-16 18:57 ` Mikko Vinni 2012-04-16 19:59 ` Rafael J. Wysocki @ 2012-04-16 20:35 ` Rafael J. Wysocki 2012-04-16 20:35 ` Linus Torvalds 1 sibling, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-16 20:35 UTC (permalink / raw) To: Bjorn Helgaas Cc: Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci, Linus Torvalds 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); ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Retry BARs restoration for Type 0 headers only 2012-04-16 20:35 ` [PATCH] PCI: Retry BARs restoration for Type 0 headers only Rafael J. Wysocki @ 2012-04-16 20:35 ` Linus Torvalds 2012-04-16 21:07 ` Rafael J. Wysocki 0 siblings, 1 reply; 40+ messages in thread From: Linus Torvalds @ 2012-04-16 20:35 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Retry BARs restoration for Type 0 headers only 2012-04-16 20:35 ` Linus Torvalds @ 2012-04-16 21:07 ` Rafael J. Wysocki 0 siblings, 0 replies; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-16 21:07 UTC (permalink / raw) To: Linus Torvalds Cc: Bjorn Helgaas, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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); ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-15 18:30 ` Rafael J. Wysocki 2012-04-15 19:52 ` Rafael J. Wysocki @ 2012-04-16 5:15 ` Mikko Vinni 2012-04-16 16:14 ` Rafael J. Wysocki 1 sibling, 1 reply; 40+ messages in thread From: Mikko Vinni @ 2012-04-16 5:15 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-16 5:15 ` [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 Mikko Vinni @ 2012-04-16 16:14 ` Rafael J. Wysocki 0 siblings, 0 replies; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-16 16:14 UTC (permalink / raw) To: Mikko Vinni Cc: Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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! ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-15 10:39 ` Mikko Vinni (?) (?) @ 2012-04-15 18:32 ` Rafael J. Wysocki 2012-04-15 18:36 ` Linus Torvalds -1 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-15 18:32 UTC (permalink / raw) To: Bjorn Helgaas Cc: Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci, Linus Torvalds 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); ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-15 18:32 ` [PATCH] PCI: Fix regression in pci_restore_state() Rafael J. Wysocki @ 2012-04-15 18:36 ` Linus Torvalds 2012-04-15 18:47 ` Rafael J. Wysocki 0 siblings, 1 reply; 40+ messages in thread From: Linus Torvalds @ 2012-04-15 18:36 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-15 18:36 ` Linus Torvalds @ 2012-04-15 18:47 ` Rafael J. Wysocki 2012-04-15 18:59 ` Linus Torvalds 0 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-15 18:47 UTC (permalink / raw) To: Linus Torvalds Cc: Bjorn Helgaas, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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); ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-15 18:47 ` Rafael J. Wysocki @ 2012-04-15 18:59 ` Linus Torvalds 2012-04-15 19:40 ` Rafael J. Wysocki 0 siblings, 1 reply; 40+ messages in thread From: Linus Torvalds @ 2012-04-15 18:59 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-15 18:59 ` Linus Torvalds @ 2012-04-15 19:40 ` Rafael J. Wysocki 2012-04-23 17:03 ` Bjorn Helgaas 0 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-15 19:40 UTC (permalink / raw) To: Linus Torvalds Cc: Bjorn Helgaas, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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); ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-15 19:40 ` Rafael J. Wysocki @ 2012-04-23 17:03 ` Bjorn Helgaas 2012-04-23 19:53 ` Rafael J. Wysocki 0 siblings, 1 reply; 40+ messages in thread From: Bjorn Helgaas @ 2012-04-23 17:03 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linus Torvalds, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-23 17:03 ` Bjorn Helgaas @ 2012-04-23 19:53 ` Rafael J. Wysocki 2012-04-23 20:07 ` Don Dutile 0 siblings, 1 reply; 40+ messages in thread From: Rafael J. Wysocki @ 2012-04-23 19:53 UTC (permalink / raw) To: Bjorn Helgaas Cc: Linus Torvalds, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-23 19:53 ` Rafael J. Wysocki @ 2012-04-23 20:07 ` Don Dutile 2012-04-23 22:33 ` Bjorn Helgaas 0 siblings, 1 reply; 40+ messages in thread From: Don Dutile @ 2012-04-23 20:07 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Bjorn Helgaas, Linus Torvalds, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-23 20:07 ` Don Dutile @ 2012-04-23 22:33 ` Bjorn Helgaas 0 siblings, 0 replies; 40+ messages in thread From: Bjorn Helgaas @ 2012-04-23 22:33 UTC (permalink / raw) To: Don Dutile Cc: Rafael J. Wysocki, Linus Torvalds, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() @ 2012-04-23 22:33 ` Bjorn Helgaas 0 siblings, 0 replies; 40+ messages in thread From: Bjorn Helgaas @ 2012-04-23 22:33 UTC (permalink / raw) To: Don Dutile Cc: Rafael J. Wysocki, Linus Torvalds, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-23 22:33 ` Bjorn Helgaas (?) @ 2012-04-24 16:03 ` Don Dutile 2012-04-24 17:01 ` Bjorn Helgaas -1 siblings, 1 reply; 40+ messages in thread From: Don Dutile @ 2012-04-24 16:03 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rafael J. Wysocki, Linus Torvalds, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-24 16:03 ` Don Dutile @ 2012-04-24 17:01 ` Bjorn Helgaas 2012-04-24 17:35 ` Don Dutile 0 siblings, 1 reply; 40+ messages in thread From: Bjorn Helgaas @ 2012-04-24 17:01 UTC (permalink / raw) To: Don Dutile Cc: Rafael J. Wysocki, Linus Torvalds, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-24 17:01 ` Bjorn Helgaas @ 2012-04-24 17:35 ` Don Dutile 2012-04-27 22:20 ` Bjorn Helgaas 0 siblings, 1 reply; 40+ messages in thread From: Don Dutile @ 2012-04-24 17:35 UTC (permalink / raw) To: Bjorn Helgaas Cc: Rafael J. Wysocki, Linus Torvalds, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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). ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH] PCI: Fix regression in pci_restore_state() 2012-04-24 17:35 ` Don Dutile @ 2012-04-27 22:20 ` Bjorn Helgaas 0 siblings, 0 replies; 40+ messages in thread From: Bjorn Helgaas @ 2012-04-27 22:20 UTC (permalink / raw) To: Don Dutile Cc: Rafael J. Wysocki, Linus Torvalds, Mikko Vinni, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-13 20:47 ` Mikko Vinni @ 2012-04-15 8:12 ` James Courtier-Dutton 2012-04-15 8:12 ` James Courtier-Dutton 1 sibling, 0 replies; 40+ messages in thread From: James Courtier-Dutton @ 2012-04-15 8:12 UTC (permalink / raw) To: Mikko Vinni Cc: Rafael J. Wysocki, Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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? ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 @ 2012-04-15 8:12 ` James Courtier-Dutton 0 siblings, 0 replies; 40+ messages in thread From: James Courtier-Dutton @ 2012-04-15 8:12 UTC (permalink / raw) To: Mikko Vinni Cc: Rafael J. Wysocki, Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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? -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 2012-04-15 8:12 ` James Courtier-Dutton @ 2012-04-15 10:47 ` Mikko Vinni -1 siblings, 0 replies; 40+ messages in thread From: Mikko Vinni @ 2012-04-15 10:47 UTC (permalink / raw) To: James Courtier-Dutton Cc: Rafael J. Wysocki, Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 @ 2012-04-15 10:47 ` Mikko Vinni 0 siblings, 0 replies; 40+ messages in thread From: Mikko Vinni @ 2012-04-15 10:47 UTC (permalink / raw) To: James Courtier-Dutton Cc: Rafael J. Wysocki, Bjorn Helgaas, linux-input, linux-kernel, Dmitry Torokhov, Allen Kay, Jesse Barnes, linux-pci 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2012-04-27 22:20 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-13 9:52 "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 Mikko Vinni 2012-04-13 9:52 ` Mikko Vinni 2012-04-13 19:18 ` [linux-pm] " Rafael J. Wysocki 2012-04-13 19:49 ` Mikko Vinni 2012-04-13 19:49 ` Mikko Vinni 2012-04-13 20:19 ` Rafael J. Wysocki 2012-04-13 20:47 ` Mikko Vinni 2012-04-14 22:11 ` Rafael J. Wysocki 2012-04-15 10:39 ` Mikko Vinni 2012-04-15 10:39 ` Mikko Vinni 2012-04-15 18:30 ` Rafael J. Wysocki 2012-04-15 19:52 ` Rafael J. Wysocki 2012-04-15 20:56 ` Rafael J. Wysocki 2012-04-16 7:23 ` Mikko Vinni 2012-04-16 16:17 ` Rafael J. Wysocki 2012-04-16 18:57 ` Mikko Vinni 2012-04-16 19:59 ` Rafael J. Wysocki 2012-04-16 20:35 ` [PATCH] PCI: Retry BARs restoration for Type 0 headers only Rafael J. Wysocki 2012-04-16 20:35 ` Linus Torvalds 2012-04-16 21:07 ` Rafael J. Wysocki 2012-04-16 5:15 ` [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 Mikko Vinni 2012-04-16 16:14 ` Rafael J. Wysocki 2012-04-15 18:32 ` [PATCH] PCI: Fix regression in pci_restore_state() Rafael J. Wysocki 2012-04-15 18:36 ` Linus Torvalds 2012-04-15 18:47 ` Rafael J. Wysocki 2012-04-15 18:59 ` Linus Torvalds 2012-04-15 19:40 ` Rafael J. Wysocki 2012-04-23 17:03 ` Bjorn Helgaas 2012-04-23 19:53 ` Rafael J. Wysocki 2012-04-23 20:07 ` Don Dutile 2012-04-23 22:33 ` Bjorn Helgaas 2012-04-23 22:33 ` Bjorn Helgaas 2012-04-24 16:03 ` Don Dutile 2012-04-24 17:01 ` Bjorn Helgaas 2012-04-24 17:35 ` Don Dutile 2012-04-27 22:20 ` Bjorn Helgaas 2012-04-15 8:12 ` [linux-pm] "i8042: Can't reactivate AUX port" after s2ram on 3.4-rc2 James Courtier-Dutton 2012-04-15 8:12 ` James Courtier-Dutton 2012-04-15 10:47 ` Mikko Vinni 2012-04-15 10:47 ` Mikko Vinni
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.