linux-kernel - Re: PCI PM: Restore standard config registers of all devices early

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:	Wed, 04 Feb 2009 09:59:37 +1100
From:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jesse Barnes <jesse.barnes@...el.com>,
	Andreas Schwab <schwab@...e.de>, Len Brown <lenb@...nel.org>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: PCI PM: Restore standard config registers of all devices early


> You've found a bug somewhere.

Yup :-)

> We _should_ be saving things, the legacy code does something like this:
> 
>         if (drv && drv->suspend) {
>                 pci_dev->state_saved = false;
> 
>                 i = drv->suspend(pci_dev, state);
>                 suspend_report_result(drv->suspend, i);
>                 if (i)
>                         return i;
> 
>                 if (pci_dev->state_saved)
>                         goto Fixup;
> 
>                 if (WARN_ON_ONCE(pci_dev->current_state != PCI_D0))
>                         goto Fixup;

It looks like the above is what breaks. Looks like current_state is
UNKNOWN. The device is an old mach64 that has no PCI PM capability, thus
the driver doesn't call any PCI PM stuff, the state basically stays set
to what the core set it to at probe time which appears to be
PCI_UNKNOWN.

Thus we don't call pci_save_state().

Then ...

>         }
> 
>         pci_save_state(pci_dev);
> 
> ie if your ->suspend function doesn't use pci_save_state() itself (which 
> sets that "state_saved" flag to true), then the generic code will do it 
> for you.
> 
> Also, on the resume path, we actually have
> 
>         if (pci_dev->state_saved)
>                 pci_restore_standard_config(pci_dev);
> 
> so I wonder how the heck you got that blast of all zeroes - because we
> clearly shouldn't be trying to restore any unsaved state!

Well, that's it ... we don't actually test pci_dev->state_saved in
whatever is currently upstream. The code is:

static void pci_pm_default_resume_noirq(struct pci_dev *pci_dev)
{
	pci_restore_standard_config(pci_dev);
	pci_fixup_device(pci_fixup_resume_early, pci_dev);
}

Oops... 

Rafael, the second one is trivial to fix, but what about the first one ?
Should we not count UNKNOWN in that goto or should we set legacy stuff
that don't do PCI PM to PCI_D0 somewhere ? Or both ? :-)

Cheers,
Ben.


--
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/