diff mbox

SATA / AHCI: Do not play with the link PM during suspend to RAM (was: Re: HDD not suspending properly / dead on resume)

Message ID 201007282350.09676.rjw@sisk.pl
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Rafael J. Wysocki July 28, 2010, 9:50 p.m. UTC 
On Saturday, July 10, 2010, Tejun Heo wrote:
> On 07/10/2010 08:50 AM, Stephan Diestelhorst wrote:
> >> I have a box where this problem is kind of reproducible, but it happens _very_
> >> rarely.  Also I can't reproduce it on demand running suspend-resume in a tight
> >> loop.  Are you able to reproduce it more regurarly?
> > 
> > For me it is much more reproducible. If I run multiple direct writing
> > dd-s to the disk in question I trigger it rather reliably (~75% or
> > higher). See the attached script from an earlier email.
> > Maybe that helps triggering your case more reliabl, too?
> 
> Can you please try the following git tree?
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/tj/misc.git libata-irq-expect

That didn't help, but the appended patch fixes the problem for me.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: SATA / AHCI: Do not play with the link PM during suspend to RAM

My Acer Ferrari One occasionally loses communication with the disk
(which in fact is an Intel SSD) during suspend to RAM.  The symptom
is that the IDENTIFY command times out during suspend and the device
is dropped by the kernel, so it is not available during resume and
the system is unuseable as a result.  The failure is not readily
reproducible, although it happens once every several suspends and
it always happens after the disk has been shut down by the SCSI
layer's suspend routine.

I was able to track this issue down to the link PM manipulations
carried out by ata_host_suspend(), which probably means that the
SSD's firmware is not implemented correctly.  However, the AHCI
driver, which is used on the affected box, doesn't really need to do
anything with the link PM during suspend to RAM, because the whole
controller is going to be put into D3 by ata_pci_device_do_suspend()
immediately and it will undergo full reset during the subsequent
resume anyway.  For this reason, make the AHCI driver avoid calling
ata_host_suspend() during suspend to RAM which fixes the problem and
makes sense as a general optimization.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/ata/ahci.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

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

Comments

Tejun Heo July 30, 2010, 2:18 p.m. UTC | #1 
Hello, Rafael.

Sorry about the delay.  There was a tiny crisis here and the whole
link pm seems to need a lot more work than I originally expected.  I'm
working on it now.  I'll probably have something for you to test in a
few days.

Thanks.
Stephan Diestelhorst Aug. 2, 2010, 8:48 p.m. UTC | #2 
On Wednesday 28 July 2010, 23:50:09 Rafael J. Wysocki wrote:
> On Saturday, July 10, 2010, Tejun Heo wrote:
> > On 07/10/2010 08:50 AM, Stephan Diestelhorst wrote:
> > >> I have a box where this problem is kind of reproducible, but it happens _very_
> > >> rarely.  Also I can't reproduce it on demand running suspend-resume in a tight
> > >> loop.  Are you able to reproduce it more regurarly?
> > > 
> > > For me it is much more reproducible. If I run multiple direct writing
> > > dd-s to the disk in question I trigger it rather reliably (~75% or
> > > higher). See the attached script from an earlier email.
> > > Maybe that helps triggering your case more reliabl, too?
> > 
> That didn't help, but the appended patch fixes the problem for me.

<snip>

Sorry for taking ages. Vacation and catching up after it are to blame,
as is me forgetting to build a proper initrd...

