[git patches] libata hibernation fixes

[git patches] libata hibernation fixes

[Jeff Garzik]

This adds code at a late stage (heading towards -rc4), but does
eliminate a particular spin-up overcycling behavior associated with
hibernation.

Rafael's extended description below...  Separated to make it easier to
pull-or-not, separate from the other libata fixes.  There shouldn't be
any merge trouble between the two.

	Jeff




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

Please pull from 'hibern_regress' branch of
master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/libata-dev.git hibern_regress

to receive the following updates:

 drivers/ata/ahci.c          |   32 ++++++++++++++++++
 drivers/ata/ata_piix.c      |   34 +++++++++++++++++++
 drivers/ata/libata-scsi.c   |   20 ++++++++++--
 drivers/ata/sata_sil.c      |   36 ++++++++++++++++++++-
 drivers/firmware/dmi_scan.c |   74 ++++++++++++++++++++++++++++++++-----------
 include/linux/dmi.h         |    1 +
 include/linux/libata.h      |    2 +
 include/linux/suspend.h     |    2 +
 kernel/power/disk.c         |   10 ++++++
 9 files changed, 188 insertions(+), 23 deletions(-)

Rafael J. Wysocki (6):
      Hibernation: Introduce system_entering_hibernation
      DMI: Introduce dmi_first_match to make the interface more flexible
      SATA: Blacklisting of systems that spin off disks during ACPI power off (rev. 2)
      SATA AHCI: Blacklist system that spins off disks during ACPI power off
      SATA Sil: Blacklist system that spins off disks during ACPI power off
      SATA PIIX: Blacklist system that spins off disks during ACPI power off

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index a67b8e7..8e8790b 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -2546,6 +2546,32 @@ static void ahci_p5wdh_workaround(struct ata_host *host)
 	}
 }
 
+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;
@@ -2641,6 +2667,12 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		}
 	}
 
+	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
diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
index 8e37be1..f5fff92 100644
--- a/drivers/ata/ata_piix.c
+++ b/drivers/ata/ata_piix.c
@@ -1370,6 +1370,32 @@ static void piix_iocfg_bit18_quirk(struct pci_dev *pdev)
 	}
 }
 
+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
@@ -1405,6 +1431,14 @@ static int __devinit piix_init_one(struct pci_dev *pdev,
 	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];
 
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bbb30d8..0a0aa95 100644
--- a/drivers/ata/libata-scsi.c
+++ b/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"
 
@@ -1307,6 +1308,17 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 
 		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.
@@ -1330,8 +1342,7 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 				scmd->scsi_done = qc->scsidone;
 				qc->scsidone = ata_delayed_done;
 			}
-			scmd->result = SAM_STAT_GOOD;
-			return 1;
+			goto skip;
 		}
 
 		/* Issue ATA STANDBY IMMEDIATE command */
@@ -1347,10 +1358,13 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 
 	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;
 }
 
 
diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index 031d7b7..f1dd478 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -603,11 +603,38 @@ static void sil_init_controller(struct ata_host *host)
 	}
 }
 
+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 *pdev, const struct pci_device_id *ent)
 	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;
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 3e526b6..6e88b99 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -415,6 +415,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
@@ -429,32 +450,47 @@ 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;
-
-	WARN(!dmi_initialized, KERN_ERR "dmi check: not initialized yet.\n");
-
-	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;
+	int count = 0;
+	const struct dmi_system_id *d;
+
+	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;
+
+	WARN(!dmi_initialized, KERN_ERR "dmi check: not initialized yet.\n");
+
+	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)
  *
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index e5084eb..6e20820 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -38,6 +38,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);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index f5441ed..cc0ceaf 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -187,6 +187,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 */
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index 2ce8207..195200b 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -232,6 +232,7 @@ extern unsigned long get_safe_page(gfp_t gfp_mask);
 
 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_free(struct page *p) {}
 
 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
diff --git a/kernel/power/disk.c b/kernel/power/disk.c
index c9d7408..248e243 100644
--- a/kernel/power/disk.c
+++ b/kernel/power/disk.c
@@ -72,6 +72,14 @@ void hibernation_set_ops(struct platform_hibernation_ops *ops)
 	mutex_unlock(&pm_mutex);
 }
 
