New: Suspend/Resume Regression with attached ATA devices

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

Bug ID: 218198
Summary: Suspend/Resume Regression with attached ATA devices
Product: SCSI Drivers
Version: 2.5
Hardware: Intel
OS: Linux
Status: NEW
Severity: high
Priority: P3
Component: Other
Assignee: [email protected]
Reporter: [email protected]
Regression: No

Hello,

the following commit from Kernel 6.7-rc1:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/diff/drivers/ata/libata-core.c?id=d035e4eb38b3ea5ae9ead342f888fd3c394b0fe0

introduced a regression on my system where after successful resuming the CPU won't enter lower Package Sates below pc2 even after letting it sit for 15+ minutes. Reverting this commit fixed the issue on my system with two ata drives. Anyone experiencing the same issue?

I'm happy to try any troubleshooting steps or provide more details if needed.

You may reply to this email to add a comment.

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

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

Dieter Mummenschanz ([email protected]) changed:

What |Removed |Added
----------------------------------------------------------------------------

 Bisected commit-id|                            |d035e4eb38b3ea5ae9ead342f88
                   |                            |8fd3c394b0fe0
     Kernel Version|                            |6.7-rc1
         Regression|No                          |Yes

You may reply to this email to add a comment.

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

Could you try adding this:

index 09ed67772fae..8d4871fff099 100644
@@ -6185,6 +6185,9 @@ void ata_pci_shutdown_one(struct pci_dev *pdev)
        for (i = 0; i < host->n_ports; i++) {
                struct ata_port *ap = host->ports[i];
 
+               /* Wait for EH to complete before freezing the port */
+               ata_port_wait_eh(ap);
+
                ap->pflags |= ATA_PFLAG_FROZEN;
 
                /* Disable port interrupts */

and see if this changes anything ? Beside that, I am at a loss seeing what is going on. The commit you mention essentially reverted an earlier change that is not necessary, bringing back ata_pci_shutdown_one() to the how it was for ages...

When you say "successful resuming", what exactly are you talking about ? System resume from suspend-to-ram ? from hibernation (suspend to disk) ?

I can always revert this revert, but I would rather understand why that is needed. Do you see any suspicious libata EH activity in dmesg ?

Also, are the lower Package Sates transitions automatic or driven by the kernel PM core ? I do not know that. If it is the latter, does the pc2 state also include adapters ? Isn't it limited to the CPU power state ? I ask because if it is and libata EH is in a bad loop constantly running after resume, that would explain why you cannot reach a lower CPU power state.

Damien Le Moal
Western Digital Research

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

--- Comment #1 from [email protected] ---

Could you try adding this:

index 09ed67772fae..8d4871fff099 100644
@@ -6185,6 +6185,9 @@ void ata_pci_shutdown_one(struct pci_dev *pdev)
        for (i = 0; i < host->n_ports; i++) {
                struct ata_port *ap = host->ports[i];
 
+               /* Wait for EH to complete before freezing the port */
+               ata_port_wait_eh(ap);
+
                ap->pflags |= ATA_PFLAG_FROZEN;
 
                /* Disable port interrupts */

and see if this changes anything ? Beside that, I am at a loss seeing what is going on. The commit you mention essentially reverted an earlier change that is not necessary, bringing back ata_pci_shutdown_one() to the how it was for ages...

When you say "successful resuming", what exactly are you talking about ? System resume from suspend-to-ram ? from hibernation (suspend to disk) ?

I can always revert this revert, but I would rather understand why that is needed. Do you see any suspicious libata EH activity in dmesg ?

Also, are the lower Package Sates transitions automatic or driven by the kernel PM core ? I do not know that. If it is the latter, does the pc2 state also include adapters ? Isn't it limited to the CPU power state ? I ask because if it
is and libata EH is in a bad loop constantly running after resume, that would explain why you cannot reach a lower CPU power state.

You may reply to this email to add a comment.

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

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

--- Comment #2 from Dieter Mummenschanz ([email protected]) --- 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?

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:

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

Otherwise the machine is stuck at PC2 state always.

So we have a fix I guess?

Anyway if there is anything alse I can try or provide you with more debug output please let me know.

You may reply to this email to add a comment.

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

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

--- Comment #3 from Dieter Mummenschanz ([email protected]) --- Created attachment 305490
--> https://bugzilla.kernel.org/attachment.cgi?id=305490&action=edit
dmesg partial log

You may reply to this email to add a comment.

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

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

--- Comment #4 from Dieter Mummenschanz ([email protected]) ---
Update: Please disregard my last report. After a second longer suspend I'm stuck at pc2 again :(. logs attached.

You may reply to this email to add a comment.

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

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

--- Comment #5 from Dieter Mummenschanz ([email protected]) --- Created attachment 305491
--> https://bugzilla.kernel.org/attachment.cgi?id=305491&action=edit
dmesg partial log stuck at pc2

You may reply to this email to add a comment.

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

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

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

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.

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

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

--- Comment #6 from [email protected] ---

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

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

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.

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

You may reply to this email to add a comment.

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

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

--- Comment #7 from Dieter Mummenschanz ([email protected]) ---
(In reply to Niklas.Cassel from comment #6)

Hello Niklas,

thanks for looking into this.

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.

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

I guess this is it?

[    1.031169] ata5.00: Features: Trust Dev-Sleep NCQ-sndrcv

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

That would explain why I'm seeing "max_performance" in link_power_management_policy without overriding it.

Thanks for the insight.

I understand however I do not know of any other way to enable lower package states on my machine.

Anything we can do to preserve the old bwhaviour until 6.6?

Yes, I had no issues up to 6.7-rc1.

Yweah, see logs.

I've attached the dmesg logs from kernel 6.6.

Thanks again for your help
Dieter

You may reply to this email to add a comment.

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

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

--- Comment #8 from Dieter Mummenschanz ([email protected]) --- Created attachment 305512
--> https://bugzilla.kernel.org/attachment.cgi?id=305512&action=edit
dmesg 6.7-rc3 with device info debug

You may reply to this email to add a comment.

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

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

--- Comment #9 from Dieter Mummenschanz ([email protected]) --- Created attachment 305513
--> https://bugzilla.kernel.org/attachment.cgi?id=305513&action=edit
logs from kernel 6.6

You may reply to this email to add a comment.

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

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?

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

--- Comment #10 from Phillip Susi ([email protected]) ---

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?

You may reply to this email to add a comment.

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

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

I've seen similar claims from people at AMD.

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.

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

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

--- Comment #11 from [email protected] ---

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

I've seen similar claims from people at AMD.

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.

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

You may reply to this email to add a comment.

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

I would say it's the hardware that requires the ata link to be powered down, not the firmware. They seem to have decided that the way to get the hardware to power down was to add a new state to ALPM instead of using runtime pm to actually disable the port. I would love to hear from Intel why they went this route.

I'm not quite sure what you are getting at here. The point of the ATA SLEEP command is to inform the device that it should power down everything, including the SATA PHY, and only keep a simple logic gate active that can detect the big and obvious RESET signal from the host. It seems to accomplish the same goal as DEVSLP without requiring hacking in another logic line somewhere that wasn't meant for that purpose.

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

--- Comment #12 from Phillip Susi ([email protected]) ---

I would say it's the hardware that requires the ata link to be powered down, not the firmware. They seem to have decided that the way to get the hardware to power down was to add a new state to ALPM instead of using runtime pm to actually disable the port. I would love to hear from Intel why they went this route.

I'm not quite sure what you are getting at here. The point of the ATA SLEEP command is to inform the device that it should power down everything, including the SATA PHY, and only keep a simple logic gate active that can detect the big and obvious RESET signal from the host. It seems to accomplish the same goal as DEVSLP without requiring hacking in another logic line somewhere that wasn't meant for that purpose.

You may reply to this email to add a comment.

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

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

--- Comment #13 from Dieter Mummenschanz ([email protected]) --- Maybe it helps to mention that if I issue

hdparm -Y /dev/sda && hdparm -Y /dev/sdb

a couple of seconds after successful resume my system transitions down to pc8. hdparm manual doesn't explicitly tell what command is issued with the -Y switch.

You may reply to this email to add a comment.

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

SLEEP. One problem with using it though is that several things tend to wake the drive up from it soon. I've had patches to address this for years. Maybe I'll finally get them merged soon. Those things are:

1) smartd or udisks2 polling the SMART status of the drive. They already issue CHECK POWER CONDITION first to see if the drive is in STANDBY and if so, don't bother with the SMART read. That avoids waking up a disk in STANDBY mode, but in SLEEP, you can't even ask the drive whether it's in standby without waking it up.

2) A mounted filesystem periodically issues a FLUSH CACHE command, even if nothing has been written. A drive in STANDBY just ignores it but in SLEEP, it wakes up.

I would imagine at once you access the disk in some way and so it wakes up, you won't get back to pc8 again without another hdparm -Y? Is that correct?

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

--- Comment #14 from Phillip Susi ([email protected]) ---

SLEEP. One problem with using it though is that several things tend to wake the drive up from it soon. I've had patches to address this for years. Maybe I'll finally get them merged soon. Those things are:

1) smartd or udisks2 polling the SMART status of the drive. They already issue CHECK POWER CONDITION first to see if the drive is in STANDBY and if so, don't bother with the SMART read. That avoids waking up a disk in STANDBY mode, but in SLEEP, you can't even ask the drive whether it's in standby without waking it up.

