2008-02-25 13:46:00

by Pavel Machek

[permalink] [raw]
Subject: [ugly patch] Save .15W-.5W by AHCI powersaving

Hi!

This is a patch (very ugly, assumes you have just one disk) to bring
powersaving to AHCI. You need Alan's SCSI autosuspend (attached) patch
as a base.

It saves .5W compared to config with disk spinning, and even .15W
compared to hdparm -y... on my thinkpad x60 anyway.

It is also mandatory first step towards sleepy linux ;-).

Pavel
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 29e71bd..0197b1f 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -259,8 +260,8 @@ static void ahci_fill_cmd_slot(struct ah
u32 opts);
#ifdef CONFIG_PM
static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg);
-static int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
-static int ahci_pci_device_resume(struct pci_dev *pdev);
+int ahci_pci_device_suspend(struct pci_dev *pdev, pm_message_t mesg);
+int ahci_pci_device_resume(struct pci_dev *pdev);
#endif

static struct class_device_attribute *ahci_shost_attrs[] = {
@@ -268,6 +269,35 @@ static struct class_device_attribute *ah
NULL
};

+struct pci_dev *my_pdev;
+int autosuspend_enabled;
+
+/* The host and its devices are all idle so we can autosuspend */
+static int autosuspend(struct Scsi_Host *host)
+{
+ if (my_pdev && autosuspend_enabled) {
+ printk("ahci: should autosuspend\n");
+ ahci_pci_device_suspend(my_pdev, PMSG_SUSPEND);
+ return 0;
+ }
+ printk("ahci: autosuspend disabled\n");
+ return -EINVAL;
+}
+
+/* The host needs to be autoresumed */
+static int autoresume(struct Scsi_Host *host)
+{
+ if (my_pdev && autosuspend_enabled) {
+ printk("ahci: should autoresume\n");
+ ahci_pci_device_resume(my_pdev);
+ return 0;
+ }
+ printk("ahci: autoresume disabled\n");
+ return -EINVAL;
+}
+
+
+
static struct scsi_host_template ahci_sht = {
.module = THIS_MODULE,
.name = DRV_NAME,
@@ -286,6 +322,8 @@ static struct scsi_host_template ahci_sh
.slave_destroy = ata_scsi_slave_destroy,
.bios_param = ata_std_bios_param,
.shost_attrs = ahci_shost_attrs,
+ .autosuspend = autosuspend,
+ .autoresume = autoresume,
};