+static bool entering_platform_hibernation;
+
+bool system_entering_hibernation(void)
+{
+	return entering_platform_hibernation;
+}
+EXPORT_SYMBOL_GPL(system_entering_hibernation);
+
 #ifdef CONFIG_PM_DEBUG
 static void hibernation_debug_sleep(void)
 {
@@ -416,6 +424,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 +460,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();

Re: [git patches] libata hibernation fixes

[Linus Torvalds]

On Tue, 4 Nov 2008, Jeff Garzik wrote:
> 
> This adds code at a late stage (heading towards -rc4), but does
> eliminate a particular spin-up overcycling behavior associated with
> hibernation.

What does this have to do with hibernation? 

If it's a hibernation-only issue, then there is something wrong. 

Also, if it is an issue for normal power-off as well, then I wonder why 
this isn't an issue on Windows. Does windows not spin down disks at all?

IOW, I really don't think this is correct. 

I _do_ think that correct might be:

 - maybe we just do something odd and different, triggering some BIOS 
   behavior that isn't there under Windows.

   So we should power down thigns differently so that the BIOS.

 - quite possibly: we just should not spin down disks at all, and just 
   flush them and do the "park" command thing. If we're _really_ powering 
   off, the disks will spin down on their own when power goes away. Maybe 
   that's what Windows does?

So I really don't want to pull this, because I want to get more of an 
explanation for why we need to do this at all. I also don't think this is 
even appropriate at this stage in -rc.

Is it a regression? If so, that just strengthens the questions above - 
what did _we_ start doing wrong that this is needed at all? Let's just 
stop doing that, not add some idiotic black-list for somethign that _we_ 
do wrong.

			Linus

Re: [git patches] libata hibernation fixes

[Rafael J. Wysocki]

On Tuesday, 4 of November 2008, Linus Torvalds wrote:
> 
> On Tue, 4 Nov 2008, Jeff Garzik wrote:
> > 
> > This adds code at a late stage (heading towards -rc4), but does
> > eliminate a particular spin-up overcycling behavior associated with
> > hibernation.
> 
> What does this have to do with hibernation? 
> 
> If it's a hibernation-only issue, then there is something wrong. 

No, it is not.  On some machines it is a power-off issue, on the others it is
hibernation and power-off issue.

> Also, if it is an issue for normal power-off as well, then I wonder why 
> this isn't an issue on Windows. Does windows not spin down disks at all?

In fact, AFAICS, it is an issue on Windows as well, at least if
other-than-HP-preloaded version of Windows is used.

> IOW, I really don't think this is correct. 
> 
> I _do_ think that correct might be:
> 
>  - maybe we just do something odd and different, triggering some BIOS 
>    behavior that isn't there under Windows.
> 
>    So we should power down thigns differently so that the BIOS.
> 
>  - quite possibly: we just should not spin down disks at all, and just 
>    flush them and do the "park" command thing. If we're _really_ powering 
>    off, the disks will spin down on their own when power goes away. Maybe 
>    that's what Windows does?
> 
> So I really don't want to pull this, because I want to get more of an 
> explanation for why we need to do this at all. I also don't think this is 
> even appropriate at this stage in -rc.
> 
> Is it a regression? If so, that just strengthens the questions above - 
> what did _we_ start doing wrong that this is needed at all? Let's just 
> stop doing that, not add some idiotic black-list for somethign that _we_ 
> do wrong.

This is a regression, but from something like 2.6.25 or even earlier.
I think what happened is we started to power-off disks at one point and these
BIOS-es just don't like that.

[Note that the issue only appears in _some_ HP boxes, other vendors don't
seem to be affected at all.]

Thanks,
Rafael

Re: [git patches] libata hibernation fixes

[Mark Lord]

Rafael J. Wysocki wrote:
> On Tuesday, 4 of November 2008, Linus Torvalds wrote:
>> On Tue, 4 Nov 2008, Jeff Garzik wrote:
>>> This adds code at a late stage (heading towards -rc4), but does
>>> eliminate a particular spin-up overcycling behavior associated with
>>> hibernation.
>> What does this have to do with hibernation? 
>>
>> If it's a hibernation-only issue, then there is something wrong. 
> 
> No, it is not.  On some machines it is a power-off issue, on the others it is
> hibernation and power-off issue.
> 
>> Also, if it is an issue for normal power-off as well, then I wonder why 
>> this isn't an issue on Windows. Does windows not spin down disks at all?
> 
> In fact, AFAICS, it is an issue on Windows as well, at least if
> other-than-HP-preloaded version of Windows is used.
> 
>> IOW, I really don't think this is correct. 
>>
>> I _do_ think that correct might be:
>>
>>  - maybe we just do something odd and different, triggering some BIOS 
>>    behavior that isn't there under Windows.
>>
>>    So we should power down thigns differently so that the BIOS.
>>
>>  - quite possibly: we just should not spin down disks at all, and just 
>>    flush them and do the "park" command thing. If we're _really_ powering 
>>    off, the disks will spin down on their own when power goes away. Maybe 
>>    that's what Windows does?
>>
>> So I really don't want to pull this, because I want to get more of an 
>> explanation for why we need to do this at all. I also don't think this is 
>> even appropriate at this stage in -rc.
>>
>> Is it a regression? If so, that just strengthens the questions above - 
>> what did _we_ start doing wrong that this is needed at all? Let's just 
>> stop doing that, not add some idiotic black-list for somethign that _we_ 
>> do wrong.
> 
> This is a regression, but from something like 2.6.25 or even earlier.
> I think what happened is we started to power-off disks at one point and these
> BIOS-es just don't like that.
> 
> [Note that the issue only appears in _some_ HP boxes, other vendors don't
> seem to be affected at all.]
..

So, what happens if we just don't ever spin them down from the kernel?
Presumably they still spin-down normally (HP or otherwise) when the BIOS
actually cuts the power at the end of all of this?

Just curious..

Re: [git patches] libata hibernation fixes

[Rafael J. Wysocki]

On Tuesday, 4 of November 2008, Mark Lord wrote:
> Rafael J. Wysocki wrote:
> > On Tuesday, 4 of November 2008, Linus Torvalds wrote:
> >> On Tue, 4 Nov 2008, Jeff Garzik wrote:
> >>> This adds code at a late stage (heading towards -rc4), but does
> >>> eliminate a particular spin-up overcycling behavior associated with
> >>> hibernation.
> >> What does this have to do with hibernation? 
> >>
> >> If it's a hibernation-only issue, then there is something wrong. 
> > 
> > No, it is not.  On some machines it is a power-off issue, on the others it is
> > hibernation and power-off issue.
> > 
> >> Also, if it is an issue for normal power-off as well, then I wonder why 
> >> this isn't an issue on Windows. Does windows not spin down disks at all?
> > 
> > In fact, AFAICS, it is an issue on Windows as well, at least if
> > other-than-HP-preloaded version of Windows is used.
> > 
> >> IOW, I really don't think this is correct. 
> >>
> >> I _do_ think that correct might be:
> >>
> >>  - maybe we just do something odd and different, triggering some BIOS 
> >>    behavior that isn't there under Windows.
> >>
> >>    So we should power down thigns differently so that the BIOS.
> >>
> >>  - quite possibly: we just should not spin down disks at all, and just 
> >>    flush them and do the "park" command thing. If we're _really_ powering 
> >>    off, the disks will spin down on their own when power goes away. Maybe 
> >>    that's what Windows does?
> >>
> >> So I really don't want to pull this, because I want to get more of an 
> >> explanation for why we need to do this at all. I also don't think this is 
> >> even appropriate at this stage in -rc.
> >>
> >> Is it a regression? If so, that just strengthens the questions above - 
> >> what did _we_ start doing wrong that this is needed at all? Let's just 
> >> stop doing that, not add some idiotic black-list for somethign that _we_ 
> >> do wrong.
> > 
> > This is a regression, but from something like 2.6.25 or even earlier.
> > I think what happened is we started to power-off disks at one point and these
> > BIOS-es just don't like that.
> > 
> > [Note that the issue only appears in _some_ HP boxes, other vendors don't
> > seem to be affected at all.]
> ..
> 
> So, what happens if we just don't ever spin them down from the kernel?
> Presumably they still spin-down normally (HP or otherwise) when the BIOS
> actually cuts the power at the end of all of this?
> 
> Just curious..

Well, I'll let Tejun answer that. :-)

Thanks,
Rafael

Re: [git patches] libata hibernation fixes

[Pavel Machek]

Hi!

>  - quite possibly: we just should not spin down disks at all, and just 
>    flush them and do the "park" command thing. If we're _really_ powering 
>    off, the disks will spin down on their own when power goes away. Maybe 
>    that's what Windows does?

I believe that 'emergency' spindown (on power fail) is different from
regular spindown, and that emergency spindown damages the disk more.

But perhaps park + power fail is similar to regular spindown?

(Is park command normally supported on modern disks? IIRC hdaps people
had issues with not all disks supporting it?)

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [git patches] libata hibernation fixes

[Rafael J. Wysocki]

On Tuesday, 4 of November 2008, Pavel Machek wrote:
> Hi!
> 
> >  - quite possibly: we just should not spin down disks at all, and just 
> >    flush them and do the "park" command thing. If we're _really_ powering 
> >    off, the disks will spin down on their own when power goes away. Maybe 
> >    that's what Windows does?
> 
> I believe that 'emergency' spindown (on power fail) is different from
> regular spindown, and that emergency spindown damages the disk more.
> 
> But perhaps park + power fail is similar to regular spindown?
> 
> (Is park command normally supported on modern disks? IIRC hdaps people
> had issues with not all disks supporting it?)

I don't know really and that's why I'd prefer it if Tejun commented here.

Anyway, I created these patches because the people who discussed this issue
believed it was the right thing to do.  They are based on a Tejun's patch
posted as an attachment to http://bugzilla.kernel.org/show_bug.cgi?id=8855 .
Please look into that bug entry for further references.

Thanks,
Rafael

Re: [git patches] libata hibernation fixes

[Linus Torvalds]

On Tue, 4 Nov 2008, Pavel Machek wrote:
>
> (Is park command normally supported on modern disks? IIRC hdaps people
> had issues with not all disks supporting it?)

The modern version of parking is called "idle immediate". It may be that 
only laptop drives support the "unload" part. But it's definitely not an 
ancient and deprecated thing (although calling it "parking" is apparently 
old-fashioned :)

			Linus

Re: [git patches] libata hibernation fixes

[Tejun Heo]

Rafael J. Wysocki wrote:
>> So, what happens if we just don't ever spin them down from the kernel?
>> Presumably they still spin-down normally (HP or otherwise) when the BIOS
>> actually cuts the power at the end of all of this?
>>
>> Just curious..
> 
> Well, I'll let Tejun answer that. :-)

The BIOSen on the affected HP ones will spin the drives down during
power off or hibernation, so there won't be any problem (that's what the
patch does, BTW).  For all other machines, the drive will do emergency
head unload which usually sounds a bit differently and shortens the
lifespan of the drive.

