LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* Regression: SATA disk double spin-off during hibernation on hp nx6325
@ 2008-08-28 22:02 Rafael J. Wysocki
  2008-08-29 10:03 ` Tejun Heo
       [not found] ` <200808291245.27436.elendil@planet.nl>
  0 siblings, 2 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-08-28 22:02 UTC (permalink / raw)
  To: ACPI Devel Maling List
  Cc: linux-ide, Thomas Renninger, Tejun Heo, Robert Hancock, LKML

Hi,

It appears that the "double spin-off" problem described in

http://bugzilla.kernel.org/show_bug.cgi?id=8855

has recently started to appear during hibernation as well as during shutdown.
It didn't appear during hibernation on my hp nx6325 with 2.6.26, so this is a
recent regression.

I have prepared the appended patch that fixes the problem for me, based on the
 earlier Tejun's patch at

http://bugzilla.kernel.org/attachment.cgi?id=15441&action=view

but I'm not really sure if this is an acceptable solution.  Please advise.

Thanks,
Rafael


not-yet-signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/ata/libata-scsi.c |   16 +++++++++++++---
 drivers/ata/sata_sil.c    |   30 +++++++++++++++++++++++++++++-
 include/linux/libata.h    |    1 +
 kernel/power/disk.c       |    2 ++
 4 files changed, 45 insertions(+), 4 deletions(-)

Index: linux-2.6/drivers/ata/libata-scsi.c
===================================================================
--- linux-2.6.orig/drivers/ata/libata-scsi.c
+++ linux-2.6/drivers/ata/libata-scsi.c
@@ -1181,6 +1181,14 @@ static unsigned int ata_scsi_start_stop_
 
 		tf->command = ATA_CMD_VERIFY;	/* READ VERIFY */
 	} else {
+		/* Some odd clown BIOSen issue spindown on shutdown
+		 * causing some drives to spin up and down again.
+		 */
+		if ((qc->ap->flags & ATA_FLAG_NO_SHUTDOWN_SPINDOWN) &&
+		    (system_state == SYSTEM_POWER_OFF ||
+		     system_state == SYSTEM_SUSPEND_DISK))
+			goto skip;
+
 		/* XXX: This is for backward compatibility, will be
 		 * removed.  Read Documentation/feature-removal-schedule.txt
 		 * for more info.
@@ -1204,8 +1212,7 @@ static unsigned int ata_scsi_start_stop_
 				scmd->scsi_done = qc->scsidone;
 				qc->scsidone = ata_delayed_done;
 			}
-			scmd->result = SAM_STAT_GOOD;
-			return 1;
+			goto skip;
 		}
 
 		/* Issue ATA STANDBY IMMEDIATE command */
@@ -1221,10 +1228,13 @@ static unsigned int ata_scsi_start_stop_
 
 	return 0;
 
-invalid_fld:
+ invalid_fld:
 	ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x24, 0x0);
 	/* "Invalid field in cbd" */
 	return 1;
+ skip:
+	scmd->result = SAM_STAT_GOOD;
+	return 1;
 }
 
 
Index: linux-2.6/drivers/ata/sata_sil.c
===================================================================
--- linux-2.6.orig/drivers/ata/sata_sil.c
+++ linux-2.6/drivers/ata/sata_sil.c
@@ -603,11 +603,33 @@ static void sil_init_controller(struct a
 	}
 }
 
+static int sil_broken_shutdown(struct pci_dev *pdev)
+{
+	static const struct dmi_system_id sysid_nx6325[] = {
+		{
+			.ident = "HP Compaq nx6325",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nx6325"),
+			},
+		},
+
+		{ }	/* terminate list */
+	};
+
+	/* apply the quirk only to the on-board controller */
+	if (dmi_check_system(sysid_nx6325) && PCI_SLOT(pdev->devfn) == 18)
+		return 1;
+
+	return 0;
+}
+
 static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version;
 	int board_id = ent->driver_data;
-	const struct ata_port_info *ppi[] = { &sil_port_info[board_id], NULL };
+	struct ata_port_info pi = sil_port_info[board_id];
+	const struct ata_port_info *ppi[] = { &pi, NULL };
 	struct ata_host *host;
 	void __iomem *mmio_base;
 	int n_ports, rc;
@@ -621,6 +643,12 @@ static int sil_init_one(struct pci_dev *
 	if (board_id == sil_3114)
 		n_ports = 4;
 
+	if (sil_broken_shutdown(pdev)) {
+		pi.flags |= ATA_FLAG_NO_SHUTDOWN_SPINDOWN;
+		dev_printk(KERN_INFO, &pdev->dev,
+			   "quirky BIOS, skipping spindown on shutdown\n");
+	}
+
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
 	if (!host)
 		return -ENOMEM;
Index: linux-2.6/include/linux/libata.h
===================================================================
--- linux-2.6.orig/include/linux/libata.h
+++ linux-2.6/include/linux/libata.h
@@ -187,6 +187,7 @@ enum {
 					     * doesn't handle PIO interrupts */
 	ATA_FLAG_NCQ		= (1 << 10), /* host supports NCQ */
 	ATA_FLAG_DEBUGMSG	= (1 << 13),
+	ATA_FLAG_NO_SHUTDOWN_SPINDOWN = (1 << 14), /* don't spindown on shutdown */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
 	ATA_FLAG_ACPI_SATA	= (1 << 17), /* need native SATA ACPI layout */
Index: linux-2.6/kernel/power/disk.c
===================================================================
--- linux-2.6.orig/kernel/power/disk.c
+++ linux-2.6/kernel/power/disk.c
@@ -416,6 +416,7 @@ int hibernation_platform_enter(void)
 	if (error)
 		goto Close;
 
+	system_state = SYSTEM_SUSPEND_DISK;
 	suspend_console();
 	ftrace_save = __ftrace_enabled_save();
 	error = device_suspend(PMSG_HIBERNATE);
@@ -451,6 +452,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
+	system_state = SYSTEM_RUNNING;
 	device_resume(PMSG_RESTORE);
 	__ftrace_enabled_restore(ftrace_save);
 	resume_console();

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Regression: SATA disk double spin-off during hibernation on hp nx6325
  2008-08-28 22:02 Regression: SATA disk double spin-off during hibernation on hp nx6325 Rafael J. Wysocki
@ 2008-08-29 10:03 ` Tejun Heo
  2008-08-29 10:41   ` Rafael J. Wysocki
       [not found] ` <200808291245.27436.elendil@planet.nl>
  1 sibling, 1 reply; 49+ messages in thread
From: Tejun Heo @ 2008-08-29 10:03 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML

Hello,

Rafael J. Wysocki wrote:
> It appears that the "double spin-off" problem described in
> 
> http://bugzilla.kernel.org/show_bug.cgi?id=8855
> 
> has recently started to appear during hibernation as well as during shutdown.
> It didn't appear during hibernation on my hp nx6325 with 2.6.26, so this is a
> recent regression.
> 
> I have prepared the appended patch that fixes the problem for me, based on the
>  earlier Tejun's patch at
> 
> http://bugzilla.kernel.org/attachment.cgi?id=15441&action=view
> 
> but I'm not really sure if this is an acceptable solution.  Please advise.
> 
> Thanks,
> Rafael
> 
> 
> not-yet-signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

The libata part looks fine to me but what are those SYSTEM_SUSPEND_DISK
stuff?  They don't look like belonging to this patch.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Regression: SATA disk double spin-off during hibernation on hp nx6325
  2008-08-29 10:03 ` Tejun Heo
@ 2008-08-29 10:41   ` Rafael J. Wysocki
  2008-08-29 10:42     ` Tejun Heo
  0 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-08-29 10:41 UTC (permalink / raw)
  To: Tejun Heo
  Cc: ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML

On Friday, 29 of August 2008, Tejun Heo wrote:
> Hello,
> 
> Rafael J. Wysocki wrote:
> > It appears that the "double spin-off" problem described in
> > 
> > http://bugzilla.kernel.org/show_bug.cgi?id=8855
> > 
> > has recently started to appear during hibernation as well as during shutdown.
> > It didn't appear during hibernation on my hp nx6325 with 2.6.26, so this is a
> > recent regression.
> > 
> > I have prepared the appended patch that fixes the problem for me, based on the
> >  earlier Tejun's patch at
> > 
> > http://bugzilla.kernel.org/attachment.cgi?id=15441&action=view
> > 
> > but I'm not really sure if this is an acceptable solution.  Please advise.
> > 
> > Thanks,
> > Rafael
> > 
> > 
> > not-yet-signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> The libata part looks fine to me but what are those SYSTEM_SUSPEND_DISK
> stuff?  They don't look like belonging to this patch.

Actaully, they do belong to it.  This is the part "fixing" the hibernation code
path, in which the disk is also powered off unnecessarily.

Well, probably I should use SYSTEM_HIBERNATE_ENTER or something similar
instead of SYSTEM_SUSPEND_DISK.

In short, the idea is to change system_state to something specific to the last
phase of hibernation (after saving the image) and check that in
ata_scsi_start_stop_xlat().  In fact that's completely analogous to what's done
for SYSTEM_POWER_OFF in there.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Regression: SATA disk double spin-off during hibernation on hp nx6325
  2008-08-29 10:41   ` Rafael J. Wysocki
@ 2008-08-29 10:42     ` Tejun Heo
  2008-09-06 17:23       ` [PATCH] SATA: Blacklist systems that spin off disks during ACPI power off Rafael J. Wysocki
  0 siblings, 1 reply; 49+ messages in thread
From: Tejun Heo @ 2008-08-29 10:42 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML

Rafael J. Wysocki wrote:
> Actaully, they do belong to it.  This is the part "fixing" the hibernation code
> path, in which the disk is also powered off unnecessarily.
> 
> Well, probably I should use SYSTEM_HIBERNATE_ENTER or something similar
> instead of SYSTEM_SUSPEND_DISK.
> 
> In short, the idea is to change system_state to something specific to the last
> phase of hibernation (after saving the image) and check that in
> ata_scsi_start_stop_xlat().  In fact that's completely analogous to what's done
> for SYSTEM_POWER_OFF in there.

Ah.. right, missed the added check for SUSPEND_DISK in libata-scsi.c.
Maybe it's a good idea to note it in the commit message later?

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Regression: SATA disk double spin-off during hibernation on hp nx6325
       [not found] ` <200808291245.27436.elendil@planet.nl>
@ 2008-08-29 10:57   ` Rafael J. Wysocki
  2008-08-29 11:30     ` Frans Pop
  0 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-08-29 10:57 UTC (permalink / raw)
  To: Frans Pop; +Cc: linux-acpi, linux-ide, trenn, htejun, hancockr, linux-kernel

On Friday, 29 of August 2008, Frans Pop wrote:
> Rafael J. Wysocki wrote:
> > It appears that the "double spin-off" problem described in
> > http://bugzilla.kernel.org/show_bug.cgi?id=8855
> > 
> > has recently started to appear during hibernation as well as during
> > shutdown. It didn't appear during hibernation on my hp nx6325 with
> > 2.6.26, so this is a recent regression.
> 
> FWIW, I also have the double spin-off on my HP Compaq 2510p, but only hear 
> it on shutdown and not on suspend.

Suspend to RAM is fine, hibernation (aka suspend to disk) is not (on my box).
Does it mean that hibernation is fine on your box with 2.6.27-rc5, although
shutdown is not?  That would require special DMI-dependent handling, so please
double check.

Anyway, please attach the output of dmidecode to
http://bugzilla.kernel.org/show_bug.cgi?id=8855
so I can include your box into the final version of the patch.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Regression: SATA disk double spin-off during hibernation on hp nx6325
  2008-08-29 10:57   ` Regression: SATA disk double spin-off during hibernation on hp nx6325 Rafael J. Wysocki
@ 2008-08-29 11:30     ` Frans Pop
  2008-09-04 14:05       ` Rafael J. Wysocki
  0 siblings, 1 reply; 49+ messages in thread
From: Frans Pop @ 2008-08-29 11:30 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-acpi, linux-ide, trenn, htejun, hancockr, linux-kernel

On Friday 29 August 2008, Rafael J. Wysocki wrote:
> Suspend to RAM is fine, hibernation (aka suspend to disk) is not (on my
> box). Does it mean that hibernation is fine on your box with
> 2.6.27-rc5, although shutdown is not?  That would require special
> DMI-dependent handling, so please double check.

Ah, right. The double spin down happens during hibernation too.

> Anyway, please attach the output of dmidecode to
> http://bugzilla.kernel.org/show_bug.cgi?id=8855
> so I can include your box into the final version of the patch.

Done.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Regression: SATA disk double spin-off during hibernation on hp nx6325
  2008-08-29 11:30     ` Frans Pop
@ 2008-09-04 14:05       ` Rafael J. Wysocki
  2008-09-05 22:25         ` Frans Pop
  0 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-04 14:05 UTC (permalink / raw)
  To: Frans Pop
  Cc: linux-acpi, linux-ide, trenn, htejun, hancockr, linux-kernel,
	Maciej Rutecki

On Friday, 29 of August 2008, Frans Pop wrote:
> On Friday 29 August 2008, Rafael J. Wysocki wrote:
> > Suspend to RAM is fine, hibernation (aka suspend to disk) is not (on my
> > box). Does it mean that hibernation is fine on your box with
> > 2.6.27-rc5, although shutdown is not?  That would require special
> > DMI-dependent handling, so please double check.
> 
> Ah, right. The double spin down happens during hibernation too.
> 
> > Anyway, please attach the output of dmidecode to
> > http://bugzilla.kernel.org/show_bug.cgi?id=8855
> > so I can include your box into the final version of the patch.
> 
> Done.

I have uploaded a new version of the patch into the Bugzilla:
http://bugzilla.kernel.org/attachment.cgi?id=17618&action=view

Can you test it, please (also please attach dmesg output to the Bugzilla
entry)?

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Regression: SATA disk double spin-off during hibernation on hp nx6325
  2008-09-04 14:05       ` Rafael J. Wysocki
@ 2008-09-05 22:25         ` Frans Pop
  2008-09-05 23:18           ` Rafael J. Wysocki
  0 siblings, 1 reply; 49+ messages in thread
From: Frans Pop @ 2008-09-05 22:25 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-acpi, linux-ide, trenn, htejun, hancockr, linux-kernel,
	Maciej Rutecki

On Thursday 04 September 2008, Rafael J. Wysocki wrote:
> I have uploaded a new version of the patch into the Bugzilla:
> http://bugzilla.kernel.org/attachment.cgi?id=17618&action=view

This version does not work for me, possibly because I use a different hard 
disk driver (ata_piix instead of sata_sil)?

I've seen there are later versions of the patch, but as the comments 
mention that 2510p was removed from those I haven't tested them.

> (also please attach dmesg output to the Bugzilla entry)

Added.

Cheers,
FJP

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Regression: SATA disk double spin-off during hibernation on hp nx6325
  2008-09-05 22:25         ` Frans Pop
@ 2008-09-05 23:18           ` Rafael J. Wysocki
  2008-09-06  0:09             ` Frans Pop
  0 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-05 23:18 UTC (permalink / raw)
  To: Frans Pop
  Cc: linux-acpi, linux-ide, trenn, htejun, hancockr, linux-kernel,
	Maciej Rutecki

On Saturday, 6 of September 2008, Frans Pop wrote:
> On Thursday 04 September 2008, Rafael J. Wysocki wrote:
> > I have uploaded a new version of the patch into the Bugzilla:
> > http://bugzilla.kernel.org/attachment.cgi?id=17618&action=view
> 
> This version does not work for me, possibly because I use a different hard 
> disk driver (ata_piix instead of sata_sil)?
> 
> I've seen there are later versions of the patch, but as the comments 
> mention that 2510p was removed from those I haven't tested them.
> 
> > (also please attach dmesg output to the Bugzilla entry)
> 
> Added.

Thanks.  There's a new patch for you to test in there already. :-)

Rafael

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Regression: SATA disk double spin-off during hibernation on hp nx6325
  2008-09-05 23:18           ` Rafael J. Wysocki
@ 2008-09-06  0:09             ` Frans Pop
  2008-09-06 11:06               ` Rafael J. Wysocki
  0 siblings, 1 reply; 49+ messages in thread
From: Frans Pop @ 2008-09-06  0:09 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-acpi, linux-ide, trenn, htejun, hancockr, linux-kernel,
	Maciej Rutecki

On Saturday 06 September 2008, Rafael J. Wysocki wrote:
> Thanks.  There's a new patch for you to test in there already. :-)

Seems to work correctly. The first spindown no longer happens.

In the BTS you noted the support is "partial". What's partial about it?

Thanks Rafael.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: Regression: SATA disk double spin-off during hibernation on hp nx6325
  2008-09-06  0:09             ` Frans Pop
@ 2008-09-06 11:06               ` Rafael J. Wysocki
  0 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-06 11:06 UTC (permalink / raw)
  To: Frans Pop
  Cc: linux-acpi, linux-ide, trenn, htejun, hancockr, linux-kernel,
	Maciej Rutecki

On Saturday, 6 of September 2008, Frans Pop wrote:
> On Saturday 06 September 2008, Rafael J. Wysocki wrote:
> > Thanks.  There's a new patch for you to test in there already. :-)
> 
> Seems to work correctly. The first spindown no longer happens.
> 
> In the BTS you noted the support is "partial". What's partial about it?

I want the quirk to be only applied to on-board controllers and that wasn't
there yet for your system.

Updated patch (hopefully final) has been uploaded as:
http://bugzilla.kernel.org/attachment.cgi?id=17646&action=view

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH] SATA: Blacklist systems that spin off disks during ACPI power off
  2008-08-29 10:42     ` Tejun Heo
@ 2008-09-06 17:23       ` Rafael J. Wysocki
  2008-09-08 11:38         ` Tejun Heo
  0 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-06 17:23 UTC (permalink / raw)
  To: Tejun Heo
  Cc: ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Jeff Garzik,
	Andrew Morton

On Friday, 29 of August 2008, Tejun Heo wrote:
> Rafael J. Wysocki wrote:
> > Actaully, they do belong to it.  This is the part "fixing" the hibernation code
> > path, in which the disk is also powered off unnecessarily.
> > 
> > Well, probably I should use SYSTEM_HIBERNATE_ENTER or something similar
> > instead of SYSTEM_SUSPEND_DISK.
> > 
> > In short, the idea is to change system_state to something specific to the last
> > phase of hibernation (after saving the image) and check that in
> > ata_scsi_start_stop_xlat().  In fact that's completely analogous to what's done
> > for SYSTEM_POWER_OFF in there.
> 
> Ah.. right, missed the added check for SUSPEND_DISK in libata-scsi.c.
> Maybe it's a good idea to note it in the commit message later?

After some more debugging and refinements I've obtained the patch below.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw@sisk.pl>

SATA: Blacklist systems that spin off disks during ACPI power off

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information.  However, only the on-board controlles should
be blacklisted and their PCI slot numbers can be used for this
purpose.  Unfortunately the existing interface for checking DMI
information of the system is not very convenient for this purpose,
because to use it, we would have to define special callback functions
or create a separate struct dmi_system_id table for each blacklisted
system.

To overcome this difficulty introduce a new function
dmi_first_match() returning a pointer to the first entry in an array
of struct dmi_system_id elements that matches the system DMI
information.  Then, we can use this pointer to access the entry's
.driver_data field containing the additional information, such as
the PCI slot number, allowing us to do the desired blacklisting.

Introduce a new libata flag ATA_FLAG_NO_POWEROFF_SPINDOWN that, if
set, will prevent disks from being spun off during system power off
and hibernation (to handle the hibernation case we need a new system
state SYSTEM_HIBERNATE_ENTER that can be checked against by libata,
in analogy with SYSTEM_POWER_OFF).  Use dmi_first_match() to set this
flag for some systems affected by the problem described above (HP nx6325,
HP nx6310, HP 2510p).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/ata/ahci.c          |   32 ++++++++++++++++++++
 drivers/ata/ata_piix.c      |   33 +++++++++++++++++++++
 drivers/ata/libata-scsi.c   |   16 ++++++++--
 drivers/ata/sata_sil.c      |   35 +++++++++++++++++++++-
 drivers/firmware/dmi_scan.c |   69 +++++++++++++++++++++++++++++++++-----------
 include/linux/dmi.h         |    1 
 include/linux/kernel.h      |    2 -
 include/linux/libata.h      |    1 
 kernel/power/disk.c         |    2 +
 9 files changed, 170 insertions(+), 21 deletions(-)

Index: linux-2.6/drivers/ata/libata-scsi.c
===================================================================
--- linux-2.6.orig/drivers/ata/libata-scsi.c
+++ linux-2.6/drivers/ata/libata-scsi.c
@@ -1181,6 +1181,14 @@ static unsigned int ata_scsi_start_stop_
 
 		tf->command = ATA_CMD_VERIFY;	/* READ VERIFY */
 	} else {
+		/* Some odd clown BIOSen issue spindown on power off (ACPI S4
+		 * or S5) causing some drives to spin up and down again.
+		 */
+		if ((qc->ap->flags & ATA_FLAG_NO_POWEROFF_SPINDOWN) &&
+		    (system_state == SYSTEM_POWER_OFF ||
+		     system_state == SYSTEM_HIBERNATE_ENTER))
+			goto skip;
+
 		/* XXX: This is for backward compatibility, will be
 		 * removed.  Read Documentation/feature-removal-schedule.txt
 		 * for more info.
@@ -1204,8 +1212,7 @@ static unsigned int ata_scsi_start_stop_
 				scmd->scsi_done = qc->scsidone;
 				qc->scsidone = ata_delayed_done;
 			}
-			scmd->result = SAM_STAT_GOOD;
-			return 1;
+			goto skip;
 		}
 
 		/* Issue ATA STANDBY IMMEDIATE command */
@@ -1221,10 +1228,13 @@ static unsigned int ata_scsi_start_stop_
 
 	return 0;
 
-invalid_fld:
+ invalid_fld:
 	ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x24, 0x0);
 	/* "Invalid field in cbd" */
 	return 1;
+ skip:
+	scmd->result = SAM_STAT_GOOD;
+	return 1;
 }
 
 
