RE: [PATCH] ahci: document and clarify unconventional intel quirk

[Dan Williams <dan.j.williams@intel.com>]

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@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.

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@intel.com>