Thanks.

-- 
tejun

Re: [git patches] libata hibernation fixes

[Tejun Heo]

Linus Torvalds wrote:
> 
> On Tue, 4 Nov 2008, Pavel Machek wrote:
>> (Is park command normally supported on modern disks? IIRC hdaps people
>> had issues with not all disks supporting it?)
> 
> The modern version of parking is called "idle immediate". It may be that 
> only laptop drives support the "unload" part. But it's definitely not an 
> ancient and deprecated thing (although calling it "parking" is apparently 
> old-fashioned :)

I'm afraid trying to use that new extension during shutdown would cause
far more trouble which would require extensive black/white list at the
end.  For hdaps, it's okay as the laptops w/ those sensors are supposed
to have matching hard drive but using it on generic machines doesn't
sound like a sane idea.

Thanks.

-- 
tejun

Re: [git patches] libata hibernation fixes

[Tejun Heo]

Linus Torvalds wrote:
>  - maybe we just do something odd and different, triggering some BIOS 
>    behavior that isn't there under Windows.
>    So we should power down thigns differently so that the BIOS.

These HP machines require patched storage driver to avoid the same
problem, so it's not a generic problem and Windows is doing its own
blacklisting in its own way.

>  - quite possibly: we just should not spin down disks at all, and just 
>    flush them and do the "park" command thing. If we're _really_ powering 
>    off, the disks will spin down on their own when power goes away. Maybe 
>    that's what Windows does?

