ata: ahci: Revert "ata: ahci: Add Intel Alder Lake-P AHCI controller to low power chipsets list"

From: Jason Nader <[email protected]>

Commit b8b8b4e0c052b2c06e1c4820a8001f4e0f77900f ("ata: ahci: Add Intel Alder Lake-P AHCI controller to low power chipsets list") enabled LPM for Alder Lake-P AHCI adaptors, however this introduced a regression on at least one system which causes the SATA ports to become unusable [1].

The original commit stated it is for Alder Lake-P, which I understand is a mobile CPU, however the device ID added (0x7ae2) matches the one reported by my system which has an Alder Lake-S desktop CPU [2]. Searching for this device on other websites points to 0x7ae2 being for the desktop "-S" suffix [3] and not for the "-P" suffix, which is apparently 0x51d3 [4][5].

Reverting this commit restores SATA port functionality on my system [6][7].

[1] This Ubuntu bug report also appears to suffer from the same issue, so there are more affected systems out there:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2063229

[2] System details:
CPU: Intel i5-12400
Motherboard: Biostar B660GTN
BIOS Settings: Intel VMD off, SATA hot plug off, CSM off

00:17.0 SATA controller [0106]: Intel Corporation Alder Lake-S PCH SATA Controller [AHCI Mode] [8086:7ae2] (rev 11)

[3] https://devicehunt.com/view/type/pci/vendor/8086/device/7AE2
[4] https://linux-hardware.org/?id=pci:8086-51d3-1462-1333
[5] https://linux-hardware.org/?view=search&vendorid=8086&deviceid=51d3#list

[6] Kernel logs before revert:
ahci 0000:00:17.0: AHCI 0001.0301 32 slots 4 ports 6 Gbps 0xf0 impl SATA mode
ata5: SATA max UDMA/133 abar m2048@0x80702000 port 0x80702300 irq 124 lpm-pol 3
ata6: SATA max UDMA/133 abar m2048@0x80702000 port 0x80702380 irq 124 lpm-pol 3
ata7: SATA max UDMA/133 abar m2048@0x80702000 port 0x80702400 irq 124 lpm-pol 3
ata8: SATA max UDMA/133 abar m2048@0x80702000 port 0x80702480 irq 124 lpm-pol 3
ata5: SATA link down (SStatus 4 SControl 300)
ata6: SATA link down (SStatus 4 SControl 300)
ata8: SATA link down (SStatus 4 SControl 300)
ata7: SATA link down (SStatus 4 SControl 300)

[7] Kernel logs after revert:
ahci 0000:00:17.0: AHCI 0001.0301 32 slots 4 ports 6 Gbps 0xf0 impl SATA mode
ata5: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802300 irq 125 lpm-pol 0
ata6: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802380 irq 125 lpm-pol 0
ata7: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802400 irq 125 lpm-pol 0
ata8: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802480 irq 125 lpm-pol 0
ata8: SATA link down (SStatus 0 SControl 300)
ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
ata5: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
ata6: SATA link up 6.0 Gbps (SStatus 133 SControl 300)

Jason Nader (1):
ata: ahci: Revert "ata: ahci: Add Intel Alder Lake-P AHCI controller
to low power chipsets list"

 drivers/ata/ahci.c | 1 -
 1 file changed, 1 deletion(-)

2.45.0

From: Jason Nader <[email protected]>

Commit b8b8b4e0c052b2c06e1c4820a8001f4e0f77900f ("ata: ahci: Add Intel Alder Lake-P AHCI controller to low power chipsets list") enabled low power mode for Alder Lake-P AHCI adaptors in order to reduce idle power consumption, however this introduced a regression on at least one system. Revert the patch until a better solution is found.

---
 drivers/ata/ahci.c | 1 -
 1 file changed, 1 deletion(-)

index 6548f10e61d9..07d66d2c5f0d 100644
@@ -429,7 +429,6 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x02d7), board_ahci_pcs_quirk }, /* Comet Lake PCH RAID */
 	/* Elkhart Lake IDs 0x4b60 & 0x4b62 https://sata-io.org/product/8803 not tested yet */
 	{ PCI_VDEVICE(INTEL, 0x4b63), board_ahci_pcs_quirk }, /* Elkhart Lake AHCI */
