Re: [linux-pm] ehci_hcd related S3 lockup on ASUS laptops, again

[Andrey Rahmatullin <wrar-Bxt/JK/G85ezQB+pC5nmwQ@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1215 bytes --]

On Tue, Apr 17, 2012 at 12:58:03PM -0400, Alan Stern wrote:
> > > 	1. This change in hcd-pci.c:
> > > +	pci_dev->current_state = PCI_UNKNOWN;
> > It locks up if this line is removed.
> 
> Very bizarre indeed.  All right, leave that line in place and try 
> removing the other changes, one at a time.  In fact, let's add one more 
> thing to remove from the patch
> 
> > > 	2. This change in ehci-pci.c:
> > > +	ehci_silence_controller(ehci);
> > > 
> > > 	3. This change in hcd-pci.c:
> > > +	pci_disable_device(pci_dev);
> > > 
> > > 	4. This change in hcd-pci.c:
> > > +	iounmap(hcd->regs);
> > > 
> > > 	5. This change in hcd-pci.c:
> > > +	free_irq(hcd->irq, hcd);
> > > 
> > > 	6. This change in hcd-pci.c:
> > > -	pci_disable_device(pci_dev);
> > > +	pci_disable_enabled_device(pci_dev);
> > > 
> > > 	7. This change in ehci-pci.c:
> > > -	ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup);
> 
> 	8. This change in ehci-pci.c:
> +	ehci_reset(ehci);
> 
> If you manage to reach the end of this list, you'll essentially be
> doing a normal suspend (except for the PCI_UNKNOWN part).
It works without changes #2..8. I'm attaching the resulting patch.

-- 
WBR, wRAR

[-- Attachment #1.2: current.patch --]
[-- Type: text/x-diff, Size: 3465 bytes --]

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 6b54b23..8a9edf4 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -713,6 +713,8 @@ static int pci_pm_suspend_noirq(struct device *dev)
 
 	if (!pm) {
 		pci_save_state(pci_dev);
+		pci_prepare_to_sleep(pci_dev);
+		pci_pm_set_unknown_state(pci_dev);
 		return 0;
 	}
 
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 8156744..3111105 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1348,6 +1348,7 @@ void pci_disable_enabled_device(struct pci_dev *dev)
 	if (pci_is_enabled(dev))
 		do_pci_disable_device(dev);
 }
+EXPORT_SYMBOL(pci_disable_enabled_device);
 
 /**
  * pci_disable_device - Disable PCI device after use
@@ -1710,6 +1711,7 @@ pci_power_t pci_target_state(struct pci_dev *dev)
  */
 int pci_prepare_to_sleep(struct pci_dev *dev)
 {
+	pci_power_t cur_state = dev->current_state;
 	pci_power_t target_state = pci_target_state(dev);
 	int error;
 
@@ -1723,6 +1725,8 @@ int pci_prepare_to_sleep(struct pci_dev *dev)
 	if (error)
 		pci_enable_wake(dev, target_state, false);
 
+	dev_info(&dev->dev, "cur %d target %d error %d\n", cur_state,
+			target_state, error);
 	return error;
 }
 
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index 622b4a4..d7dc939 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -381,6 +381,8 @@ static int check_root_hub_suspended(struct device *dev)
 }
 
 #if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME)
+extern void pci_disable_enabled_device(struct pci_dev *);
+
 static int suspend_common(struct device *dev, bool do_wakeup)
 {
 	struct pci_dev		*pci_dev = to_pci_dev(dev);
@@ -427,12 +429,18 @@ static int suspend_common(struct device *dev, bool do_wakeup)
 	if (!hcd->msix_enabled)
 		synchronize_irq(pci_dev->irq);
 
+//	free_irq(hcd->irq, hcd); // 5
+//	iounmap(hcd->regs); // 4
+//	pci_disable_device(pci_dev); // 3
+
 	/* Downstream ports from this root hub should already be quiesced, so
 	 * there will be no DMA activity.  Now we can shut down the upstream
 	 * link (except maybe for PME# resume signaling).  We'll enter a
 	 * low power state during suspend_noirq, if the hardware allows.
 	 */
+//	pci_disable_enabled_device(pci_dev); // 6
 	pci_disable_device(pci_dev);
+	pci_dev->current_state = PCI_UNKNOWN; // 1
 	return retval;
 }
 
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 01bb7241d..5fbfc3e 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -342,7 +342,7 @@ static int ehci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 	 * mark HW unaccessible.  The PM and USB cores make sure that
 	 * the root hub is either suspended or stopped.
 	 */
-	ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup);
+	ehci_prepare_ports_for_controller_suspend(ehci, do_wakeup); // 7
 	spin_lock_irqsave (&ehci->lock, flags);
 	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
 	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
@@ -350,6 +350,9 @@ static int ehci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 	spin_unlock_irqrestore (&ehci->lock, flags);
 
+//	ehci_silence_controller(ehci); // 2
+//	ehci_reset(ehci); // 8
+
 	// could save FLADJ in case of Vaux power loss
 	// ... we'd only use it to handle clock skew
 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]