Index: linux-2.6/drivers/ata/sata_sil.c
===================================================================
--- linux-2.6.orig/drivers/ata/sata_sil.c
+++ linux-2.6/drivers/ata/sata_sil.c
@@ -603,11 +603,38 @@ static void sil_init_controller(struct a
 	}
 }
 
+static bool sil_broken_system_poweroff(struct pci_dev *pdev)
+{
+	static const struct dmi_system_id broken_systems[] = {
+		{
+			.ident = "HP Compaq nx6325",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nx6325"),
+			},
+			/* PCI slot number of the controller */
+			.driver_data = (void *)0x12UL,
+		},
+
+		{ }	/* terminate list */
+	};
+	const struct dmi_system_id *dmi = dmi_first_match(broken_systems);
+
+	if (dmi) {
+		unsigned long slot = (unsigned long)dmi->driver_data;
+		/* apply the quirk only to on-board controllers */
+		return (slot == PCI_SLOT(pdev->devfn));
+	}
+
+	return false;
+}
+
 static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version;
 	int board_id = ent->driver_data;
-	const struct ata_port_info *ppi[] = { &sil_port_info[board_id], NULL };
+	struct ata_port_info pi = sil_port_info[board_id];
+	const struct ata_port_info *ppi[] = { &pi, NULL };
 	struct ata_host *host;
 	void __iomem *mmio_base;
 	int n_ports, rc;
@@ -621,6 +648,12 @@ static int sil_init_one(struct pci_dev *
 	if (board_id == sil_3114)
 		n_ports = 4;
 
+	if (sil_broken_system_poweroff(pdev)) {
+		pi.flags |= ATA_FLAG_NO_POWEROFF_SPINDOWN;
+		dev_info(&pdev->dev,
+			   "quirky BIOS, skipping spindown on poweroff\n");
+	}
+
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
 	if (!host)
 		return -ENOMEM;
Index: linux-2.6/include/linux/libata.h
===================================================================
--- linux-2.6.orig/include/linux/libata.h
+++ linux-2.6/include/linux/libata.h
@@ -187,6 +187,7 @@ enum {
 					     * doesn't handle PIO interrupts */
 	ATA_FLAG_NCQ		= (1 << 10), /* host supports NCQ */
 	ATA_FLAG_DEBUGMSG	= (1 << 13),
+	ATA_FLAG_NO_POWEROFF_SPINDOWN = (1 << 14), /* don't spindown before poweroff */
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */
 	ATA_FLAG_ACPI_SATA	= (1 << 17), /* need native SATA ACPI layout */
Index: linux-2.6/kernel/power/disk.c
===================================================================
--- linux-2.6.orig/kernel/power/disk.c
+++ linux-2.6/kernel/power/disk.c
@@ -416,6 +416,7 @@ int hibernation_platform_enter(void)
 	if (error)
 		goto Close;
 
+	system_state = SYSTEM_HIBERNATE_ENTER;
 	suspend_console();
 	ftrace_save = __ftrace_enabled_save();
 	error = device_suspend(PMSG_HIBERNATE);
@@ -451,6 +452,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
+	system_state = SYSTEM_RUNNING;
 	device_resume(PMSG_RESTORE);
 	__ftrace_enabled_restore(ftrace_save);
 	resume_console();
Index: linux-2.6/include/linux/kernel.h
===================================================================
--- linux-2.6.orig/include/linux/kernel.h
+++ linux-2.6/include/linux/kernel.h
@@ -247,7 +247,7 @@ extern enum system_states {
 	SYSTEM_HALT,
 	SYSTEM_POWER_OFF,
 	SYSTEM_RESTART,
-	SYSTEM_SUSPEND_DISK,
+	SYSTEM_HIBERNATE_ENTER,
 } system_state;
 
 #define TAINT_PROPRIETARY_MODULE	(1<<0)
Index: linux-2.6/drivers/ata/ahci.c
===================================================================
--- linux-2.6.orig/drivers/ata/ahci.c
+++ linux-2.6/drivers/ata/ahci.c
@@ -2515,6 +2515,32 @@ static void ahci_p5wdh_workaround(struct
 	}
 }
 
+static bool ahci_broken_system_poweroff(struct pci_dev *pdev)
+{
+	static const struct dmi_system_id broken_systems[] = {
+		{
+			.ident = "HP Compaq nx6310",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nx6310"),
+			},
+			/* PCI slot number of the controller */
+			.driver_data = (void *)0x1FUL,
+		},
+
+		{ }	/* terminate list */
+	};
+	const struct dmi_system_id *dmi = dmi_first_match(broken_systems);
+
+	if (dmi) {
+		unsigned long slot = (unsigned long)dmi->driver_data;
+		/* apply the quirk only to on-board controllers */
+		return (slot == PCI_SLOT(pdev->devfn));
+	}
+
+	return false;
+}
+
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version;
@@ -2604,6 +2630,12 @@ static int ahci_init_one(struct pci_dev 
 		}
 	}
 
