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

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

Message-Id: <200902040023.03843.rjw@sisk.pl>
Date:	Wed, 4 Feb 2009 00:23:02 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	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

On Tuesday 03 February 2009, Benjamin Herrenschmidt wrote:
> 
> > 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.

Overlooked, sorry.

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

The assumption here is that the state will be saved either by the driver
or by the core, so the bug here is a consequence of the previous one.

I'll add a check.

> 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 ? :-)

Well, if the state is UNKNOWN, I think it's safe to save the config, so the
WARN_ON should really catch the low power states only.  I'll fix that.

Thanks for debugging it!

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