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@xxxxxxxxxxxxxxx
> Signed-off-by: Niklas Cassel <cassel@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
> + 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
