Re: [PATCH v2] ahci: implement aggressive SATA device sleep support
From: Jeff Garzik <hidden>
Date: 2012-08-23 15:09:44
On 08/23/2012 06:31 PM, Shane Huang wrote:
This patch enables Aggressive Device Sleep only if both host controller and device support it.
[...]
quoted hunk ↗ jump to hunk
@@ -2323,6 +2324,26 @@ int ata_dev_configure(struct ata_device *dev) } } + if (ata_id_has_devslp(dev->id)) { + err_mask = ata_dev_set_feature(dev, + SETFEATURES_SATA_ENABLE, + SATA_DEVSLP); + if (err_mask) + ata_dev_err(dev, + "failed to enable DEVSLP (err_mask=0x%x)\n", + err_mask); + else { + dev->flags |= ATA_DFLAG_DEVSLP; + err_mask = ata_read_log_page(dev, + ATA_LOG_SATA_SETTINGS, + dev->sata_settings, + 1); + if (err_mask) + ata_dev_warn(dev, + "failed to get Identify Device Data\n"); + } + } + dev->cdb_len = 16; }
1) Contra the patch description (quoted above), you are enabling
SATA_DEVSLP on the device regardless of power policy or host controller
support.
I'm not sure about the ata_dev_configure() change. If the power policy
is not enabling aggressive sleep, we should not send this command to the
device.
2) If we are going to unconditionally add ATA_SECT_SIZE bytes to every
ata_device structure, let's at least move the ata_read_log_page() call
outside of the devslp test, so that others may have this information
even if the device does not support devslp.
3) please define constants in linux/ata.h for sata_settings information,
rather than using hexidecimal constants ("magic numbers").
4) is it wise to issue SET_FEATURES / SATA_DEVSLP prior to programming
the host controller? that order seems wrong.
Jeff