2) A mounted filesystem periodically issues a FLUSH CACHE command, even if nothing has been written. A drive in STANDBY just ignores it but in SLEEP, it wakes up.

I would imagine at once you access the disk in some way and so it wakes up, you won't get back to pc8 again without another hdparm -Y? Is that correct?

You may reply to this email to add a comment.

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

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

--- Comment #15 from Dieter Mummenschanz ([email protected]) --- (In reply to Phillip Susi from comment #14)

No or at least I don't see this behaviour. However the drives occasional seem to have some trouble waking up after resume and hdparm -Y:

[59984.831894] ata5.00: exception Emask 0x0 SAct 0x2000000 SErr 0x10000 action
0x6
[59984.831907] ata5.00: waking up from sleep
[59984.831914] ata5: hard resetting link
[59985.145261] ata5: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
[59985.148840] ata5.00: supports DRM functions and may not be fully accessible
[59985.153322] ata5.00: supports DRM functions and may not be fully accessible
[59985.156822] ata5.00: configured for UDMA/133
[59985.166903] ahci 0000:00:17.0: port does not support device sleep
[59985.167045] ata5: EH complete
[59985.167268] ata5.00: Enabling discard_zeroes_data

When that happens the system is stuck in pc2/pc3 for a couple of minutes but then transitions back in partial to pc8.

