Re: [patch 3/4] Enable link power management for ata drivers
Andrew, can you fold this into this patch
change to KERN_INFO for failure to set pm policy
Signed-off-by: Kristen Carlson Accardi [EMAIL PROTECTED]
---
drivers/ata/libata-scsi.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: 2.6-git/drivers/ata/libata-scsi.c
===
--- 2.6-git.orig/drivers/ata/libata-scsi.c 2007-08-13 09:40:18.0
-0700
+++ 2.6-git/drivers/ata/libata-scsi.c 2007-08-13 10:23:04.0 -0700
@@ -3009,7 +3009,7 @@ int ata_scsi_set_link_pm_policy(struct a
*/
if ((dev-horkage ATA_HORKAGE_IPM) ||
!(dev-flags ATA_DFLAG_IPM)) {
- ata_dev_printk(dev, KERN_ERR,
+ ata_dev_printk(dev, KERN_INFO,
Unable to set Link PM policy\n);
ap-pm_policy = MAX_PERFORMANCE;
}
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/4] Enable link power management for ata drivers
On Thu, 09 Aug 2007 14:24:16 PDT, Kristen Carlson Accardi said:
+++ 2.6-git/drivers/ata/libata-scsi.c
@@ -2904,6 +2976,52 @@ void ata_scsi_simulate(struct ata_device
+ if ((dev-horkage ATA_HORKAGE_IPM) ||
+ !(dev-flags ATA_DFLAG_IPM)) {
+ ata_dev_printk(dev, KERN_ERR,
+ Unable to set Link PM policy\n);
+ ap-pm_policy = MAX_PERFORMANCE;
+ }
KERN_INFO please, or KERN_WARNING at the highest, at least until such time
as enough drivers support enough hardware that it really *does* qualify for
this should not fail status.
(OK, so I'm just cranky because I'm tired of seeing a KERN_ERR thrown at every
reboot, just because the ata_piix driver doesn't know how to set this stuff up
for the DVD?RW drive in my laptop. But when this goes upstream, lots of
*other* people are going to get hit by the exact same thing and think there's
something actually *wrong* with their hardware.)
pgpqy1Uql0IBz.pgp
Description: PGP signature
Re: [patch 3/4] Enable link power management for ata drivers
Kristen Carlson Accardi wrote:
libata drivers can define a function (enable_pm) that will
perform hardware specific actions to enable whatever power
management policy the user set up from the scsi sysfs
interface if the driver supports it. This power management
policy will be activated after all disks have been
enumerated and intialized. Drivers should also define
disable_pm, which will turn off link power management, but
not change link power management policy.
Signed-off-by: Kristen Carlson Accardi [EMAIL PROTECTED]
Please update the patch against the current libata-dev#upstream and
there are several lines where tab follows space and git-applymbox isn't
happy about it. Those lines are in other patches too.
+int ata_scsi_set_link_pm_policy(struct Scsi_Host *shost,
+ enum scsi_host_link_pm policy)
+{
+ struct ata_port *ap = ata_shost_to_port(shost);
+ int rc = -EINVAL;
+ int i;
+
+ /*
+ * make sure no broken devices are on this port,
+ * and that all devices support interface power
+ * management
+ */
+ for (i = 0; i ATA_MAX_DEVICES; i++) {
+ struct ata_device *dev = ap-device[i];
+
+ /* only check drives which exist */
+ if (!ata_dev_enabled(dev))
+ continue;
+
+ /*
+ * do we need to handle the case where we've hotplugged
+ * a broken drive (since hotplug and ALPM are mutually
+ * exclusive) ?
+ *
+ * If so, if we detect a broken drive on a port with
+ * alpm already enabled, then we should reset the policy
+ * to off for the entire port.
+ */
+ if ((dev-horkage ATA_HORKAGE_ALPM) ||
+ !(dev-flags ATA_DFLAG_IPM)) {
+ ata_dev_printk(dev, KERN_ERR,
+ Unable to set Link PM policy\n);
+ ap-pm_policy = SHOST_MAX_PERFORMANCE;
+ }
+ }
+
+ if (ap-ops-enable_pm)
+ rc = ap-ops-enable_pm(ap, policy);
+
+ if (!rc)
+ shost-shost_link_pm_policy = ap-pm_policy;
+ return rc;
+}
+EXPORT_SYMBOL_GPL(ata_scsi_set_link_pm_policy);
This function is directly called from SCSI sysfs write, right? You
can't do this without synchronizing with EH. The best way is to define
an EH action and ask EH to transit between powersave states.
@@ -2021,6 +2021,9 @@ int ata_dev_configure(struct ata_device
if (dev-flags ATA_DFLAG_LBA48)
dev-max_sectors = ATA_MAX_SECTORS_LBA48;
+ if (ata_id_has_hipm(dev-id) || ata_id_has_dipm(dev-id))
+ dev-flags |= ATA_DFLAG_IPM;
Is it safe to use ALPM on a device which only claims to support DIPM?
@@ -2046,6 +2049,13 @@ int ata_dev_configure(struct ata_device
dev-max_sectors = min_t(unsigned int, ATA_MAX_SECTORS_128,
dev-max_sectors);
+ if (ata_device_blacklisted(dev) ATA_HORKAGE_ALPM) {
+ dev-horkage |= ATA_HORKAGE_ALPM;
I think this should be ATA_HORKAGE_IPM.
@@ -6385,6 +6426,8 @@ int ata_host_register(struct ata_host *h
struct ata_port *ap = host-ports[i];
ata_scsi_scan_host(ap);
+ ata_scsi_set_link_pm_policy(ap-scsi_host,
+ ap-pm_policy);
Again, this should be done from EH.
@@ -321,6 +321,8 @@ struct ata_taskfile {
((u64) (id)[(n) + 0]) )
#define ata_id_cdb_intr(id) (((id)[0] 0x60) == 0x20)
+#define ata_id_has_hipm(id) ((id)[76] (1 9))
+#define ata_id_has_dipm(id) ((id)[78] (1 3))
We probably need !0x test here.
--
tejun
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/4] Enable link power management for ata drivers
On 8/1/07, Tejun Heo [EMAIL PROTECTED] wrote: + if (ata_id_has_hipm(dev-id) || ata_id_has_dipm(dev-id)) + dev-flags |= ATA_DFLAG_IPM; Is it safe to use ALPM on a device which only claims to support DIPM? I have tested on a Dell Inspiron 6400, it has ICH7-M (82801GBM) chipset. The harddisk claims to support only DIPM, and the ALPM patches work with it. Kristen said either DIPM or HIPM will work with ALPM accord to the spec, see this email: http://www.bughost.org/pipermail/power/2007-June/000691.html --Edwin - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/4] Enable link power management for ata drivers
On Wed, 01 Aug 2007 17:27:39 +0900
Tejun Heo [EMAIL PROTECTED] wrote:
Kristen Carlson Accardi wrote:
snippy
Is it safe to use ALPM on a device which only claims to support DIPM?
Yes - I doubled checked this with the AHCI people - and of course you
have Edvin's testing to prove it does fine.
I think this should be ATA_HORKAGE_IPM.
OK - I changed it.
@@ -321,6 +321,8 @@ struct ata_taskfile {
((u64) (id)[(n) + 0]) )
#define ata_id_cdb_intr(id)(((id)[0] 0x60) == 0x20)
+#define ata_id_has_hipm(id)((id)[76] (1 9))
+#define ata_id_has_dipm(id)((id)[78] (1 3))
We probably need !0x test here.
Thanks, I fixed that too.
As far as moving the enable/disable_pm calls to EH - can you take
a look at the other patch I sent which implements the shost_attrs
to see if I still need to do this? I really don't know much about
the EH stuff - can you explain why we need to use it to set the
link pm?
thanks for your review,
Kristen
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/4] Enable link power management for ata drivers
Kristen Carlson Accardi wrote: On Wed, 01 Aug 2007 17:27:39 +0900 Tejun Heo [EMAIL PROTECTED] wrote: Kristen Carlson Accardi wrote: snippy Is it safe to use ALPM on a device which only claims to support DIPM? Yes - I doubled checked this with the AHCI people - and of course you have Edvin's testing to prove it does fine. Alright. As far as moving the enable/disable_pm calls to EH - can you take a look at the other patch I sent which implements the shost_attrs to see if I still need to do this? I really don't know much about the EH stuff - can you explain why we need to use it to set the link pm? Unfortunately, yes, you still need to. Only two threads of execution (one is not a real thread tho) are allowed to access a libata port - command execution and EH, and the two are mutually exclusive. Invoking something from the outside is done by doing the following. 1. recording what to do in ehi-[dev_]action, ap-pflags or dev-flags 2. schedule EH by calling either ata_port_schedule_eh(), ata_port_abort() or ata_port_freeze(). The first one waits for the currently in-flight commands to finish before entering EH. The second one aborts all in-flight commands and enters EH. The third one aborts all commands and freezes the port and enters EH. 3. wait for EH to finish by calling ata_port_wait_eh(). This achieves correct synchronization and other EH functionalities can be easily used. e.g. Resuming requires resetting the bus and revalidating the attached devices, so the resume handler can just request such actions together. For link PS, I think it would probably be a good idea to revalidate after mode change to make sure the device works in the new mode. If revalidation fails, it can reset and back off. EH is done in three large steps - autopsy, report and recover. To implement an action, the 'recover' stage needs to be extended. It basically comes down to hooking the enable/disable functions into the right places in ata_eh_recover(). Unconditionally disabling link PS prior to reset and enabling it back again before revalidation would be a pretty good choice, but haven't thought about it too hard so take it with a grain of salt. I'm not sure whether it would be necessary now but it would be nice to have a proper recovery logic later. e.g. If more than certain number of ATA bus occurs in certain mount of time, disable link PS. This kind of logic is used during autopsy to determine whether stepping down link/transfer speed is needed. Please take a look at ata_eh_speed_down(). It might be enough to piggy back on ata_eh_speed_down() tho such that the first step of speeding down is turning off link PS. Hope the brief introduction to libata-EH-hacking helps. -- tejun - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/4] Enable link power management for ata drivers
On Thu, 5 Jul 2007 13:05:30 -0700 Kristen Carlson Accardi [EMAIL PROTECTED] wrote: + ATA_DFLAG_IPM = (1 6), /* device supports interface PM */ ATA_DFLAG_CFG_MASK = (1 8) - 1, I had to bump this to (17), so we've run out. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/4] Enable link power management for ata drivers
On Thu, 5 Jul 2007 15:33:34 -0700
Andrew Morton [EMAIL PROTECTED] wrote:
On Thu, 5 Jul 2007 13:05:30 -0700
Kristen Carlson Accardi [EMAIL PROTECTED] wrote:
+ ATA_DFLAG_IPM = (1 6), /* device supports interface PM */
ATA_DFLAG_CFG_MASK = (1 8) - 1,
I had to bump this to (17), so we've run out.
err, no, we've more than run out because you AN patches took the last one.
I guess we can bump ATA_DFLAG_CFG_MASK up to 12, like this?
---
a/include/linux/libata.h~ata-ahci-alpm-enable-link-power-management-for-ata-drivers
+++ a/include/linux/libata.h
@@ -140,11 +140,12 @@ enum {
ATA_DFLAG_ACPI_PENDING = (1 5), /* ACPI resume action pending */
ATA_DFLAG_ACPI_FAILED = (1 6), /* ACPI on devcfg has failed */
ATA_DFLAG_AN= (1 7), /* device supports Async
notification */
- ATA_DFLAG_CFG_MASK = (1 8) - 1,
+ ATA_DFLAG_IPM = (1 8), /* device supports interface PM */
+ ATA_DFLAG_CFG_MASK = (1 12) - 1,
- ATA_DFLAG_PIO = (1 8), /* device limited to PIO mode */
- ATA_DFLAG_NCQ_OFF = (1 9), /* device limited to non-NCQ mode */
- ATA_DFLAG_SPUNDOWN = (1 10), /* XXX: for spindown_compat */
+ ATA_DFLAG_PIO = (1 12), /* device limited to PIO mode */
+ ATA_DFLAG_NCQ_OFF = (1 13), /* device limited to non-NCQ mode
*/
+ ATA_DFLAG_SPUNDOWN = (1 14), /* XXX: for spindown_compat */
ATA_DFLAG_INIT_MASK = (1 16) - 1,
ATA_DFLAG_DETACH= (1 16),
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/4] Enable link power management for ata drivers
On Thu, 05 Jul 2007 20:02:08 -0400 Jeff Garzik [EMAIL PROTECTED] wrote: May I assume that I may delete the patches from Kristen, and assume that you will resend an updated version of her AN and ALPM patches to me? Sure. But I have a sneaking feeling that Kristen sneaks sneaky fixes into her patches without telling me, so I won't be 100% confident about their uptodateness (hint). - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/4] Enable link power management for ata drivers
Andrew Morton wrote: On Thu, 5 Jul 2007 13:05:30 -0700 Kristen Carlson Accardi [EMAIL PROTECTED] wrote: + ATA_DFLAG_IPM = (1 6), /* device supports interface PM */ ATA_DFLAG_CFG_MASK = (1 8) - 1, I had to bump this to (17), so we've run out. You can shuffle the numbers a bit, as long as the masks (*_MASK) stay correct for their purpose Jeff - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 3/4] Enable link power management for ata drivers
Andrew Morton wrote: I guess we can bump ATA_DFLAG_CFG_MASK up to 12, like this? Yep Jeff - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
