Re: [PATCH v2] ahci: clean up intel_pcs_quirk

[Niklas Cassel <cassel@kernel.org>]

Hello Dan,

On Fri, Feb 09, 2024 at 10:23:01AM -0800, Dan Williams wrote:
> 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?

The for-next branch of:
git://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git

https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-next


Kind regards,
Niklas