linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Tejun Heo <tj@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>,
	Matthew Garrett <mjg59@srcf.ucam.org>,
	linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH] libata: Add new med_power_with_dipm link_power_management_policy setting
Date: Thu, 14 Sep 2017 12:35:36 +0200	[thread overview]
Message-ID: <20170914103536.7650-2-hdegoede@redhat.com> (raw)
In-Reply-To: <20170914103536.7650-1-hdegoede@redhat.com>

As described by Matthew Garret quite a while back:
https://mjg59.dreamwidth.org/34868.html

Intel CPUs starting with the Haswell generation need SATA links to power
down for the "package" part of the CPU to reach low power-states like
PC7 / P8 which bring a significant power-saving with them.

The default max_performance lpm policy does not allow for these high
PC states, both the medium_power and min_power policies do allow this.

The min_power policy saves significantly more power, but there are some
reports of some disks / SSDs not liking min_power leading to system
crashes and in some cases even data corruption has been reported.

Matthew has found a document documenting the default settings of
Intel's IRST Windows driver with which most laptops ship:
https://www-ssl.intel.com/content/dam/doc/reference-guide/sata-devices-implementation-recommendations.pdf

Matthew wrote a patch changing med_power to match those defaults, but
that never got anywhere as some people where reporting issues with the
patch-set that patch was a part of.

This commit is another attempt to make the default IRST driver settings
available under Linux, but instead of changing medium_power and
potentially introducing regressions, this commit adds a new
med_power_with_dipm setting which is identical to the existing
medium_power accept that it enables dipm on top, which makes it match
the Windows IRST driver settings, which should hopefully be safe to
use on most devices.

The med_power_with_dipm setting is close to min_power, except that:
a) It does not use host-initiated slumber mode (ASP not set),
   but it does allow device-initiated slumber
b) It does not enable DevSlp mode

On my T440s test laptop I get the following power savings when idle:
medium_power		0.9W
med_power_with_dipm	1.2W
min_power		1.2W

Suggested-by: Matthew Garrett <mjg59@srcf.ucam.org>
Cc: Matthew Garrett <mjg59@srcf.ucam.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/ata/libata-core.c |  1 +
 drivers/ata/libata-eh.c   | 10 +++++-----
 drivers/ata/libata-scsi.c |  9 +++++----
 include/linux/libata.h    |  1 +
 4 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1945a8ea2099..f165d95c780f 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3964,6 +3964,7 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 		scontrol &= ~(0x1 << 8);
 		scontrol |= (0x6 << 8);
 		break;
+	case ATA_LPM_MED_POWER_WITH_DIPM:
 	case ATA_LPM_MIN_POWER:
 		if (ata_link_nr_enabled(link) > 0)
 			/* no restrictions on LPM transitions */
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 3dbd05532c09..aefe9a9971ad 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3456,9 +3456,9 @@ static int ata_eh_maybe_retry_flush(struct ata_device *dev)
  *	@r_failed_dev: out parameter for failed device
  *
  *	Enable SATA Interface power management.  This will enable
- *	Device Interface Power Management (DIPM) for min_power
- * 	policy, and then call driver specific callbacks for
- *	enabling Host Initiated Power management.
+ *	Device Interface Power Management (DIPM) for min_power and
+ *	medium_power_with_dipm policies, and then call driver specific
+ *	callbacks for enabling Host Initiated Power management.
  *
  *	LOCKING:
  *	EH context.
@@ -3504,7 +3504,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			hints &= ~ATA_LPM_HIPM;
 
 		/* disable DIPM before changing link config */
-		if (policy != ATA_LPM_MIN_POWER && dipm) {
+		if (policy < ATA_LPM_MED_POWER_WITH_DIPM && dipm) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_DISABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
@@ -3547,7 +3547,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 
 	/* host config updated, enable DIPM if transitioning to MIN_POWER */
 	ata_for_each_dev(dev, link, ENABLED) {
-		if (policy == ATA_LPM_MIN_POWER && !no_dipm &&
+		if (policy >= ATA_LPM_MED_POWER_WITH_DIPM && !no_dipm &&
 		    ata_id_has_dipm(dev->id)) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_ENABLE, SATA_DIPM);
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 44ba292f2cd7..673e72f438eb 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -106,10 +106,11 @@ static const u8 def_control_mpage[CONTROL_MPAGE_LEN] = {
 };
 
 static const char *ata_lpm_policy_names[] = {
-	[ATA_LPM_UNKNOWN]	= "max_performance",
-	[ATA_LPM_MAX_POWER]	= "max_performance",
-	[ATA_LPM_MED_POWER]	= "medium_power",
-	[ATA_LPM_MIN_POWER]	= "min_power",
+	[ATA_LPM_UNKNOWN]		= "max_performance",
+	[ATA_LPM_MAX_POWER]		= "max_performance",
+	[ATA_LPM_MED_POWER]		= "medium_power",
+	[ATA_LPM_MED_POWER_WITH_DIPM]	= "med_power_with_dipm",
+	[ATA_LPM_MIN_POWER]		= "min_power",
 };
 
 static ssize_t ata_scsi_lpm_store(struct device *device,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 931c32f1f18d..ed9826b21c5e 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -522,6 +522,7 @@ enum ata_lpm_policy {
 	ATA_LPM_UNKNOWN,
 	ATA_LPM_MAX_POWER,
 	ATA_LPM_MED_POWER,
+	ATA_LPM_MED_POWER_WITH_DIPM, /* Med power + DIPM as win IRST does */
 	ATA_LPM_MIN_POWER,
 };
 
-- 
2.14.1

  reply	other threads:[~2017-09-14 10:35 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-14 10:35 [PATCH 0/1] libata: Add new med_power_with_dipm link_power_management_policy setting Hans de Goede
2017-09-14 10:35 ` Hans de Goede [this message]
2017-09-19  3:23   ` [PATCH] " Tejun Heo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170914103536.7650-2-hdegoede@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=linux-ide@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mjg59@srcf.ucam.org \
    --cc=tj@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).