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

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

[Niklas Cassel]

On Tue, Nov 28, 2023 at 08:24:01AM +0000, bugzilla-daemon@kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=218198
> 
> --- Comment #2 from Dieter Mummenschanz (dmummenschanz@web.de) ---
> Thanks for the swift reply.
> 
> I've applied your patch. Booted up my machine and waited until it transitions
> into lower package states (pc8 at the lowest). After that I closed the laptop
> LID and let the machine suspend to RAM (S3). After that I reopened the LID and
> gave the machine 1-3 minutes time to transition to lower package states which
> it now does.
> I've attached the dmesg part including your patch when the machine enters
> suspend. One thing is odd though:
> 
> [  109.424369] ata5.00: qc timeout after 5000 msecs (cmd 0xe0)
> [  109.424397] ata5.00: STANDBY IMMEDIATE failed (err_mask=0x4)
> 
> this shouldn't be there, right?
>

Hello Dieter,


I took a look at your logs, but they are very stripped.

It would be nice if you could test with latest v6.7-rcX.

And then provide these messages at boot:

[   50.101909] ahci 0000:00:17.0: flags: 64bit ncq sntf pm led clo only pio slum part ems deso sadm sds apst 
[   50.375109] ata10: SATA max UDMA/133 abar m524288@0xa5700000 port 0xa5700480 irq 270 lpm-pol 4
[   50.783496] ata10.00: Features: Dev-Sleep NCQ-sndrcv NCQ-prio


Because, for devsleep to be enabled, you need support in:
1) The SATA device
2) The SATA controller
3) The SATA port (even if the controller supports devsleep,
                  not all SATA ports have to support it)



For devsleep to get enabled, you need "sadm sds" in the SATA controller print,
and "Dev-Sleep" in the SATA device print.
Unfortunately, there does not seem to be a print for the port.

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


> Regarding automatic transitioning I'm not sure how this works. However even
> though I've set CONFIG_SATA_MOBILE_LPM_POLICY=3 in the kernel config, I have to
> call an init script explicitly forcing the scsi host to use low power when
> idle:

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
to either ATA_LPM_MIN_POWER_WITH_PARTIAL or ATA_LPM_MIN_POWER, see:
https://github.com/torvalds/linux/blob/master/drivers/ata/ahci.c#L1639

The best way is to show the:
[   50.375109] ata10: SATA max UDMA/133 abar m524288@0xa5700000 port 0xa5700480 irq 270 lpm-pol 4
print as this is printed after ahci_update_initial_lpm_policy().

The reason why many platforms do this override, is because:
"One of the requirement for modern x86 system to enter lowest power mode
(SLP_S0) is SATA IP block to be off. This is true even during when
platform is suspended to idle and not only in opportunistic (runtime)
suspend."

"SATA IP block doesn't get turned off till SATA is in DEVSLP mode. Here
user has to either use scsi-host sysfs or tools like powertop to set
the sata-host link_power_management_policy to min_power."

See:
https://github.com/torvalds/linux/commit/b1a9585cc396cac5a9e5a09b2721f3b8568e62d0


> 
> for foo in /sys/class/scsi_host/host*/link_power_management_policy;
>   do echo med_power_with_dipm > $foo;
> done

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.


Looking at your logs, we see:
[ 2022.700556] ahci 0000:00:17.0: port does not support device sleep

Which comes from:
https://github.com/torvalds/linux/blob/master/drivers/ata/libahci.c#L2260

So it appears that your port does not support devsleep...

PxDEVSLP.DSP (in AHCI specification) is the bit that determines if devsleep
is supported for a specific port. This bit is initialized by BIOS.

So this could be a BIOS bug...
But you said that it works if you revert Damien's patch...



So the question is, is PxDEVSLP.DSP always 0?
(Even on the first boot, before you have done any suspend/resume).
If so, it could be a BIOS bug...

Perhaps you could test this patch:

And show us all the prints, and tell us which prints are before/after
the suspend/resume.

--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2255,6 +2255,9 @@ static void ahci_set_aggressive_devslp(struct ata_port *ap, bool sleep)
        int rc;
        unsigned int err_mask;
 
+       dev_info(ap->host->dev, "setting devsleep to: %d port support %d\n",
+                sleep, readl(port_mmio + PORT_DEVSLP));
+
        devslp = readl(port_mmio + PORT_DEVSLP);
        if (!(devslp & PORT_DEVSLP_DSP)) {
                dev_info(ap->host->dev, "port does not support device sleep\n");



You mentioned that it works when you revert Damien's patch.
It could be interesting to see these prints, before/after the
suspend/resume both with and without the revert.

I would expect us to be able to read PxDEVSLP even when the SATA device
is in suspend state... I could imagine that we could get a bogus value
back from the read if the from the SATA controller itself is in a suspend
state, but I don't see how Damien's patch that you bisected to:
https://github.com/torvalds/linux/commit/fd3a6837d8e18cb7be80dcca1283276290336a7a

changed any of that. It does touch ata_pci_shutdown_one(), which should
only get called on shutdown... not suspend/resume AFAICT.

So the fact that this patch changes things for you is weird in the first
place. Damien, is it possible that ata_pci_shutdown_one() is incorrectly
called during suspend/resume? Got any ideas?


Kind regards,
Niklas

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

[Phillip Susi]

Niklas Cassel <Niklas.Cassel@wdc.com> writes:

> The reason why many platforms do this override, is because:
> "One of the requirement for modern x86 system to enter lowest power mode
> (SLP_S0) is SATA IP block to be off. This is true even during when
> platform is suspended to idle and not only in opportunistic (runtime)
> suspend."
>
> "SATA IP block doesn't get turned off till SATA is in DEVSLP mode. Here
> user has to either use scsi-host sysfs or tools like powertop to set
> the sata-host link_power_management_policy to min_power."

What?  I'm confused.  Here are several things that come to mind that
when taken together with this statement confuse me.  Perhaps I am wrong
about one or more of them?

1)  DEVSLP is an odd thing they invented to have ACPI twiggle a GPIO bit
to cut the power rails to CDROM drives that aren't in active use.

2)  SATA ALPM has the SATA controller automatically transition the sata
link to lower power states when it isn't being used, and back again on
use.

3)  The SATA controller can not be suspended until all of its ports are
suspended.

4)  Suspending a SATA port does not happen during ALPM, but rather only
either during system suspend, or when you enable runtime pm on the disk
and the ata port in sysfs ( both are disabled by default ).

A quick google led me to this: https://smarthdd.com/device_sleep.htm

Which indicates that this DEVSLP thing is now being used to send an OOB
signal to a SATA SSD so that it can power down its PHY completely, but
isn't that what ATA SLEEP is for?  aka. hdparm -Y.  That command tells
the drive that it can power down its PHY and then the host can power
down its PHY and it takes a sata link reset to wake the drive back up.
The link reset is easy to detect with minimum circuitry that is far
simpler than the full sata PHY.

It sounds like somebody decided to hack an OOB signal into ALPM rather
than use ATA SLEEP, but why?

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

[Niklas Cassel]

On Wed, Nov 29, 2023 at 01:47:16PM -0500, Phillip Susi wrote:
> Niklas Cassel <Niklas.Cassel@wdc.com> writes:
> 
> > The reason why many platforms do this override, is because:
> > "One of the requirement for modern x86 system to enter lowest power mode
> > (SLP_S0) is SATA IP block to be off. This is true even during when
> > platform is suspended to idle and not only in opportunistic (runtime)
> > suspend."
> >
> > "SATA IP block doesn't get turned off till SATA is in DEVSLP mode. Here
> > user has to either use scsi-host sysfs or tools like powertop to set
> > the sata-host link_power_management_policy to min_power."

Note that I copied this text from the original patch authored by someone
at Intel.

I've seen similar claims from people at AMD.


> 
> What?  I'm confused.  Here are several things that come to mind that
> when taken together with this statement confuse me.  Perhaps I am wrong
> about one or more of them?
> 
> 1)  DEVSLP is an odd thing they invented to have ACPI twiggle a GPIO bit
> to cut the power rails to CDROM drives that aren't in active use.
> 
> 2)  SATA ALPM has the SATA controller automatically transition the sata
> link to lower power states when it isn't being used, and back again on
> use.

