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

[Aaron Lu <aaron.lwe@gmail.com>]

On 08/23/2012 11:09 PM, Jeff Garzik wrote:
> 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.
> [...]
> 
>> @@ -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.

My understanding is that the devslp link power state will only be
entered when the host asserts the devslp signal(an out of band signal),
this may be done autonomously by the host or manually programmed by
software, while this patch cares about the former one.

The prerequisite for this would be the device's devslp feature being
already enabled, but enabling this feature for the device doesn't mean
the device wants to initiate a link power state transition.

So I think we can simply enable this feature while configuring this
device and the enter/exit of devslp is taken care of by ahci_set_lpm,
does this make sense?

Please correct me if I'm wrong, thanks.

-Aaron