Re: [PATCH v2] ahci: implement aggressive SATA device sleep support

[Jeff Garzik <jgarzik@pobox.com>]

On 08/24/2012 06:46 AM, Huang, Shane wrote:
> Jeff,
>
>> 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.
>
> Agreed, the info in that page is not limited to DevSlp variables.
> Should I also move it outside of the ata_dev_configure() function?
> Do you have better place to suggest?

ata_dev_configure() is the proper place.  Just move the log-read outside 
the ata_id_devslp test.

To avoid attempting to enable on older devices, you will need an 
appropriate test (ata_id_has_ncq perhaps?)


>> 3) please define constants in linux/ata.h for sata_settings information,
>> rather than using hexidecimal constants ("magic numbers").
>
> OK, I will define ATA_ID_FEATURE_SUPP to replace all the 78.

As you figured out in the other email, I was referring to sata_settings


>> 4) is it wise to issue SET_FEATURES / SATA_DEVSLP prior to programming
>> the host controller?  that order seems wrong.
>
> Per our understanding, there is no sequence requirement on this,
> please correct me if you see risk.

I just do not like programming the device, when power policy may 
indicate otherwise.

Most conservative is to leave devslp feature in reset state and not 
touch device or host until power policy dictates it is time to program 
host + device.

	Jeff