ahci: document and clarify unconventional intel quirk
Niklas Cassel
maintainerPATCH 1/1 ahci: document and clarify unconventional intel quirk
The ahci_intel_pcs_quirk is unconventional in several ways:
First of all because it has a board ID for which the quirk should NOT be applied (board_ahci_pcs7), instead of the usual way where we have a board ID for which the quirk should be applied.
The second reason is that other than only excluding board_ahci_pcs7 from the quirk, PCI devices that make use of the generic entry in ahci_pci_tbl (which matches on AHCI class code) are also excluded.
This can of course lead to very subtle breakage, and did indeed do so in:
commit 104ff59af73a ("ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"), which added an explicit entry with board_ahci_low_power to ahci_pci_tbl.
This caused many users to complain that their SATA drives disappeared. The logical assumption was of course that the issue was related to LPM, and was therefore reverted in commit 6210038aeaf4 ("ata: ahci: Revert "ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"").
It took a lot of time to figure out that this was all completely unrelated to LPM, and was instead caused by an unconventional Intel quirk.
While this quirk should definitely be cleaned up to be implemented like all other quirks, for now, at least document the behavior of this quirk.
--- drivers/ata/ahci.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)
drivers/ata/ahci.c +23 -4
@@ -69,8 +69,8 @@ enum board_ids { board_ahci_vt8251,/* -* board IDs for Intel chipsets that support more than 6 ports -* *and* end up needing the PCS quirk. +* board IDs for Intel chipsets that should NOT have the +* ahci_intel_pcs_quirk applied. Yes, this is not a typo. */ board_ahci_pcs7,
@@ -1670,14 +1670,33 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap, ap->target_lpm_policy = policy; } +/* +* NOTE: this quirk is applied for all board IDs in ahci_pci_tbl, where +* the PCI vendor ID == PCI_VENDOR_ID_INTEL (except for board_ahci_pcs7). +* +* This quirk causes some Intel AHCI controllers (e.g. Intel Tiger Lake) +* to not get a link up when Intel VMD is enabled, see: +* https://bugzilla.kernel.org/show_bug.cgi?id=217114 +* +* Since the quirk is only applied for explicit entries in ahci_pci_tbl +* (it does not apply to the generic entry in ahci_pci_tbl that matches on +* AHCI class code), if your Intel AHCI controller does not get a link up +* because of this quirk, try to remove the explicit entry from ahci_pci_tbl. +*/ 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 ;
Signature · 1 line
2.43.0
Quoted 18 lines · Niklas Cassel
The
ahci_intel_pcs_quirkis unconventional in several ways:
First of all because it has a board ID for which the quirk should NOT be applied (board_ahci_pcs7), instead of the usual way where we have a board ID for which the quirk should be applied.The second reason is that other than only excluding
board_ahci_pcs7from the quirk, PCI devices that make use of the generic entry inahci_pci_tbl(which matches on AHCI class code) are also excluded.This can of course lead to very subtle breakage, and did indeed do so in:
commit104ff59af73a("ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"), which added an explicit entry withboard_ahci_low_powertoahci_pci_tbl.This caused many users to complain that their SATA drives disappeared. The logical assumption was of course that the issue was related to LPM, and was therefore reverted in commit
6210038aeaf4("ata: ahci: Revert "ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"").
Quoted 8 lines
It took a lot of time to figure out that this was all completely unrelated to LPM, and was instead caused by an unconventional Intel quirk.
While this quirk should definitely be cleaned up to be implemented like all other quirks, for now, at least document the behavior of this quirk.
Thanks!
Damien Le Moal
maintainerQuoted 61 lines · Niklas Cassel
The
ahci_intel_pcs_quirkis unconventional in several ways:
First of all because it has a board ID for which the quirk should NOT be applied (board_ahci_pcs7), instead of the usual way where we have a board ID for which the quirk should be applied.The second reason is that other than only excluding
board_ahci_pcs7from the quirk, PCI devices that make use of the generic entry inahci_pci_tbl(which matches on AHCI class code) are also excluded.This can of course lead to very subtle breakage, and did indeed do so in:
commit104ff59af73a("ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"), which added an explicit entry withboard_ahci_low_powertoahci_pci_tbl.This caused many users to complain that their SATA drives disappeared. The logical assumption was of course that the issue was related to LPM, and was therefore reverted in commit
6210038aeaf4("ata: ahci: Revert "ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"").It took a lot of time to figure out that this was all completely unrelated to LPM, and was instead caused by an unconventional Intel quirk.
While this quirk should definitely be cleaned up to be implemented like all other quirks, for now, at least document the behavior of this quirk.
Signed-off-by Niklas Cassel <[email protected]>--- drivers/ata/ahci.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)drivers/ata/ahci.c +14
-2@@ -69,8 +69,8 @@ enum board_ids { board_ahci_vt8251,/* -* board IDs for Intel chipsets that support more than 6 ports -* *and* end up needing the PCS quirk. +* board IDs for Intel chipsets that should NOT have the +* ahci_intel_pcs_quirk applied. Yes, this is not a typo. */ board_ahci_pcs7,@@ -1670,14 +1670,33 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap, ap->target_lpm_policy = policy; } +/* +* NOTE: this quirk is applied for all board IDs in ahci_pci_tbl, where +* the PCI vendor ID == PCI_VENDOR_ID_INTEL (except for board_ahci_pcs7). +* +* This quirk causes some Intel AHCI controllers (e.g. Intel Tiger Lake) +* to not get a link up when Intel VMD is enabled, see: +* https://bugzilla.kernel.org/show_bug.cgi?id=217114 +* +* Since the quirk is only applied for explicit entries in ahci_pci_tbl +* (it does not apply to the generic entry in ahci_pci_tbl that matches on +* AHCI class code), if your Intel AHCI controller does not get a link up +* because of this quirk, try to remove the explicit entry from ahci_pci_tbl.
drivers/ata/ahci.c static void ahci_update_initial_lpm_policy(struct ata_port *ap, · line 1693
Quoted 10 lines
drivers/ata/ahci.c +5
-1+*/ 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 ;
Can't we also skip TigerLake ?
Quoted 10 lines
drivers/ata/ahci.c +4
-1+/* -* 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 ;
Signature · 2 lines
Damien Le Moal Western Digital Research
Quoted 13 lines · Niklas Cassel
The
ahci_intel_pcs_quirkis unconventional in several ways:
First of all because it has a board ID for which the quirk should NOT be applied (board_ahci_pcs7), instead of the usual way where we have a board ID for which the quirk should be applied.The second reason is that other than only excluding
board_ahci_pcs7from the quirk, PCI devices that make use of the generic entry inahci_pci_tbl(which matches on AHCI class code) are also excluded.This can of course lead to very subtle breakage, and did indeed do so in:
commit104ff59af73a("ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"), which added an explicit entry withboard_ahci_low_powertoahci_pci_tbl.
Quoted 7 lines
This caused many users to complain that their SATA drives disappeared. The logical assumption was of course that the issue was related to LPM, and was therefore reverted in commit
6210038aeaf4("ata: ahci: Revert "ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"").It took a lot of time to figure out that this was all completely unrelated to LPM, and was instead caused by an unconventional Intel quirk.
Ugh, sorry about that.
Quoted 15 lines
While this quirk should definitely be cleaned up to be implemented like all other quirks, for now, at least document the behavior of this quirk.
Signed-off-by Niklas Cassel <[email protected]>--- drivers/ata/ahci.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)drivers/ata/ahci.c +0
-0
drivers/ata/ahci.c enum board_ids { · line 73
Quoted 7 lines
drivers/ata/ahci.c +2
-2@@ -69,8 +69,8 @@ enum board_ids { board_ahci_vt8251,/* -* board IDs for Intel chipsets that support more than 6 ports -* *and* end up needing the PCS quirk. +* board IDs for Intel chipsets that should NOT have the +* ahci_intel_pcs_quirk applied. Yes, this is not a typo.
I am not sure if this wording helps the next person without the context of this patch. How about this?
/*
* NOTE NOTE NOTE this board id is identifying a point in history where
* the "PCS Quirk" from 2007 should *stop* being applied to more modern
* platforms. So this is a "stop-quirk" point and board_ids >= to this
* value do not run the quirk, see ahci_intel_pcs_quirk() for details. */
Quoted 23 lines
drivers/ata/ahci.c +13
-1*/ board_ahci_pcs7,@@ -1670,14 +1670,33 @@ static void ahci_update_initial_lpm_policy(struct ata_port *ap, ap->target_lpm_policy = policy; } +/* +* NOTE: this quirk is applied for all board IDs in ahci_pci_tbl, where +* the PCI vendor ID == PCI_VENDOR_ID_INTEL (except for board_ahci_pcs7). +* +* This quirk causes some Intel AHCI controllers (e.g. Intel Tiger Lake) +* to not get a link up when Intel VMD is enabled, see: +* https://bugzilla.kernel.org/show_bug.cgi?id=217114 +* +* Since the quirk is only applied for explicit entries in ahci_pci_tbl +* (it does not apply to the generic entry in ahci_pci_tbl that matches on +* AHCI class code), if your Intel AHCI controller does not get a link up +* because of this quirk, try to remove the explicit entry from ahci_pci_tbl. +*/ 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);
drivers/ata/ahci.c static void ahci_update_initial_lpm_policy(struct ata_port *ap, · line 1696
Quoted 10 lines
drivers/ata/ahci.c +6
-1+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
Perhaps:
s/explicit entry/explicit Intel-vendor entry/
?
drivers/ata/ahci.c static void ahci_update_initial_lpm_policy(struct ata_port *ap, · line 1700
Quoted 4 lines
drivers/ata/ahci.c +2
-0+* 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 )
Otherwise you can add:
Niklas Cassel
maintainerQuoted 19 lines · Dan Williams
Quoted 18 lines · Niklas Cassel
The
ahci_intel_pcs_quirkis unconventional in several ways:
First of all because it has a board ID for which the quirk should NOT be applied (board_ahci_pcs7), instead of the usual way where we have a board ID for which the quirk should be applied.The second reason is that other than only excluding
board_ahci_pcs7from the quirk, PCI devices that make use of the generic entry inahci_pci_tbl(which matches on AHCI class code) are also excluded.This can of course lead to very subtle breakage, and did indeed do so in:
commit104ff59af73a("ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"), which added an explicit entry withboard_ahci_low_powertoahci_pci_tbl.This caused many users to complain that their SATA drives disappeared. The logical assumption was of course that the issue was related to LPM, and was therefore reverted in commit
6210038aeaf4("ata: ahci: Revert "ata: ahci: Add Tiger Lake UP{3,4} AHCI controller"").
Quoted 4 lines
Quoted 2 lines
It took a lot of time to figure out that this was all completely unrelated to LPM, and was instead caused by an unconventional Intel quirk.
Ugh, sorry about that.
Out of most bad things come something good :)
I've been reading up on LPM in SATA + AHCI specs because of this, so some of the patches related to LPM probably wouldn't have been submitted if it wasn't for this :)
Quoted 27 lines
Quoted 16 lines
While this quirk should definitely be cleaned up to be implemented like all other quirks, for now, at least document the behavior of this quirk.
Signed-off-by Niklas Cassel <[email protected]>--- drivers/ata/ahci.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)drivers/ata/ahci.c +0
-0drivers/ata/ahci.c enum board_ids { · line 73
Quoted 7 lines
drivers/ata/ahci.c +2
-2@@ -69,8 +69,8 @@ enum board_ids { board_ahci_vt8251,/* -* board IDs for Intel chipsets that support more than 6 ports -* *and* end up needing the PCS quirk. +* board IDs for Intel chipsets that should NOT have the +* ahci_intel_pcs_quirk applied. Yes, this is not a typo. I am not sure if this wording helps the next person without the context of this patch. How about this?
Quoted 6 lines
/*
* NOTE NOTE NOTE this board id is identifying a point in history where
* the "PCS Quirk" from 2007 should *stop* being applied to more modern
* platforms. So this is a "stop-quirk" point andboard_ids>= to this
* value do not run the quirk, seeahci_intel_pcs_quirk()for details. */
The amout of comments needed to describe how "out of the ordinary" something is, is probably a sign that you should just do a proper cleanup instead, so that is what I decided to do:
https://lore.kernel.org/linux-ide/[email protected]/
So feel free to review that one instead :)
Kind regards,
Niklas