linux-kernel - Re: [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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <200812061448.31468.rjw@sisk.pl>
Date:	Sat, 6 Dec 2008 14:48:30 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Frans Pop <elendil@...net.nl>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	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: Re: [patch,rfc] usb: restore config before enabling device on resume

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/