+	if (ahci_broken_system_poweroff(pdev)) {
+		pi.flags |= ATA_FLAG_NO_POWEROFF_SPINDOWN;
+		dev_info(&pdev->dev,
+			"quirky BIOS, skipping spindown on poweroff\n");
+	}
+
 	/* CAP.NP sometimes indicate the index of the last enabled
 	 * port, at other times, that of the last possible port, so
 	 * determining the maximum port number requires looking at
Index: linux-2.6/drivers/firmware/dmi_scan.c
===================================================================
--- linux-2.6.orig/drivers/firmware/dmi_scan.c
+++ linux-2.6/drivers/firmware/dmi_scan.c
@@ -407,6 +407,27 @@ void __init dmi_scan_machine(void)
 }
 
 /**
+ *	dmi_match - check if dmi_system_id structure matches system DMI data
+ *	@dmi: pointer to the dmi_system_id structure to check
+ */
+static bool dmi_match(const struct dmi_system_id *dmi)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dmi->matches); i++) {
+		int s = dmi->matches[i].slot;
+		if (s == DMI_NONE)
+			continue;
+		if (dmi_ident[s]
+		    && strstr(dmi_ident[s], dmi->matches[i].substr))
+			continue;
+		/* No match */
+		return false;
+	}
+	return true;
+}
+
+/**
  *	dmi_check_system - check system DMI data
  *	@list: array of dmi_system_id structures to match against
  *		All non-null elements of the list must match
@@ -421,30 +442,46 @@ void __init dmi_scan_machine(void)
  */
 int dmi_check_system(const struct dmi_system_id *list)
 {
-	int i, count = 0;
-	const struct dmi_system_id *d = list;
+	int count = 0;
+	const struct dmi_system_id *d;
 
-	while (d->ident) {
-		for (i = 0; i < ARRAY_SIZE(d->matches); i++) {
-			int s = d->matches[i].slot;
-			if (s == DMI_NONE)
-				continue;
-			if (dmi_ident[s] && strstr(dmi_ident[s], d->matches[i].substr))
-				continue;
-			/* No match */
-			goto fail;
+	for (d = list; d->ident; d++)
+		if (dmi_match(d)) {
+			count++;
+			if (d->callback && d->callback(d))
+				break;
 		}
-		count++;
-		if (d->callback && d->callback(d))
-			break;
-fail:		d++;
-	}
 
 	return count;
 }
 EXPORT_SYMBOL(dmi_check_system);
 
 /**
+ *	dmi_first_match - find dmi_system_id structure matching system DMI data
+ *	@list: array of dmi_system_id structures to match against
+ *		All non-null elements of the list must match
+ *		their slot's (field index's) data (i.e., each
+ *		list string must be a substring of the specified
+ *		DMI slot's string data) to be considered a
+ *		successful match.
+ *
+ *	Walk the blacklist table until the first match is found.  Return the
+ *	pointer to the matching entry or NULL if there's no match.
+ */
+struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list)
+{
+	int i;
+	const struct dmi_system_id *d;
+
+	for (d = list; d->ident; d++)
+		if (dmi_match(d))
+			return d;
+
+	return NULL;
+}
+EXPORT_SYMBOL(dmi_first_match);
+
+/**
  *	dmi_get_system_info - return DMI data value
  *	@field: data index (see enum dmi_field)
  *
Index: linux-2.6/include/linux/dmi.h
===================================================================
--- linux-2.6.orig/include/linux/dmi.h
+++ linux-2.6/include/linux/dmi.h
@@ -75,6 +75,7 @@ struct dmi_device {
 #ifdef CONFIG_DMI
 
 extern int dmi_check_system(const struct dmi_system_id *list);
+struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
 extern const char * dmi_get_system_info(int field);
 extern const struct dmi_device * dmi_find_device(int type, const char *name,
 	const struct dmi_device *from);
Index: linux-2.6/drivers/ata/ata_piix.c
===================================================================
--- linux-2.6.orig/drivers/ata/ata_piix.c
+++ linux-2.6/drivers/ata/ata_piix.c
@@ -1449,6 +1449,32 @@ static void piix_iocfg_bit18_quirk(struc
 	}
 }
 
+static bool piix_broken_system_poweroff(struct pci_dev *pdev)
+{
+	static const struct dmi_system_id broken_systems[] = {
+		{
+			.ident = "HP Compaq 2510p",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq 2510p"),
+			},
+			/* PCI slot number of the controller */
+			.driver_data = (void *)0x1FUL,
+		},
+
+		{ }	/* terminate list */
+	};
+	const struct dmi_system_id *dmi = dmi_first_match(broken_systems);
+
+	if (dmi) {
+		unsigned long slot = (unsigned long)dmi->driver_data;
+		/* apply the quirk only to on-board controllers */
+		return (slot == PCI_SLOT(pdev->devfn));
+	}
+
+	return false;
+}
+
 /**
  *	piix_init_one - Register PIIX ATA PCI device with kernel services
  *	@pdev: PCI device to register
@@ -1484,6 +1510,13 @@ static int __devinit piix_init_one(struc
 	if (!in_module_init)
 		return -ENODEV;
 
+	if (piix_broken_system_poweroff(pdev)) {
+		piix_port_info[ent->driver_data].flags |=
+						ATA_FLAG_NO_POWEROFF_SPINDOWN;
+		dev_info(&pdev->dev,
+			   "quirky BIOS, skipping spindown on poweroff\n");
+	}
+
 	port_info[0] = piix_port_info[ent->driver_data];
 	port_info[1] = piix_port_info[ent->driver_data];
 

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] SATA: Blacklist systems that spin off disks during ACPI power off
  2008-09-06 17:23       ` [PATCH] SATA: Blacklist systems that spin off disks during ACPI power off Rafael J. Wysocki
@ 2008-09-08 11:38         ` Tejun Heo
  2008-09-14  2:21           ` Jeff Garzik
  0 siblings, 1 reply; 49+ messages in thread
From: Tejun Heo @ 2008-09-08 11:38 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Jeff Garzik,
	Andrew Morton

Rafael J. Wysocki wrote:
> SATA: Blacklist systems that spin off disks during ACPI power off
> 
> Some notebooks from HP have the problem that their BIOSes attempt to
> spin down hard drives before entering ACPI system states S4 and S5.
> This leads to a yo-yo effect during system power-off shutdown and the
> last phase of hibernation when the disk is first spun down by the
> kernel and then almost immediately turned on and off by the BIOS.
> This, in turn, may result in shortening the disk's life times.
> 
> To prevent this from happening we can blacklist the affected systems
> using DMI information.  However, only the on-board controlles should
> be blacklisted and their PCI slot numbers can be used for this
> purpose.  Unfortunately the existing interface for checking DMI
> information of the system is not very convenient for this purpose,
> because to use it, we would have to define special callback functions
> or create a separate struct dmi_system_id table for each blacklisted
> system.
> 
> To overcome this difficulty introduce a new function
> dmi_first_match() returning a pointer to the first entry in an array
> of struct dmi_system_id elements that matches the system DMI
> information.  Then, we can use this pointer to access the entry's
> .driver_data field containing the additional information, such as
> the PCI slot number, allowing us to do the desired blacklisting.
> 
> Introduce a new libata flag ATA_FLAG_NO_POWEROFF_SPINDOWN that, if
> set, will prevent disks from being spun off during system power off
> and hibernation (to handle the hibernation case we need a new system
> state SYSTEM_HIBERNATE_ENTER that can be checked against by libata,
> in analogy with SYSTEM_POWER_OFF).  Use dmi_first_match() to set this
> flag for some systems affected by the problem described above (HP nx6325,
> HP nx6310, HP 2510p).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

libata part looks good to me but I think it would be better to
separate out DMI changes into a separate patch.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] SATA: Blacklist systems that spin off disks during ACPI power off
  2008-09-08 11:38         ` Tejun Heo
@ 2008-09-14  2:21           ` Jeff Garzik
  2008-09-15  0:00             ` Rafael J. Wysocki
  0 siblings, 1 reply; 49+ messages in thread
From: Jeff Garzik @ 2008-09-14  2:21 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Tejun Heo, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

Tejun Heo wrote:
> Rafael J. Wysocki wrote:
>> SATA: Blacklist systems that spin off disks during ACPI power off
>>
>> Some notebooks from HP have the problem that their BIOSes attempt to
>> spin down hard drives before entering ACPI system states S4 and S5.
>> This leads to a yo-yo effect during system power-off shutdown and the
>> last phase of hibernation when the disk is first spun down by the
>> kernel and then almost immediately turned on and off by the BIOS.
>> This, in turn, may result in shortening the disk's life times.
>>
>> To prevent this from happening we can blacklist the affected systems
>> using DMI information.  However, only the on-board controlles should
>> be blacklisted and their PCI slot numbers can be used for this
>> purpose.  Unfortunately the existing interface for checking DMI
>> information of the system is not very convenient for this purpose,
>> because to use it, we would have to define special callback functions
>> or create a separate struct dmi_system_id table for each blacklisted
>> system.
>>
>> To overcome this difficulty introduce a new function
>> dmi_first_match() returning a pointer to the first entry in an array
>> of struct dmi_system_id elements that matches the system DMI
>> information.  Then, we can use this pointer to access the entry's
>> .driver_data field containing the additional information, such as
>> the PCI slot number, allowing us to do the desired blacklisting.
>>
>> Introduce a new libata flag ATA_FLAG_NO_POWEROFF_SPINDOWN that, if
>> set, will prevent disks from being spun off during system power off
>> and hibernation (to handle the hibernation case we need a new system
>> state SYSTEM_HIBERNATE_ENTER that can be checked against by libata,
>> in analogy with SYSTEM_POWER_OFF).  Use dmi_first_match() to set this
>> flag for some systems affected by the problem described above (HP nx6325,
>> HP nx6310, HP 2510p).
>>
>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> libata part looks good to me but I think it would be better to
> separate out DMI changes into a separate patch.

Did these changes ever get separated out?


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] SATA: Blacklist systems that spin off disks during ACPI power off
  2008-09-14  2:21           ` Jeff Garzik
@ 2008-09-15  0:00             ` Rafael J. Wysocki
  2008-09-17 21:50               ` Jeff Garzik
  0 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-15  0:00 UTC (permalink / raw)
  To: Jeff Garzik
  Cc: Tejun Heo, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

On Sunday, 14 of September 2008, Jeff Garzik wrote:
> Tejun Heo wrote:
> > Rafael J. Wysocki wrote:
> >> SATA: Blacklist systems that spin off disks during ACPI power off
> >>
> >> Some notebooks from HP have the problem that their BIOSes attempt to
> >> spin down hard drives before entering ACPI system states S4 and S5.
> >> This leads to a yo-yo effect during system power-off shutdown and the
> >> last phase of hibernation when the disk is first spun down by the
> >> kernel and then almost immediately turned on and off by the BIOS.
> >> This, in turn, may result in shortening the disk's life times.
> >>
> >> To prevent this from happening we can blacklist the affected systems
> >> using DMI information.  However, only the on-board controlles should
> >> be blacklisted and their PCI slot numbers can be used for this
> >> purpose.  Unfortunately the existing interface for checking DMI
> >> information of the system is not very convenient for this purpose,
> >> because to use it, we would have to define special callback functions
> >> or create a separate struct dmi_system_id table for each blacklisted
> >> system.
> >>
> >> To overcome this difficulty introduce a new function
> >> dmi_first_match() returning a pointer to the first entry in an array
> >> of struct dmi_system_id elements that matches the system DMI
> >> information.  Then, we can use this pointer to access the entry's
> >> .driver_data field containing the additional information, such as
> >> the PCI slot number, allowing us to do the desired blacklisting.
> >>
> >> Introduce a new libata flag ATA_FLAG_NO_POWEROFF_SPINDOWN that, if
> >> set, will prevent disks from being spun off during system power off
> >> and hibernation (to handle the hibernation case we need a new system
> >> state SYSTEM_HIBERNATE_ENTER that can be checked against by libata,
> >> in analogy with SYSTEM_POWER_OFF).  Use dmi_first_match() to set this
> >> flag for some systems affected by the problem described above (HP nx6325,
> >> HP nx6310, HP 2510p).
> >>
> >> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > libata part looks good to me but I think it would be better to
> > separate out DMI changes into a separate patch.
> 
> Did these changes ever get separated out?

I only have the 'combo' patch if that's what you're asking about.  [The latest
version is at http://bugzilla.kernel.org/attachment.cgi?id=17702&action=view]

Still, I can easily split the patch, although in that case the reason for the
DMI changes won't be very clear without a reference to the SATA changes IMO.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH] SATA: Blacklist systems that spin off disks during ACPI power off
  2008-09-15  0:00             ` Rafael J. Wysocki
@ 2008-09-17 21:50               ` Jeff Garzik
  2008-09-29 22:06                 ` [PATCH 0/6] " Rafael J. Wysocki
  0 siblings, 1 reply; 49+ messages in thread
From: Jeff Garzik @ 2008-09-17 21:50 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Tejun Heo, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

Rafael J. Wysocki wrote:
> On Sunday, 14 of September 2008, Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Rafael J. Wysocki wrote:
>>>> SATA: Blacklist systems that spin off disks during ACPI power off
>>>>
>>>> Some notebooks from HP have the problem that their BIOSes attempt to
>>>> spin down hard drives before entering ACPI system states S4 and S5.
>>>> This leads to a yo-yo effect during system power-off shutdown and the
>>>> last phase of hibernation when the disk is first spun down by the
>>>> kernel and then almost immediately turned on and off by the BIOS.
>>>> This, in turn, may result in shortening the disk's life times.
>>>>
>>>> To prevent this from happening we can blacklist the affected systems
>>>> using DMI information.  However, only the on-board controlles should
>>>> be blacklisted and their PCI slot numbers can be used for this
>>>> purpose.  Unfortunately the existing interface for checking DMI
>>>> information of the system is not very convenient for this purpose,
>>>> because to use it, we would have to define special callback functions
>>>> or create a separate struct dmi_system_id table for each blacklisted
>>>> system.
>>>>
>>>> To overcome this difficulty introduce a new function
>>>> dmi_first_match() returning a pointer to the first entry in an array
>>>> of struct dmi_system_id elements that matches the system DMI
>>>> information.  Then, we can use this pointer to access the entry's
>>>> .driver_data field containing the additional information, such as
>>>> the PCI slot number, allowing us to do the desired blacklisting.
>>>>
>>>> Introduce a new libata flag ATA_FLAG_NO_POWEROFF_SPINDOWN that, if
>>>> set, will prevent disks from being spun off during system power off
>>>> and hibernation (to handle the hibernation case we need a new system
>>>> state SYSTEM_HIBERNATE_ENTER that can be checked against by libata,
>>>> in analogy with SYSTEM_POWER_OFF).  Use dmi_first_match() to set this
>>>> flag for some systems affected by the problem described above (HP nx6325,
>>>> HP nx6310, HP 2510p).
>>>>
>>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>>> libata part looks good to me but I think it would be better to
>>> separate out DMI changes into a separate patch.
>> Did these changes ever get separated out?
> 
> I only have the 'combo' patch if that's what you're asking about.  [The latest
> version is at http://bugzilla.kernel.org/attachment.cgi?id=17702&action=view]
> 
> Still, I can easily split the patch, although in that case the reason for the
> DMI changes won't be very clear without a reference to the SATA changes IMO.

That's the nature of every single API change -- you have the change, and 
then you have the users.

Respectfully, please split up the patch as requested, into DMI subsystem 
and ata subsystem pieces.

Re-reviewing the patch, I would even think that you should split out the 
kernel/power/disk and linux/kernel.h changes as well.

	Jeff





^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 0/6] SATA: Blacklist systems that spin off disks during ACPI power off
  2008-09-17 21:50               ` Jeff Garzik
@ 2008-09-29 22:06                 ` Rafael J. Wysocki
  2008-09-29 22:10                   ` [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation Rafael J. Wysocki
                                     ` (5 more replies)
  0 siblings, 6 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-29 22:06 UTC (permalink / raw)
  To: Jeff Garzik
  Cc: Tejun Heo, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

On Wednesday, 17 of September 2008, Jeff Garzik wrote:
> Rafael J. Wysocki wrote:
> > On Sunday, 14 of September 2008, Jeff Garzik wrote:
> >> Tejun Heo wrote:
> >>> Rafael J. Wysocki wrote:
> >>>> SATA: Blacklist systems that spin off disks during ACPI power off
> >>>>
> >>>> Some notebooks from HP have the problem that their BIOSes attempt to
> >>>> spin down hard drives before entering ACPI system states S4 and S5.
> >>>> This leads to a yo-yo effect during system power-off shutdown and the
> >>>> last phase of hibernation when the disk is first spun down by the
> >>>> kernel and then almost immediately turned on and off by the BIOS.
> >>>> This, in turn, may result in shortening the disk's life times.
> >>>>
> >>>> To prevent this from happening we can blacklist the affected systems
> >>>> using DMI information.  However, only the on-board controlles should
> >>>> be blacklisted and their PCI slot numbers can be used for this
> >>>> purpose.  Unfortunately the existing interface for checking DMI
> >>>> information of the system is not very convenient for this purpose,
> >>>> because to use it, we would have to define special callback functions
> >>>> or create a separate struct dmi_system_id table for each blacklisted
> >>>> system.
> >>>>
> >>>> To overcome this difficulty introduce a new function
> >>>> dmi_first_match() returning a pointer to the first entry in an array
> >>>> of struct dmi_system_id elements that matches the system DMI
> >>>> information.  Then, we can use this pointer to access the entry's
> >>>> .driver_data field containing the additional information, such as
> >>>> the PCI slot number, allowing us to do the desired blacklisting.
> >>>>
> >>>> Introduce a new libata flag ATA_FLAG_NO_POWEROFF_SPINDOWN that, if
> >>>> set, will prevent disks from being spun off during system power off
> >>>> and hibernation (to handle the hibernation case we need a new system
> >>>> state SYSTEM_HIBERNATE_ENTER that can be checked against by libata,
> >>>> in analogy with SYSTEM_POWER_OFF).  Use dmi_first_match() to set this
> >>>> flag for some systems affected by the problem described above (HP nx6325,
> >>>> HP nx6310, HP 2510p).
> >>>>
> >>>> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >>> libata part looks good to me but I think it would be better to
> >>> separate out DMI changes into a separate patch.
> >> Did these changes ever get separated out?
> > 
> > I only have the 'combo' patch if that's what you're asking about.  [The latest
> > version is at http://bugzilla.kernel.org/attachment.cgi?id=17702&action=view]
> > 
> > Still, I can easily split the patch, although in that case the reason for the
> > DMI changes won't be very clear without a reference to the SATA changes IMO.
> 
> That's the nature of every single API change -- you have the change, and 
> then you have the users.
> 
> Respectfully, please split up the patch as requested, into DMI subsystem 
> and ata subsystem pieces.
> 
> Re-reviewing the patch, I would even think that you should split out the 
> kernel/power/disk and linux/kernel.h changes as well.

Here you go (sorry for the delay).  I have also split the SATA changes so that
the driver patches are droppable individually if there are problems with them
(not anticipated).

Thanks,
Rafael

---
SATA: Blacklist systems that spin off disks during ACPI power off

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information, which is implemented by the following series of
patches.


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation
  2008-09-29 22:06                 ` [PATCH 0/6] " Rafael J. Wysocki
@ 2008-09-29 22:10                   ` Rafael J. Wysocki
  2008-10-01  3:32                     ` Tejun Heo
  2008-10-03  8:35                     ` Andrew Morton
  2008-09-29 22:13                   ` [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible Rafael J. Wysocki
                                     ` (4 subsequent siblings)
  5 siblings, 2 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-29 22:10 UTC (permalink / raw)
  To: Jeff Garzik
  Cc: Tejun Heo, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

From: Rafael J. Wysocki <rjw@sisk.pl>

Hibernation: Introduce new system state for the last phase of hibernation

Replace unused SYSTEM_SUSPEND_DISK with a new system_state value
SYSTEM_HIBERNATE_ENTER that can be used by device drivers to check if
the system is in the last phase of hibernation.

In particular, some SATA drivers are going to use it for blacklisting
systems in which the disks should not be spun down during the last
phase of hibernation (the BIOS will do that anyway).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/kernel.h |    2 +-
 kernel/power/disk.c    |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6/kernel/power/disk.c
===================================================================
--- linux-2.6.orig/kernel/power/disk.c
+++ linux-2.6/kernel/power/disk.c
@@ -416,6 +416,7 @@ int hibernation_platform_enter(void)
 	if (error)
 		goto Close;
 
+	system_state = SYSTEM_HIBERNATE_ENTER;
 	suspend_console();
 	ftrace_save = __ftrace_enabled_save();
 	error = device_suspend(PMSG_HIBERNATE);
@@ -451,6 +452,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
+	system_state = SYSTEM_RUNNING;
 	device_resume(PMSG_RESTORE);
 	__ftrace_enabled_restore(ftrace_save);
 	resume_console();
Index: linux-2.6/include/linux/kernel.h
===================================================================
--- linux-2.6.orig/include/linux/kernel.h
+++ linux-2.6/include/linux/kernel.h
@@ -247,7 +247,7 @@ extern enum system_states {
 	SYSTEM_HALT,
 	SYSTEM_POWER_OFF,
 	SYSTEM_RESTART,
-	SYSTEM_SUSPEND_DISK,
+	SYSTEM_HIBERNATE_ENTER,
 } system_state;
 
 #define TAINT_PROPRIETARY_MODULE	(1<<0)


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible
  2008-09-29 22:06                 ` [PATCH 0/6] " Rafael J. Wysocki
  2008-09-29 22:10                   ` [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation Rafael J. Wysocki
@ 2008-09-29 22:13                   ` Rafael J. Wysocki
  2008-10-01  3:31                     ` Tejun Heo
  2008-09-29 22:14                   ` [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off Rafael J. Wysocki
                                     ` (3 subsequent siblings)
  5 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-29 22:13 UTC (permalink / raw)
  To: Jeff Garzik
  Cc: Tejun Heo, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

From: Rafael J. Wysocki <rjw@sisk.pl>

DMI: Introduce dmi_first_match to make the interface more flexible

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information.  However, only the on-board controlles should
be blacklisted and their PCI slot numbers can be used for this
purpose.  Unfortunately the existing interface for checking DMI
information of the system is not very convenient for this purpose,
because to use it, we would have to define special callback functions
or create a separate struct dmi_system_id table for each blacklisted
system.

To overcome this difficulty introduce a new function
dmi_first_match() returning a pointer to the first entry in an array
of struct dmi_system_id elements that matches the system DMI
information.  Then, we can use this pointer to access the entry's
.driver_data field containing the additional information, such as
the PCI slot number, allowing us to do the desired blacklisting.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/firmware/dmi_scan.c |   68 +++++++++++++++++++++++++++++++++-----------
 include/linux/dmi.h         |    1 
 2 files changed, 53 insertions(+), 16 deletions(-)

Index: linux-2.6/drivers/firmware/dmi_scan.c
===================================================================
--- linux-2.6.orig/drivers/firmware/dmi_scan.c
+++ linux-2.6/drivers/firmware/dmi_scan.c
@@ -407,6 +407,27 @@ void __init dmi_scan_machine(void)
 }
 
 /**
+ *	dmi_match - check if dmi_system_id structure matches system DMI data
+ *	@dmi: pointer to the dmi_system_id structure to check
+ */
+static bool dmi_match(const struct dmi_system_id *dmi)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dmi->matches); i++) {
+		int s = dmi->matches[i].slot;
+		if (s == DMI_NONE)
+			continue;
+		if (dmi_ident[s]
+		    && strstr(dmi_ident[s], dmi->matches[i].substr))
+			continue;
+		/* No match */
+		return false;
+	}
+	return true;
+}
+
+/**
  *	dmi_check_system - check system DMI data
  *	@list: array of dmi_system_id structures to match against
  *		All non-null elements of the list must match
@@ -421,30 +442,45 @@ void __init dmi_scan_machine(void)
  */
 int dmi_check_system(const struct dmi_system_id *list)
 {
-	int i, count = 0;
-	const struct dmi_system_id *d = list;
+	int count = 0;
+	const struct dmi_system_id *d;
 
-	while (d->ident) {
-		for (i = 0; i < ARRAY_SIZE(d->matches); i++) {
-			int s = d->matches[i].slot;
-			if (s == DMI_NONE)
-				continue;
-			if (dmi_ident[s] && strstr(dmi_ident[s], d->matches[i].substr))
-				continue;
-			/* No match */
-			goto fail;
+	for (d = list; d->ident; d++)
+		if (dmi_match(d)) {
+			count++;
+			if (d->callback && d->callback(d))
+				break;
 		}
-		count++;
-		if (d->callback && d->callback(d))
-			break;
-fail:		d++;
-	}
 
 	return count;
 }
 EXPORT_SYMBOL(dmi_check_system);
 
 /**
+ *	dmi_first_match - find dmi_system_id structure matching system DMI data
+ *	@list: array of dmi_system_id structures to match against
+ *		All non-null elements of the list must match
+ *		their slot's (field index's) data (i.e., each
+ *		list string must be a substring of the specified
+ *		DMI slot's string data) to be considered a
+ *		successful match.
+ *
+ *	Walk the blacklist table until the first match is found.  Return the
+ *	pointer to the matching entry or NULL if there's no match.
+ */
+const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list)
+{
+	const struct dmi_system_id *d;
+
+	for (d = list; d->ident; d++)
+		if (dmi_match(d))
+			return d;
+
+	return NULL;
+}
+EXPORT_SYMBOL(dmi_first_match);
+
+/**
  *	dmi_get_system_info - return DMI data value
  *	@field: data index (see enum dmi_field)
  *
Index: linux-2.6/include/linux/dmi.h
===================================================================
--- linux-2.6.orig/include/linux/dmi.h
+++ linux-2.6/include/linux/dmi.h
@@ -75,6 +75,7 @@ struct dmi_device {
 #ifdef CONFIG_DMI
 
 extern int dmi_check_system(const struct dmi_system_id *list);
+const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
 extern const char * dmi_get_system_info(int field);
 extern const struct dmi_device * dmi_find_device(int type, const char *name,
 	const struct dmi_device *from);


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off
  2008-09-29 22:06                 ` [PATCH 0/6] " Rafael J. Wysocki
  2008-09-29 22:10                   ` [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation Rafael J. Wysocki
  2008-09-29 22:13                   ` [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible Rafael J. Wysocki
@ 2008-09-29 22:14                   ` Rafael J. Wysocki
  2008-10-01  3:34                     ` Tejun Heo
  2008-09-29 22:15                   ` [PATCH 4/6] SATA AHCI: Blacklist system that spins " Rafael J. Wysocki
                                     ` (2 subsequent siblings)
  5 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-29 22:14 UTC (permalink / raw)
  To: Jeff Garzik
  Cc: Tejun Heo, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

From: Rafael J. Wysocki <rjw@sisk.pl>

SATA: Blacklisting of systems that spin off disks during ACPI power off

Introduce new libata flags ATA_FLAG_NO_POWEROFF_SPINDOWN and
ATA_FLAG_NO_HIBERNATE_SPINDOWN that, if set, will prevent disks from
being spun off during system power off and hibernation, respectively
(to handle the hibernation case we need the new system state
SYSTEM_HIBERNATE_ENTER that can be checked against by libata, in
analogy with SYSTEM_POWER_OFF).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/ata/libata-scsi.c |   19 ++++++++++++++++---
 include/linux/libata.h    |    2 ++
 2 files changed, 18 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/ata/libata-scsi.c
===================================================================
--- linux-2.6.orig/drivers/ata/libata-scsi.c
+++ linux-2.6/drivers/ata/libata-scsi.c
@@ -1181,6 +1181,17 @@ static unsigned int ata_scsi_start_stop_
 
 		tf->command = ATA_CMD_VERIFY;	/* READ VERIFY */
 	} else {
+		/* Some odd clown BIOSen issue spindown on power off (ACPI S4
+		 * or S5) causing some drives to spin up and down again.
+		 */
+		if ((qc->ap->flags & ATA_FLAG_NO_POWEROFF_SPINDOWN) &&
+		    system_state == SYSTEM_POWER_OFF)
+			goto skip;
+
+		if ((qc->ap->flags & ATA_FLAG_NO_HIBERNATE_SPINDOWN) &&
+		     system_state == SYSTEM_HIBERNATE_ENTER)
+			goto skip;
+
 		/* XXX: This is for backward compatibility, will be
 		 * removed.  Read Documentation/feature-removal-schedule.txt
 		 * for more info.
@@ -1204,8 +1215,7 @@ static unsigned int ata_scsi_start_stop_
 				scmd->scsi_done = qc->scsidone;
 				qc->scsidone = ata_delayed_done;
 			}
-			scmd->result = SAM_STAT_GOOD;
-			return 1;
+			goto skip;
 		}
 
 		/* Issue ATA STANDBY IMMEDIATE command */
@@ -1221,10 +1231,13 @@ static unsigned int ata_scsi_start_stop_
 
 	return 0;
 
-invalid_fld:
+ invalid_fld:
 	ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x24, 0x0);
 	/* "Invalid field in cbd" */
 	return 1;
