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

[Jeff Garzik <jgarzik@pobox.com>]

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.

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