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 !0xffff test here.
--
tejun
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
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 !0xffff 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
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