+ skip:
+	scmd->result = SAM_STAT_GOOD;
+	return 1;
 }
 
 
Index: linux-2.6/include/linux/libata.h
===================================================================
--- linux-2.6.orig/include/linux/libata.h
+++ linux-2.6/include/linux/libata.h
@@ -186,6 +186,8 @@ enum {
 	ATA_FLAG_PIO_POLLING	= (1 << 9), /* use polling PIO if LLD
 					     * doesn't handle PIO interrupts */
 	ATA_FLAG_NCQ		= (1 << 10), /* host supports NCQ */
+	ATA_FLAG_NO_POWEROFF_SPINDOWN = (1 << 11), /* don't spindown before poweroff */
+	ATA_FLAG_NO_HIBERNATE_SPINDOWN = (1 << 12), /* don't spindown before hibernation */
 	ATA_FLAG_DEBUGMSG	= (1 << 13),
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 4/6] SATA AHCI: Blacklist system that spins off disks during ACPI power off
  2008-09-29 22:06                 ` [PATCH 0/6] " Rafael J. Wysocki
                                     ` (2 preceding siblings ...)
  2008-09-29 22:14                   ` [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off Rafael J. Wysocki
@ 2008-09-29 22:15                   ` Rafael J. Wysocki
  2008-09-29 22:16                   ` [PATCH 5/6] SATA Sil: " Rafael J. Wysocki
  2008-09-29 22:18                   ` [PATCH 6/6] SATA PIIX: " Rafael J. Wysocki
  5 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-29 22:15 UTC (permalink / raw)
  To: Jeff Garzik
  Cc: Tejun Heo, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

