diff mbox

Long delays while hibernating

Message ID 4B34D2AC.8040908@kernel.org
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Tejun Heo Dec. 25, 2009, 2:56 p.m. UTC
On 12/25/2009 12:59 AM, Pedro Ribeiro wrote:
> I just noticed that this patch is for ata_piix. I don't have this
> module loaded, as I have an ICH9 chip. The module used is ahci. Here
> is the output of lsmod:
> http://pastebin.com/m5468985b

Oh... Can you please try this one then?

Thanks.

Comments

Pedro Ribeiro Dec. 25, 2009, 6:10 p.m. UTC | #1
On Fri, Dec 25, 2009 at 2:56 PM, Tejun Heo <tj@kernel.org> wrote:
> On 12/25/2009 12:59 AM, Pedro Ribeiro wrote:
>> I just noticed that this patch is for ata_piix. I don't have this
>> module loaded, as I have an ICH9 chip. The module used is ahci. Here
>> is the output of lsmod:
>> http://pastebin.com/m5468985b
>
> Oh... Can you please try this one then?
>
> Thanks.
>
> --
> tejun
>

Sorry, still hangs with this patch :(

This only happens in hibernation... suspend is fine.

Pedro
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo Dec. 26, 2009, 1:26 a.m. UTC | #2
(cc'ing Rafael)

On 12/26/2009 03:10 AM, Pedro Ribeiro wrote:
>> Oh... Can you please try this one then?
> 
> Sorry, still hangs with this patch :(
> 
> This only happens in hibernation... suspend is fine.

Hmmm... the patch does apply to both suspend and hibernation.  Rafael,
Pedro's ThinkPad T400 burns cpu cycle for 30secs on "Atomic
copy/restore" step of hibernation if the ultrabay is powered off.  The
original report can be found at...

  http://thread.gmane.org/gmane.linux.ide/44190

Please note that it's with TuxOnIce patches.  Anyways, ata_piix on
certain configurations have similar issues where after the OS is done
suspending and powering off the piix controller, the BIOS tries to
access it and ends up burning cpu cycles which can be worked around by
leaving the controller on after suspend.  Applying the same workaround
didn't resolve the issue.  Do you know what can make cpu burn for
30secs on the atomic copy/restore step?

Pedro, can you please try to reproduce the problem without TuxOnIce
patches?

Thanks.
Pedro Ribeiro Dec. 26, 2009, 4:32 p.m. UTC | #3
On Sat, Dec 26, 2009 at 1:26 AM, Tejun Heo <tj@kernel.org> wrote:
> (cc'ing Rafael)
>
> On 12/26/2009 03:10 AM, Pedro Ribeiro wrote:
>>> Oh... Can you please try this one then?
>>
>> Sorry, still hangs with this patch :(
>>
>> This only happens in hibernation... suspend is fine.
>
> Hmmm... the patch does apply to both suspend and hibernation.  Rafael,
> Pedro's ThinkPad T400 burns cpu cycle for 30secs on "Atomic
> copy/restore" step of hibernation if the ultrabay is powered off.  The
> original report can be found at...
>
>  http://thread.gmane.org/gmane.linux.ide/44190
>
> Please note that it's with TuxOnIce patches.  Anyways, ata_piix on
> certain configurations have similar issues where after the OS is done
> suspending and powering off the piix controller, the BIOS tries to
> access it and ends up burning cpu cycles which can be worked around by
> leaving the controller on after suspend.  Applying the same workaround
> didn't resolve the issue.  Do you know what can make cpu burn for
> 30secs on the atomic copy/restore step?
>
> Pedro, can you please try to reproduce the problem without TuxOnIce
> patches?
>
> Thanks.
>
> --
> tejun
>

Hi Rafael and Tejun,

I am experiencing the same delay with the kernel swsusp, so this is
definitely not a TuxOnIce issue.

Regards,
Pedro
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Dec. 26, 2009, 9:21 p.m. UTC | #4
On Saturday 26 December 2009, Pedro Ribeiro wrote:
> On Sat, Dec 26, 2009 at 1:26 AM, Tejun Heo <tj@kernel.org> wrote:
> > (cc'ing Rafael)
> >
> > On 12/26/2009 03:10 AM, Pedro Ribeiro wrote:
> >>> Oh... Can you please try this one then?
> >>
> >> Sorry, still hangs with this patch :(
> >>
> >> This only happens in hibernation... suspend is fine.
> >
> > Hmmm... the patch does apply to both suspend and hibernation.  Rafael,
> > Pedro's ThinkPad T400 burns cpu cycle for 30secs on "Atomic
> > copy/restore" step of hibernation if the ultrabay is powered off.  The
> > original report can be found at...
> >
> >  http://thread.gmane.org/gmane.linux.ide/44190
> >
> > Please note that it's with TuxOnIce patches.  Anyways, ata_piix on
> > certain configurations have similar issues where after the OS is done
> > suspending and powering off the piix controller, the BIOS tries to
> > access it and ends up burning cpu cycles which can be worked around by
> > leaving the controller on after suspend.  Applying the same workaround
> > didn't resolve the issue.  Do you know what can make cpu burn for
> > 30secs on the atomic copy/restore step?

No idea.

Is this reproducible with /sys/power/pm_test = core?

[If you echo "core" to /sys/power/pm_test and then attempt to hibernate,
it should just simulate snapshotting the system, sleep for 5 seconds and go
back to your command prompt (or wherever you started the "hibernation").]

> > Pedro, can you please try to reproduce the problem without TuxOnIce
> > patches?
> >
> > Thanks.
> >
> > --
> > tejun
> >
> 
> Hi Rafael and Tejun,
> 
> I am experiencing the same delay with the kernel swsusp, so this is
> definitely not a TuxOnIce issue.

I don't think this can be a TuxOnIce.  Most likely, ACPI is trying to do
something stupid in _PTS.

Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pedro Ribeiro Dec. 26, 2009, 10:35 p.m. UTC | #5
On Sat, Dec 26, 2009 at 9:21 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Saturday 26 December 2009, Pedro Ribeiro wrote:
>> On Sat, Dec 26, 2009 at 1:26 AM, Tejun Heo <tj@kernel.org> wrote:
>> > (cc'ing Rafael)
>> >
>> > On 12/26/2009 03:10 AM, Pedro Ribeiro wrote:
>> >>> Oh... Can you please try this one then?
>> >>
>> >> Sorry, still hangs with this patch :(
>> >>
>> >> This only happens in hibernation... suspend is fine.
>> >
>> > Hmmm... the patch does apply to both suspend and hibernation.  Rafael,
>> > Pedro's ThinkPad T400 burns cpu cycle for 30secs on "Atomic
>> > copy/restore" step of hibernation if the ultrabay is powered off.  The
>> > original report can be found at...
>> >
>> >  http://thread.gmane.org/gmane.linux.ide/44190
>> >
>> > Please note that it's with TuxOnIce patches.  Anyways, ata_piix on
>> > certain configurations have similar issues where after the OS is done
>> > suspending and powering off the piix controller, the BIOS tries to
>> > access it and ends up burning cpu cycles which can be worked around by
>> > leaving the controller on after suspend.  Applying the same workaround
>> > didn't resolve the issue.  Do you know what can make cpu burn for
>> > 30secs on the atomic copy/restore step?
>
> No idea.
>
> Is this reproducible with /sys/power/pm_test = core?
>
> [If you echo "core" to /sys/power/pm_test and then attempt to hibernate,
> it should just simulate snapshotting the system, sleep for 5 seconds and go
> back to your command prompt (or wherever you started the "hibernation").]
>
>> > Pedro, can you please try to reproduce the problem without TuxOnIce
>> > patches?
>> >
>> > Thanks.
>> >
>> > --
>> > tejun
>> >
>>
>> Hi Rafael and Tejun,
>>
>> I am experiencing the same delay with the kernel swsusp, so this is
>> definitely not a TuxOnIce issue.
>
> I don't think this can be a TuxOnIce.  Most likely, ACPI is trying to do
> something stupid in _PTS.
>
> Rafael
>

Hi,

The issue also occurs with pm_test.
Attached are two files, the one ending with "ok" shows a good pm_test
cycle, with the ultrabay powered on. The one ending in "bad" shows the
bad pm_test cycle with a 30 second delay between:

[  545.764159] ACPI: Waking up from system sleep state S4
[  575.844179] ACPI: \_SB_.PCI0.SATA.PRT1 - docking
(lines 48/49)

Please let me know how I can assist you.

Thanks for everything,
Pedro
Rafael J. Wysocki Dec. 27, 2009, 2:09 p.m. UTC | #6
On Saturday 26 December 2009, Pedro Ribeiro wrote:
> On Sat, Dec 26, 2009 at 9:21 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Saturday 26 December 2009, Pedro Ribeiro wrote:
> >> On Sat, Dec 26, 2009 at 1:26 AM, Tejun Heo <tj@kernel.org> wrote:
> >> > (cc'ing Rafael)
> >> >
> >> > On 12/26/2009 03:10 AM, Pedro Ribeiro wrote:
> >> >>> Oh... Can you please try this one then?
> >> >>
> >> >> Sorry, still hangs with this patch :(
> >> >>
> >> >> This only happens in hibernation... suspend is fine.
> >> >
> >> > Hmmm... the patch does apply to both suspend and hibernation.  Rafael,
> >> > Pedro's ThinkPad T400 burns cpu cycle for 30secs on "Atomic
> >> > copy/restore" step of hibernation if the ultrabay is powered off.  The
> >> > original report can be found at...
> >> >
> >> >  http://thread.gmane.org/gmane.linux.ide/44190
> >> >
> >> > Please note that it's with TuxOnIce patches.  Anyways, ata_piix on
> >> > certain configurations have similar issues where after the OS is done
> >> > suspending and powering off the piix controller, the BIOS tries to
> >> > access it and ends up burning cpu cycles which can be worked around by
> >> > leaving the controller on after suspend.  Applying the same workaround
> >> > didn't resolve the issue.  Do you know what can make cpu burn for
> >> > 30secs on the atomic copy/restore step?
> >
> > No idea.
> >
> > Is this reproducible with /sys/power/pm_test = core?
> >
> > [If you echo "core" to /sys/power/pm_test and then attempt to hibernate,
> > it should just simulate snapshotting the system, sleep for 5 seconds and go
> > back to your command prompt (or wherever you started the "hibernation").]
> >
> >> > Pedro, can you please try to reproduce the problem without TuxOnIce
> >> > patches?
> >> >
> >> > Thanks.
> >> >
> >> > --
> >> > tejun
> >> >
> >>
> >> Hi Rafael and Tejun,
> >>
> >> I am experiencing the same delay with the kernel swsusp, so this is
> >> definitely not a TuxOnIce issue.
> >
> > I don't think this can be a TuxOnIce.  Most likely, ACPI is trying to do
> > something stupid in _PTS.
> >
> > Rafael
> >
> 
> Hi,
> 
> The issue also occurs with pm_test.
> Attached are two files, the one ending with "ok" shows a good pm_test
> cycle, with the ultrabay powered on. The one ending in "bad" shows the
> bad pm_test cycle with a 30 second delay between:
> 
> [  545.764159] ACPI: Waking up from system sleep state S4
> [  575.844179] ACPI: \_SB_.PCI0.SATA.PRT1 - docking
> (lines 48/49)
> 
> Please let me know how I can assist you.

Please echo "shutdown" to /sys/power/disk and rerun the test with
/sys/power/pm_test = core.

Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pedro Ribeiro Dec. 27, 2009, 5:59 p.m. UTC | #7
>
> Please echo "shutdown" to /sys/power/disk and rerun the test with
> /sys/power/pm_test = core.
>
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Right on the money Rafael!

This time there was no delay, as both logs show.

Pedro
diff mbox

Patch

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index b8bea10..dadf8b2 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -2980,6 +2980,35 @@  static inline void ahci_gtf_filter_workaround(struct ata_host *host)
 {}
 #endif
 
+#ifdef CONFIG_PM
+static void ahci_insomnia_workaround(struct ata_host *host)
+{
+	static const struct dmi_system_id sysids[] = {
+		{
+			.ident = "ThinkPad T400",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "7417PLU"),
+			},
+		},
+
+		{ }	/* terminate list */
+	};
+	struct pci_dev *pdev = to_pci_dev(host->dev);
+
+	if (pdev->bus->number != 0 || pdev->devfn != PCI_DEVFN(0x1f, 2) ||
+	    !dmi_check_system(sysids))
+		return;
+
+	dev_printk(KERN_INFO, &pdev->dev, "BIOS may access controller "
+		   "after suspend, setting INSOMNIA\n");
+	host->flags |= ATA_HOST_INSOMNIA;
+}
+#else
+static inline void ahci_insomnia_workaround(struct ata_host *host)
+{}
+#endif
+
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version;
@@ -3156,6 +3185,9 @@  static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* apply gtf filter quirk */
 	ahci_gtf_filter_workaround(host);
 
+	/* need to set insomnia? */
+	ahci_insomnia_workaround(host);
+
 	/* initialize adapter */
 	rc = ahci_configure_dma_masks(pdev, hpriv->cap & HOST_CAP_64);
 	if (rc)
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 19136a7..5ef948d 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -125,9 +125,6 @@  enum {
 	RV			= -3, /* reserved */
 
 	PIIX_AHCI_DEVICE	= 6,
-
-	/* host->flags bits */
-	PIIX_HOST_BROKEN_SUSPEND = (1 << 24),
 };
 
 enum piix_controller_ids {
@@ -173,10 +170,6 @@  static int piix_sidpr_scr_read(struct ata_link *link,
 			       unsigned int reg, u32 *val);
 static int piix_sidpr_scr_write(struct ata_link *link,
 				unsigned int reg, u32 val);
-#ifdef CONFIG_PM
-static int piix_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
-static int piix_pci_device_resume(struct pci_dev *pdev);
-#endif
 
 static unsigned int in_module_init = 1;
 
@@ -300,8 +293,8 @@  static struct pci_driver piix_pci_driver = {
 	.probe			= piix_init_one,
 	.remove			= piix_remove_one,
 #ifdef CONFIG_PM
-	.suspend		= piix_pci_device_suspend,
-	.resume			= piix_pci_device_resume,
+	.suspend		= ata_pci_device_suspend,
+	.resume			= ata_pci_device_resume,
 #endif
 };
 
@@ -963,7 +956,7 @@  static int piix_sidpr_scr_write(struct ata_link *link,
 }
 
 #ifdef CONFIG_PM
-static int piix_broken_suspend(void)
+static void piix_insomnia_workaround(struct ata_host *host)
 {
 	static const struct dmi_system_id sysids[] = {
 		{
@@ -1091,14 +1084,15 @@  static int piix_broken_suspend(void)
 	static const char *oemstrs[] = {
 		"Tecra M3,",
 	};
+	struct pci_dev *pdev = to_pci_dev(host->dev);
 	int i;
 
 	if (dmi_check_system(sysids))
-		return 1;
+		goto apply;
 
 	for (i = 0; i < ARRAY_SIZE(oemstrs); i++)
 		if (dmi_find_device(DMI_DEV_TYPE_OEM_STRING, oemstrs[i], NULL))
-			return 1;
+			goto apply;
 
 	/* TECRA M4 sometimes forgets its identify and reports bogus
 	 * DMI information.  As the bogus information is a bit
@@ -1113,76 +1107,18 @@  static int piix_broken_suspend(void)
 	    dmi_match(DMI_BOARD_VENDOR, "TOSHIBA") &&
 	    dmi_match(DMI_BOARD_NAME, "Portable PC") &&
 	    dmi_match(DMI_BOARD_VERSION, "Version A0"))
-		return 1;
-
-	return 0;
-}
-
-static int piix_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg)
-{
-	struct ata_host *host = dev_get_drvdata(&pdev->dev);
-	unsigned long flags;
-	int rc = 0;
-
-	rc = ata_host_suspend(host, mesg);
-	if (rc)
-		return rc;
-
-	/* Some braindamaged ACPI suspend implementations expect the
-	 * controller to be awake on entry; otherwise, it burns cpu
-	 * cycles and power trying to do something to the sleeping
-	 * beauty.
-	 */
-	if (piix_broken_suspend() && (mesg.event & PM_EVENT_SLEEP)) {
-		pci_save_state(pdev);
-
-		/* 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 (pdev->current_state == PCI_D0)
-			pdev->current_state = PCI_UNKNOWN;
-
-		/* tell resume that it's waking up from broken suspend */
-		spin_lock_irqsave(&host->lock, flags);
-		host->flags |= PIIX_HOST_BROKEN_SUSPEND;
-		spin_unlock_irqrestore(&host->lock, flags);
-	} else
-		ata_pci_device_do_suspend(pdev, mesg);
-
-	return 0;
-}
-
-static int piix_pci_device_resume(struct pci_dev *pdev)
-{
-	struct ata_host *host = dev_get_drvdata(&pdev->dev);
-	unsigned long flags;
-	int rc;
-
-	if (host->flags & PIIX_HOST_BROKEN_SUSPEND) {
-		spin_lock_irqsave(&host->lock, flags);
-		host->flags &= ~PIIX_HOST_BROKEN_SUSPEND;
-		spin_unlock_irqrestore(&host->lock, flags);
-
-		pci_set_power_state(pdev, PCI_D0);
-		pci_restore_state(pdev);
+		goto apply;
 
-		/* PCI device wasn't disabled during suspend.  Use
-		 * pci_reenable_device() to avoid affecting the enable
-		 * count.
-		 */
-		rc = pci_reenable_device(pdev);
-		if (rc)
-			dev_printk(KERN_ERR, &pdev->dev, "failed to enable "
-				   "device after resume (%d)\n", rc);
-	} else
-		rc = ata_pci_device_do_resume(pdev);
+	return;
 
-	if (rc == 0)
-		ata_host_resume(host);
-
-	return rc;
+apply:
+	dev_printk(KERN_INFO, &pdev->dev, "BIOS may access controller "
+		   "after suspend, setting INSOMNIA\n");
+	host->flags |= ATA_HOST_INSOMNIA;
 }
+#else
+static inline void piix_insomnia_workaround(struct ata_host *host)
+{ }
 #endif
 
 static u8 piix_vmw_bmdma_status(struct ata_port *ap)
@@ -1604,6 +1540,9 @@  static int __devinit piix_init_one(struct pci_dev *pdev,
 		host->ports[1]->mwdma_mask = 0;
 		host->ports[1]->udma_mask = 0;
 	}
+
+	piix_insomnia_workaround(host);
+
 	host->flags |= ATA_HOST_PARALLEL_SCAN;
 
 	pci_set_master(pdev);
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 22ff51b..3c7a1f3 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6381,21 +6381,62 @@  int pci_test_config_bits(struct pci_dev *pdev, const struct pci_bits *bits)
 #ifdef CONFIG_PM
 void ata_pci_device_do_suspend(struct pci_dev *pdev, pm_message_t mesg)
 {
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	unsigned long flags;
+
 	pci_save_state(pdev);
-	pci_disable_device(pdev);
 
-	if (mesg.event & PM_EVENT_SLEEP)
-		pci_set_power_state(pdev, PCI_D3hot);
+	/*
+	 * Some braindamaged ACPI suspend implementations expect the
+	 * controller to be awake on entry; otherwise, it burns cpu
+	 * cycles and power trying to do something to the sleeping
+	 * beauty.
+	 */
+	if ((host->flags & ATA_HOST_INSOMNIA) &&
+	    (mesg.event & PM_EVENT_SLEEP)) {
+		/*
+		 * 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 (pdev->current_state == PCI_D0)
+			pdev->current_state = PCI_UNKNOWN;
+
+		/* tell resume that it's waking up from insomnia */
+		spin_lock_irqsave(&host->lock, flags);
+		host->flags |= ATA_HOST_IN_INSOMNIA;
+		spin_unlock_irqrestore(&host->lock, flags);
+	} else {
+		pci_disable_device(pdev);
+
+		if (mesg.event & PM_EVENT_SLEEP)
+			pci_set_power_state(pdev, PCI_D3hot);
+	}
 }
 
 int ata_pci_device_do_resume(struct pci_dev *pdev)
 {
+	struct ata_host *host = dev_get_drvdata(&pdev->dev);
+	unsigned long flags;
 	int rc;
 
 	pci_set_power_state(pdev, PCI_D0);
 	pci_restore_state(pdev);
 
-	rc = pcim_enable_device(pdev);
+	if (host->flags & ATA_HOST_IN_INSOMNIA) {
+		spin_lock_irqsave(&host->lock, flags);
+		host->flags &= ~ATA_HOST_IN_INSOMNIA;
+		spin_unlock_irqrestore(&host->lock, flags);
+
+		/*
+		 * PCI device wasn't disabled during suspend.  Use
+		 * pci_reenable_device() to avoid affecting the enable
+		 * count.
+		 */
+		rc = pci_reenable_device(pdev);
+	} else
+		rc = pcim_enable_device(pdev);
+
 	if (rc) {
 		dev_printk(KERN_ERR, &pdev->dev,
 			   "failed to enable device after resume (%d)\n", rc);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 6a9c4dd..8b60fed 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -243,9 +243,11 @@  enum {
 	ATA_QCFLAG_EH_SCHEDULED = (1 << 18), /* EH scheduled (obsolete) */
 
 	/* host set flags */
-	ATA_HOST_SIMPLEX	= (1 << 0),	/* Host is simplex, one DMA channel per host only */
-	ATA_HOST_STARTED	= (1 << 1),	/* Host started */
-	ATA_HOST_PARALLEL_SCAN	= (1 << 2),	/* Ports on this host can be scanned in parallel */
+	ATA_HOST_SIMPLEX	= (1 << 0), /* Host is simplex, one DMA channel per host only */
+	ATA_HOST_STARTED	= (1 << 1), /* Host started */
+	ATA_HOST_PARALLEL_SCAN	= (1 << 2), /* Ports on this host can be scanned in parallel */
+	ATA_HOST_INSOMNIA	= (1 << 3), /* Don't power down on suspend */
+	ATA_HOST_IN_INSOMNIA	= (1 << 4), /* Insomnia in progress */
 
 	/* bits 24:31 of host->flags are reserved for LLD specific flags */