Niklas Cassel wrote:
> 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.
Ugh, sorry about that.
>
> 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@xxxxxxxxxx>
> ---
> 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.
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.
*/
> */
> 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
Perhaps:
s/explicit entry/explicit Intel-vendor entry/
?
> + * 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:
Acked-by: Dan Williams <dan.j.williams@xxxxxxxxx>
