Re: [PATCH 2/2] ahci: Add PxSCTL.IPM control on actual LPM capability

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 4/21/22 18:43, Runa Guo-oc wrote:
> On some platform, when OS enables LPM by default (eg, min_power),
> then, DIPM slumber request cannot be disallowed if ahci's CAP.PSC
> is set to '1' and CAP.SSC is cleared to '0', which may cause ahci
> to be an uncertain state (same for Partial).
> 
> In ahci spec, when CAP.PSC/SSC is cleared to '0', the PxSCTL.IPM
> field must be programmed to disallow device initiated Partial/
> Slumber request.
> 
> Adds support to control this case on actual LPM capability.

s/Adds/Add

Overall, I need to reread the specs to confirm all this.

> 
> Signed-off-by: Runa Guo-oc <RunaGuo-oc@zhaoxin.com>
> ---
>  drivers/ata/libata-sata.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index 7a5fe41..e6195cf 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -394,9 +394,19 @@ int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy,
>  	case ATA_LPM_MED_POWER_WITH_DIPM:
>  	case ATA_LPM_MIN_POWER_WITH_PARTIAL:
>  	case ATA_LPM_MIN_POWER:
> -		if (ata_link_nr_enabled(link) > 0)
> +		if (ata_link_nr_enabled(link) > 0) {
>  			/* no restrictions on LPM transitions */>  			scontrol &= ~(0x7 << 8);

Given that the added code below adds restrictions, this comment is
strange. Better change it to something like:

			/* Assume no restrictions on LPM transitions */

> +
> +			/* if controller does not support partial, then disallows it,
> +			 * the same for slumber
> +			 */

Please correctly format the comment and check the grammar. Some like below
is easier to read.

			/*
			 * If the controller does not support partial or
			 * slumber then disallow these transitions.
			 */

> +			if (!(link->ap->host->flags & ATA_HOST_PART))
> +				scontrol |= (0x1 << 8);
> +
> +			if (!(link->ap->host->flags & ATA_HOST_SSC))
> +				scontrol |= (0x2 << 8);
> +		}
>  		else {

Please do not leave this else here. Put it on the same line as the closing
bracket and enclose the below statements in brackets too.

>  			/* empty port, power off */
>  			scontrol &= ~0xf;

		} else {
			/* empty port, power off */
 			scontrol &= ~0xf;
		}


-- 
Damien Le Moal
Western Digital Research