I'm fairly sure they do about the same thing we do (FLUSH followed by
STANDBY_IMMEDIATE).  The only problem we've seen regarding harddrive
shutdown or suspend sequence is a BIOS wrongfully assuming the
controller is turned on and goes bonkers on suspend (this, we might want
to change, not much point in turning off all the PCI controllers before
entering suspend, is there?) and these HP machines which like to issue
STANDBY_IMMEDIATE of its own and also breaks on stock Windows.

> So I really don't want to pull this, because I want to get more of an 
> explanation for why we need to do this at all. I also don't think this is 
> even appropriate at this stage in -rc.

They were supposed to go in during -rc1 but there was a misunderstanding
while handing off patches I collected during Jeff's vacation and they
got lost inbetween.  I apologize for the late submission but this
problem can shorten life span of hard drives considerably && applies
only to a small set of machines, so I hope this can go in for 2.6.28.

> Is it a regression? If so, that just strengthens the questions above - 
> what did _we_ start doing wrong that this is needed at all? Let's just 
> stop doing that, not add some idiotic black-list for somethign that _we_ 
> do wrong.

It's a regression in a sense that a long while ago, libata didn't do any
spindown at all, which, again, was a regression from drivers/ide.  So,
the question whether this problem is a regression or not is sort of
irrelevant here.  It's plainly a ugly workaround for ugly hardware
situation and Windows does it in even uglier way.

Thanks.

-- 
tejun

Re: [git patches] libata hibernation fixes

[Tejun Heo]

So, Linus, Jeff, what should be done about this patchset?  Should it be
postponed to 2.6.29-rc1 or do you still think it's the wrong thing to do?

Thanks.

-- 
tejun

Re: [git patches] libata hibernation fixes

[Tejun Heo]

Jeff Garzik wrote:
> This adds code at a late stage (heading towards -rc4), but does
> eliminate a particular spin-up overcycling behavior associated with
> hibernation.
> 
> Rafael's extended description below...  Separated to make it easier to
> pull-or-not, separate from the other libata fixes.  There shouldn't be
> any merge trouble between the two.
> 
> SATA: Blacklist systems that spin off disks during ACPI power off

Jeff, I think this should be merged into 2.6.29 unless Linus still
objects.  Linus, as discussed in this thread, this is workaround for a
hardware / firmware problem and vanilla windows also suffers the
problem, so as ugly as it is, we need this to prevent double spin down
on the machine.

Thanks and happy new year.

-- 
tejun

Re: [git patches] libata hibernation fixes

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> This adds code at a late stage (heading towards -rc4), but does
>> eliminate a particular spin-up overcycling behavior associated with
>> hibernation.
>>
>> Rafael's extended description below...  Separated to make it easier to
>> pull-or-not, separate from the other libata fixes.  There shouldn't be
>> any merge trouble between the two.
>>
>> SATA: Blacklist systems that spin off disks during ACPI power off
> 
> Jeff, I think this should be merged into 2.6.29 unless Linus still
> objects.  Linus, as discussed in this thread, this is workaround for a
> hardware / firmware problem and vanilla windows also suffers the
> problem, so as ugly as it is, we need this to prevent double spin down
> on the machine.

