[PATCH 0/1] libata: Add new med_power_with_dipm link_power_management_policy setting

[PATCH 0/1] libata: Add new med_power_with_dipm link_power_management_policy setting

[Hans de Goede]

Hi All,

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

On Intel CPUs starting with the Haswell generation SATA link power
management can save a significant amount of power.

Previous attempts to try and enable SATA LPM by default have gotten stuck
on ome reports of some disks / SSDs not liking min_power leading to system
crashes and in some cases even data corruption has been reported.

This patch is another attempt to make the default Windows IRST driver
settings Matthew found available under Linux, but instead of changing
medium_power and potentially introducing regressions, this commit adds
a new med_power_with_dipm setting.

I've done a blog post asking people to test this new settings on laptops:
https://hansdegoede.livejournal.com/18412.html

As mentioned there my goal is to enable med_power_with_dipm as default
LPM setting for laptops for Fedora 28 (to be released aprox. May 2018).

How exactly this will be done is still up for debate, one option would
be a kernel patch which recognizes the mobile variant of Intel's chipset
and changes the default on those, another option is punting this to
userspace.

Regards,

Hans

[PATCH] libata: Add new med_power_with_dipm link_power_management_policy setting

[Hans de Goede]

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

Re: [PATCH] libata: Add new med_power_with_dipm link_power_management_policy setting

[Tejun Heo]

On Thu, Sep 14, 2017 at 12:35:36PM +0200, Hans de Goede wrote:
> 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>

Applied to libata/for-4.15.

Thanks.

-- 
tejun