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

[Niklas Cassel <cassel@kernel.org>]

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