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

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

[Niklas Cassel]

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)))
-- 
2.41.0

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

[Damien Le Moal]

On 9/5/23 21: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

Looks good, but maybe print the lpm-pol value at the end, after the IRQ number,
to preserve the beginning of the message as it was before.

Or even better: why not print the LPM modes supported by the port and the target
lpm policy (lpm-pol) as a new ata_port_info() message right after the port desc
message ?

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

-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

Hello Damien,

On Wed, Sep 06, 2023 at 03:48:02PM +0900, Damien Le Moal wrote:
> On 9/5/23 21: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
> 
> Looks good, but maybe print the lpm-pol value at the end, after the IRQ number,
> to preserve the beginning of the message as it was before.
> 
> Or even better: why not print the LPM modes supported by the port and the target
> lpm policy (lpm-pol) as a new ata_port_info() message right after the port desc
> message ?

This print already exists:
[    1.515960] ahci 10000:e0:17.0: AHCI 0001.0301 32 slots 1 ports 6 Gbps 0x1 impl SATA mode
[    1.515963] ahci 10000:e0:17.0: flags: 64bit ncq sntf pm clo only pio slum part deso sadm sds

Which prints the capabilities of the HBA.

The only LPM capability per port, that seems to be able to differ from the
global HBA capability, is PxDEVSLP.DSP:

"
Device Sleep Present (DSP): If set to ‘1’, the platform supports Device Sleep
on this port. If cleared to ‘0’, the platform does not support Device Sleep on
this port. This bit may only be set to ‘1’ if CAP2.SDS is set to ‘1’.
"

E.g. on my machine, the HBA has support for DevSleep (supported features print
includes "sds"), but none of the ports seem to have support for DevSleep.

Would you like PxDEVSLP.DSP printed?




What would be interesting is to be able to know which features the connected
device supports, as e.g. DevSleep needs support both in HBA and the device,
but as the problem is that we usually don't get link up, this is information
is still unknown at this time.


Kind regards,
Niklas

> 
> > 
> > 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)))
> 
> -- 
> Damien Le Moal
> Western Digital Research
> 

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

[Damien Le Moal]

On 9/6/23 16:54, Niklas Cassel wrote:
> Hello Damien,
> 
> On Wed, Sep 06, 2023 at 03:48:02PM +0900, Damien Le Moal wrote:
>> On 9/5/23 21: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
>>
>> Looks good, but maybe print the lpm-pol value at the end, after the IRQ number,
>> to preserve the beginning of the message as it was before.
>>
>> Or even better: why not print the LPM modes supported by the port and the target
>> lpm policy (lpm-pol) as a new ata_port_info() message right after the port desc
>> message ?
> 
> This print already exists:
> [    1.515960] ahci 10000:e0:17.0: AHCI 0001.0301 32 slots 1 ports 6 Gbps 0x1 impl SATA mode
> [    1.515963] ahci 10000:e0:17.0: flags: 64bit ncq sntf pm clo only pio slum part deso sadm sds
> 
> Which prints the capabilities of the HBA.

Right.

> 
> The only LPM capability per port, that seems to be able to differ from the
> global HBA capability, is PxDEVSLP.DSP:
> 
> "
> Device Sleep Present (DSP): If set to ‘1’, the platform supports Device Sleep
> on this port. If cleared to ‘0’, the platform does not support Device Sleep on
> this port. This bit may only be set to ‘1’ if CAP2.SDS is set to ‘1’.
> "
> 
> E.g. on my machine, the HBA has support for DevSleep (supported features print
> includes "sds"), but none of the ports seem to have support for DevSleep.
> 
> Would you like PxDEVSLP.DSP printed?

We could.

> What would be interesting is to be able to know which features the connected
> device supports, as e.g. DevSleep needs support both in HBA and the device,
> but as the problem is that we usually don't get link up, this is information
> is still unknown at this time.

Yes. And if the link up is OK, devsleep capability is reported if supported by
the device.

OK. So let's just keep the lpm-pol message addition, but maybe move it at the
end of the message, after the IRQ info.

> 
> 
> Kind regards,
> Niklas
> 
>>
>>>
>>> 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)))
>>
>> -- 
>> Damien Le Moal
>> Western Digital Research

-- 
Damien Le Moal
Western Digital Research

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

[Hannes Reinecke]

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.
Also one might consider a sysfs attribute to retrieve the information
from userspace.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

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

[Niklas Cassel]

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