[PATCH 0/2] Power off empty ports

[PATCH 0/2] Power off empty ports

[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

[PATCH 1/2] libata: add power_off policy for unoccupied ports

[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

[PATCH 2/2] Documentation: document new link power management policy option

[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

Re: [PATCH 0/2] Power off empty ports

[Tejun Heo]

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

[PATCH] libata: Power off empty ports

[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

Re: [PATCH] libata: Power off empty ports

[Tejun Heo]

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

Re: [PATCH] libata: Power off empty ports

[Kristen Carlson Accardi]

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.
 

Re: [PATCH] libata: Power off empty ports

[Tejun Heo]

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

Re: [PATCH] libata: Power off empty ports

[Kristen Carlson Accardi]

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.

Re: [PATCH] libata: Power off empty ports

[Tejun Heo]

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