Regarding your patches: Anything I can test for you? :)

You may reply to this email to add a comment.

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

That's strange. Once the drive is woken back up that should once again, prevent pc8, just like it did before being put to SLEEP.

That's normal. The way to wake the drive up from SLEEP is to issue a link hard reset. That's just the kernel showing that it's doing that. At least the first part is normal, I'm not sure about the bit about DRM functions. The bit about the port not supporting device sleep seems to be your main issue with it not getting to pc8 using ALPM.

Keep an eye on the libata list. Hopefully I'll clean them up and post them in the next few days. Initially it will just be to keep hdparm -Y from being woken up for silly reasons. Sorting out the bigger problems with runtime pm so that the kernel can automatically put things to sleep when idle is going to be a bigger issue than I first thought.

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

--- Comment #16 from Phillip Susi ([email protected]) ---

That's strange. Once the drive is woken back up that should once again, prevent pc8, just like it did before being put to SLEEP.

That's normal. The way to wake the drive up from SLEEP is to issue a link hard reset. That's just the kernel showing that it's doing that. At least the first part is normal, I'm not sure about the bit about DRM functions. The bit about the port not supporting device sleep seems to be your main issue with it not getting to pc8 using ALPM.

Keep an eye on the libata list. Hopefully I'll clean them up and post them in the next few days. Initially it will just be to keep hdparm -Y from being woken up for silly reasons. Sorting out the bigger problems with runtime pm so that the kernel can automatically put things to sleep when idle is going to be a bigger issue than I first thought.

You may reply to this email to add a comment.

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

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

The Linux kernel's regression tracker (Thorsten Leemhuis) ([email protected]) changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |[email protected]

--- Comment #17 from The Linux kernel's regression tracker (Thorsten Leemhuis) ([email protected]) --- What's the status of this? I have this in my list of tracked regressions and it
seems nothing happened for about a week -- but I might have missed something?

