Re: [PATCH 1/2] ata: ahci: Add force LPM policy quirk for ASUS B1400CEAE
From: Niklas Cassel
Date: Tue Feb 06 2024 - 08:12:53 EST
On Tue, Feb 06, 2024 at 04:39:02PM +0800, Jian-Hong Pan wrote:
> Niklas Cassel <cassel@xxxxxxxxxx> 於 2024年2月5日 週一 下午7:33寫道：
>
> Have the comparison:
>
> * Bind LPM policy with the patch "ata: ahci: Add force LPM policy
> quirk for ASUS B1400CEAE" based on kernel v6.8-rc2:
>
> $ dmesg | grep -E "(SATA|ata1|ahci)"
> [ 0.791497] ahci 10000:e0:17.0: version 3.0
> [ 0.791499] ahci 10000:e0:17.0: force controller follow LPM policy
> [ 0.791517] ahci 10000:e0:17.0: can't derive routing for PCI INT A
> [ 0.791518] ahci 10000:e0:17.0: PCI INT A: no GSI
> [ 0.791637] ahci 10000:e0:17.0: ahci_update_initial_lpm_policy: policy 3
> [ 0.791652] ahci 10000:e0:17.0: ahci_intel_pcs_quirk: not Intel,
> the vendor is 0xffffffff
> [ 0.791662] ahci 10000:e0:17.0: AHCI 0001.0301 32 slots 1 ports 6
> Gbps 0x1 impl SATA mode
> [ 0.791663] ahci 10000:e0:17.0: flags: 64bit ncq sntf pm clo only
> pio slum part deso sadm sds
> [ 0.791771] scsi host0: ahci
> [ 0.791806] ata1: SATA max UDMA/133 abar m2048@0x76102000 port
> 0x76102100 irq 145 lpm-pol 3
> [ 0.791808] ahci 10000:e0:17.0: ahci_init_one: probed
> [ 1.109393] ata1: sata_link_resume: rc=0
> [ 1.109415] ata1: BUSY ? 0 (status: 0x50) SStatus.DET: 0x3
> [ 1.109418] ata1: sata_link_hardreset: is 0
> [ 1.109420] ata1: sata_link_hardreset: is on line, returns 0
> [ 1.109444] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> [ 1.110161] ata1.00: ATA-10: WDC WD10SPZX-80Z10T2, 04.01A04, max UDMA/133
> [ 1.112047] ata1.00: 1953525168 sectors, multi 16: LBA48 NCQ (depth 32), AA
> [ 1.112054] ata1.00: Features: NCQ-prio
> [ 1.114814] ata1.00: configured for UDMA/133
> [ 1.114821] ata1: ahci_set_lpm: policy=3
> [ 1.114837] ata1: sata_link_scr_lpm: policy is 3 and original
> scontrol 0x00000300
> [ 1.114840] ata1: sata_link_scr_lpm: write scontrol 0x00000000
>
> The SATA link is up and SATA storage shows up.
> Full dmesg as the attachment of
> https://bugzilla.kernel.org/show_bug.cgi?id=217114#c28
>
> * Bind LPM policy with PCI IDs like commit 104ff59af73a ("ata: ahci:
> Add Tiger Lake UP{3,4} AHCI controller"):
>
> $ dmesg | grep -E "(SATA|ata1|ahci)"
> [ 0.783125] ahci 10000:e0:17.0: version 3.0
> [ 0.783143] ahci 10000:e0:17.0: can't derive routing for PCI INT A
> [ 0.783145] ahci 10000:e0:17.0: PCI INT A: no GSI
> [ 0.783257] ahci 10000:e0:17.0: ahci_update_initial_lpm_policy: policy 3
> [ 0.783280] ahci 10000:e0:17.0: ahci_intel_pcs_quirk: PCS_6 is 0x0000
> [ 0.783281] ahci 10000:e0:17.0: ahci_intel_pcs_quirk: write PCS_6 with 0x0001
> [ 0.783296] ahci 10000:e0:17.0: AHCI 0001.0301 32 slots 1 ports 6
> Gbps 0x1 impl SATA mode
> [ 0.783298] ahci 10000:e0:17.0: flags: 64bit ncq sntf pm clo only
> pio slum part deso sadm sds
> [ 0.783402] scsi host0: ahci
> [ 0.783440] ata1: SATA max UDMA/133 abar m2048@0x76102000 port
> 0x76102100 irq 144 lpm-pol 3
> [ 0.783442] ahci 10000:e0:17.0: ahci_init_one: probed
> [ 1.096930] ata1: sata_link_resume: rc=0
> [ 1.096960] ata1: sata_link_hardreset: ata_phys_link_offline is True
> [ 1.096962] ata1: sata_link_hardreset: is off line, returns 0
> [ 1.097000] ata1: SATA link down (SStatus 4 SControl 300)
> [ 1.097025] ata1: ahci_set_lpm: policy=3
> [ 1.097051] ata1: sata_link_scr_lpm: policy is 3 and original
> scontrol 0x00000300
> [ 1.097054] ata1: sata_link_scr_lpm: write scontrol 0x00000304
>
> The SATA link is down and SATA storage disappears.
> Full dmesg as the attachment of
> https://bugzilla.kernel.org/show_bug.cgi?id=217114#c29
>
So in summary:
When Intel VMD is on, and the ahci_intel_pcs_quirk is applied => NOT OK
When Intel VMD is on, and the ahci_intel_pcs_quirk is not applied => OK
When Intel VMD is off, and the ahci_intel_pcs_quirk is applied => OK
When Intel VMD is off, and the ahci_intel_pcs_quirk is not applied => ?
Excellent find!
In the bad case:
sata_link_hardreset() sets SControl.DET to 1, to establish the interface
communication. Then sleeps for 1 ms.
Then it calls sata_link_resume(), which clears SControl.DET to 0.
(This matches the AHCI spec which says that SControl.DET should be set
to 1 for at least 1 ms.)
sata_link_hardreset() then calls ata_phys_link_offline(),
which is essentially defined as: return !(SStatus.DET == 0x3)
ata_phys_link_offline() returns true, since SStatus.DET == 0x4.
SStatus.DET == 0x4 means: Phy in offline mode as a result of the
interface being disabled or running in a BIST loopback mode.
If the physical link is not established, there is no point to call
ata_wait_ready() (which waits for the device to become ready on the
protocol level), as the physical link could not even be established.
After that, we write SControl.DET to set bit 4 to disable the port,
in order to save power. This is only done because sata_link_hardreset()
failed to establish a link after toggling SControl.DET == 1.
So the problem is that SStatus.DET never changed to 0x3 after toggling
SControl.DET == 1.
>
> However, I notice more interesting thing:
> "drivers/ata/ahci.c:ahci_intel_pcs_quirk()"!
> If bind LPM policy with PCI IDs matching, then it does the PCS quirk.
> But, binding with the patch "ata: ahci: Add force LPM policy quirk for
> ASUS B1400CEAE" does not, because the vendor is ANY vendor, not Intel.
>
> So, I did following test:
>
> If I modify the PCI vendor check condition with the pdev, not the PCI
> ID's vendor:
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 7ecd56c8262a..ece709ac20d6 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1706,12 +1709,16 @@ static void ahci_intel_pcs_quirk(struct
> pci_dev *pdev, struct ahci_host_priv *hp
> /*
> * Only apply the 6-port PCS quirk for known legacy platforms.
> */
> - if (!id || id->vendor != PCI_VENDOR_ID_INTEL)
> + if (!id || pdev->vendor != PCI_VENDOR_ID_INTEL) {
> + dev_info(&pdev->dev, "%s: not Intel, the vendor is
> 0x%08x\n", __func__, id->vendor);
> return;
> + }
The reason why you are seeing this is because Tiger Lake does not have
an entry in the ahci_pci_tbl in mainline, so it uses the generic entry
which matches on the AHCI class code:
https://github.com/torvalds/linux/blob/v6.8-rc3/drivers/ata/ahci.c#L636
If you revert 6210038aeaf4 ("ata: ahci: Revert "ata: ahci: Add Tiger Lake
UP{3,4} AHCI controller""), you will get an explicit entry in the
ahci_pci_tbl.
But to clarify, I think that it would make sense to add:
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index d2460fa985b7..e462509a45e8 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1672,12 +1672,18 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap,
static void ahci_intel_pcs_quirk(struct pci_dev *pdev, struct ahci_host_priv *hpriv)
{
- const struct pci_device_id *id = pci_match_id(ahci_pci_tbl, pdev);
+ const struct pci_device_id *id;
u16 tmp16;
+ /* If the detected PCI device is not an Intel device, skip. */
+ if (pdev->vendor != PCI_VENDOR_ID_INTEL)
+ return;
+
/*
- * Only apply the 6-port PCS quirk for known legacy platforms.
+ * See if there is an explicit entry for this PCI device in
+ * ahci_pci_tbl, if there is not, do not apply the quirk.
*/
+ id = pci_match_id(ahci_pci_tbl, pdev);
if (!id || id->vendor != PCI_VENDOR_ID_INTEL)
return;
>
> Then, the SATA HDD always disappears like binding the LPM policy with
> PCI IDs matching, even with the patch "ata: ahci: Add force LPM policy
> quirk for ASUS B1400CEAE".
> So, I think ahci_intel_pcs_quirk() is the key point.
I agree.
Can you verify that things work as expected when doing a:
$ git revert 6210038aeaf49c395c2da57572246d93ec67f6d4
to re-add the explicit entry, if you also do a:
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1672,6 +1672,7 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap,
static void ahci_intel_pcs_quirk(struct pci_dev *pdev, struct ahci_host_priv *hpriv)
{
+#if 0
const struct pci_device_id *id = pci_match_id(ahci_pci_tbl, pdev);
u16 tmp16;
@@ -1698,6 +1699,7 @@ static void ahci_intel_pcs_quirk(struct pci_dev *pdev, struct ahci_host_priv *hp
tmp16 |= hpriv->port_map;
pci_write_config_word(pdev, PCS_6, tmp16);
}
+#endif
}
To make the quirk a no-op?
To be honest, this quirk looks horrible.
Looking at the original commit:
c312ef176399 ("libata/ahci: Drop PCS quirk for Denverton and beyond")
It claims that:
Rather than try to fix the PCS quirk to consider the DNV register layout
instead require explicit opt-in. The assumption is that the OS driver
need not touch this register, and platforms can be added with a new
boad_ahci_pcs7 board-id when / if problematic platforms are found in the
future.
However, it does NOT require an explicit opt-in!
If we were to add an entry with board type "board_ahci" or
"board_ahci_low_power" for Tiger Lake, the quirk gets applied...
See also:
09d6ac8dc51a ("libata/ahci: Fix PCS quirk application")
So basically, what ahci_intel_pcs_quirk() does is that it checks
if there is an explicit entry in ahci_pci_tbl.
If there is not, the quirk is not applied.
If there is an entry, and the enum for that board has a value that
is less than board_ahci_pcs7, the quirk is applied...
But that will be *ALL* other board types since board_ahci_pcs7 is
defined last in the enum:
https://github.com/torvalds/linux/blob/v6.8-rc3/drivers/ata/ahci.c#L75
Not only that but the comment for that enum is wrong:
https://github.com/torvalds/linux/blob/v6.8-rc3/drivers/ata/ahci.c#L71-L74
/*
* board IDs for Intel chipsets that support more than 6 ports
* *and* end up needing the PCS quirk.
*/
Is is the opposite... board IDs that do NOT need the PCS quirk...
But this is not the way we add quirks.
We add a flag and a new board_id and mark the PCI device and vendor ids
that are affected to use that board, see e.g.
20730e9b2778 ("ahci: add 43-bit DMA address quirk for ASMedia ASM1061 controllers")
We don't add a quirk and apply it for everything (board_ahci,
board_ahci_low_power) except for a specific entry (board_ahci_pcs7).
It seems that at least Intel AHCI controllers that also have Intel VMD
enabled break when this quirk is applied.
I guess one way would be to do a:
git show c312ef176399:drivers/ata/ahci.c | grep "PCI_VDEVICE(INTEL"
and replace everything that is not: board_ahci_pcs7
with a board_ahci_pcs_quirk, board_ahci_low_power_pcs_quirk, and
board_ahci_avn_pcs_quirk, and after that change all board_ahci_pcs7
entries to board_ahci, and assume that entries added since c312ef176399
do not need the quirk.
But it would be nice if someone from Intel could clean this up.
Kind regards,
Niklas