linux-kernel - [patch,rfc] usb: restore config before enabling device on resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 6 Dec 2008 10:20:17 +0100
From:	Frans Pop <elendil@...net.nl>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>, Greg KH <greg@...ah.com>,
	Ingo Molnar <mingo@...e.hu>, jbarnes@...tuousgeek.org,
	lenb@...nel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	tiwai@...e.de, Andrew Morton <akpm@...ux-foundation.org>,
	USB list <linux-usb@...r.kernel.org>
Subject: [patch,rfc] usb: restore config before enabling device on resume

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?

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?

Cheers,
FJP


View attachment "usb_resume.patch" of type "text/x-diff" (1107 bytes)

View attachment "dmesg_resume.diff" of type "text/x-diff" (15762 bytes)