static const struct ata_port_operations ahci_ops = {
@@ -2300,8 +2356,12 @@ static int ahci_init_one(struct pci_dev
ahci_print_info(host);

pci_set_master(pdev);
- return ata_host_activate(host, pdev->irq, ahci_interrupt, IRQF_SHARED,
+
+ rc = ata_host_activate(host, pdev->irq, ahci_interrupt, IRQF_SHARED,
&ahci_sht);
+ pci_save_state(pdev);
+ my_pdev = pdev;
+ return rc;
}

static int __init ahci_init(void)
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 4e31071..5c40ac2 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -380,7 +381,7 @@ enum scsi_eh_timer_return ata_scsi_timed
* Inherited from SCSI layer (none, can sleep)
*
* RETURNS:
- * Zero.
+ * Nothing.
*/
void ata_scsi_error(struct Scsi_Host *host)
{
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c838e65..0edc25e 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -484,6 +484,8 @@ static int scsi_try_host_reset(struct sc
if (scsi_autoresume_host(shost) != 0)
return FAILED;

+ rtn = shost->hostt->eh_host_reset_handler(scmd);
+
if (rtn == SUCCESS) {
if (!shost->hostt->skip_settle_delay)
ssleep(HOST_RESET_SETTLE_TIME);
@@ -1577,7 +1579,11 @@ int scsi_error_handler(void *data)
* what we need to do to get it up and online again (if we can).
* If we fail, we end up taking the thing offline.
*/
+#if 0
+ /* libata uses scsi_error_handler to suspend its parts; we deadlock
+ if we try to autoresume here */
autoresume_rc = scsi_autoresume_host(shost);
+#endif
if (shost->transportt->eh_strategy_handler)
shost->transportt->eh_strategy_handler(shost);
else
@@ -1591,8 +1597,10 @@ int scsi_error_handler(void *data)
* which are still online.
*/
scsi_restart_operations(shost);
+#if 0
if (autoresume_rc == 0)
scsi_autosuspend_host(shost);
+#endif
set_current_state(TASK_INTERRUPTIBLE);
}
__set_current_state(TASK_RUNNING);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 233feee..3c598e0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -67,6 +67,8 @@ #undef SP

static struct kmem_cache *scsi_bidi_sdb_cache;

+void scsi_run_queue(struct request_queue *q);
+
/*
* Function: scsi_unprep_request()
*
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 5393e15..25bebc1 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -340,6 +340,21 @@ static int scsi_bus_uevent(struct device
return 0;
}

+static int scsi_bus_remove(struct device *dev)
+{
+ struct device_driver *drv = dev->driver;
+ struct scsi_device *sdev = to_scsi_device(dev);
+ int err = 0;
+
+ /* reset the prep_fn back to the default since the
+ * driver may have altered it and it's being removed */
+ blk_queue_prep_rq(sdev->request_queue, scsi_prep_fn);
+
+ if (drv && drv->remove)
+ err = drv->remove(dev);
+
+ return 0;
+}

struct bus_type scsi_bus_type = {
.name = "scsi",
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 16add9a..9ca12d1 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1596,6 +1596,8 @@ static int sd_revalidate_disk(struct gen
return 0;
}

+struct device *my_scsi_disk;
+
/**
* sd_probe - called during driver initialization and whenever a
* new scsi device is attached to the system. It is called once
@@ -1715,6 +1717,7 @@ static int sd_probe(struct device *dev)

scsi_use_ULD_pm(sdp, 1);
scsi_autosuspend_device(sdp);
+ my_scsi_disk = dev;
return 0;

out_suspend:
@@ -1835,6 +1838,8 @@ static int sd_suspend(struct device *dev
struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
int ret = 0;

+ printk("sleepy: sd_suspend start\n");
+
if (!sdkp)
return 0; /* this can happen */




--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


Attachments:
(No filename) (5.86 kB)
delme2 (27.64 kB)
Download all attachments

2008-02-25 22:23:32

by Mark Lord

[permalink] [raw]
Subject: Re: [ugly patch] Save .15W-.5W by AHCI powersaving

Pavel Machek wrote:
> Hi!
>
> This is a patch (very ugly, assumes you have just one disk) to bring
> powersaving to AHCI. You need Alan's SCSI autosuspend (attached) patch
> as a base.
>
> It saves .5W compared to config with disk spinning, and even .15W
> compared to hdparm -y... on my thinkpad x60 anyway.
..

There was a discussion of this here today.
It makes good use of AHCI-specific features.

Has it been tested with a Port-Multiplier yet?

This is cool enough that we really ought to do a hardware-independent
version, so that all SATA interfaces could benefit. Especially ata_piix,
but others too.

Cheers

2008-02-25 22:34:35

by Pavel Machek

[permalink] [raw]
Subject: Re: [ugly patch] Save .15W-.5W by AHCI powersaving

Hi!

>> This is a patch (very ugly, assumes you have just one disk) to bring
>> powersaving to AHCI. You need Alan's SCSI autosuspend (attached) patch
>> as a base.
>>
>> It saves .5W compared to config with disk spinning, and even .15W
>> compared to hdparm -y... on my thinkpad x60 anyway.
> ..
>
> There was a discussion of this here today.

Real-life discussion, or something I could read? :-).

> It makes good use of AHCI-specific features.
>
> Has it been tested with a Port-Multiplier yet?

I do not know what port-multiplier is, sorry. But it was not really
tested. It is not expected to work on any other config than notebook
very similar to mine.

> This is cool enough that we really ought to do a hardware-independent
> version, so that all SATA interfaces could benefit. Especially ata_piix,
> but others too.

Well, it seems like it is 10 lines per driver once Alan's SCSI
autosuspend patches are in...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2008-02-25 22:43:19

by Jeff Garzik

[permalink] [raw]
Subject: Re: [ugly patch] Save .15W-.5W by AHCI powersaving

Mark Lord wrote:
> Pavel Machek wrote:
>> This is a patch (very ugly, assumes you have just one disk) to bring
>> powersaving to AHCI. You need Alan's SCSI autosuspend (attached) patch
>> as a base.
>>
>> It saves .5W compared to config with disk spinning, and even .15W
>> compared to hdparm -y... on my thinkpad x60 anyway.
> ..
>
> There was a discussion of this here today.
> It makes good use of AHCI-specific features.
>
> Has it been tested with a Port-Multiplier yet?
>
> This is cool enough that we really ought to do a hardware-independent
> version, so that all SATA interfaces could benefit. Especially ata_piix,
> but others too.

BTW we can also save power by allowing the user to choose to disable
hotplugging support. Then we can power down PHYs that are not in use.

That requires the addition of some policy controls, because it is
user-specific whether or not to waste power waiting for a plug-in event.

Jeff


2008-02-26 15:50:06

by Mark Lord

[permalink] [raw]
Subject: Re: [ugly patch] Save .15W-.5W by AHCI powersaving

Pavel Machek wrote:
> Hi!
>
>>> This is a patch (very ugly, assumes you have just one disk) to bring
>>> powersaving to AHCI. You need Alan's SCSI autosuspend (attached) patch
>>> as a base.
>>>
>>> It saves .5W compared to config with disk spinning, and even .15W
>>> compared to hdparm -y... on my thinkpad x60 anyway.
>> ..
>>
>> There was a discussion of this here today.
>
> Real-life discussion, or something I could read? :-).
>
>> It makes good use of AHCI-specific features.
>>
>> Has it been tested with a Port-Multiplier yet?
>
> I do not know what port-multiplier is, sorry. But it was not really
> tested. It is not expected to work on any other config than notebook
> very similar to mine.
>
>> This is cool enough that we really ought to do a hardware-independent
>> version, so that all SATA interfaces could benefit. Especially ata_piix,
>> but others too.
>
> Well, it seems like it is 10 lines per driver once Alan's SCSI
> autosuspend patches are in...
..

Cool (literally)!

I think I might have gotten your patch confused in my mind
with another AHCI patch, which uses features of the chip itself
to automatically negotiate/change link power status on the fly
(no s/w needed, other than to turn it on).

That one is very ACPI specific, though.

2008-02-26 16:22:51

by Matthew Garrett

[permalink] [raw]
Subject: Re: [ugly patch] Save .15W-.5W by AHCI powersaving

On Mon, Feb 25, 2008 at 05:42:58PM -0500, Jeff Garzik wrote:

> BTW we can also save power by allowing the user to choose to disable
> hotplugging support. Then we can power down PHYs that are not in use.
>
> That requires the addition of some policy controls, because it is
> user-specific whether or not to waste power waiting for a plug-in event.

For AHCI, if you've enabled link power management then you've already
disabled hotplug. We might as well power down unused phys in that case.
Note that laptop bays still seem to tend to use platform-specific
hotplug notification, even when they're sata - we'll get the hotplug
notify for them even if the phy's powered down, so that case also needs
to be handled.

--
Matthew Garrett | [email protected]

2008-02-26 21:48:52

by Pavel Machek

[permalink] [raw]
Subject: Re: [ugly patch] Save .15W-.5W by AHCI powersaving

On Tue 2008-02-26 10:52:36, Mark Lord wrote:
> Pavel Machek wrote:
>> Hi!
>>
>>>> This is a patch (very ugly, assumes you have just one disk) to bring
>>>> powersaving to AHCI. You need Alan's SCSI autosuspend (attached) patch
>>>> as a base.
>>>>
>>>> It saves .5W compared to config with disk spinning, and even .15W
>>>> compared to hdparm -y... on my thinkpad x60 anyway.
>>> ..
>>>
>>> There was a discussion of this here today.
>>
>> Real-life discussion, or something I could read? :-).
>>
>>> It makes good use of AHCI-specific features.
>>>
>>> Has it been tested with a Port-Multiplier yet?
>>
>> I do not know what port-multiplier is, sorry. But it was not really
>> tested. It is not expected to work on any other config than notebook
>> very similar to mine.
>>
>>> This is cool enough that we really ought to do a hardware-independent
>>> version, so that all SATA interfaces could benefit. Especially ata_piix,
>>> but others too.
>>
>> Well, it seems like it is 10 lines per driver once Alan's SCSI
>> autosuspend patches are in...
> ..
>
> Cool (literally)!
>
> I think I might have gotten your patch confused in my mind
> with another AHCI patch, which uses features of the chip itself
> to automatically negotiate/change link power status on the fly
> (no s/w needed, other than to turn it on).
>
> That one is very ACPI specific, though.

Yep, I seen that, too. I'm just afraid to check, maybe AHCI powerdown
does not save that much power when links in powersave already...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2008-03-21 13:20:22

by Oliver Neukum

[permalink] [raw]
Subject: Re: [linux-pm] [ugly patch] Save .15W-.5W by AHCI powersaving

Am Montag, 25. Februar 2008 23:42:58 schrieb Jeff Garzik:
> > This is cool enough that we really ought to do a hardware-independent
> > version, so that all SATA interfaces could benefit. ?Especially ata_piix,
> > but others too.
>
> BTW we can also save power by allowing the user to choose to disable
> hotplugging support. ?Then we can power down PHYs that are not in use.

Can't we power them up periodically to check for new hardware?

Regards
Oliver