Re: [PATCH v2] ata: ahci: Do not enable LPM if no LPM states are supported by the HBA

[Damien Le Moal <dlemoal@kernel.org>]

On 6/19/24 00:28, Niklas Cassel wrote:
> LPM consists of HIPM (host initiated power management) and DIPM
> (device initiated power management).
> 
> ata_eh_set_lpm() will only enable HIPM if both the HBA and the device
> supports it.
> 
> However, DIPM will be enabled as long as the device supports it.
> The HBA will later reject the device's request to enter a power state
> that it does not support (Slumber/Partial/DevSleep) (DevSleep is never
> initiated by the device).
> 
> For a HBA that doesn't support any LPM states, simply don't set a LPM
> policy such that all the HIPM/DIPM probing/enabling will be skipped.
> 
> Not enabling HIPM or DIPM in the first place is safer than relying on
> the device following the AHCI specification and respecting the NAK.
> (There are comments in the code that some devices misbehave when
> receiving a NAK.)
> 
> Performing this check in ahci_update_initial_lpm_policy() also has the
> advantage that a HBA that doesn't support any LPM states will take the
> exact same code paths as a port that is external/hot plug capable.
> 
> Side note: the port in ata_port_dbg() has not been given a unique id yet,
> but this is not overly important as the debug print is disabled unless
> explicitly enabled using dynamic debug. A follow-up series will make sure
> that the unique id assignment will be done earlier. For now, the important
> thing is that the function returns before setting the LPM policy.
> 
> Fixes: 7627a0edef54 ("ata: ahci: Drop low power policy board type")
> Cc: stable@vger.kernel.org
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> Changes since v1: Add debug print as suggested by Mika.
> 
>  drivers/ata/ahci.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 07d66d2c5f0d..5eb38fbbbecd 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1735,6 +1735,14 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
>  	if (ap->pflags & ATA_PFLAG_EXTERNAL)
>  		return;
>  
> +	/* If no LPM states are supported by the HBA, do not bother with LPM */
> +	if ((ap->host->flags & ATA_HOST_NO_PART) &&
> +	    (ap->host->flags & ATA_HOST_NO_SSC) &&
> +	    (ap->host->flags & ATA_HOST_NO_DEVSLP)) {

Nit: Maybe:

#define ATA_HOST_NO_LPM		\
	(ATA_HOST_NO_PART | ATA_HOST_NO_SSC | ATA_HOST_NO_DEVSLP)

and then the if becomes:

	if ((ap->host->flags & ATA_HOST_NO_LPM) == ATA_HOST_NO_LPM) {

But no strong feelings about it. So:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

> +		ata_port_dbg(ap, "no LPM states supported, not enabling LPM\n");
> +		return;
> +	}
> +
>  	/* user modified policy via module param */
>  	if (mobile_lpm_policy != -1) {
>  		policy = mobile_lpm_policy;

-- 
Damien Le Moal
Western Digital Research