(Just returned from New Year's holiday)

Yes, I definitely want to put it in as well.

	Jeff

Re: [git patches] libata hibernation fixes

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> This adds code at a late stage (heading towards -rc4), but does
>> eliminate a particular spin-up overcycling behavior associated with
>> hibernation.
>>
>> Rafael's extended description below...  Separated to make it easier to
>> pull-or-not, separate from the other libata fixes.  There shouldn't be
>> any merge trouble between the two.
>>
>> SATA: Blacklist systems that spin off disks during ACPI power off
> 
> Jeff, I think this should be merged into 2.6.29 unless Linus still
> objects.  Linus, as discussed in this thread, this is workaround for a
> hardware / firmware problem and vanilla windows also suffers the
> problem, so as ugly as it is, we need this to prevent double spin down
> on the machine.

Oh, I forgot to reply to this...  libata-2.6.git#hibern_regress no 
longer applies cleanly.

I'll try poking at it on Monday and get it cleaned up, as it is IMO a 
bug fix and still should go in.

	Jeff

Re: [git patches] libata hibernation fixes

[Rafael J. Wysocki]

On Sunday 11 January 2009, Jeff Garzik wrote:
> Tejun Heo wrote:
> > Jeff Garzik wrote:
> >> This adds code at a late stage (heading towards -rc4), but does
> >> eliminate a particular spin-up overcycling behavior associated with
> >> hibernation.
> >>
> >> Rafael's extended description below...  Separated to make it easier to
> >> pull-or-not, separate from the other libata fixes.  There shouldn't be
> >> any merge trouble between the two.
> >>
> >> SATA: Blacklist systems that spin off disks during ACPI power off
> > 
> > Jeff, I think this should be merged into 2.6.29 unless Linus still
> > objects.  Linus, as discussed in this thread, this is workaround for a
> > hardware / firmware problem and vanilla windows also suffers the
> > problem, so as ugly as it is, we need this to prevent double spin down
> > on the machine.
> 
> Oh, I forgot to reply to this...  libata-2.6.git#hibern_regress no 
> longer applies cleanly.

There is a name conflict with a change applied after this patchset had been
created.  The appended patch fixes it for me.

> I'll try poking at it on Monday and get it cleaned up, as it is IMO a 
> bug fix and still should go in.

Thanks!

Best,
Rafael

---
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 9a81508..8f0f7c4 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -415,10 +415,10 @@ void __init dmi_scan_machine(void)
 }
 
 /**
- *	dmi_match - check if dmi_system_id structure matches system DMI data
+ *	dmi_matches - 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)
+static bool dmi_matches(const struct dmi_system_id *dmi)
 {
 	int i;
 
@@ -456,7 +456,7 @@ int dmi_check_system(const struct dmi_system_id *list)
 	const struct dmi_system_id *d;
 
 	for (d = list; d->ident; d++)
-		if (dmi_match(d)) {
+		if (dmi_matches(d)) {
 			count++;
 			if (d->callback && d->callback(d))
 				break;
@@ -483,7 +483,7 @@ 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))
+		if (dmi_matches(d))
 			return d;
 
 	return NULL;

Re: [git patches] libata hibernation fixes

[Frans Pop]

Rafael J. Wysocki wrote:
> There is a name conflict with a change applied after this patchset had
> been created.  The appended patch fixes it for me.
[...]
> @@ -483,7 +483,7 @@ 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))
> +               if (dmi_matches(d))
>                         return d;

I get two warnings when compiling your patch with this correction on top:
drivers/firmware/dmi_scan.c: In function ‘dmi_first_match’:
drivers/firmware/dmi_scan.c:488: warning: return discards qualifiers from pointer target type
drivers/firmware/dmi_scan.c:483: warning: unused variable ‘i’

The second is trivial, but the first is beyond me :-/

Cheers,
FJP

Re: [git patches] libata hibernation fixes

[Rafael J. Wysocki]

On Sunday 18 January 2009, Frans Pop wrote:
> Rafael J. Wysocki wrote:
> > There is a name conflict with a change applied after this patchset had
> > been created.  The appended patch fixes it for me.
> [...]
> > @@ -483,7 +483,7 @@ 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))
> > +               if (dmi_matches(d))
> >                         return d;
> 
> I get two warnings when compiling your patch with this correction on top:

Hm, there were more patches than one, so I'm not sure.  When I apply all of
them, I don't see these error, but perhaps I should resend the entire series.
Will do tomorrow.

> drivers/firmware/dmi_scan.c: In function ‘dmi_first_match’:
> drivers/firmware/dmi_scan.c:488: warning: return discards qualifiers from pointer target type
> drivers/firmware/dmi_scan.c:483: warning: unused variable ‘i’
> 
> The second is trivial, but the first is beyond me :-/

Thanks,
Rafael

Re: [git patches] libata hibernation fixes

[Jeff Garzik]

Rafael J. Wysocki wrote:
> On Sunday 18 January 2009, Frans Pop wrote:
>> Rafael J. Wysocki wrote:
>>> There is a name conflict with a change applied after this patchset had
>>> been created.  The appended patch fixes it for me.
>> [...]
>>> @@ -483,7 +483,7 @@ 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))
>>> +               if (dmi_matches(d))
>>>                         return d;
>> I get two warnings when compiling your patch with this correction on top:
> 
> Hm, there were more patches than one, so I'm not sure.  When I apply all of
> them, I don't see these error, but perhaps I should resend the entire series.
> Will do tomorrow.

That would be useful...  I was planning to regenerate the series, but 
this would save time.

Thanks!

	Jeff

Re: [git patches] libata hibernation fixes

[Frans Pop]

On Sunday 18 January 2009, Rafael J. Wysocki wrote:
> > I get two warnings when compiling your patch with this correction on
> > top:
>
> Hm, there were more patches than one, so I'm not sure.  When I apply
> all of them, I don't see these error, but perhaps I should resend the
> entire series. Will do tomorrow.

It's possible that I do not have the very latest version of your patches. 
If you send your latest version I'll check, and test them.

Thanks,
FJP

P.S. .29-rc2 is failing both STR and STD on my HP 2510p :-(
     Will mail details sometime this week.

Re: [git patches] libata hibernation fixes

[Rafael J. Wysocki]

On Sunday 18 January 2009, Frans Pop wrote:
> On Sunday 18 January 2009, Rafael J. Wysocki wrote:
> > > I get two warnings when compiling your patch with this correction on
> > > top:
> >
> > Hm, there were more patches than one, so I'm not sure.  When I apply
> > all of them, I don't see these error, but perhaps I should resend the
> > entire series. Will do tomorrow.
> 
> It's possible that I do not have the very latest version of your patches. 
> If you send your latest version I'll check, and test them.
> 
> Thanks,
> FJP
> 
> P.S. .29-rc2 is failing both STR and STD on my HP 2510p :-(
>      Will mail details sometime this week.

There are some known problems, so please watch the next report of recent
regressions (most probably tomorrow).

Thanks,
Rafael

[PATCH 0/6] Hibernation/poweroff quirks (was: Re: [git patches] libata hibernation fixes)

[Rafael J. Wysocki]

On Sunday 18 January 2009, Jeff Garzik wrote:
> Rafael J. Wysocki wrote:
> > On Sunday 18 January 2009, Frans Pop wrote:
> >> Rafael J. Wysocki wrote:
> >>> There is a name conflict with a change applied after this patchset had
> >>> been created.  The appended patch fixes it for me.
> >> [...]
> >>> @@ -483,7 +483,7 @@ 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))
> >>> +               if (dmi_matches(d))
> >>>                         return d;
> >> I get two warnings when compiling your patch with this correction on top:
> > 
> > Hm, there were more patches than one, so I'm not sure.  When I apply all of
> > them, I don't see these error, but perhaps I should resend the entire series.
> > Will do tomorrow.
> 
> That would be useful...  I was planning to regenerate the series, but 
> this would save time.

The rebased series follows.

Thanks,
Rafael

[PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Rafael J. Wysocki]

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

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     |   10 ++++++++++
 2 files changed, 12 insertions(+)

Index: linux-2.6/include/linux/suspend.h
===================================================================
--- linux-2.6.orig/include/linux/suspend.h
+++ linux-2.6/include/linux/suspend.h
@@ -237,6 +237,7 @@ extern int hibernate_nvs_alloc(void);
 extern void hibernate_nvs_free(void);
 extern void hibernate_nvs_save(void);
 extern void hibernate_nvs_restore(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) {}
@@ -252,6 +253,7 @@ static inline int hibernate_nvs_alloc(vo
 static inline void hibernate_nvs_free(void) {}
 static inline void hibernate_nvs_save(void) {}
 static inline void hibernate_nvs_restore(void) {}
+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
@@ -71,6 +71,14 @@ void hibernation_set_ops(struct platform
 	mutex_unlock(&pm_mutex);
 }
 
+static bool entering_platform_hibernation;
+
+bool system_entering_hibernation(void)
+{
+	return entering_platform_hibernation;
+}
+EXPORT_SYMBOL(system_entering_hibernation);
+
 #ifdef CONFIG_PM_DEBUG
 static void hibernation_debug_sleep(void)
 {
@@ -411,6 +419,7 @@ int hibernation_platform_enter(void)
 	if (error)
 		goto Close;
 
+	entering_platform_hibernation = true;
 	suspend_console();
 	error = device_suspend(PMSG_HIBERNATE);
 	if (error) {
@@ -445,6 +454,7 @@ int hibernation_platform_enter(void)
  Finish:
 	hibernation_ops->finish();
  Resume_devices:
+	entering_platform_hibernation = false;
 	device_resume(PMSG_RESTORE);
 	resume_console();
  Close:

[PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible

[Rafael J. Wysocki]

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

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 |   72 +++++++++++++++++++++++++++++++++-----------
 include/linux/dmi.h         |    1 
 2 files changed, 55 insertions(+), 18 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
@@ -415,6 +415,29 @@ void __init dmi_scan_machine(void)
 }
 
 /**
+ *	dmi_matches - check if dmi_system_id structure matches system DMI data
+ *	@dmi: pointer to the dmi_system_id structure to check
+ */
+static bool dmi_matches(const struct dmi_system_id *dmi)
+{
+	int i;
+
+	WARN(!dmi_initialized, KERN_ERR "dmi check: not initialized yet.\n");
+
+	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
@@ -429,32 +452,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;
 
-	WARN(!dmi_initialized, KERN_ERR "dmi check: not initialized yet.\n");
-
-	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_matches(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_matches(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
@@ -38,6 +38,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);

[PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off

[Rafael J. Wysocki]

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

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 |   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"
 
@@ -1303,6 +1304,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.
@@ -1326,8 +1338,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 */
@@ -1343,10 +1354,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
@@ -187,6 +187,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 */

[PATCH 4/6] SATA AHCI: Blacklist system that spins off disks during ACPI power off

[Rafael J. Wysocki]

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

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
@@ -2548,6 +2548,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;
@@ -2647,6 +2673,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

[PATCH 5/6] SATA Sil: Blacklist system that spins off disks during ACPI power off

[Rafael J. Wysocki]

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

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
@@ -695,11 +695,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;
@@ -713,6 +740,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;

[PATCH 6/6] SATA PIIX: Blacklist system that spins off disks during ACPI power off

[Rafael J. Wysocki]

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

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
@@ -1387,6 +1387,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
@@ -1422,6 +1448,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];
 

Re: [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible

[Frans Pop]

On Monday 19 January 2009, Rafael J. Wysocki wrote:
> +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_matches(d))
> +                       return d;
> +
> +       return NULL;
> +}
> +EXPORT_SYMBOL(dmi_first_match);

I did indeed have an older version of the patches (before you split them 
out) and that was missing the "const" in the first quoted line, so that 
explains the warning I had.

If I compare the version I've been running with the past months and this 
series I see only minor changes and I don't expect any problems from 
them. If I notice anything I'll let you know. No news is good news :-)

Thanks,
FJP

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Frederic Weisbecker]

