Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
IronPort-SDR: HqtdZ8A9iiaP9/LN+LREO2zvcCWuKB1+SXgmKRzUa2Ibks+uaqWpN0KUoTjXovBGQFxCR+3OTl
 uvLjAadFwcVHFxdgUXGsDoM/Kq/4DT6ne6apWmoZlfS+n0IRZPvvudNNQeqTYk1gbPIv1aHy77
 teusf+Mx0ekdwEn31rIXQNOvS/Rdvxzln+UMH/VQtqjUDakJDdCF+EerpVL6eBsY/8ez1ZPaLs
 HmSr3fr2kLqQbKjLLrvcp6IbjItYZ3pflo7EbLFMtQsHpx0Rp7m5v5xbneHjsD/qtKf+yEuQuO
 nx1R0y3JXJF2Wj/DqORyqMdt
IronPort-SDR: kvv/E00mRuyhNNa/D+W2Fcs3vTLKuh0qLFC8m5WvMRQ0ZM/WksGqhtUah+1AW4t9N0MD2YXf3U
 bxZSS0S866qSIEPWU7NPQKVZ2KceDejvhyy+dvDqgkUlQ4TrDgXuA1uc2UHsbZ29uGHeSNo5Ra
 Mnh4WUlJuM/SUh/bVxCsopSzlz5R2ta8BQ1j580N4knYUFzweykAu4wDrckR0ZItUzhKFTju3N
 krKDeAoeOPus74f93LD1Dlao0WFWwQZL6FxEyitur842g6dIWILbCh7bi8zg5r5A3cqUqafDMh
 ynQ=
WDCIronportException: Internal
Message-ID: <2a91c02d-4f7c-535b-e047-889df87ebebd@opensource.wdc.com>
Date:   Thu, 21 Apr 2022 19:53:40 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH 2/2] ahci: Add PxSCTL.IPM control on actual LPM capability
Content-Language: en-US
To:     Runa Guo-oc <RunaGuo-oc@zhaoxin.com>, linux-ide@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1650534217-14052-1-git-send-email-RunaGuo-oc@zhaoxin.com>
 <1650534217-14052-3-git-send-email-RunaGuo-oc@zhaoxin.com>
From:   Damien Le Moal <damien.lemoal@opensource.wdc.com>
Organization: Western Digital Research
In-Reply-To: <1650534217-14052-3-git-send-email-RunaGuo-oc@zhaoxin.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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