Thanks for the patch! It certainly changes behaviour, however, in a
very strange way for me. With your patch my machine does not suspend
to ram anymore (a simple echo mem > /proc/sys/state blocks), and
nothing happens in dmesg if there is a lot of write I/O while
suspending. (A number of parallel dd's with oflag=direct)

If I stop the I/O, the system eventually goes into suspend to RAM.
However, that takes a while, after the I/O has stopped, and also
from "Preparing system for suspend" log entry until it is actually
done.

Is this intentional? Let me know how I can debug this further!
Ideally I'd like to be able to suspend the machine under I/O load,
too. (E.g. during a compile job.)

Can you reproduce this at your end, too?

Many thanks,
  Stephan
--
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 Aug. 2, 2010, 9:38 p.m. UTC | #3 
On Monday, August 02, 2010, Stephan Diestelhorst wrote:
> On Wednesday 28 July 2010, 23:50:09 Rafael J. Wysocki wrote:
> > On Saturday, July 10, 2010, Tejun Heo wrote:
> > > On 07/10/2010 08:50 AM, Stephan Diestelhorst wrote:
> > > >> I have a box where this problem is kind of reproducible, but it happens _very_
> > > >> rarely.  Also I can't reproduce it on demand running suspend-resume in a tight
> > > >> loop.  Are you able to reproduce it more regurarly?
> > > > 
> > > > For me it is much more reproducible. If I run multiple direct writing
> > > > dd-s to the disk in question I trigger it rather reliably (~75% or
> > > > higher). See the attached script from an earlier email.
> > > > Maybe that helps triggering your case more reliabl, too?
> > > 
> > That didn't help, but the appended patch fixes the problem for me.
> 
> <snip>
> 
> Sorry for taking ages. Vacation and catching up after it are to blame,
> as is me forgetting to build a proper initrd...
> 
> Thanks for the patch! It certainly changes behaviour, however, in a
> very strange way for me. With your patch my machine does not suspend
> to ram anymore (a simple echo mem > /proc/sys/state blocks), and
> nothing happens in dmesg if there is a lot of write I/O while
> suspending. (A number of parallel dd's with oflag=direct)
> 
> If I stop the I/O, the system eventually goes into suspend to RAM.
> However, that takes a while, after the I/O has stopped, and also
> from "Preparing system for suspend" log entry until it is actually
> done.
> 
> Is this intentional?

It surely isn't.

> Let me know how I can debug this further!
> Ideally I'd like to be able to suspend the machine under I/O load,
> too. (E.g. during a compile job.)
> 
> Can you reproduce this at your end, too?

Well, I didn't try suspending with a number of parallel dd's with oflag=direct
in the background, but otherwise I'm not reproducing the issue with
the patch applied.

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
Stephan Diestelhorst Aug. 3, 2010, 8:36 a.m. UTC | #4 
On Monday 02 August 2010, 23:38:05 Rafael J. Wysocki wrote:
> On Monday, August 02, 2010, Stephan Diestelhorst wrote:
> > On Wednesday 28 July 2010, 23:50:09 Rafael J. Wysocki wrote:
> > > On Saturday, July 10, 2010, Tejun Heo wrote:
> > > > On 07/10/2010 08:50 AM, Stephan Diestelhorst wrote:
> > > > >> I have a box where this problem is kind of reproducible, but it happens _very_
> > > > >> rarely.  Also I can't reproduce it on demand running suspend-resume in a tight
> > > > >> loop.  Are you able to reproduce it more regurarly?
> > > > > 
> > > > > For me it is much more reproducible. If I run multiple direct writing
> > > > > dd-s to the disk in question I trigger it rather reliably (~75% or
> > > > > higher). See the attached script from an earlier email.
> > > > > Maybe that helps triggering your case more reliabl, too?
> > > > 
> > > That didn't help, but the appended patch fixes the problem for me.
> > 
> > <snip>
> > 
> > Sorry for taking ages. Vacation and catching up after it are to blame,
> > as is me forgetting to build a proper initrd...
> > 
> > Thanks for the patch! It certainly changes behaviour, however, in a
> > very strange way for me. With your patch my machine does not suspend
> > to ram anymore (a simple echo mem > /proc/sys/state blocks), and
> > nothing happens in dmesg if there is a lot of write I/O while
> > suspending. (A number of parallel dd's with oflag=direct)
> > 
> > If I stop the I/O, the system eventually goes into suspend to RAM.
> > However, that takes a while, after the I/O has stopped, and also
> > from "Preparing system for suspend" log entry until it is actually
> > done.
> > 
> > Is this intentional?
> 
> It surely isn't.
> 
> > Let me know how I can debug this further!
> > Ideally I'd like to be able to suspend the machine under I/O load,
> > too. (E.g. during a compile job.)
> > 
> > Can you reproduce this at your end, too?
> 
> Well, I didn't try suspending with a number of parallel dd's with oflag=direct
> in the background, but otherwise I'm not reproducing the issue with
> the patch applied.

Mhmhm, I have tried to reproduce my issue again, and also added some
dev_printk's around your code to understand where the delay is
happening.

However, I have not been able to reproduce the issue (with and without
the debug output) anymore, and I am happy to report that for now your
patch helps.

I'd like to keep this under observation for a little while longer,
though.

Many thanks,
  Stephan
Rafael J. Wysocki Aug. 3, 2010, 9:13 p.m. UTC | #5 
On Tuesday, August 03, 2010, Stephan Diestelhorst wrote:
> On Monday 02 August 2010, 23:38:05 Rafael J. Wysocki wrote:
> > On Monday, August 02, 2010, Stephan Diestelhorst wrote:
> > > On Wednesday 28 July 2010, 23:50:09 Rafael J. Wysocki wrote:
> > > > On Saturday, July 10, 2010, Tejun Heo wrote:
> > > > > On 07/10/2010 08:50 AM, Stephan Diestelhorst wrote:
> > > > > >> I have a box where this problem is kind of reproducible, but it happens _very_
> > > > > >> rarely.  Also I can't reproduce it on demand running suspend-resume in a tight
> > > > > >> loop.  Are you able to reproduce it more regurarly?
> > > > > > 
> > > > > > For me it is much more reproducible. If I run multiple direct writing
> > > > > > dd-s to the disk in question I trigger it rather reliably (~75% or
> > > > > > higher). See the attached script from an earlier email.
> > > > > > Maybe that helps triggering your case more reliabl, too?
> > > > > 
> > > > That didn't help, but the appended patch fixes the problem for me.
> > > 
> > > <snip>
> > > 
> > > Sorry for taking ages. Vacation and catching up after it are to blame,
> > > as is me forgetting to build a proper initrd...
> > > 
> > > Thanks for the patch! It certainly changes behaviour, however, in a
> > > very strange way for me. With your patch my machine does not suspend
> > > to ram anymore (a simple echo mem > /proc/sys/state blocks), and
> > > nothing happens in dmesg if there is a lot of write I/O while
> > > suspending. (A number of parallel dd's with oflag=direct)
> > > 
> > > If I stop the I/O, the system eventually goes into suspend to RAM.
> > > However, that takes a while, after the I/O has stopped, and also
> > > from "Preparing system for suspend" log entry until it is actually
> > > done.
> > > 
> > > Is this intentional?
> > 
> > It surely isn't.
> > 
> > > Let me know how I can debug this further!
> > > Ideally I'd like to be able to suspend the machine under I/O load,
> > > too. (E.g. during a compile job.)
> > > 
> > > Can you reproduce this at your end, too?
> > 
> > Well, I didn't try suspending with a number of parallel dd's with oflag=direct
> > in the background, but otherwise I'm not reproducing the issue with
> > the patch applied.
> 
> Mhmhm, I have tried to reproduce my issue again, and also added some
> dev_printk's around your code to understand where the delay is
> happening.
> 
> However, I have not been able to reproduce the issue (with and without
> the debug output) anymore, and I am happy to report that for now your
> patch helps.

Good.

What you might be seeing is that the patch generally changes the timing of
suspend and since it is done asynchronously by default the change might trigger
an independent bug that was sensitive to timing.

> I'd like to keep this under observation for a little while longer, though.

You can try to remove the noise produced by asynchronous suspend from the
picture by dong "echo 0 > /sys/power/pm_async" (just once after bootup).

Thanks,
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
diff mbox

Patch

Index: linux-2.6/drivers/ata/ahci.c
===================================================================
--- linux-2.6.orig/drivers/ata/ahci.c
+++ linux-2.6/drivers/ata/ahci.c
@@ -595,6 +595,7 @@  static int ahci_pci_device_suspend(struc
 	struct ahci_host_priv *hpriv = host->private_data;
 	void __iomem *mmio = hpriv->mmio;
 	u32 ctl;
+	int rc = 0;
 
 	if (mesg.event & PM_EVENT_SUSPEND &&
 	    hpriv->flags & AHCI_HFLAG_NO_SUSPEND) {
@@ -614,7 +615,15 @@  static int ahci_pci_device_suspend(struc
 		readl(mmio + HOST_CTL); /* flush */
 	}
 
-	return ata_pci_device_suspend(pdev, mesg);
+	if (mesg.event == PM_EVENT_SUSPEND)
+		pdev->dev.power.power_state = mesg;
+	else
+		rc = ata_host_suspend(host, mesg);
+
+	if (!rc)
+		ata_pci_device_do_suspend(pdev, mesg);
+
+	return rc;
 }
 
 static int ahci_pci_device_resume(struct pci_dev *pdev)