* [PATCH 0/2] Power off empty ports
@ 2011-03-03 18:21 Kristen Carlson Accardi
2011-03-03 18:21 ` [PATCH 1/2] libata: add power_off policy for unoccupied ports Kristen Carlson Accardi
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Kristen Carlson Accardi @ 2011-03-03 18:21 UTC (permalink / raw)
To: linux-ide; +Cc: Kristen Carlson Accardi
Give users the option of completely powering off unoccupied
SATA ports by introducing a new link_power_mananagement policy
option "power_off". When the user selects this option on an
empty port, we will power off the port by setting DET to off.
Kristen Carlson Accardi (2):
libata: add power_off policy for unoccupied ports
Documentation: document new link power management policy option
.../scsi/link_power_management_policy.txt | 6 ++++--
drivers/ata/libata-core.c | 4 ++++
drivers/ata/libata-eh.c | 5 +++++
drivers/ata/libata-scsi.c | 1 +
include/linux/libata.h | 1 +
5 files changed, 15 insertions(+), 2 deletions(-)
--
1.7.3.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] libata: add power_off policy for unoccupied ports 2011-03-03 18:21 [PATCH 0/2] Power off empty ports Kristen Carlson Accardi @ 2011-03-03 18:21 ` Kristen Carlson Accardi 2011-03-03 18:21 ` [PATCH 2/2] Documentation: document new link power management policy option Kristen Carlson Accardi 2011-03-04 9:06 ` [PATCH 0/2] Power off empty ports Tejun Heo 2 siblings, 0 replies; 10+ messages in thread From: Kristen Carlson Accardi @ 2011-03-03 18:21 UTC (permalink / raw) To: linux-ide; +Cc: Kristen Carlson Accardi Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> --- drivers/ata/libata-core.c | 4 ++++ drivers/ata/libata-eh.c | 5 +++++ drivers/ata/libata-scsi.c | 1 + include/linux/libata.h | 1 + 4 files changed, 11 insertions(+), 0 deletions(-) diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index d4e52e2..c795466 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -3622,6 +3622,10 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy, /* no restrictions on LPM transitions */ scontrol &= ~(0x3 << 8); break; + case ATA_LPM_POWER_OFF: + scontrol &= ~0xf; + scontrol |= (0x1 << 2); + break; default: WARN_ON(1); } diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 17a6378..cfac236 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -3754,6 +3754,11 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset, config_lpm: /* configure link power saving */ if (link->lpm_policy != ap->target_lpm_policy) { + /* refuse to power off occupied port */ + if (ap->target_lpm_policy == ATA_LPM_POWER_OFF && + (ata_link_nr_enabled(link) > 0)) + goto out; + rc = ata_eh_set_lpm(link, ap->target_lpm_policy, &dev); if (rc) goto rest_fail; diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 600f635..70f1c66 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -108,6 +108,7 @@ static const char *ata_lpm_policy_names[] = { [ATA_LPM_MAX_POWER] = "max_performance", [ATA_LPM_MED_POWER] = "medium_power", [ATA_LPM_MIN_POWER] = "min_power", + [ATA_LPM_POWER_OFF] = "power_off", }; static ssize_t ata_scsi_lpm_store(struct device *dev, diff --git a/include/linux/libata.h b/include/linux/libata.h index c9c5d7a..1ad44dd 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -473,6 +473,7 @@ enum ata_lpm_policy { ATA_LPM_MAX_POWER, ATA_LPM_MED_POWER, ATA_LPM_MIN_POWER, + ATA_LPM_POWER_OFF, }; enum ata_lpm_hints { -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] Documentation: document new link power management policy option 2011-03-03 18:21 [PATCH 0/2] Power off empty ports Kristen Carlson Accardi 2011-03-03 18:21 ` [PATCH 1/2] libata: add power_off policy for unoccupied ports Kristen Carlson Accardi @ 2011-03-03 18:21 ` Kristen Carlson Accardi 2011-03-04 9:06 ` [PATCH 0/2] Power off empty ports Tejun Heo 2 siblings, 0 replies; 10+ messages in thread From: Kristen Carlson Accardi @ 2011-03-03 18:21 UTC (permalink / raw) To: linux-ide; +Cc: Kristen Carlson Accardi Describe new link power management option, power_off. Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> --- .../scsi/link_power_management_policy.txt | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Documentation/scsi/link_power_management_policy.txt b/Documentation/scsi/link_power_management_policy.txt index d18993d..6a024e6 100644 --- a/Documentation/scsi/link_power_management_policy.txt +++ b/Documentation/scsi/link_power_management_policy.txt @@ -1,5 +1,5 @@ This parameter allows the user to set the link (interface) power management. -There are 3 possible options: +There are 4 possible options: Value Effect ---------------------------------------------------------------------------- @@ -16,4 +16,6 @@ medium_power Tell the controller to enter a lower power state when possible, but do not enter the lowest power state, thus improving latency over min_power setting. - +power_off If the port is unoccupied, the port will power + completely down. For occupied ports, this option + is not valid. -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] Power off empty ports 2011-03-03 18:21 [PATCH 0/2] Power off empty ports Kristen Carlson Accardi 2011-03-03 18:21 ` [PATCH 1/2] libata: add power_off policy for unoccupied ports Kristen Carlson Accardi 2011-03-03 18:21 ` [PATCH 2/2] Documentation: document new link power management policy option Kristen Carlson Accardi @ 2011-03-04 9:06 ` Tejun Heo 2011-03-04 18:24 ` [PATCH] libata: " Kristen Carlson Accardi 2 siblings, 1 reply; 10+ messages in thread From: Tejun Heo @ 2011-03-04 9:06 UTC (permalink / raw) To: Kristen Carlson Accardi; +Cc: linux-ide On Thu, Mar 03, 2011 at 10:21:16AM -0800, Kristen Carlson Accardi wrote: > Give users the option of completely powering off unoccupied > SATA ports by introducing a new link_power_mananagement policy > option "power_off". When the user selects this option on an > empty port, we will power off the port by setting DET to off. Why introduce another LPM state? Just do power off if min_power && port empty. Thanks. -- tejun ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] libata: Power off empty ports 2011-03-04 9:06 ` [PATCH 0/2] Power off empty ports Tejun Heo @ 2011-03-04 18:24 ` Kristen Carlson Accardi 2011-03-04 18:38 ` Tejun Heo 0 siblings, 1 reply; 10+ messages in thread From: Kristen Carlson Accardi @ 2011-03-04 18:24 UTC (permalink / raw) To: linux-ide; +Cc: Kristen Carlson Accardi Give users the option of completely powering off unoccupied SATA ports using the existing min_power link_power_management_policy option. When the use selects this option on an empty port, we will power the port off by setting DET to off. For occupied ports, behavior is unchanged. Signed-off-by: Kristen Carlson Accardi <kristen@linux.intel.com> --- drivers/ata/libata-core.c | 10 ++++++++-- drivers/ata/libata-eh.c | 2 +- include/linux/libata.h | 1 + 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index d4e52e2..a650bef 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -3619,8 +3619,14 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy, scontrol |= (0x2 << 8); break; case ATA_LPM_MIN_POWER: - /* no restrictions on LPM transitions */ - scontrol &= ~(0x3 << 8); + if (ata_link_nr_enabled(link) > 0) + /* no restrictions on LPM transitions */ + scontrol &= ~(0x3 << 8); + else { + /* empty port, power off */ + scontrol &= ~0xf; + scontrol |= (0x1 << 2); + } break; default: WARN_ON(1); diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c index 17a6378..f0a31b7 100644 --- a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -3381,7 +3381,7 @@ fail: return rc; } -static int ata_link_nr_enabled(struct ata_link *link) +int ata_link_nr_enabled(struct ata_link *link) { struct ata_device *dev; int cnt = 0; diff --git a/include/linux/libata.h b/include/linux/libata.h index c9c5d7a..8e78052 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1155,6 +1155,7 @@ extern void ata_do_eh(struct ata_port *ap, ata_prereset_fn_t prereset, ata_reset_fn_t softreset, ata_reset_fn_t hardreset, ata_postreset_fn_t postreset); extern void ata_std_error_handler(struct ata_port *ap); +extern int ata_link_nr_enabled(struct ata_link *link); /* * Base operations to inherit from and initializers for sht -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] libata: Power off empty ports 2011-03-04 18:24 ` [PATCH] libata: " Kristen Carlson Accardi @ 2011-03-04 18:38 ` Tejun Heo 2011-03-04 18:47 ` Kristen Carlson Accardi 0 siblings, 1 reply; 10+ messages in thread From: Tejun Heo @ 2011-03-04 18:38 UTC (permalink / raw) To: Kristen Carlson Accardi; +Cc: linux-ide Hello, On Fri, Mar 04, 2011 at 10:24:11AM -0800, Kristen Carlson Accardi wrote: > case ATA_LPM_MIN_POWER: > - /* no restrictions on LPM transitions */ > - scontrol &= ~(0x3 << 8); > + if (ata_link_nr_enabled(link) > 0) > + /* no restrictions on LPM transitions */ > + scontrol &= ~(0x3 << 8); > + else { > + /* empty port, power off */ > + scontrol &= ~0xf; > + scontrol |= (0x1 << 2); > + } Why not just do the following? scontrol &= ~(0x3 << 8); /* if empty, power off */ if (!ata_link_nr_enabled(link)) { scontrol &= ~0xf; scontrol |= 0x1 << 2; } Also, can you please provide some details on on which hardware you tested the change and how much power it actually saved? Thanks. -- tejun ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libata: Power off empty ports 2011-03-04 18:38 ` Tejun Heo @ 2011-03-04 18:47 ` Kristen Carlson Accardi 2011-03-05 8:01 ` Tejun Heo 0 siblings, 1 reply; 10+ messages in thread From: Kristen Carlson Accardi @ 2011-03-04 18:47 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-ide On Fri, 4 Mar 2011 19:38:36 +0100 Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Fri, Mar 04, 2011 at 10:24:11AM -0800, Kristen Carlson Accardi wrote: > > case ATA_LPM_MIN_POWER: > > - /* no restrictions on LPM transitions */ > > - scontrol &= ~(0x3 << 8); > > + if (ata_link_nr_enabled(link) > 0) > > + /* no restrictions on LPM transitions */ > > + scontrol &= ~(0x3 << 8); > > + else { > > + /* empty port, power off */ > > + scontrol &= ~0xf; > > + scontrol |= (0x1 << 2); > > + } > > Why not just do the following? > > scontrol &= ~(0x3 << 8); because, the IPM value is meaningless if we have no devices, so this is a wasted operation. > /* if empty, power off */ > if (!ata_link_nr_enabled(link)) { > scontrol &= ~0xf; > scontrol |= 0x1 << 2; > } > > Also, can you please provide some details on on which hardware you > tested the change and how much power it actually saved? > This change was tested on Intel's Sandy Bridge platform. Your power savings will depend on how many implemented ports you have, and how many are not being used. For a system I tested with which had 2 ports which I could turn off, I saved 300mW. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libata: Power off empty ports 2011-03-04 18:47 ` Kristen Carlson Accardi @ 2011-03-05 8:01 ` Tejun Heo 2011-03-07 17:37 ` Kristen Carlson Accardi 0 siblings, 1 reply; 10+ messages in thread From: Tejun Heo @ 2011-03-05 8:01 UTC (permalink / raw) To: Kristen Carlson Accardi; +Cc: linux-ide Hello, On Fri, Mar 04, 2011 at 10:47:56AM -0800, Kristen Carlson Accardi wrote: > > Why not just do the following? > > > > scontrol &= ~(0x3 << 8); > > because, the IPM value is meaningless if we have no devices, > so this is a wasted operation. What? You're trying to save one AND operation to a value which is already in register in libata powersave path? That's wrong on so many levels (it's not even a proper optimization, think about it). Just make it easier on the eyes. > This change was tested on Intel's Sandy Bridge platform. Your power > savings will depend on how many implemented ports you have, and how > many are not being used. For a system I tested with which had 2 ports > which I could turn off, I saved 300mW. Can you please test at least on a couple different hardware and make sure that the port can be brought back after put to sleep? ie. Verify that hotplug still works after min_power -> max_performance? Also, please add the test results in the patch description. Thanks. -- tejun ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libata: Power off empty ports 2011-03-05 8:01 ` Tejun Heo @ 2011-03-07 17:37 ` Kristen Carlson Accardi 2011-03-07 17:48 ` Tejun Heo 0 siblings, 1 reply; 10+ messages in thread From: Kristen Carlson Accardi @ 2011-03-07 17:37 UTC (permalink / raw) To: Tejun Heo; +Cc: linux-ide On Sat, 5 Mar 2011 09:01:58 +0100 Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Fri, Mar 04, 2011 at 10:47:56AM -0800, Kristen Carlson Accardi wrote: > > > Why not just do the following? > > > > > > scontrol &= ~(0x3 << 8); > > > > because, the IPM value is meaningless if we have no devices, > > so this is a wasted operation. > > What? You're trying to save one AND operation to a value which is > already in register in libata powersave path? That's wrong on so many > levels (it's not even a proper optimization, think about it). Just > make it easier on the eyes. debating this is even more of a waste of an operation, so I'll be glad to do this whatever way suites your taste. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] libata: Power off empty ports 2011-03-07 17:37 ` Kristen Carlson Accardi @ 2011-03-07 17:48 ` Tejun Heo 0 siblings, 0 replies; 10+ messages in thread From: Tejun Heo @ 2011-03-07 17:48 UTC (permalink / raw) To: Kristen Carlson Accardi; +Cc: linux-ide On Mon, Mar 07, 2011 at 09:37:42AM -0800, Kristen Carlson Accardi wrote: > debating this is even more of a waste of an operation, so I'll be glad > to do this whatever way suites your taste. Great, thank you. -- tejun ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-03-07 17:58 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-03 18:21 [PATCH 0/2] Power off empty ports Kristen Carlson Accardi 2011-03-03 18:21 ` [PATCH 1/2] libata: add power_off policy for unoccupied ports Kristen Carlson Accardi 2011-03-03 18:21 ` [PATCH 2/2] Documentation: document new link power management policy option Kristen Carlson Accardi 2011-03-04 9:06 ` [PATCH 0/2] Power off empty ports Tejun Heo 2011-03-04 18:24 ` [PATCH] libata: " Kristen Carlson Accardi 2011-03-04 18:38 ` Tejun Heo 2011-03-04 18:47 ` Kristen Carlson Accardi 2011-03-05 8:01 ` Tejun Heo 2011-03-07 17:37 ` Kristen Carlson Accardi 2011-03-07 17:48 ` Tejun Heo
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).