On Mon, Jan 19, 2009 at 08:54:54PM +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> 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).
>

Hi Rafael,

Why not using the power event notifier?

Frederic.

 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  include/linux/suspend.h |    2 ++
>  kernel/power/disk.c     |   10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> Index: linux-2.6/include/linux/suspend.h
> ===================================================================
> --- linux-2.6.orig/include/linux/suspend.h
> +++ linux-2.6/include/linux/suspend.h
> @@ -237,6 +237,7 @@ extern int hibernate_nvs_alloc(void);
>  extern void hibernate_nvs_free(void);
>  extern void hibernate_nvs_save(void);
>  extern void hibernate_nvs_restore(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) {}
> @@ -252,6 +253,7 @@ static inline int hibernate_nvs_alloc(vo
>  static inline void hibernate_nvs_free(void) {}
>  static inline void hibernate_nvs_save(void) {}
>  static inline void hibernate_nvs_restore(void) {}
> +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
> @@ -71,6 +71,14 @@ void hibernation_set_ops(struct platform
>  	mutex_unlock(&pm_mutex);
>  }
>  
> +static bool entering_platform_hibernation;
> +
> +bool system_entering_hibernation(void)
> +{
> +	return entering_platform_hibernation;
> +}
> +EXPORT_SYMBOL(system_entering_hibernation);
> +
>  #ifdef CONFIG_PM_DEBUG
>  static void hibernation_debug_sleep(void)
>  {
> @@ -411,6 +419,7 @@ int hibernation_platform_enter(void)
>  	if (error)
>  		goto Close;
>  
> +	entering_platform_hibernation = true;
>  	suspend_console();
>  	error = device_suspend(PMSG_HIBERNATE);
>  	if (error) {
> @@ -445,6 +454,7 @@ int hibernation_platform_enter(void)
>   Finish:
>  	hibernation_ops->finish();
>   Resume_devices:
> +	entering_platform_hibernation = false;
>  	device_resume(PMSG_RESTORE);
>  	resume_console();
>   Close:
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Rafael J. Wysocki]

On Monday 19 January 2009, Frederic Weisbecker wrote:
> On Mon, Jan 19, 2009 at 08:54:54PM +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > 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).
> >
> 
> Hi Rafael,
> 
> Why not using the power event notifier?

I'm not sure what you mean exactly, care to elaborate?

Anyway, however, this is a pretty old patch series and I'm not very much
interested in reworking it once again.  If you think it can be made better,
please go on. :-)

