Re: [PATCH v2] ahci: clean up intel_pcs_quirk

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

Niklas Cassel wrote:
> 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

Thanks, you can add:

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

Now, one of the implications of doing this proper fixup might also be
that some of the other open-coded quirks want to move into ahci_pci_tbl,
like ahci_broken_devslp(). Otherwise, I concur with your "No functional
changes intended" assertion and this looks much less error prone for the
next person that wander in here.