Linux ATA/IDE development
 help / color / mirror / Atom feed
From: Niklas Cassel <cassel@kernel.org>
To: Damien Le Moal <dlemoal@kernel.org>, Niklas Cassel <cassel@kernel.org>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>,
	Dan Williams <dan.j.williams@intel.com>,
	linux-ide@vger.kernel.org
Subject: [PATCH] ahci: document and clarify unconventional intel quirk
Date: Wed,  7 Feb 2024 10:10:53 +0100	[thread overview]
Message-ID: <20240207091054.1697236-1-cassel@kernel.org> (raw)

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.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=217114
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index da2e74fce2d9..122278438092 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -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;
 
-- 
2.43.0


             reply	other threads:[~2024-02-07  9:11 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-07  9:10 Niklas Cassel [this message]
2024-02-08 16:09 ` [PATCH] ahci: document and clarify unconventional intel quirk Mika Westerberg
2024-02-08 23:33 ` Damien Le Moal
2024-02-09  0:58 ` Dan Williams
2024-02-09 13:26   ` Niklas Cassel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240207091054.1697236-1-cassel@kernel.org \
    --to=cassel@kernel.org \
    --cc=dan.j.williams@intel.com \
    --cc=dlemoal@kernel.org \
    --cc=linux-ide@vger.kernel.org \
    --cc=mika.westerberg@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox