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

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

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
>