From: Rafael J. Wysocki <rjw@sisk.pl>

SATA AHCI: Blacklist system that spins off disks during ACPI power off

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information.

Blacklist HP nx6310 that uses the AHCI driver.

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

Index: linux-2.6/drivers/ata/ahci.c
===================================================================
--- linux-2.6.orig/drivers/ata/ahci.c
+++ linux-2.6/drivers/ata/ahci.c
@@ -2528,6 +2528,32 @@ static void ahci_p5wdh_workaround(struct
 	}
 }
 
+static bool ahci_broken_system_poweroff(struct pci_dev *pdev)
+{
+	static const struct dmi_system_id broken_systems[] = {
+		{
+			.ident = "HP Compaq nx6310",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nx6310"),
+			},
+			/* PCI slot number of the controller */
+			.driver_data = (void *)0x1FUL,
+		},
+
+		{ }	/* terminate list */
+	};
+	const struct dmi_system_id *dmi = dmi_first_match(broken_systems);
+
+	if (dmi) {
+		unsigned long slot = (unsigned long)dmi->driver_data;
+		/* apply the quirk only to on-board controllers */
+		return slot == PCI_SLOT(pdev->devfn);
+	}
+
+	return false;
+}
+
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version;
@@ -2623,6 +2649,12 @@ static int ahci_init_one(struct pci_dev 
 		}
 	}
 
+	if (ahci_broken_system_poweroff(pdev)) {
+		pi.flags |= ATA_FLAG_NO_POWEROFF_SPINDOWN;
+		dev_info(&pdev->dev,
+			"quirky BIOS, skipping spindown on poweroff\n");
+	}
+
 	/* CAP.NP sometimes indicate the index of the last enabled
 	 * port, at other times, that of the last possible port, so
 	 * determining the maximum port number requires looking at


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 5/6] SATA Sil: Blacklist system that spins off disks during ACPI power off
  2008-09-29 22:06                 ` [PATCH 0/6] " Rafael J. Wysocki
                                     ` (3 preceding siblings ...)
  2008-09-29 22:15                   ` [PATCH 4/6] SATA AHCI: Blacklist system that spins " Rafael J. Wysocki
@ 2008-09-29 22:16                   ` Rafael J. Wysocki
  2008-09-29 22:18                   ` [PATCH 6/6] SATA PIIX: " Rafael J. Wysocki
  5 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-29 22:16 UTC (permalink / raw)
  To: Jeff Garzik
  Cc: Tejun Heo, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

From: Rafael J. Wysocki <rjw@sisk.pl>

SATA Sil: Blacklist system that spins off disks during ACPI power off

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information.

Blacklist HP nx6325 that uses the sata_sil driver.

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

Index: linux-2.6/drivers/ata/sata_sil.c
===================================================================
--- linux-2.6.orig/drivers/ata/sata_sil.c
+++ linux-2.6/drivers/ata/sata_sil.c
@@ -603,11 +603,38 @@ static void sil_init_controller(struct a
 	}
 }
 
+static bool sil_broken_system_poweroff(struct pci_dev *pdev)
+{
+	static const struct dmi_system_id broken_systems[] = {
+		{
+			.ident = "HP Compaq nx6325",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nx6325"),
+			},
+			/* PCI slot number of the controller */
+			.driver_data = (void *)0x12UL,
+		},
+
+		{ }	/* terminate list */
+	};
+	const struct dmi_system_id *dmi = dmi_first_match(broken_systems);
+
+	if (dmi) {
+		unsigned long slot = (unsigned long)dmi->driver_data;
+		/* apply the quirk only to on-board controllers */
+		return slot == PCI_SLOT(pdev->devfn);
+	}
+
+	return false;
+}
+
 static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version;
 	int board_id = ent->driver_data;
-	const struct ata_port_info *ppi[] = { &sil_port_info[board_id], NULL };
+	struct ata_port_info pi = sil_port_info[board_id];
+	const struct ata_port_info *ppi[] = { &pi, NULL };
 	struct ata_host *host;
 	void __iomem *mmio_base;
 	int n_ports, rc;
@@ -621,6 +648,13 @@ static int sil_init_one(struct pci_dev *
 	if (board_id == sil_3114)
 		n_ports = 4;
 
+	if (sil_broken_system_poweroff(pdev)) {
+		pi.flags |= ATA_FLAG_NO_POWEROFF_SPINDOWN |
+					ATA_FLAG_NO_HIBERNATE_SPINDOWN;
+		dev_info(&pdev->dev, "quirky BIOS, skipping spindown "
+				"on poweroff and hibernation\n");
+	}
+
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
 	if (!host)
 		return -ENOMEM;


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 6/6] SATA PIIX: Blacklist system that spins off disks during ACPI power off
  2008-09-29 22:06                 ` [PATCH 0/6] " Rafael J. Wysocki
                                     ` (4 preceding siblings ...)
  2008-09-29 22:16                   ` [PATCH 5/6] SATA Sil: " Rafael J. Wysocki
@ 2008-09-29 22:18                   ` Rafael J. Wysocki
  5 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-09-29 22:18 UTC (permalink / raw)
  To: Jeff Garzik
  Cc: Tejun Heo, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

From: Rafael J. Wysocki <rjw@sisk.pl>

SATA PIIX: Blacklist system that spins off disks during ACPI power off

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information.

Blacklist HP 2510p that uses the ata_piix driver.

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

Index: linux-2.6/drivers/ata/ata_piix.c
===================================================================
--- linux-2.6.orig/drivers/ata/ata_piix.c
+++ linux-2.6/drivers/ata/ata_piix.c
@@ -1449,6 +1449,32 @@ static void piix_iocfg_bit18_quirk(struc
 	}
 }
 
