RE: [PATCH v2] ahci: clean up intel_pcs_quirk

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

Niklas Cassel wrote:
> The comment in front of board_ahci_pcs7 is completely wrong.
> It claims that board_ahci_pcs7 is needing the quirk, but in fact,
> the logic implemented in ahci_intel_pcs_quirk() is the exact opposite,
> only board_ahci_pcs7 is _excluded_ from the quirk.
> 
> This way of implementing a 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.
> 
> Clean up the quirk so that it behaves like other quirks, i.e. define a
> board where the quirk is applied. Platforms that were using
> board_ahci_pcs7 are converted to use board_ahci, this is safe since the
> boards were identical, and board_ahci_pcs7 did not define any custom
> port_ops.
> 
> This way, new Intel platforms can be added using the correct "board_ahci"
> board, without getting any unexpected quirks applied.
> 
> This means that we currently have some modern platforms defined that are
> using the Intel PCS quirk, but that is identical to the behavior that
> was there before this commit.
> 
> No functional changes intended.

*crosses fingers*

> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217114
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
> Changes since v1: Just do the actual cleanup instead of documenting how
> weird the existing quirk is. (Simply documenting the quirk would not have
> stopped people from encountering the same problem as we encountered when
> trying to add support for Tiger Lake.)
> 
>  drivers/ata/ahci.c | 361 ++++++++++++++++++++++-----------------------
>  drivers/ata/ahci.h |   1 +
>  2 files changed, 180 insertions(+), 182 deletions(-)

It's large, it's noisy, but I see no lies here. I think this is the way.

Acked-by: Dan Williams <dan.j.williams@intel.com>

...I wanted to give a reviewed-by, but this patch does not apply to
current mainline so I can double check the result, can you share the
baseline for this diff?