-	{ PCI_VDEVICE(INTEL, 0x7ae2), board_ahci_pcs_quirk }, /* Alder Lake-P AHCI */
 
 	/* JMicron 360/1/3/5/6, match class to avoid IDE function */
 	{ PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,

2.45.0

The patch itself looks fine to me, but the commit message needs to be rewritten.

Right now, we will enable LPM if the controller supports it (unless the port is hot plug capable or external), so we no longer have the "low power" board type.

Thus, it does not make sense to say that LPM is what introduced the regression.

If v6.9 does not work, and v6.9 + this patch works, then the proper commit message should be something like:

Commit b8b8b4e0c052 ("ata: ahci: Add Intel Alder Lake-P AHCI controller to low power chipsets list") added Intel Alder Lake to the ahci_pci_tbl.

Because of the way that the Intel PCS quirk was implemented, having an explicit entry in the ahci_pci_tbl caused the Intel PCS quirk to be applied. (The quirk was not being applied if there was no explict entry.)

Thus, entries that were added to the ahci_pci_tbl also got the Intel PCS quirk applied.

The quirk was cleaned up in commit 7edbb6059274 ("ahci: clean up intel_pcs_quirk"), such that it is clear which entries that actually applies the Intel PCS quirk.

Newer Intel AHCI controllers do not need the Intel PCS quirk, and applying it when not needed actually breaks some platforms.

Do not apply the Intel PCS quirk for Intel Alder Lake.
This is in line with how things worked before commit b8b8b4e0c052 ("ata:
ahci: Add Intel Alder Lake-P AHCI controller to low power chipsets list"), such that certain platforms using Intel Alder Lake will work once again.

Kind regards,
Niklas

Hello Jason,

These logs do not make sense to me.

Why is lpm-pol 3 before and lpm-pol 0 after removing the entry? Are you using the same kernel config?

Could you please take:
v6.9 + the following debug print:

index 6548f10e61d9..0a14a09070ea 100644
@@ -1733,8 +1733,10 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap)
         * Management Interaction in AHCI 1.3.1. Therefore, do not enable
         * LPM if the port advertises itself as an external port.
         */
-       if (ap->pflags & ATA_PFLAG_EXTERNAL)
+       if (ap->pflags & ATA_PFLAG_EXTERNAL) {
+               ata_port_info(ap, "external port, not enabling LPM\n");
                return;
+       }
 
        /* user modified policy via module param */
        if (mobile_lpm_policy != -1) {

And then:
v6.9 + the debug print above + the removal of the Alder Lake-P AHCI entry.
- { PCI_VDEVICE(INTEL, 0x7ae2), board_ahci_pcs_quirk }, /* Alder Lake-P AHCI */

And paste the same kernel prints as you did in this email?

Thank you!

Kind regards,
Niklas

From: Jason Nader <[email protected]>

Changes - updated commit message as per the kind guidance from Niklas.

I was not sure whether to cc stable, but if so it would allow affected users to start to be able to use kernel versions > 6.6.10.

Below are logs including the extra logging asked for by Niklas.

Kernel logs with this patch:

Linux version linux6.9.disableAlderLakequirk@archlinux
ata4294967295: external port, not enabling LPM
ata4294967295: external port, not enabling LPM
ata4294967295: external port, not enabling LPM
ata4294967295: external port, not enabling LPM
ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode
ata5: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802300 irq 125 lpm-pol 3
ata6: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802380 irq 125 lpm-pol 3
ata7: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802400 irq 125 lpm-pol 3
ata8: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802480 irq 125 lpm-pol 3
ata5: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
ata8: SATA link down (SStatus 0 SControl 300)
ata6: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)

Kernel logs without this patch:

Linux version linux6.9.enableAlderLakequirk@archlinux
ata4294967295: external port, not enabling LPM
ata4294967295: external port, not enabling LPM
ata4294967295: external port, not enabling LPM
ata4294967295: external port, not enabling LPM
ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode
ata5: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802300 irq 125 lpm-pol 3
ata6: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802380 irq 125 lpm-pol 3
ata7: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802400 irq 125 lpm-pol 3
ata8: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802480 irq 125 lpm-pol 3
ata8: SATA link down (SStatus 4 SControl 300)
ata5: SATA link down (SStatus 4 SControl 300)
ata6: SATA link down (SStatus 4 SControl 300)
ata7: SATA link down (SStatus 4 SControl 300)

Note I also tested a patch that changes the Alder Lake entry from board_ahci_pcs_quirk to board_ahci, and that booted fine as well. However since it is not clear whether it is Alder Lake-S or Alder Lake-P that was meant to be added to the list in the first place, I have not committed that patch.

Kernel logs for board_ahci_pcs_quirk to board_ahci patch:

Linux version linux6.9.enableAlderLakeboardahci@archlinux
ata4294967295: external port, not enabling LPM
ata4294967295: external port, not enabling LPM
ata4294967295: external port, not enabling LPM
ata4294967295: external port, not enabling LPM
ahci 0000:00:17.0: AHCI vers 0001.0301, 32 command slots, 6 Gbps, SATA mode
ata5: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802300 irq 125 lpm-pol 3
ata6: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802380 irq 125 lpm-pol 3
ata7: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802400 irq 125 lpm-pol 3
ata8: SATA max UDMA/133 abar m2048@0x80802000 port 0x80802480 irq 125 lpm-pol 3
ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
ata5: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
ata8: SATA link down (SStatus 0 SControl 300)
ata6: SATA link up 6.0 Gbps (SStatus 133 SControl 300)

Jason Nader (1):
ata: ahci: Revert "ata: ahci: Add Intel Alder Lake-P AHCI controller
to low power chipsets list"

 drivers/ata/ahci.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

2.45.1

From: Jason Nader <[email protected]>

Commit b8b8b4e0c052 ("ata: ahci: Add Intel Alder Lake-P AHCI controller to low power chipsets list") added Intel Alder Lake to the ahci_pci_tbl.

Because of the way that the Intel PCS quirk was implemented, having an explicit entry in the ahci_pci_tbl caused the Intel PCS quirk to be applied. (The quirk was not being applied if there was no explicit entry.)

Thus, entries that were added to the ahci_pci_tbl also got the Intel PCS quirk applied.

The quirk was cleaned up in commit 7edbb6059274 ("ahci: clean up intel_pcs_quirk"), such that it is clear which entries that actually applies the Intel PCS quirk.

Newer Intel AHCI controllers do not need the Intel PCS quirk, and applying it when not needed actually breaks some platforms.

Do not apply the Intel PCS quirk for Intel Alder Lake.
This is in line with how things worked before commit b8b8b4e0c052 ("ata:
ahci: Add Intel Alder Lake-P AHCI controller to low power chipsets list"), such that certain platforms using Intel Alder Lake will work once again.

---
 drivers/ata/ahci.c | 1 -
 1 file changed, 1 deletion(-)

index 6548f10e61d9..07d66d2c5f0d 100644
@@ -429,7 +429,6 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x02d7), board_ahci_pcs_quirk }, /* Comet Lake PCH RAID */
 	/* Elkhart Lake IDs 0x4b60 & 0x4b62 https://sata-io.org/product/8803 not tested yet */
 	{ PCI_VDEVICE(INTEL, 0x4b63), board_ahci_pcs_quirk }, /* Elkhart Lake AHCI */
