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

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

[Niklas Cassel]

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)) {
+		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;
-- 
2.45.2

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

[Mario Limonciello]

On 6/18/2024 10: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.
> 

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

>   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)) {
> +		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;

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

[Mika Westerberg]

On Tue, Jun 18, 2024 at 05:28:29PM +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.
> 
> 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>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

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

[Damien Le Moal]

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

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

[Niklas Cassel]

On Wed, Jun 19, 2024 at 12:45:51PM +0900, Damien Le Moal wrote:
> 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>

Thank you for the R-b and your suggestion.

Personally, I do not think that your suggestion is significantly easier to
read than what is already there (especially with the comment to give
context).

My brain always has to read a:
if ((foo & bar) == bar)
twice anyway.

I guess a:
if (!ata_host_has_lpm(ap->host))
would be clearer, but considering that we wouldn't be able to use this
helper function anywhere else in the libata subsystem, I'm not sure if
it is worth it, so I will just apply this patch as is.


Kind regards,
Niklas

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

[Niklas Cassel]

On Tue, 18 Jun 2024 17:28:29 +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).
> 
> [...]

Applied to libata/linux.git (for-6.10-fixes), thanks!

[1/1] ata: ahci: Do not enable LPM if no LPM states are supported by the HBA
      https://git.kernel.org/libata/linux/c/fa997b05

Kind regards,
-- 
Niklas Cassel <cassel@kernel.org>