Yes, the HBA can initiate a transition to DevSleep automatically using ALPM,
however, the specs says that:
-DevSleep is disabled by the device on power up (and has to be enabled by a
SET FEATURES command).
-PxDEVSLP.ADSE (Aggressive Device Sleep Enable) for the port is 0 on reset
(the kernel resets the HBA on boot, so ADSE will be disabled for the port).

(DevSleep is never initiated by the device.)

The kernel will only do:
-SET FEATURES to enable the DevSleep feature on the device, and
-Set PxDEVSLP.ADSE to 1

If _all_ of the following are true:
1) SADM and SDS bits are set in the HBA
2) the device reports that it supports DevSleep
3) PxDEVSLP.DSP is set for the port
4) the kernel LPM policy is either ATA_LPM_MIN_POWER or
   ATA_LPM_MIN_POWER_WITH_PARTIAL

See:
https://github.com/torvalds/linux/blob/v6.7-rc3/drivers/ata/libahci.c#L2322-L2324


This means that if any of 1-5 it not true, the HBA ALPM will never transition
the device into DevSleep state.

AFAICT, the HBA asserts the DEVSLP signal as long as being in DevSleep state.

To exit from DevSleep state you can just issue an I/O like normal using PxCI,
or write to PxCMD.ICC to force a state change.


> 3)  The SATA controller can not be suspended until all of its ports are
> suspended.
> 
> 4)  Suspending a SATA port does not happen during ALPM, but rather only
> either during system suspend, or when you enable runtime pm on the disk
> and the ata port in sysfs ( both are disabled by default ).
> 
> A quick google led me to this: https://smarthdd.com/device_sleep.htm
> 
> Which indicates that this DEVSLP thing is now being used to send an OOB
> signal to a SATA SSD so that it can power down its PHY completely, but
> isn't that what ATA SLEEP is for?  aka. hdparm -Y.  That command tells
> the drive that it can power down its PHY and then the host can power
> down its PHY and it takes a sata link reset to wake the drive back up.
> The link reset is easy to detect with minimum circuitry that is far
> simpler than the full sata PHY.
> 
> It sounds like somebody decided to hack an OOB signal into ALPM rather
> than use ATA SLEEP, but why?

I guess you need to ask Intel.

I assume that their firmware simply requires the DEVSLP signal to be
asserted in order to enter lower CPU power states.

If you have a signal, it is easy for the HW logic to detect if the signal
is set or not.

If you just send a command to the device, if it not easy for HW logic
to determine which state is in. It would need to read some registers
or similar. Sounds way more complex than just having a logic gate.


Kind regards,
Niklas

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

[Niklas Cassel]

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

Suspend/Resume Regression with attached ATA devices

[Niklas Cassel]

Hello Dieter,

On Mon, Dec 18, 2023 at 10:18:43AM +0000, bugzilla-daemon@kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=218198
> 
> --- Comment #19 from Dieter Mummenschanz (dmummenschanz@web.de) ---
> Hallo Niklas,
> 
> thanks for getting back to me on that.
> 
> > 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.
> 
> Could point me to that mailing list please. I can't seem to find it. Do you
> want me to repost the issue there or is it okay to just copy & past the
> messages from this bug?

I'm truly sorry for not replying to you sooner.
For some reason my corporate email managed to put all emails from bugzilla in
the spam folder.

The proper mailing list is:
linux-ide@vger.kernel.org

I've added it to this email. You can just reply all to this email, no need to
describe your problem again.

For what it is worth, Damien has sent out a series that reverts
the commit that you managed to bisect your regression to:
https://lore.kernel.org/linux-ide/20240111115123.1258422-1-dlemoal@kernel.org/T/#t

Your help testing the series would be much appreciated.


