linux-kernel - Re: Regression from 2.6.26: Hibernation (possibly suspend) broken on Toshiba R500 (bisected)

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: <200812060104.50050.rjw@sisk.pl>
Date:	Sat, 6 Dec 2008 01:04:49 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Frans Pop <elendil@...net.nl>, 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>
Subject: Re: Regression from 2.6.26: Hibernation (possibly suspend) broken on Toshiba R500 (bisected)

On Friday, 5 of December 2008, Linus Torvalds wrote:
> 
> On Fri, 5 Dec 2008, Rafael J. Wysocki wrote:
> > 
> > Well, actually I think we should go further and save the standard config
> > registers of _all_ PCI devices in the _late() callbacks (ie. with interrupts
> > disabled) and restore them in the _early() callbacks.
> 
> That would certainly simplify the code. 
> 
> > I don't really understand why pci_restore_state() is not called by the core
> > and every single driver calls it by itself.
> 
> The idea was to allow PCI drivers to override it if they wanted to.
> 
> That said, the ones that do their own state restore generally do it wrong 
> (eg the USB host controllers doing things in the wrong order and enabling 
> the device before having actually written back the BAR values), so it's 
> arguably broken to let drivers override it.
> 
> > IOW, I would split the resume of PCI devices into two parts, the first of
> > which will call pci_restore_state() with interrupts disabled and the second
> > will do the remaining stuff.
> 
> I would definitely not disagree with that - leave the regular 
> "suspend/resume" callbacks for purely higher-level actions. It would 
> interesting to hear what it does for you.

I tested the appended patch with suspend-to-RAM and it just hangs during
resume.

Next, I'll try to do that only for devices the drivers of which don't define their own
suspend-resume callbacks at all.

Thanks,
Rafael

---
 drivers/pci/pci-driver.c |   50 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci-driver.c
+++ linux-2.6/drivers/pci/pci-driver.c
@@ -300,6 +300,46 @@ static void pci_device_shutdown(struct d
 
 #ifdef CONFIG_PM_SLEEP
 
+static void pci_default_suspend_noirq(struct pci_dev *pci_dev)
+{
+	dev_info(&pci_dev->dev, "saving standard PCI config registers\n");
+
+	/* save the PCI config space */
+	pci_save_state(pci_dev);
+	/*
+	 * mark its power state as "unknown", since we don't know if
+	 * e.g. the BIOS will change its device state when we suspend.
+	 */
+	if (pci_dev->current_state == PCI_D0)
+		pci_dev->current_state = PCI_UNKNOWN;
+}
+
+static void pci_default_resume_noirq(struct pci_dev *pci_dev)
+{
+	dev_info(&pci_dev->dev, "restoring standard PCI config registers\n");
+
+	/* restore the PCI config space */
+	pci_restore_state(pci_dev);
+}
+
+static int pci_default_resume(struct pci_dev *pci_dev)
+{
+	int retval = 0;
+
+	dev_info(&pci_dev->dev, "trying to reenable device\n");
+
+	/* if the device was enabled before suspend, reenable */
+	retval = pci_reenable_device(pci_dev);
+	/*
+	 * if the device was busmaster before the suspend, make it busmaster
+	 * again
+	 */
+	if (pci_dev->is_busmaster)
+		pci_set_master(pci_dev);
+
+	return retval;
+}
+
 /*
  * Default "suspend" method for devices that have no driver provided suspend,
  * or not even a driver at all.
@@ -346,8 +386,6 @@ static int pci_legacy_suspend(struct dev
 	if (drv && drv->suspend) {
 		i = drv->suspend(pci_dev, state);
 		suspend_report_result(drv->suspend, i);
-	} else {
-		pci_default_pm_suspend(pci_dev);
 	}
 	return i;
 }
@@ -362,6 +400,9 @@ static int pci_legacy_suspend_late(struc
 		i = drv->suspend_late(pci_dev, state);
 		suspend_report_result(drv->suspend_late, i);
 	}
+
+	pci_default_suspend_noirq(pci_dev);
+
 	return i;
 }
 
@@ -374,7 +415,7 @@ static int pci_legacy_resume(struct devi
 	if (drv && drv->resume)
 		error = drv->resume(pci_dev);
 	else
-		error = pci_default_pm_resume(pci_dev);
+		error = pci_default_resume(pci_dev);
 	return error;
 }
 
@@ -384,8 +425,11 @@ static int pci_legacy_resume_early(struc
 	struct pci_dev * pci_dev = to_pci_dev(dev);
 	struct pci_driver * drv = pci_dev->driver;
 
+	pci_default_resume_noirq(pci_dev);
+
 	if (drv && drv->resume_early)
 		error = drv->resume_early(pci_dev);
+
 	return error;
 }
 
--
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/