Patchwork libata: Power off empty ports

login
register
mail settings
Submitter Kristen Carlson Accardi
Date March 4, 2011, 6:24 p.m.
Message ID <1299263051-3739-1-git-send-email-kristen@linux.intel.com>
Download mbox | patch
Permalink /patch/85432/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Kristen Carlson Accardi - March 4, 2011, 6:24 p.m.
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(-)
Tejun Heo - March 4, 2011, 6:38 p.m.
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.
Kristen Carlson Accardi - March 4, 2011, 6:47 p.m.
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.
 
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - March 5, 2011, 8:01 a.m.
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.
Kristen Carlson Accardi - March 7, 2011, 5:37 p.m.
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.

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - March 7, 2011, 5:48 p.m.
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.

Patch

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