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@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>
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