Or is this not considered a regression for some good reason?

You may reply to this email to add a comment.

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

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.

Ok, good 1/4.

Yes, good, 2/4.

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.

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

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

--- Comment #18 from [email protected] ---
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.

Ok, good 1/4.

Yes, good, 2/4.

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.

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

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

--- Comment #19 from Dieter Mummenschanz ([email protected]) --- Hallo Niklas,

thanks for getting back to me on that.

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?

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.

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

so somehow the LPM Policy is either not set or reset by the kernel I assume.

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.

Attached to this bug for both (identical) drives.

My Laptop BIOS (INSYDE) does not offer any knobs to tune dleep for any ATA or PCI devices. Only thing I have set is the powersave profile.

Thats confusing :(

Me neither but it works. :(

As described above I'm doing this for years during bootup otherwise I'm stuck at PC2.

How can I tell?

Okay and how do I so that?

Kind regards,
Dieter

You may reply to this email to add a comment.

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

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

--- Comment #20 from Dieter Mummenschanz ([email protected]) --- Created attachment 305626
--> https://bugzilla.kernel.org/attachment.cgi?id=305626&action=edit
hdparm -I output for sda

hdparm -I output for sda

You may reply to this email to add a comment.

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

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

--- Comment #21 from Dieter Mummenschanz ([email protected]) --- Created attachment 305627
--> https://bugzilla.kernel.org/attachment.cgi?id=305627&action=edit
hdparm -I output for sdb

hdparm -I output for sdb

You may reply to this email to add a comment.

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

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

--- Comment #22 from Dieter Mummenschanz ([email protected]) --- Created attachment 305628
--> https://bugzilla.kernel.org/attachment.cgi?id=305628&action=edit
dmesg with no pc8 transition

dmesg with no pc8 transition

You may reply to this email to add a comment.

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

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

--- Comment #23 from Dieter Mummenschanz ([email protected]) --- Created attachment 305629
--> https://bugzilla.kernel.org/attachment.cgi?id=305629&action=edit
dmesg with pc8 transition

dmesg with pc8 transition

You may reply to this email to add a comment.

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

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

--- Comment #24 from Dieter Mummenschanz ([email protected]) --- Created attachment 305630
--> https://bugzilla.kernel.org/attachment.cgi?id=305630&action=edit
kernel config 6.7-rc

kernel config 6.7-rc

You may reply to this email to add a comment.

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

For any subsystem, you can see the emails to use by running:

scripts/get_maintainer.pl

So for ATA:

scripts/get_maintainer.pl drivers/ata/
Damien Le Moal <[email protected]> (maintainer:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers))
[email protected] (open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers))
[email protected] (open list)

Given the high traffic, no need to add [email protected]. But please add [email protected] to the bugzilla case.

And also please add Niklas and myself.

Niklas Cassel <[email protected]>
Damien Le Moal <[email protected]>

Damien Le Moal
Western Digital Research

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

--- Comment #25 from [email protected] ---

For any subsystem, you can see the emails to use by running:

scripts/get_maintainer.pl

So for ATA:

scripts/get_maintainer.pl drivers/ata/
Damien Le Moal <[email protected]> (maintainer:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers))
[email protected] (open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers))
[email protected] (open list)

Given the high traffic, no need to add [email protected]. But please add [email protected] to the bugzilla case.

And also please add Niklas and myself.

Niklas Cassel <[email protected]>
Damien Le Moal <[email protected]>

You may reply to this email to add a comment.

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

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

--- Comment #26 from The Linux kernel's regression tracker (Thorsten Leemhuis) ([email protected]) --- Please also include the regressions list:

Linux kernel regressions list <[email protected]>

You may reply to this email to add a comment.

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

Hello Dieter,

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:
[email protected]

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/[email protected]/T/#t

Your help testing the series would be much appreciated.

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.

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

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/[email protected]/
or if you need to apply both.

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.

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

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

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

--- Comment #27 from The Linux kernel's regression tracker (Thorsten Leemhuis) ([email protected]) --- Dieter (or Damien/Niklas), what is the status of this? Was this fixed?

You may reply to this email to add a comment.

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