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

[RunaGuo-oc <RunaGuo-oc@zhaoxin.com>]

On 2022/4/21 18:53, Damien Le Moal wrote:
> 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

Sorry, here should use 'Add' instead of 'Adds'.

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

Ok.

>> 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;
> 		}

  
I'll change it like below,
+		if (ata_link_nr_enabled(link) > 0) {
-			/* no restrictions on LPM transitions */
+			/* Assume no restrictions on LPM transitions */
			scontrol &= ~(0x7 << 8);
  
+			/*
+			 * 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 {
			/* empty port, power off */
  			scontrol &= ~0xf;
		}