Messages in this thread | | | From | "Rafael J. Wysocki" <> | Subject | Re: [patch,rfc] usb: restore config before enabling device on resume | Date | Sat, 6 Dec 2008 14:48:30 +0100 |
| |
On Saturday, 6 of December 2008, Frans Pop wrote: > On Thursday 04 December 2008, Linus Torvalds wrote: > > Greg, Jesse, can you think about and look at the USB PCI resume > > ordering? > [...] > > In many ways the bigger worry is actually in the totally unrelated USB > > UHCI and EHCI drivers that resume _before_ the bridge does: > > > > uhci_hcd 0000:00:1d.2: enabling device (0000 -> 0001) > > uhci_hcd 0000:00:1d.2: PCI INT C -> GSI 18 (level, low) -> IRQ 18 > > uhci_hcd 0000:00:1d.2: setting latency timer to 64 > > uhci_hcd 0000:00:1d.2: restoring config space at offset 0xf (was 0x300, writing 0x30b) > > uhci_hcd 0000:00:1d.2: restoring config space at offset 0x8 (was 0x1, writing 0x2101) > > usb usb7: root hub lost power or was reset > > ehci_hcd 0000:00:1d.7: enabling device (0000 -> 0002) > > ehci_hcd 0000:00:1d.7: PCI INT A -> GSI 20 (level, low) -> IRQ 20 > > ehci_hcd 0000:00:1d.7: setting latency timer to 64 > > ehci_hcd 0000:00:1d.7: restoring config space at offset 0xf (was 0x100, writing 0x10a) > > ehci_hcd 0000:00:1d.7: restoring config space at offset 0x4 (was 0x0, writing 0xe0648000) > > > > and the worry I have here is that we actually enable the device > > _before_ we've restored the BAR information. That sounds very iffy. It > > sounds doubly iffy in the 'resume from hibernate' case, where we are > > going to have an already-set-up PCI bus and the config space values are > > going to all be live as we reprogram them. > > > > That "restoring config space at offset 0x8" thing is where we restore > > the BAR (dword 0x8 = offset 0x20 = PCI_BASE_ADDRESS_4), and we're > > changing it from 0x1 to 0x2101, with the IO BAR enabled. In this case, > > the old value meant that the BAR started out disabled, but hibernate > > would have been different. > > > > So I'd _much_ rather have seen the sequence have the BAR restore > > sequence be something like > > > > uhci_hcd 0000:00:1d.2: restoring config space at offset 0xf (was 0x300, writing 0x30b) > > uhci_hcd 0000:00:1d.2: restoring config space at offset 0x8 (was 0x1, writing 0x2101) > > uhci_hcd 0000:00:1d.2: enabling device (0000 -> 0001) > > uhci_hcd 0000:00:1d.2: PCI INT C -> GSI 18 (level, low) -> IRQ 18 > > uhci_hcd 0000:00:1d.2: setting latency timer to 64 > > > > instead. Possibly even with an explicit disable of the > > memory/IO/busmaster bits before the whole sequence. > > I've taken a very naive look at this, basically by comparing what usb > (usb/core/hcd-pci.c) is doing compared to other drivers. > > I used the following command to get an overview: > $ git grep -E -n -C5 "pci_(enable_device|set_master|restore_state|power_state.*D0)" > (The line numbers give some indication whether work is split over functions.) > > Most drivers seem to do some variation of the following, which looks > logical and is in line with Documentation/power/pci.txt: > pci_set_power_state(dev, PCI_D0); > pci_restore_state(dev); > pci_enable_device(dev); > pci_set_master(dev); > > But quite a lot of drivers (including usb and e.g. ide/setup-pci.c) do > something like: > pci_enable_device(dev); > pci_set_master(dev); > pci_restore_state(dev); > > Maybe the whole tree should get a review for this?
I think so.
> Anyway, I gave the patch below a try on both my notebook and desktop. > My desktop has USB keyboard and mouse and the notebook has wireless and > a fingerprint scanner on USB. Everything still worked after resume. > > Diff of the resume dmesg of my motebook attached. Looks better I think?
Yes, to me it does.
In fact, I think you could even move the pci_restore_state(dev) into usb_hcd_pci_resume_early() that would be executed with interrupts off and drop the pci_set_power_state(dev, PCI_D0); entirely (the pci_enable_device(dev); would invoke it anyway).
Thanks, Rafael
PS Please append patches instead of attaching them, they are a lot easier to discuss this way.
| |