| Message ID | 20170914103536.7650-2-hdegoede@redhat.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | libata: Add new med_power_with_dipm link_power_management_policy setting | expand |
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.
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, };
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(-)