> 
> 
> > It appears that you have built your kernel with:
> > CONFIG_SATA_MOBILE_LPM_POLICY=0
> 
> 
> Definately not! I've attached my kernel config. LPM policy is definately set tp
> "3" CONFIG_SATA_MOBILE_LPM_POLICY=3. Thats puzzles me because after booting the 
> link_power_management_policy is set to performance. Somehow the kernel
> overrides the config or uses parameters suggested by the BIOS I don't know.
> I've also tried to to switch the ASPM polity to Powersave instead of BIOS
> default. Didn't help.

I see. You have built your kernel with CONFIG_SATA_MOBILE_LPM_POLICY=3,
however we still see:
ata5: SATA max UDMA/133 abar m2048@0x42233000 port 0x42233300 irq 124 lpm-pol 0

lpm-pol 0 at boot.

The reason for this can only be that your AHCI controller is not marked to
support LPM:
https://github.com/torvalds/linux/blob/v6.7/drivers/ata/ahci.c#L1629-L1631

Perhaps you could show the output of:
$ lspci -nn | grep AHCI

I guess one option could be to either add (or modify if there already is an
entry) your AHCI controller's PCI device and vendor id to the list of supported
AHCI controllers:
https://github.com/torvalds/linux/blob/v6.7/drivers/ata/ahci.c#L278

And use "board_ahci_low_power" instead of "board_ahci".

See e.g.:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/ata/ahci.c?id=b8b8b4e0c052b2c06e1c4820a8001f4e0f77900f


That way, you should not need to do any:

for foo in /sys/class/scsi_host/host*/link_power_management_policy;
  do echo med_power_with_dipm > $foo;
done

in order for your system to enter lower power states.

In fact, it would be interesting to know if that alone solves your problem,
or if you still need the revert/Damien's recent patch series as well.


> > 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.
> 
> The print is not called, right but not because of
> CONFIG_SATA_MOBILE_LPM_POLICY=0 since It is set to "3". However when I force 
> link_power_management_policy to "echo med_power_with_dipm" the message appears
> in dmesg:
> 
> [    9.434663] ahci 0000:00:17.0: setting devsleep to: 0 port support 0
> [    9.434668] ahci 0000:00:17.0: port does not support device sleep
> [    9.434674] ahci 0000:00:17.0: setting devsleep to: 0 port support 0
> [    9.434678] ahci 0000:00:17.0: port does not support device sleep

I see, so your AHCI port does not support DevSleep.


> > But you say that, for these logs, v6.6 can enter low CPU power states,
> > but v6.7-rcX can not?
> 
> Not quite. In 6.6 the PC8 transition worked only when forcing
> link_power_management_policy to "echo med_power_with_dipm". The policy was
> still performance after bootup like it is in 6.7-rc. The only difference I see
> is that after resume my laptop won't enter PC8 again. For 6.7-rc I have to call
> hdparm -Y for sda and sdb after resume to enable the transition.

I see.
You shouldn't need "echo med_power_with_dipm",
you should have an entry in ahci.c with "board_ahci_low_power" instead.


You also shouldn't need to do
$ hdparm -Y

It would be interesting to know if it is enough to only apply only
the first patch in Damien's series:
https://lore.kernel.org/linux-ide/20240111115123.1258422-2-dlemoal@kernel.org/
or if you need to apply both.


> 
> 
> > 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).
> 
> Attached to this bug for both (identical) drives.

Ok, this shows us that DevSleep is not enabled in the devices,
so it appears that your system does not need DevSleep to enter
lower power states, but it probably needs some other LPM feature
that is enabled by ata_eh_set_lpm() (since when lpm-pol is 0,
ata_eh_set_lpm() is never called, and this is when it doesn't work,
yet when you force a different lpm-pol using sysfs, ata_eh_set_lpm()
will get called and you appear to be able to enter lower power states.


> > 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?
> 
> How can I tell?

It seems quite clear right now that the ports on your AHCI controller
does not support DevSleep, yet your system still appears to be able to
enter lower power states anyway (if ata_eh_set_lpm() is allowed to
run to enable partial/slumber and DIPM).


> 
> > 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.
> 
> Okay and how do I so that?

There doesn't appear to be an option to disable/enable an arbitrary
feature using hdparm.

Anyway, there is no need, as you have already shown that DevSleep feature
is not enabled on your devices.


Kind regards,
Niklas