-	{ PCI_VDEVICE(INTEL, 0x7ae2), board_ahci_pcs_quirk }, /* Alder Lake-P AHCI */
 
 	/* JMicron 360/1/3/5/6, match class to avoid IDE function */
 	{ PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,

2.45.1

The patch looks good, I will queue it up once 6.10-rc1 is out!

Kind regards,
Niklas

Thinking more about this, could you please send a v3 with:

and change the subject to something like:
ata: ahci: Do not apply Intel PCS quirk on Intel Alder Lake

Kind regards,
Niklas

Since the prints below are for ata5-ata8, that most likely means that the "external port, not enabling LPM" prints above are for port ata1-ata4, so they are not really relevant and might even belong to a different SATA controller.

Anyway, lpm-policy for ata5-ata8 is the same before and after this patch, so everything looks fine.

Kind regards,
Niklas

Commit b8b8b4e0c052 ("ata: ahci: Add Intel Alder Lake-P AHCI controller to low power chipsets list") added Intel Alder Lake to the ahci_pci_tbl.

Because of the way that the Intel PCS quirk was implemented, having an explicit entry in the ahci_pci_tbl caused the Intel PCS quirk to be applied. (The quirk was not being applied if there was no explict entry.)

Thus, entries that were added to the ahci_pci_tbl also got the Intel PCS quirk applied.

The quirk was cleaned up in commit 7edbb6059274 ("ahci: clean up intel_pcs_quirk"), such that it is clear which entries that actually applies the Intel PCS quirk.

Newer Intel AHCI controllers do not need the Intel PCS quirk, and applying it when not needed actually breaks some platforms.

Do not apply the Intel PCS quirk for Intel Alder Lake.
This is in line with how things worked before commit b8b8b4e0c052 ("ata:
ahci: Add Intel Alder Lake-P AHCI controller to low power chipsets list"), such that certain platforms using Intel Alder Lake will work once again.

---
 drivers/ata/ahci.c | 1 -
 1 file changed, 1 deletion(-)

index 6548f10e61d9..07d66d2c5f0d 100644
@@ -429,7 +429,6 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x02d7), board_ahci_pcs_quirk }, /* Comet Lake PCH RAID */
 	/* Elkhart Lake IDs 0x4b60 & 0x4b62 https://sata-io.org/product/8803 not tested yet */
 	{ PCI_VDEVICE(INTEL, 0x4b63), board_ahci_pcs_quirk }, /* Elkhart Lake AHCI */
-	{ PCI_VDEVICE(INTEL, 0x7ae2), board_ahci_pcs_quirk }, /* Alder Lake-P AHCI */
 
 	/* JMicron 360/1/3/5/6, match class to avoid IDE function */
 	{ PCI_VENDOR_ID_JMICRON, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,

2.45.1

Applied:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-6.10-fixes

Thank you a lot for the fix, at last, i will see again my ssd :)