Thanks,
Rafael

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Frederic Weisbecker]

On Mon, Jan 19, 2009 at 10:35:22PM +0100, Rafael J. Wysocki wrote:
> On Monday 19 January 2009, Frederic Weisbecker wrote:
> > On Mon, Jan 19, 2009 at 08:54:54PM +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rjw@sisk.pl>
> > > 
> > > 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).
> > >
> > 
> > Hi Rafael,
> > 
> > Why not using the power event notifier?
> 
> I'm not sure what you mean exactly, care to elaborate?


Sorry, I just thought about using the pm_notifier to listen on the suspend/resume
events to check the current hibernation state.

 
> Anyway, however, this is a pretty old patch series and I'm not very much
> interested in reworking it once again.  If you think it can be made better,
> please go on. :-)


It was just a suggestion, or more likely a question for curiosity, not really
a criticism since I didn't follow the discussions about these patches and the
problem they solve :-)

Thanks.


> Thanks,
> Rafael

Re: [PATCH 6/6] SATA PIIX: Blacklist system that spins off disks during ACPI power off

[Frans Pop]

On Monday 19 January 2009, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> 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>

As I have a system that is affected by this and as I have been a happy 
user of this patch for 4 months or so, it may make sense to add my

Tested-by: Frans Pop <elendil@planet.nl>

Same goes indirectly for the rest of the series, except for patch 5.

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Maciej Rutecki]

2009/1/19 Rafael J. Wysocki <rjw@sisk.pl>:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> 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>

Tested-by: Maciej Rutecki <maciej.rutecki@gmail.com>

Tested on HP Compaq nx6310

Thanks for patches

-- 
Maciej Rutecki
http://www.maciek.unixy.pl

Re: [PATCH 2/6] DMI: Introduce dmi_first_match to make the interface more flexible

[Maciej Rutecki]

2009/1/19 Rafael J. Wysocki <rjw@sisk.pl>:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> 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>

Tested-by: Maciej Rutecki <maciej.rutecki@gmail.com>

Tested on HP Compaq nx6310

Thanks for patches


-- 
Maciej Rutecki
http://www.maciek.unixy.pl

Re: [PATCH 3/6] SATA: Blacklisting of systems that spin off disks during ACPI power off

[Maciej Rutecki]

2009/1/19 Rafael J. Wysocki <rjw@sisk.pl>:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> 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>

Tested-by: Maciej Rutecki <maciej.rutecki@gmail.com>

Tested on HP Compaq nx6310

Thanks for patches

-- 
Maciej Rutecki
http://www.maciek.unixy.pl

Re: [PATCH 4/6] SATA AHCI: Blacklist system that spins off disks during ACPI power off

[Maciej Rutecki]

2009/1/19 Rafael J. Wysocki <rjw@sisk.pl>:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> 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>

Tested-by: Maciej Rutecki <maciej.rutecki@gmail.com>

Tested on HP Compaq nx6310

Thanks for patches


-- 
Maciej Rutecki
http://www.maciek.unixy.pl

Re: [PATCH 5/6] SATA Sil: Blacklist system that spins off disks during ACPI power off

[Maciej Rutecki]

2009/1/19 Rafael J. Wysocki <rjw@sisk.pl>:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> 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>

Tested-by: Maciej Rutecki <maciej.rutecki@gmail.com>

Tested on HP Compaq nx6310

Thanks for patches
-- 
Maciej Rutecki
http://www.maciek.unixy.pl

Re: [PATCH 6/6] SATA PIIX: Blacklist system that spins off disks during ACPI power off

[Maciej Rutecki]

2009/1/19 Rafael J. Wysocki <rjw@sisk.pl>:
> From: Rafael J. Wysocki <rjw@sisk.pl>
>
> 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>

Tested-by: Maciej Rutecki <maciej.rutecki@gmail.com>

Tested on HP Compaq nx6310

Thanks for patches

-- 
Maciej Rutecki
http://www.maciek.unixy.pl

STR/STD OK with -rc2 + selected pending patches (was: [git patches] libata hibernation fixes)

[Frans Pop]

Just a FYI; most CCs dropped.

