Re: [PATCH v2] ahci: clean up intel_pcs_quirk

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi,

On Fri, Feb 09, 2024 at 02:03:06PM +0100, 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.

Right, I think with this one we can actually just drop those "modern"
entries and use the SATA PCI class since that is also "board_ahci",
therefore there is no need to add any new PCI IDs to the driver (unless
it requires some special quirks).

> No functional changes intended.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217114
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>