On Fri, May 31, 2024 at 02:07:11PM +0200, 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.
>
> Fixes: 7627a0edef54 ("ata: ahci: Drop low power policy board type")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> ---
> We have not received any bug reports with this.
> The devices that were quirked recently all supported both Partial and
> Slumber.
> This is more a defensive action, as it seems unnecessary to enable DIPM
> in the first place, if the HBA doesn't support any LPM states.
>
> drivers/ata/ahci.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 07d66d2c5f0d..214de08de642 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1735,6 +1735,12 @@ 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))
For debugging purposes in case of potential issues, perhaps add a debug
log here so it is visible that we don't enable LPM?
> + return;
> +
> /* user modified policy via module param */
> if (mobile_lpm_policy != -1) {
> policy = mobile_lpm_policy;
> --
> 2.45.1
