Re: [PATCH] ata: ahci: print the lpm policy on boot

[Niklas Cassel <Niklas.Cassel@wdc.com>]

Hello Hannes,

On Wed, Sep 06, 2023 at 11:29:21AM +0200, Hannes Reinecke wrote:
> On 9/5/23 14:49, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> > 
> > The target LPM policy can be set using either a Kconfig or a kernel module
> > parameter.
> > 
> > However, if the board type is set to anything but board_ahci_low_power,
> > then the LPM policy will overridden and set to ATA_LPM_UNKNOWN.
> > 
> > Additionally, if the default suspend is suspend to idle, depending on the
> > hardware capabilities of the HBA, ahci_update_initial_lpm_policy() might
> > override the LPM policy to either ATA_LPM_MIN_POWER_WITH_PARTIAL or
> > ATA_LPM_MIN_POWER.
> > 
> > All this means that it is very hard to know which LPM policy a user will
> > actually be using on a given system.
> > 
> > In order to make it easier to debug LPM related issues, print the LPM
> > policy on boot.
> > 
> > One common LPM related issue is that the device fails to link up.
> > Because of that, we cannot add this print to ata_dev_configure(), as that
> > function is only called after a successful link up. Instead, add the info
> > using ata_port_desc(). The port description is printed once during boot.
> > 
> > Before changes:
> > ata1: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780100 irq 170
> > ata2: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780180 irq 170
> > 
> > After changes:
> > ata1: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780100 lpm-pol 4 irq 170
> > ata2: SATA max UDMA/133 abar m524288@0xa5780000 port 0xa5780180 lpm-pol 4 irq 170
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> >   drivers/ata/ahci.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> > index abb5911c9d09..541f6ec7f395 100644
> > --- a/drivers/ata/ahci.c
> > +++ b/drivers/ata/ahci.c
> > @@ -1898,6 +1898,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> >   			ap->em_message_type = hpriv->em_msg_type;
> >   		ahci_update_initial_lpm_policy(ap, hpriv);
> > +		ata_port_desc(ap, "lpm-pol %d", ap->target_lpm_policy);
> >   		/* disabled/not-implemented port */
> >   		if (!(hpriv->port_map & (1 << i)))
> 
> Hmm. Not very instructive ('pol 4' ?).
> Please use a string mapping here to translate it into something
> human-readable.

While we do have such a string mapping:
https://github.com/torvalds/linux/blob/v6.5/drivers/ata/libata-sata.c#L774-L781

I do think that printing such longs strings will spam dmesg more than the
current proposal.



Add to that that the the descriptions are extremely misleading... E.g.
min_power_with_partial and min_power does include dipm as well
(if the connected device supports it), even if dipm is not in the name..

Likewise, min_power_with_partial sounds like it is like min_power,
but additionally has support for partial, but no...
min_power has support for both slumber and partial, the difference is
that for min_power_with_partial aggressive link power management (ALPM)
will jump to partial instead of slumber when ALPM initiates the sleep.
However, since DIPM is enabled, if the device is in state Active, the
device is still perfectly capable of requesting a jump to slumber.

aggressive link power management (ALPM) is enabled for medium_power,
med_power_with_dipm, min_power_with_partial, and min_power.
(Since ALPM is enabled for both med_ and min_ states, one can wonder
why we don't have a med_power_with_partial state...)

Oh, and both min_power_with_partial and min_power will, in addition to
enabling ALPM, also enable devsleep, but only if the HBA, the specific port,
and the device supports it...

So this is a quite messy... but not something this patch tries to address.


I guess the problem is that you try to map:
-slumber, partial (which both have their own supported bit in the HBA's CAP reg)
 which can be initiated by:
 -the HBA if it supports ALPM, and ALPM is enabled
 -software, if told explicitly to jump to that state using PxCMD.ICC, but linux
  never does this, except before probe or revalidate.
 -the device (if it supports dipm, and dipm is enabled)
-devsleep, which needs support in HBA, port and device.
 -cannot be initiated by the device.
-automatic partial to slumber (apst) - appears to never be enabled by linux
-aggressive link power management (ALPM)

all to a single integer.


> Also one might consider a sysfs attribute to retrieve the information
> from userspace.

Yes, that is a very good idea (although out of scope for this patch).
For now, considering the vast amount of information libata already dumps to
dmesg on boot, and considering the issues we see quite often with LPM, e.g.:
https://bugzilla.kernel.org/show_bug.cgi?id=217114

I think it is helpful to also dump the lpm policy currently in use.

Because, usually people only upload dmesg, since that is where the "link up",
"link down", and link reset messages appear.

So I do think that adding the lpm policy to dmesg is helpful, regardless if
we eventually add a sysfs attribute for this as well.


Kind regards,
Niklas