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

RunaGuo-oc
2022-04-22T09:58:34Z

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