+static bool piix_broken_system_poweroff(struct pci_dev *pdev)
+{
+	static const struct dmi_system_id broken_systems[] = {
+		{
+			.ident = "HP Compaq 2510p",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq 2510p"),
+			},
+			/* PCI slot number of the controller */
+			.driver_data = (void *)0x1FUL,
+		},
+
+		{ }	/* terminate list */
+	};
+	const struct dmi_system_id *dmi = dmi_first_match(broken_systems);
+
+	if (dmi) {
+		unsigned long slot = (unsigned long)dmi->driver_data;
+		/* apply the quirk only to on-board controllers */
+		return slot == PCI_SLOT(pdev->devfn);
+	}
+
+	return false;
+}
+
 /**
  *	piix_init_one - Register PIIX ATA PCI device with kernel services
  *	@pdev: PCI device to register
@@ -1484,6 +1510,14 @@ static int __devinit piix_init_one(struc
 	if (!in_module_init)
 		return -ENODEV;
 
+	if (piix_broken_system_poweroff(pdev)) {
+		piix_port_info[ent->driver_data].flags |=
+				ATA_FLAG_NO_POWEROFF_SPINDOWN |
+					ATA_FLAG_NO_HIBERNATE_SPINDOWN;
+		dev_info(&pdev->dev, "quirky BIOS, skipping spindown "
+				"on poweroff and hibernation\n");
+	}
+
 	port_info[0] = piix_port_info[ent->driver_data];
 	port_info[1] = piix_port_info[ent->driver_data];
 


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible
  2008-09-29 22:13                   ` [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible Rafael J. Wysocki
@ 2008-10-01  3:31                     ` Tejun Heo
  0 siblings, 0 replies; 49+ messages in thread
From: Tejun Heo @ 2008-10-01  3:31 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Jeff Garzik, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton,
	Len Brown, Jean Delvare, ralf

Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> DMI: Introduce dmi_first_match to make the interface more flexible
> 
> Some notebooks from HP have the problem that their BIOSes attempt to
> spin down hard drives before entering ACPI system states S4 and S5.
> This leads to a yo-yo effect during system power-off shutdown and the
> last phase of hibernation when the disk is first spun down by the
> kernel and then almost immediately turned on and off by the BIOS.
> This, in turn, may result in shortening the disk's life times.
> 
> To prevent this from happening we can blacklist the affected systems
> using DMI information.  However, only the on-board controlles should
> be blacklisted and their PCI slot numbers can be used for this
> purpose.  Unfortunately the existing interface for checking DMI
> information of the system is not very convenient for this purpose,
> because to use it, we would have to define special callback functions
> or create a separate struct dmi_system_id table for each blacklisted
> system.
> 
> To overcome this difficulty introduce a new function
> dmi_first_match() returning a pointer to the first entry in an array
> of struct dmi_system_id elements that matches the system DMI
> information.  Then, we can use this pointer to access the entry's
> .driver_data field containing the additional information, such as
> the PCI slot number, allowing us to do the desired blacklisting.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Acked-by: Tejun Heo <tj@kernel.org>

(cc'ing recent committers to dmi)

If no one objects, I'd like to push this through libata-dev#upstream.
Any objections?

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation
  2008-09-29 22:10                   ` [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation Rafael J. Wysocki
@ 2008-10-01  3:32                     ` Tejun Heo
  2008-10-01 11:36                       ` Rafael J. Wysocki
  2008-10-03  8:35                     ` Andrew Morton
  1 sibling, 1 reply; 49+ messages in thread
From: Tejun Heo @ 2008-10-01  3:32 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Jeff Garzik, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Hibernation: Introduce new system state for the last phase of hibernation
> 
> Replace unused SYSTEM_SUSPEND_DISK with a new system_state value
> SYSTEM_HIBERNATE_ENTER that can be used by device drivers to check if
> the system is in the last phase of hibernation.
> 
> In particular, some SATA drivers are going to use it for blacklisting
> systems in which the disks should not be spun down during the last
> phase of hibernation (the BIOS will do that anyway).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Rafael, may I push this through libata-dev?

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off
  2008-09-29 22:14                   ` [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off Rafael J. Wysocki
@ 2008-10-01  3:34                     ` Tejun Heo
  2008-10-01 11:36                       ` Rafael J. Wysocki
  0 siblings, 1 reply; 49+ messages in thread
From: Tejun Heo @ 2008-10-01  3:34 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Jeff Garzik, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> SATA: Blacklisting of systems that spin off disks during ACPI power off
> 
> Introduce new libata flags ATA_FLAG_NO_POWEROFF_SPINDOWN and
> ATA_FLAG_NO_HIBERNATE_SPINDOWN that, if set, will prevent disks from
> being spun off during system power off and hibernation, respectively
> (to handle the hibernation case we need the new system state
> SYSTEM_HIBERNATE_ENTER that can be checked against by libata, in
> analogy with SYSTEM_POWER_OFF).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

ACK 3-6.  I'll push these through libata-dev after the first two
prerequsite patches are cleared.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation
  2008-10-01  3:32                     ` Tejun Heo
@ 2008-10-01 11:36                       ` Rafael J. Wysocki
  0 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-01 11:36 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Jeff Garzik, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

On Wednesday, 1 of October 2008, Tejun Heo wrote:
> Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Hibernation: Introduce new system state for the last phase of hibernation
> > 
> > Replace unused SYSTEM_SUSPEND_DISK with a new system_state value
> > SYSTEM_HIBERNATE_ENTER that can be used by device drivers to check if
> > the system is in the last phase of hibernation.
> > 
> > In particular, some SATA drivers are going to use it for blacklisting
> > systems in which the disks should not be spun down during the last
> > phase of hibernation (the BIOS will do that anyway).
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Rafael, may I push this through libata-dev?

Sure, please go ahead.

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off
  2008-10-01  3:34                     ` Tejun Heo
@ 2008-10-01 11:36                       ` Rafael J. Wysocki
  0 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-01 11:36 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Jeff Garzik, ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki, Andrew Morton

On Wednesday, 1 of October 2008, Tejun Heo wrote:
> Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > SATA: Blacklisting of systems that spin off disks during ACPI power off
> > 
> > Introduce new libata flags ATA_FLAG_NO_POWEROFF_SPINDOWN and
> > ATA_FLAG_NO_HIBERNATE_SPINDOWN that, if set, will prevent disks from
> > being spun off during system power off and hibernation, respectively
> > (to handle the hibernation case we need the new system state
> > SYSTEM_HIBERNATE_ENTER that can be checked against by libata, in
> > analogy with SYSTEM_POWER_OFF).
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> ACK 3-6.  I'll push these through libata-dev after the first two
> prerequsite patches are cleared.

OK, thanks.

Best,
Rafael

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation
  2008-09-29 22:10                   ` [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation Rafael J. Wysocki
  2008-10-01  3:32                     ` Tejun Heo
@ 2008-10-03  8:35                     ` Andrew Morton
  2008-10-03 11:27                       ` Rafael J. Wysocki
  1 sibling, 1 reply; 49+ messages in thread
From: Andrew Morton @ 2008-10-03  8:35 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

On Tue, 30 Sep 2008 00:10:37 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> Hibernation: Introduce new system state for the last phase of hibernation
> 
> Replace unused SYSTEM_SUSPEND_DISK with a new system_state value
> SYSTEM_HIBERNATE_ENTER that can be used by device drivers to check if
> the system is in the last phase of hibernation.

Violent objections.

We just don't know what this change will do.  It potentially affects
every code site in the kernel which looks at system_state.  We've had
problems in the past with this thing and the more complex we make it,
the worse any future problems will be.  It's Just Bad.

Can we just create a new global?

	extern bool system_entering_hibernation_or_whatever;

?

It'll add four bytes of bss, it'll save *more* than four bytes of
text and it is plainly obviously non-injurious to present and future
code.

hmm?

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation
  2008-10-03  8:35                     ` Andrew Morton
@ 2008-10-03 11:27                       ` Rafael J. Wysocki
  2008-10-03 12:43                         ` Andrew Morton
  2008-10-03 12:49                         ` [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation Arjan van de Ven
  0 siblings, 2 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-03 11:27 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

On Friday, 3 of October 2008, Andrew Morton wrote:
> On Tue, 30 Sep 2008 00:10:37 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > Hibernation: Introduce new system state for the last phase of hibernation
> > 
> > Replace unused SYSTEM_SUSPEND_DISK with a new system_state value
> > SYSTEM_HIBERNATE_ENTER that can be used by device drivers to check if
> > the system is in the last phase of hibernation.
> 
> Violent objections.
> 
> We just don't know what this change will do.  It potentially affects
> every code site in the kernel which looks at system_state.

Do you mean anyone checking 'system_state != SOMETHING' ?  Oh well.

> We've had problems in the past with this thing and the more complex we make it,
> the worse any future problems will be.  It's Just Bad.
> 
> Can we just create a new global?
> 
> 	extern bool system_entering_hibernation_or_whatever;

Yes, we can, but what about:

extern bool system_entering_hibernation(void);

that will become a static inline in case of !CONFIG_HIBERNATION,
and using a static variable?

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation
  2008-10-03 11:27                       ` Rafael J. Wysocki
@ 2008-10-03 12:43                         ` Andrew Morton
  2008-10-03 15:03                           ` Rafael J. Wysocki
  2008-10-03 12:49                         ` [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation Arjan van de Ven
  1 sibling, 1 reply; 49+ messages in thread
From: Andrew Morton @ 2008-10-03 12:43 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

On Fri, 3 Oct 2008 13:27:18 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> > Can we just create a new global?
> > 
> > 	extern bool system_entering_hibernation_or_whatever;
> 
> Yes, we can, but what about:
> 
> extern bool system_entering_hibernation(void);
> 
> that will become a static inline in case of !CONFIG_HIBERNATION,
> and using a static variable?

Sure, even better.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation
  2008-10-03 11:27                       ` Rafael J. Wysocki
  2008-10-03 12:43                         ` Andrew Morton
@ 2008-10-03 12:49                         ` Arjan van de Ven
  2008-10-03 15:05                           ` Rafael J. Wysocki
  1 sibling, 1 reply; 49+ messages in thread
From: Arjan van de Ven @ 2008-10-03 12:49 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Andrew Morton, Jeff Garzik, Tejun Heo, ACPI Devel Maling List,
	linux-ide, Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

On Fri, 3 Oct 2008 13:27:18 +0200
"Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> On Friday, 3 of October 2008, Andrew Morton wrote:
> > On Tue, 30 Sep 2008 00:10:37 +0200 "Rafael J. Wysocki"
> > <rjw@sisk.pl> wrote:
> > 
> > > Hibernation: Introduce new system state for the last phase of
> > > hibernation
> > > 
> > > Replace unused SYSTEM_SUSPEND_DISK with a new system_state value
> > > SYSTEM_HIBERNATE_ENTER that can be used by device drivers to
> > > check if the system is in the last phase of hibernation.
> > 
> > Violent objections.
> > 
> > We just don't know what this change will do.  It potentially affects
> > every code site in the kernel which looks at system_state.
> 
> Do you mean anyone checking 'system_state != SOMETHING' ?  Oh well.

I think Andrew also means things looking for system_state > something
like

arch/powerpc/platforms/cell/smp.c:      if (system_state < SYSTEM_RUNNING &&
arch/powerpc/kernel/smp.c:      if (system_state < SYSTEM_RUNNING)
arch/powerpc/kernel/smp.c:      if (system_state > SYSTEM_BOOTING)
drivers/xen/xenbus/xenbus_probe.c:      if (system_state > SYSTEM_RUNNING) {


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation
  2008-10-03 12:43                         ` Andrew Morton
@ 2008-10-03 15:03                           ` Rafael J. Wysocki
  2008-10-03 21:48                             ` [PATCH 0/6] SATA: Blacklist systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
  0 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-03 15:03 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

On Friday, 3 of October 2008, Andrew Morton wrote:
> On Fri, 3 Oct 2008 13:27:18 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > > Can we just create a new global?
> > > 
> > > 	extern bool system_entering_hibernation_or_whatever;
> > 
> > Yes, we can, but what about:
> > 
> > extern bool system_entering_hibernation(void);
> > 
> > that will become a static inline in case of !CONFIG_HIBERNATION,
> > and using a static variable?
> 
> Sure, even better.

OK, I'll respin the entire series, then.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation
  2008-10-03 12:49                         ` [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation Arjan van de Ven
@ 2008-10-03 15:05                           ` Rafael J. Wysocki
  0 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-03 15:05 UTC (permalink / raw)
  To: Arjan van de Ven
  Cc: Andrew Morton, Jeff Garzik, Tejun Heo, ACPI Devel Maling List,
	linux-ide, Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

On Friday, 3 of October 2008, Arjan van de Ven wrote:
> On Fri, 3 Oct 2008 13:27:18 +0200
> "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > On Friday, 3 of October 2008, Andrew Morton wrote:
> > > On Tue, 30 Sep 2008 00:10:37 +0200 "Rafael J. Wysocki"
> > > <rjw@sisk.pl> wrote:
> > > 
> > > > Hibernation: Introduce new system state for the last phase of
> > > > hibernation
> > > > 
> > > > Replace unused SYSTEM_SUSPEND_DISK with a new system_state value
> > > > SYSTEM_HIBERNATE_ENTER that can be used by device drivers to
> > > > check if the system is in the last phase of hibernation.
> > > 
> > > Violent objections.
> > > 
> > > We just don't know what this change will do.  It potentially affects
> > > every code site in the kernel which looks at system_state.
> > 
> > Do you mean anyone checking 'system_state != SOMETHING' ?  Oh well.
> 
> I think Andrew also means things looking for system_state > something
> like
> 
> arch/powerpc/platforms/cell/smp.c:      if (system_state < SYSTEM_RUNNING &&
> arch/powerpc/kernel/smp.c:      if (system_state < SYSTEM_RUNNING)
> arch/powerpc/kernel/smp.c:      if (system_state > SYSTEM_BOOTING)
> drivers/xen/xenbus/xenbus_probe.c:      if (system_state > SYSTEM_RUNNING) {

These particular ones shouldn't be affected AFAICS.

Anyway, I'm going to respin the patchset to take the Andrew's comment into
account.

^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 0/6] SATA: Blacklist systems that spin off disks during ACPI power off (rev. 2)
  2008-10-03 15:03                           ` Rafael J. Wysocki
@ 2008-10-03 21:48                             ` Rafael J. Wysocki
  2008-10-03 21:51                               ` [PATCH 1/6] Hibernation: Introduce system_entering_hibernation Rafael J. Wysocki
                                                 ` (5 more replies)
  0 siblings, 6 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-03 21:48 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

On Friday, 3 of October 2008, Rafael J. Wysocki wrote:
> On Friday, 3 of October 2008, Andrew Morton wrote:
> > On Fri, 3 Oct 2008 13:27:18 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > > > Can we just create a new global?
> > > > 
> > > > 	extern bool system_entering_hibernation_or_whatever;
> > > 
> > > Yes, we can, but what about:
> > > 
> > > extern bool system_entering_hibernation(void);
> > > 
> > > that will become a static inline in case of !CONFIG_HIBERNATION,
> > > and using a static variable?
> > 
> > Sure, even better.
> 
> OK, I'll respin the entire series, then.

As promised:

---
SATA: Blacklist systems that spin off disks during ACPI power off (rev. 2)

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information, which is implemented by the following series of
patches.


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 1/6] Hibernation: Introduce system_entering_hibernation
  2008-10-03 21:48                             ` [PATCH 0/6] SATA: Blacklist systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
@ 2008-10-03 21:51                               ` Rafael J. Wysocki
  2008-10-03 23:27                                 ` Alan Cox
  2008-10-04  7:02                                 ` Tejun Heo
  2008-10-03 21:52                               ` [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible Rafael J. Wysocki
                                                 ` (4 subsequent siblings)
  5 siblings, 2 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-03 21:51 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

From: Rafael J. Wysocki <rjw@sisk.pl>

Hibernation: Introduce system_entering_hibernation

Introduce boolean function system_entering_hibernation() returning
'true' during the last phase of hibernation, in which devices are
being put into low power states and the sleep state (for example,
ACPI S4) is finally entered.

Some device drivers need such a function to check if the system is
in the final phase of hibernation.  In particular, some SATA drivers
are going to use it for blacklisting systems in which the disks
should not be spun down during the last phase of hibernation (the
BIOS will do that anyway).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 include/linux/suspend.h |    2 ++
 kernel/power/disk.c     |    9 +++++++++
 2 files changed, 11 insertions(+)

Index: linux-2.6/include/linux/suspend.h
===================================================================
--- linux-2.6.orig/include/linux/suspend.h
+++ linux-2.6/include/linux/suspend.h
@@ -232,6 +232,7 @@ extern unsigned long get_safe_page(gfp_t
 
 extern void hibernation_set_ops(struct platform_hibernation_ops *ops);
 extern int hibernate(void);
+extern bool system_entering_hibernation(void);
 #else /* CONFIG_HIBERNATION */
 static inline int swsusp_page_is_forbidden(struct page *p) { return 0; }
 static inline void swsusp_set_page_free(struct page *p) {}
@@ -239,6 +240,7 @@ static inline void swsusp_unset_page_fre
 
 static inline void hibernation_set_ops(struct platform_hibernation_ops *ops) {}
 static inline int hibernate(void) { return -ENOSYS; }
+static inline bool system_entering_hibernation(void) { return false; }
 #endif /* CONFIG_HIBERNATION */
 
 #ifdef CONFIG_PM_SLEEP
Index: linux-2.6/kernel/power/disk.c
===================================================================
--- linux-2.6.orig/kernel/power/disk.c
+++ linux-2.6/kernel/power/disk.c
@@ -72,6 +72,13 @@ void hibernation_set_ops(struct platform
 	mutex_unlock(&pm_mutex);
 }
 
