[Bug 218198] Suspend/Resume Regression with attached ATA devices

[bugzilla-daemon@kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=218198

--- Comment #18 from Niklas.Cassel@wdc.com ---
Hello Dieter,


I'm really sorry for missing your reply.
The updates from bugzilla were not set to the libata mailing list
(only to the scsi mailing list).

Please consider writing to the libata mailing list instead of using
bugzilla, I'm quite sure that you will get more eyes on your problem
that way.

On Wed, Nov 29, 2023 at 06:10:20PM +0000, bugzilla-daemon@kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=218198
> 
> --- Comment #7 from Dieter Mummenschanz (dmummenschanz@web.de) ---
>  (In reply to Niklas.Cassel from comment #6)
> 
> Hello Niklas,
> 
> thanks for looking into this.
> 
> > It would be nice if you could test with latest v6.7-rcX.
> 
> Okay I took the latest 6.7-rc3 from Linux's git and included your patch.
> Booted
> up my Laptop, put it back to sleep and brought it up again. See attached log.
> 
> I've disabed my link_power_management_policy override so all the time my
> system
> was stuck at pc2 package state according to powertop.
> 
> > For devsleep to get enabled, you need "sadm sds" in the SATA controller
> 
> Yep I can see that:
> [    0.379531] ahci 0000:00:17.0: flags: 64bit ncq sntf stag pm clo only pio
> slum part ems sxs deso sadm sds apst

Ok, good 1/4.

> 
> > and "Dev-Sleep" in the SATA device print.
> 
> I guess this is it?
> [    1.031169] ata5.00: Features: Trust Dev-Sleep NCQ-sndrcv

Yes, good, 2/4.

> 
> > Additionally, your lpm-policy (lpm-pol) has to be either ATA_LPM_MIN_POWER
> or
> > ATA_LPM_MIN_POWER_WITH_PARTIAL (i.e. lpm-pol has to print either 4 or 5).
> 
> I'm afraid this is not the case here:
> [    0.399005] ata1: SATA max UDMA/133 abar m2048@0x42233000 port 0x42233100
> irq 125 lpm-pol 0

It appears that you have built your kernel with:
CONFIG_SATA_MOBILE_LPM_POLICY=0

All major distros use:
CONFIG_SATA_MOBILE_LPM_POLICY=3

Could you please try with CONFIG_SATA_MOBILE_LPM_POLICY=3 ?

If you build with CONFIG_SATA_MOBILE_LPM_POLICY=0, then set_lpm() in libata
will never be called, which means that we will never enable (or disable
devsleep), so you will use whatever your boot firmware has configured.
(And boot firmware might have enabled devsleep in the device.)



In your v6.7-rcX dmesg, we don't see any:
"port does not support device sleep" or
"setting devsleep to %d" (from my debug patch)
prints in your dmesg from v6.7-rcX, so set_lpm() is never called at all,
most likely because you have built with CONFIG_SATA_MOBILE_LPM_POLICY=0,
so DevSleep could be enabled if platform firmware enabled it.



In your v6.6 dmesg, we see:
"port does not support device sleep"
(but not "setting devsleep to %d" from my debug patch, so you
probably forgot to apply it to your v6.6 kernel).

However, the fact that you see "port does not support device sleep"
suggests that you either built your kernel with
CONFIG_SATA_MOBILE_LPM_POLICY set to something other than 0,
or you have manually overriden the policy via sysfs.

Note that since your platform firmware claims that the port does
not support DevSleep (PxDEVSLP.DSP is set to 0), this means that
ahci_set_aggressive_devslp() will return early:
https://github.com/torvalds/linux/blob/v6.6/drivers/ata/libahci.c#L2258-L2262
So it will neither enable nor disable DevSleep in the device,
so DevSleep could be enabled if platform firmware enabled it.


In both of your cases, it looks like you should have DevSleep set
to whatever platform firmware has set it to.
But you say that, for these logs, v6.6 can enter low CPU power states,
but v6.7-rcX can not?


You can run:
hdparm -I /dev/<your_device>
to see if DEVSLP is enabled in your device (hdparm prints a * in front
of the feature if the feature is enabled).

It could also be worth checking your BIOS, I've seen some cases where you had
to enable aggressive devsleep in BIOS for it to get enabled for the port.


> 
> > Note that even if you have LPM_POLICY=3 (ATA_LPM_MED_POWER_WITH_DIPM) in
> your
> > Kconfig, ahci_update_initial_lpm_policy() will possibly override this by
> > default
> 
> That would explain why I'm seeing "max_performance" in
> link_power_management_policy without overriding it.

The sysfs reporting for LPM is really broken in libata...

$ git grep max_performance drivers/ata/libata-sata.c
drivers/ata/libata-sata.c:      [ATA_LPM_UNKNOWN]               =
"max_performance",
drivers/ata/libata-sata.c:      [ATA_LPM_MAX_POWER]             =
"max_performance",

So it reports "max_performance" both for LPM_POLICY == 0 (ATA_LPM_UNKNOWN)
and LPM_POLICY == 1 (ATA_LPM_MAX_POWER).

In your case, it is because of ATA_LPM_UNKNOWN, which means use whatever
platform firmware has configured.

ahci_update_initial_lpm_policy() will only override your policy if your
LPM_POLICY >= 3, so it will not override it for your case.


> > I would really not recommend you doing this, because when you force set
> > lpm policy via sysfs, ahci_update_initial_lpm_policy() is not called,
> > so if your platform requires ATA_LPM_MIN_* to enter lower power states,
> > you forcing lpm-policy to ATA_LPM_MED_POWER_WITH_DIPM will ensure that
> > you never enter lower power states.
> 
> I understand however I do not know of any other way to enable lower package
> states on my machine.

I do not understand why that helps you.

If you set:
ATA_LPM_MED_POWER_WITH_DIPM (LPM_POLICY=3) on v6.6 in sysfs,
you can enter low CPU power states?

If you set 
CONFIG_SATA_MOBILE_LPM_POLICY=3 on v6.6,
can you enter low CPU power states?

Looking at AHCI 1.3.1
PM:Aggr:

State PM:DevSleep is gated with:

PxDEVSLP.ADSE = ‘1’ and CAP2.SDS = ‘1’ and CAP2.SADM
= ‘1’ and PxDEVSLP.DSP = 1’ and PxSCTL.IPM != ‘4h’ and
PxSCTL.IPM != ‘5h’ and PxSCTL.IPM != ‘6h’ and PxSCTL.IPM
!= ‘7h’ and CAP2.DESO = ‘0’ and pDitoTimeout = ‘1’

So AFAICT, ALPM by the HBA should never be able to transition
to DevSleep if PxDSP.DSP == 0.

Perhaps your platform actually does NOT support devsleep and
simply requires the device to enter slumber or partial in order
to enter the lower CPU power states?

We could test this by you disabling devslp on the device via hdparm,
and see if you can still enter the lower CPU power states.


Kind regards,
Niklas

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.