Rafael J. Wysocki wrote:
> On Sunday 18 January 2009, Frans Pop wrote:
>> P.S. .29-rc2 is failing both STR and STD on my HP 2510p :-(
>>      Will mail details sometime this week.
> 
> There are some known problems, so please watch the next report of recent
> regressions (most probably tomorrow).

I've been testing -rc2 today with the following patches on top:
* hp-wmi: fix regressions caused by missing if statement
* double disk spin off fix:
  Hibernation: Introduce system_entering_hibernation
  DMI: Introduce dmi_first_match to make the interface more flexible
  SATA: Blacklisting of systems that spin off disks during ACPI power off
  SATA AHCI: Blacklist system that spins off disks during ACPI power off
  SATA Sil: Blacklist system that spins off disks during ACPI power off
  SATA PIIX: Blacklist system that spins off disks during ACPI power off
* selected patches based on regression overview:
  USB: Fix suspend-resume of PCI USB controllers
  Revert "x86: signal: change type of paramter for sys_rt_sigreturn()"
  USB: don't enable wakeup by default for PCI host controllers
  work_on_cpu: dont try to get_online_cpus() in work_on_cpu.

With that both STR and STD work fine and -rc2 looks pretty good in general 
for me. Most "restoring config space" now happens early on resume from 
STR, as intended.

One issue so far (fan keeps running), which I'll report separately in 
bugzilla.

Cheers,
FJP

Re: STR/STD OK with -rc2 + selected pending patches (was: [git patches] libata hibernation fixes)

[Rafael J. Wysocki]

On Tuesday 20 January 2009, Frans Pop wrote:
> Just a FYI; most CCs dropped.
> 
> Rafael J. Wysocki wrote:
> > On Sunday 18 January 2009, Frans Pop wrote:
> >> P.S. .29-rc2 is failing both STR and STD on my HP 2510p :-(
> >>      Will mail details sometime this week.
> > 
> > There are some known problems, so please watch the next report of recent
> > regressions (most probably tomorrow).
> 
> I've been testing -rc2 today with the following patches on top:
> * hp-wmi: fix regressions caused by missing if statement
> * double disk spin off fix:
>   Hibernation: Introduce system_entering_hibernation
>   DMI: Introduce dmi_first_match to make the interface more flexible
>   SATA: Blacklisting of systems that spin off disks during ACPI power off
>   SATA AHCI: Blacklist system that spins off disks during ACPI power off
>   SATA Sil: Blacklist system that spins off disks during ACPI power off
>   SATA PIIX: Blacklist system that spins off disks during ACPI power off
> * selected patches based on regression overview:
>   USB: Fix suspend-resume of PCI USB controllers
>   Revert "x86: signal: change type of paramter for sys_rt_sigreturn()"
>   USB: don't enable wakeup by default for PCI host controllers
>   work_on_cpu: dont try to get_online_cpus() in work_on_cpu.
> 
> With that both STR and STD work fine and -rc2 looks pretty good in general 
> for me. Most "restoring config space" now happens early on resume from 
> STR, as intended.
> 
> One issue so far (fan keeps running), which I'll report separately in 
> bugzilla.

OK, thanks for testing!

Rafael

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Pavel Machek]

On Mon 2009-01-19 20:54:54, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> 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.

...this paragraph...

> Index: linux-2.6/kernel/power/disk.c
> ===================================================================
> --- linux-2.6.orig/kernel/power/disk.c
> +++ linux-2.6/kernel/power/disk.c
> @@ -71,6 +71,14 @@ void hibernation_set_ops(struct platform
>  	mutex_unlock(&pm_mutex);
>  }

....should go somewhere here. Because the name does not imply it is
for last phases, only...

> +static bool entering_platform_hibernation;
> +
> +bool system_entering_hibernation(void)
> +{
> +	return entering_platform_hibernation;
> +}
> +EXPORT_SYMBOL(system_entering_hibernation);
> +
>  #ifdef CONFIG_PM_DEBUG
>  static void hibernation_debug_sleep(void)
>  {
> @@ -411,6 +419,7 @@ int hibernation_platform_enter(void)
>  	if (error)
>  		goto Close;
>  
> +	entering_platform_hibernation = true;
>  	suspend_console();

Why entering _platform_ hibernation? It will not be set for echo
shutdown > disk case?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Rafael J. Wysocki]

On Thursday 29 January 2009, Pavel Machek wrote:
> On Mon 2009-01-19 20:54:54, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > 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.
> 
> ...this paragraph...
> 
> > Index: linux-2.6/kernel/power/disk.c
> > ===================================================================
> > --- linux-2.6.orig/kernel/power/disk.c
> > +++ linux-2.6/kernel/power/disk.c
> > @@ -71,6 +71,14 @@ void hibernation_set_ops(struct platform
> >  	mutex_unlock(&pm_mutex);
> >  }
> 
> ....should go somewhere here. Because the name does not imply it is
> for last phases, only...
> 
> > +static bool entering_platform_hibernation;
> > +
> > +bool system_entering_hibernation(void)
> > +{
> > +	return entering_platform_hibernation;
> > +}
> > +EXPORT_SYMBOL(system_entering_hibernation);
> > +
> >  #ifdef CONFIG_PM_DEBUG
> >  static void hibernation_debug_sleep(void)
> >  {
> > @@ -411,6 +419,7 @@ int hibernation_platform_enter(void)
> >  	if (error)
> >  		goto Close;
> >  
> > +	entering_platform_hibernation = true;
> >  	suspend_console();
> 
> Why entering _platform_ hibernation? It will not be set for echo
> shutdown > disk case?

If I remember correctly, no, it won't.

Thanks,
Rafael

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Rafael J. Wysocki]

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

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Tejun Heo]

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

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Tejun Heo]

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

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Alan Cox]

> 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

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Rafael J. Wysocki]

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

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Rafael J. Wysocki]

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?

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Tejun Heo]

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

Re: [PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Alan Cox]

> 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

[PATCH 1/6] Hibernation: Introduce system_entering_hibernation

[Rafael J. Wysocki]

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();