+static bool entering_platform_hibernation;
+
+bool system_entering_hibernation(void)
+{
+	return entering_platform_hibernation;
+}
+
 #ifdef CONFIG_PM_DEBUG
 static void hibernation_debug_sleep(void)
 {
@@ -416,6 +423,7 @@ int hibernation_platform_enter(void)
 	if (error)
 		goto Close;
 
+	entering_platform_hibernation = true;
 	suspend_console();
 	ftrace_save = __ftrace_enabled_save();
 	error = device_suspend(PMSG_HIBERNATE);
@@ -451,6 +459,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
+	entering_platform_hibernation = false;
 	device_resume(PMSG_RESTORE);
 	__ftrace_enabled_restore(ftrace_save);
 	resume_console();


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible
  2008-10-03 21:48                             ` [PATCH 0/6] SATA: Blacklist systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
  2008-10-03 21:51                               ` [PATCH 1/6] Hibernation: Introduce system_entering_hibernation Rafael J. Wysocki
@ 2008-10-03 21:52                               ` Rafael J. Wysocki
  2008-10-03 21:57                               ` [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
                                                 ` (3 subsequent siblings)
  5 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-03 21:52 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

From: Rafael J. Wysocki <rjw@sisk.pl>

DMI: Introduce dmi_first_match to make the interface more flexible

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information.  However, only the on-board controlles should
be blacklisted and their PCI slot numbers can be used for this
purpose.  Unfortunately the existing interface for checking DMI
information of the system is not very convenient for this purpose,
because to use it, we would have to define special callback functions
or create a separate struct dmi_system_id table for each blacklisted
system.

To overcome this difficulty introduce a new function
dmi_first_match() returning a pointer to the first entry in an array
of struct dmi_system_id elements that matches the system DMI
information.  Then, we can use this pointer to access the entry's
.driver_data field containing the additional information, such as
the PCI slot number, allowing us to do the desired blacklisting.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/firmware/dmi_scan.c |   68 +++++++++++++++++++++++++++++++++-----------
 include/linux/dmi.h         |    1 
 2 files changed, 53 insertions(+), 16 deletions(-)

Index: linux-2.6/drivers/firmware/dmi_scan.c
===================================================================
--- linux-2.6.orig/drivers/firmware/dmi_scan.c
+++ linux-2.6/drivers/firmware/dmi_scan.c
@@ -407,6 +407,27 @@ void __init dmi_scan_machine(void)
 }
 
 /**
+ *	dmi_match - check if dmi_system_id structure matches system DMI data
+ *	@dmi: pointer to the dmi_system_id structure to check
+ */
+static bool dmi_match(const struct dmi_system_id *dmi)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dmi->matches); i++) {
+		int s = dmi->matches[i].slot;
+		if (s == DMI_NONE)
+			continue;
+		if (dmi_ident[s]
+		    && strstr(dmi_ident[s], dmi->matches[i].substr))
+			continue;
+		/* No match */
+		return false;
+	}
+	return true;
+}
+
+/**
  *	dmi_check_system - check system DMI data
  *	@list: array of dmi_system_id structures to match against
  *		All non-null elements of the list must match
@@ -421,30 +442,45 @@ void __init dmi_scan_machine(void)
  */
 int dmi_check_system(const struct dmi_system_id *list)
 {
-	int i, count = 0;
-	const struct dmi_system_id *d = list;
+	int count = 0;
+	const struct dmi_system_id *d;
 
-	while (d->ident) {
-		for (i = 0; i < ARRAY_SIZE(d->matches); i++) {
-			int s = d->matches[i].slot;
-			if (s == DMI_NONE)
-				continue;
-			if (dmi_ident[s] && strstr(dmi_ident[s], d->matches[i].substr))
-				continue;
-			/* No match */
-			goto fail;
+	for (d = list; d->ident; d++)
+		if (dmi_match(d)) {
+			count++;
+			if (d->callback && d->callback(d))
+				break;
 		}
-		count++;
-		if (d->callback && d->callback(d))
-			break;
-fail:		d++;
-	}
 
 	return count;
 }
 EXPORT_SYMBOL(dmi_check_system);
 
 /**
+ *	dmi_first_match - find dmi_system_id structure matching system DMI data
+ *	@list: array of dmi_system_id structures to match against
+ *		All non-null elements of the list must match
+ *		their slot's (field index's) data (i.e., each
+ *		list string must be a substring of the specified
+ *		DMI slot's string data) to be considered a
+ *		successful match.
+ *
+ *	Walk the blacklist table until the first match is found.  Return the
+ *	pointer to the matching entry or NULL if there's no match.
+ */
+const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list)
+{
+	const struct dmi_system_id *d;
+
+	for (d = list; d->ident; d++)
+		if (dmi_match(d))
+			return d;
+
+	return NULL;
+}
+EXPORT_SYMBOL(dmi_first_match);
+
+/**
  *	dmi_get_system_info - return DMI data value
  *	@field: data index (see enum dmi_field)
  *
Index: linux-2.6/include/linux/dmi.h
===================================================================
--- linux-2.6.orig/include/linux/dmi.h
+++ linux-2.6/include/linux/dmi.h
@@ -75,6 +75,7 @@ struct dmi_device {
 #ifdef CONFIG_DMI
 
 extern int dmi_check_system(const struct dmi_system_id *list);
+const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
 extern const char * dmi_get_system_info(int field);
 extern const struct dmi_device * dmi_find_device(int type, const char *name,
 	const struct dmi_device *from);


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off (rev. 2)
  2008-10-03 21:48                             ` [PATCH 0/6] SATA: Blacklist systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
  2008-10-03 21:51                               ` [PATCH 1/6] Hibernation: Introduce system_entering_hibernation Rafael J. Wysocki
  2008-10-03 21:52                               ` [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible Rafael J. Wysocki
@ 2008-10-03 21:57                               ` Rafael J. Wysocki
  2008-10-03 21:57                               ` [PATCH 4/6] SATA AHCI: Blacklist system that spins off disks during ACPI power off Rafael J. Wysocki
                                                 ` (2 subsequent siblings)
  5 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-03 21:57 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

From: Rafael J. Wysocki <rjw@sisk.pl>

SATA: Blacklisting of systems that spin off disks during ACPI power off (rev. 2)

Introduce new libata flags ATA_FLAG_NO_POWEROFF_SPINDOWN and
ATA_FLAG_NO_HIBERNATE_SPINDOWN that, if set, will prevent disks from
being spun off during system power off and hibernation, respectively
(to handle the hibernation case we need the new system state
SYSTEM_HIBERNATE_ENTER that can be checked against by libata, in
analogy with SYSTEM_POWER_OFF).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-scsi.c |   20 +++++++++++++++++---
 include/linux/libata.h    |    2 ++
 2 files changed, 19 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/ata/libata-scsi.c
===================================================================
--- linux-2.6.orig/drivers/ata/libata-scsi.c
+++ linux-2.6/drivers/ata/libata-scsi.c
@@ -46,6 +46,7 @@
 #include <linux/libata.h>
 #include <linux/hdreg.h>
 #include <linux/uaccess.h>
+#include <linux/suspend.h>
 
 #include "libata.h"
 
@@ -1181,6 +1182,17 @@ static unsigned int ata_scsi_start_stop_
 
 		tf->command = ATA_CMD_VERIFY;	/* READ VERIFY */
 	} else {
+		/* Some odd clown BIOSen issue spindown on power off (ACPI S4
+		 * or S5) causing some drives to spin up and down again.
+		 */
+		if ((qc->ap->flags & ATA_FLAG_NO_POWEROFF_SPINDOWN) &&
+		    system_state == SYSTEM_POWER_OFF)
+			goto skip;
+
+		if ((qc->ap->flags & ATA_FLAG_NO_HIBERNATE_SPINDOWN) &&
+		     system_entering_hibernation())
+			goto skip;
+
 		/* XXX: This is for backward compatibility, will be
 		 * removed.  Read Documentation/feature-removal-schedule.txt
 		 * for more info.
@@ -1204,8 +1216,7 @@ static unsigned int ata_scsi_start_stop_
 				scmd->scsi_done = qc->scsidone;
 				qc->scsidone = ata_delayed_done;
 			}
-			scmd->result = SAM_STAT_GOOD;
-			return 1;
+			goto skip;
 		}
 
 		/* Issue ATA STANDBY IMMEDIATE command */
@@ -1221,10 +1232,13 @@ static unsigned int ata_scsi_start_stop_
 
 	return 0;
 
-invalid_fld:
+ invalid_fld:
 	ata_scsi_set_sense(scmd, ILLEGAL_REQUEST, 0x24, 0x0);
 	/* "Invalid field in cbd" */
 	return 1;
+ skip:
+	scmd->result = SAM_STAT_GOOD;
+	return 1;
 }
 
 
Index: linux-2.6/include/linux/libata.h
===================================================================
--- linux-2.6.orig/include/linux/libata.h
+++ linux-2.6/include/linux/libata.h
@@ -186,6 +186,8 @@ enum {
 	ATA_FLAG_PIO_POLLING	= (1 << 9), /* use polling PIO if LLD
 					     * doesn't handle PIO interrupts */
 	ATA_FLAG_NCQ		= (1 << 10), /* host supports NCQ */
+	ATA_FLAG_NO_POWEROFF_SPINDOWN = (1 << 11), /* don't spindown before poweroff */
+	ATA_FLAG_NO_HIBERNATE_SPINDOWN = (1 << 12), /* don't spindown before hibernation */
 	ATA_FLAG_DEBUGMSG	= (1 << 13),
 	ATA_FLAG_IGN_SIMPLEX	= (1 << 15), /* ignore SIMPLEX */
 	ATA_FLAG_NO_IORDY	= (1 << 16), /* controller lacks iordy */


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 4/6] SATA AHCI: Blacklist system that spins off disks during ACPI power off
  2008-10-03 21:48                             ` [PATCH 0/6] SATA: Blacklist systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
                                                 ` (2 preceding siblings ...)
  2008-10-03 21:57                               ` [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
@ 2008-10-03 21:57                               ` Rafael J. Wysocki
  2008-10-03 21:58                               ` [PATCH 5/6] SATA Sil: " Rafael J. Wysocki
  2008-10-03 21:58                               ` [PATCH 6/6] SATA PIIX: " Rafael J. Wysocki
  5 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-03 21:57 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

From: Rafael J. Wysocki <rjw@sisk.pl>

SATA AHCI: Blacklist system that spins off disks during ACPI power off

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information.

Blacklist HP nx6310 that uses the AHCI driver.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/ahci.c |   32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

Index: linux-2.6/drivers/ata/ahci.c
===================================================================
--- linux-2.6.orig/drivers/ata/ahci.c
+++ linux-2.6/drivers/ata/ahci.c
@@ -2528,6 +2528,32 @@ static void ahci_p5wdh_workaround(struct
 	}
 }
 
+static bool ahci_broken_system_poweroff(struct pci_dev *pdev)
+{
+	static const struct dmi_system_id broken_systems[] = {
+		{
+			.ident = "HP Compaq nx6310",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nx6310"),
+			},
+			/* PCI slot number of the controller */
+			.driver_data = (void *)0x1FUL,
+		},
+
+		{ }	/* terminate list */
+	};
+	const struct dmi_system_id *dmi = dmi_first_match(broken_systems);
+
+	if (dmi) {
+		unsigned long slot = (unsigned long)dmi->driver_data;
+		/* apply the quirk only to on-board controllers */
+		return slot == PCI_SLOT(pdev->devfn);
+	}
+
+	return false;
+}
+
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version;
@@ -2623,6 +2649,12 @@ static int ahci_init_one(struct pci_dev 
 		}
 	}
 
+	if (ahci_broken_system_poweroff(pdev)) {
+		pi.flags |= ATA_FLAG_NO_POWEROFF_SPINDOWN;
+		dev_info(&pdev->dev,
+			"quirky BIOS, skipping spindown on poweroff\n");
+	}
+
 	/* CAP.NP sometimes indicate the index of the last enabled
 	 * port, at other times, that of the last possible port, so
 	 * determining the maximum port number requires looking at


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 5/6] SATA Sil: Blacklist system that spins off disks during ACPI power off
  2008-10-03 21:48                             ` [PATCH 0/6] SATA: Blacklist systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
                                                 ` (3 preceding siblings ...)
  2008-10-03 21:57                               ` [PATCH 4/6] SATA AHCI: Blacklist system that spins off disks during ACPI power off Rafael J. Wysocki
@ 2008-10-03 21:58                               ` Rafael J. Wysocki
  2008-10-03 21:58                               ` [PATCH 6/6] SATA PIIX: " Rafael J. Wysocki
  5 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-03 21:58 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

From: Rafael J. Wysocki <rjw@sisk.pl>

SATA Sil: Blacklist system that spins off disks during ACPI power off

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information.

Blacklist HP nx6325 that uses the sata_sil driver.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/sata_sil.c |   36 +++++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/ata/sata_sil.c
===================================================================
--- linux-2.6.orig/drivers/ata/sata_sil.c
+++ linux-2.6/drivers/ata/sata_sil.c
@@ -603,11 +603,38 @@ static void sil_init_controller(struct a
 	}
 }
 
+static bool sil_broken_system_poweroff(struct pci_dev *pdev)
+{
+	static const struct dmi_system_id broken_systems[] = {
+		{
+			.ident = "HP Compaq nx6325",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq nx6325"),
+			},
+			/* PCI slot number of the controller */
+			.driver_data = (void *)0x12UL,
+		},
+
+		{ }	/* terminate list */
+	};
+	const struct dmi_system_id *dmi = dmi_first_match(broken_systems);
+
+	if (dmi) {
+		unsigned long slot = (unsigned long)dmi->driver_data;
+		/* apply the quirk only to on-board controllers */
+		return slot == PCI_SLOT(pdev->devfn);
+	}
+
+	return false;
+}
+
 static int sil_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	static int printed_version;
 	int board_id = ent->driver_data;
-	const struct ata_port_info *ppi[] = { &sil_port_info[board_id], NULL };
+	struct ata_port_info pi = sil_port_info[board_id];
+	const struct ata_port_info *ppi[] = { &pi, NULL };
 	struct ata_host *host;
 	void __iomem *mmio_base;
 	int n_ports, rc;
@@ -621,6 +648,13 @@ static int sil_init_one(struct pci_dev *
 	if (board_id == sil_3114)
 		n_ports = 4;
 
+	if (sil_broken_system_poweroff(pdev)) {
+		pi.flags |= ATA_FLAG_NO_POWEROFF_SPINDOWN |
+					ATA_FLAG_NO_HIBERNATE_SPINDOWN;
+		dev_info(&pdev->dev, "quirky BIOS, skipping spindown "
+				"on poweroff and hibernation\n");
+	}
+
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
 	if (!host)
 		return -ENOMEM;


^ permalink raw reply	[flat|nested] 49+ messages in thread

* [PATCH 6/6] SATA PIIX: Blacklist system that spins off disks during ACPI power off
  2008-10-03 21:48                             ` [PATCH 0/6] SATA: Blacklist systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
                                                 ` (4 preceding siblings ...)
  2008-10-03 21:58                               ` [PATCH 5/6] SATA Sil: " Rafael J. Wysocki
@ 2008-10-03 21:58                               ` Rafael J. Wysocki
  5 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-03 21:58 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jeff Garzik, Tejun Heo, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

From: Rafael J. Wysocki <rjw@sisk.pl>

SATA PIIX: Blacklist system that spins off disks during ACPI power off

Some notebooks from HP have the problem that their BIOSes attempt to
spin down hard drives before entering ACPI system states S4 and S5.
This leads to a yo-yo effect during system power-off shutdown and the
last phase of hibernation when the disk is first spun down by the
kernel and then almost immediately turned on and off by the BIOS.
This, in turn, may result in shortening the disk's life times.

To prevent this from happening we can blacklist the affected systems
using DMI information.

Blacklist HP 2510p that uses the ata_piix driver.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Acked-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/ata_piix.c |   34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Index: linux-2.6/drivers/ata/ata_piix.c
===================================================================
--- linux-2.6.orig/drivers/ata/ata_piix.c
+++ linux-2.6/drivers/ata/ata_piix.c
@@ -1449,6 +1449,32 @@ static void piix_iocfg_bit18_quirk(struc
 	}
 }
 
+static bool piix_broken_system_poweroff(struct pci_dev *pdev)
+{
+	static const struct dmi_system_id broken_systems[] = {
+		{
+			.ident = "HP Compaq 2510p",
+			.matches = {
+				DMI_MATCH(DMI_SYS_VENDOR, "Hewlett-Packard"),
+				DMI_MATCH(DMI_PRODUCT_NAME, "HP Compaq 2510p"),
+			},
+			/* PCI slot number of the controller */
+			.driver_data = (void *)0x1FUL,
+		},
+
+		{ }	/* terminate list */
+	};
+	const struct dmi_system_id *dmi = dmi_first_match(broken_systems);
+
+	if (dmi) {
+		unsigned long slot = (unsigned long)dmi->driver_data;
+		/* apply the quirk only to on-board controllers */
+		return slot == PCI_SLOT(pdev->devfn);
+	}
+
+	return false;
+}
+
 /**
  *	piix_init_one - Register PIIX ATA PCI device with kernel services
  *	@pdev: PCI device to register
@@ -1484,6 +1510,14 @@ static int __devinit piix_init_one(struc
 	if (!in_module_init)
 		return -ENODEV;
 
+	if (piix_broken_system_poweroff(pdev)) {
+		piix_port_info[ent->driver_data].flags |=
+				ATA_FLAG_NO_POWEROFF_SPINDOWN |
+					ATA_FLAG_NO_HIBERNATE_SPINDOWN;
+		dev_info(&pdev->dev, "quirky BIOS, skipping spindown "
+				"on poweroff and hibernation\n");
+	}
+
 	port_info[0] = piix_port_info[ent->driver_data];
 	port_info[1] = piix_port_info[ent->driver_data];
 


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation
  2008-10-03 21:51                               ` [PATCH 1/6] Hibernation: Introduce system_entering_hibernation Rafael J. Wysocki
@ 2008-10-03 23:27                                 ` Alan Cox
  2008-10-04 10:13                                   ` Rafael J. Wysocki
  2008-10-04  7:02                                 ` Tejun Heo
  1 sibling, 1 reply; 49+ messages in thread
From: Alan Cox @ 2008-10-03 23:27 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Andrew Morton, Jeff Garzik, Tejun Heo, ACPI Devel Maling List,
	linux-ide, Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

> in the final phase of hibernation.  In particular, some SATA drivers
> are going to use it for blacklisting systems in which the disks
> should not be spun down during the last phase of hibernation (the
> BIOS will do that anyway).

If you added the PCI ids for the controller to your tables you could
implement this series once instead of in multiple drivers


^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation
  2008-10-03 21:51                               ` [PATCH 1/6] Hibernation: Introduce system_entering_hibernation Rafael J. Wysocki
  2008-10-03 23:27                                 ` Alan Cox
@ 2008-10-04  7:02                                 ` Tejun Heo
  2008-10-04 10:14                                   ` Rafael J. Wysocki
  1 sibling, 1 reply; 49+ messages in thread
From: Tejun Heo @ 2008-10-04  7:02 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Andrew Morton, Jeff Garzik, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Hibernation: Introduce system_entering_hibernation
> 
> Introduce boolean function system_entering_hibernation() returning
> 'true' during the last phase of hibernation, in which devices are
> being put into low power states and the sleep state (for example,
> ACPI S4) is finally entered.
> 
> Some device drivers need such a function to check if the system is
> in the final phase of hibernation.  In particular, some SATA drivers
> are going to use it for blacklisting systems in which the disks
> should not be spun down during the last phase of hibernation (the
> BIOS will do that anyway).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

applied 1-6 to tj-upstream

  git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git tj-upstream
  http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=shortlog;h=tj-upstream

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation
  2008-10-03 23:27                                 ` Alan Cox
@ 2008-10-04 10:13                                   ` Rafael J. Wysocki
  2008-10-04 21:50                                     ` Alan Cox
  0 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-04 10:13 UTC (permalink / raw)
  To: Alan Cox
  Cc: Andrew Morton, Jeff Garzik, Tejun Heo, ACPI Devel Maling List,
	linux-ide, Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

On Saturday, 4 of October 2008, Alan Cox wrote:
> > in the final phase of hibernation.  In particular, some SATA drivers
> > are going to use it for blacklisting systems in which the disks
> > should not be spun down during the last phase of hibernation (the
> > BIOS will do that anyway).
> 
> If you added the PCI ids for the controller to your tables you could
> implement this series once instead of in multiple drivers

Hm, I'm not sure what you mean exactly, but the point is to blacklist the
on-board controllers only and a devfn of the controller is needed for that,
or a part of it (like the slot number I'm using).  Could that be done above
the driver level?

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation
  2008-10-04  7:02                                 ` Tejun Heo
@ 2008-10-04 10:14                                   ` Rafael J. Wysocki
  0 siblings, 0 replies; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-04 10:14 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Andrew Morton, Jeff Garzik, ACPI Devel Maling List, linux-ide,
	Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

On Saturday, 4 of October 2008, Tejun Heo wrote:
> Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Hibernation: Introduce system_entering_hibernation
> > 
> > Introduce boolean function system_entering_hibernation() returning
> > 'true' during the last phase of hibernation, in which devices are
> > being put into low power states and the sleep state (for example,
> > ACPI S4) is finally entered.
> > 
> > Some device drivers need such a function to check if the system is
> > in the final phase of hibernation.  In particular, some SATA drivers
> > are going to use it for blacklisting systems in which the disks
> > should not be spun down during the last phase of hibernation (the
> > BIOS will do that anyway).
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> applied 1-6 to tj-upstream
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata-dev.git tj-upstream
>   http://git.kernel.org/?p=linux/kernel/git/tj/libata-dev.git;a=shortlog;h=tj-upstream

Thanks a lot,
Rafael

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation
  2008-10-04 10:13                                   ` Rafael J. Wysocki
@ 2008-10-04 21:50                                     ` Alan Cox
  2008-10-04 21:52                                       ` Tejun Heo
  0 siblings, 1 reply; 49+ messages in thread
From: Alan Cox @ 2008-10-04 21:50 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Andrew Morton, Jeff Garzik, Tejun Heo, ACPI Devel Maling List,
	linux-ide, Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

> Hm, I'm not sure what you mean exactly, but the point is to blacklist the
> on-board controllers only and a devfn of the controller is needed for that,
> or a part of it (like the slot number I'm using).  Could that be done above
> the driver level?

It can be done in one place via the libata core code I think rather than
in each driver. Might need to be a helper function but you'd at least
remove all the duplication.

Alan

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation
  2008-10-04 21:50                                     ` Alan Cox
@ 2008-10-04 21:52                                       ` Tejun Heo
  2008-10-04 22:30                                         ` Rafael J. Wysocki
  0 siblings, 1 reply; 49+ messages in thread
From: Tejun Heo @ 2008-10-04 21:52 UTC (permalink / raw)
  To: Alan Cox
  Cc: Rafael J. Wysocki, Andrew Morton, Jeff Garzik,
	ACPI Devel Maling List, linux-ide, Thomas Renninger,
	Robert Hancock, LKML, Frans Pop, Maciej Rutecki

Alan Cox wrote:
>> Hm, I'm not sure what you mean exactly, but the point is to blacklist the
>> on-board controllers only and a devfn of the controller is needed for that,
>> or a part of it (like the slot number I'm using).  Could that be done above
>> the driver level?
> 
> It can be done in one place via the libata core code I think rather than
> in each driver. Might need to be a helper function but you'd at least
> remove all the duplication.

I think it's better to keep the list in each LLD but having a helper in
libata-core would be nice.

Thanks.

-- 
tejun

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation
  2008-10-04 22:30                                         ` Rafael J. Wysocki
@ 2008-10-04 22:27                                           ` Tejun Heo
  0 siblings, 0 replies; 49+ messages in thread
From: Tejun Heo @ 2008-10-04 22:27 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Alan Cox, Andrew Morton, Jeff Garzik, ACPI Devel Maling List,
	linux-ide, Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

Rafael J. Wysocki wrote:
> On Saturday, 4 of October 2008, Tejun Heo wrote:
>> Alan Cox wrote:
>>>> Hm, I'm not sure what you mean exactly, but the point is to blacklist the
>>>> on-board controllers only and a devfn of the controller is needed for that,
>>>> or a part of it (like the slot number I'm using).  Could that be done above
>>>> the driver level?
>>> It can be done in one place via the libata core code I think rather than
>>> in each driver. Might need to be a helper function but you'd at least
>>> remove all the duplication.
>> I think it's better to keep the list in each LLD but having a helper in
>> libata-core would be nice.
> 
> Well, I can try to introduce one in a future patch.  Will that be OK?

Yeah, sure.  :-)

-- 
tejun

^ permalink raw reply	[flat|nested] 49+ messages in thread

* Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation
  2008-10-04 21:52                                       ` Tejun Heo
@ 2008-10-04 22:30                                         ` Rafael J. Wysocki
  2008-10-04 22:27                                           ` Tejun Heo
  0 siblings, 1 reply; 49+ messages in thread
From: Rafael J. Wysocki @ 2008-10-04 22:30 UTC (permalink / raw)
  To: Tejun Heo
  Cc: Alan Cox, Andrew Morton, Jeff Garzik, ACPI Devel Maling List,
	linux-ide, Thomas Renninger, Robert Hancock, LKML, Frans Pop,
	Maciej Rutecki

On Saturday, 4 of October 2008, Tejun Heo wrote:
> Alan Cox wrote:
> >> Hm, I'm not sure what you mean exactly, but the point is to blacklist the
> >> on-board controllers only and a devfn of the controller is needed for that,
> >> or a part of it (like the slot number I'm using).  Could that be done above
> >> the driver level?
> > 
> > It can be done in one place via the libata core code I think rather than
> > in each driver. Might need to be a helper function but you'd at least
> > remove all the duplication.
> 
> I think it's better to keep the list in each LLD but having a helper in
> libata-core would be nice.

Well, I can try to introduce one in a future patch.  Will that be OK?

Rafael

^ permalink raw reply	[flat|nested] 49+ messages in thread

end of thread, other threads:[~2008-10-04 22:29 UTC | newest]

Thread overview: 49+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-08-28 22:02 Regression: SATA disk double spin-off during hibernation on hp nx6325 Rafael J. Wysocki
2008-08-29 10:03 ` Tejun Heo
2008-08-29 10:41   ` Rafael J. Wysocki
2008-08-29 10:42     ` Tejun Heo
2008-09-06 17:23       ` [PATCH] SATA: Blacklist systems that spin off disks during ACPI power off Rafael J. Wysocki
2008-09-08 11:38         ` Tejun Heo
2008-09-14  2:21           ` Jeff Garzik
2008-09-15  0:00             ` Rafael J. Wysocki
2008-09-17 21:50               ` Jeff Garzik
2008-09-29 22:06                 ` [PATCH 0/6] " Rafael J. Wysocki
2008-09-29 22:10                   ` [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation Rafael J. Wysocki
2008-10-01  3:32                     ` Tejun Heo
2008-10-01 11:36                       ` Rafael J. Wysocki
2008-10-03  8:35                     ` Andrew Morton
2008-10-03 11:27                       ` Rafael J. Wysocki
2008-10-03 12:43                         ` Andrew Morton
2008-10-03 15:03                           ` Rafael J. Wysocki
2008-10-03 21:48                             ` [PATCH 0/6] SATA: Blacklist systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
2008-10-03 21:51                               ` [PATCH 1/6] Hibernation: Introduce system_entering_hibernation Rafael J. Wysocki
2008-10-03 23:27                                 ` Alan Cox
2008-10-04 10:13                                   ` Rafael J. Wysocki
2008-10-04 21:50                                     ` Alan Cox
2008-10-04 21:52                                       ` Tejun Heo
2008-10-04 22:30                                         ` Rafael J. Wysocki
2008-10-04 22:27                                           ` Tejun Heo
2008-10-04  7:02                                 ` Tejun Heo
2008-10-04 10:14                                   ` Rafael J. Wysocki
2008-10-03 21:52                               ` [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible Rafael J. Wysocki
2008-10-03 21:57                               ` [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off (rev. 2) Rafael J. Wysocki
2008-10-03 21:57                               ` [PATCH 4/6] SATA AHCI: Blacklist system that spins off disks during ACPI power off Rafael J. Wysocki
2008-10-03 21:58                               ` [PATCH 5/6] SATA Sil: " Rafael J. Wysocki
2008-10-03 21:58                               ` [PATCH 6/6] SATA PIIX: " Rafael J. Wysocki
2008-10-03 12:49                         ` [PATCH 1/6] Hibernation: Introduce new system state for the last phase of hibernation Arjan van de Ven
2008-10-03 15:05                           ` Rafael J. Wysocki
2008-09-29 22:13                   ` [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible Rafael J. Wysocki
2008-10-01  3:31                     ` Tejun Heo
2008-09-29 22:14                   ` [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off Rafael J. Wysocki
2008-10-01  3:34                     ` Tejun Heo
2008-10-01 11:36                       ` Rafael J. Wysocki
2008-09-29 22:15                   ` [PATCH 4/6] SATA AHCI: Blacklist system that spins " Rafael J. Wysocki
2008-09-29 22:16                   ` [PATCH 5/6] SATA Sil: " Rafael J. Wysocki
2008-09-29 22:18                   ` [PATCH 6/6] SATA PIIX: " Rafael J. Wysocki
     [not found] ` <200808291245.27436.elendil@planet.nl>
2008-08-29 10:57   ` Regression: SATA disk double spin-off during hibernation on hp nx6325 Rafael J. Wysocki
2008-08-29 11:30     ` Frans Pop
2008-09-04 14:05       ` Rafael J. Wysocki
2008-09-05 22:25         ` Frans Pop
2008-09-05 23:18           ` Rafael J. Wysocki
2008-09-06  0:09             ` Frans Pop
2008-09-06 11:06               ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).