From: Niklas Cassel <cassel@kernel.org>
To: Dan Williams <dan.j.williams@intel.com>
Cc: Damien Le Moal <dlemoal@kernel.org>,
Mika Westerberg <mika.westerberg@linux.intel.com>,
linux-ide@vger.kernel.org
Subject: Re: [PATCH] ahci: document and clarify unconventional intel quirk
Date: Fri, 9 Feb 2024 14:26:11 +0100 [thread overview]
Message-ID: <ZcYn8y5GYMJ+7tLF@x1-carbon> (raw)
In-Reply-To: <65c578b054868_5a7f294a5@dwillia2-xfh.jf.intel.com.notmuch>
On Thu, Feb 08, 2024 at 04:58:24PM -0800, Dan Williams wrote:
> 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.
Out of most bad things come something good :)
I've been reading up on LPM in SATA + AHCI specs because of this,
so some of the patches related to LPM probably wouldn't have been
submitted if it wasn't for this :)
>
> >
> > 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.
> */
The amout of comments needed to describe how "out of the ordinary"
something is, is probably a sign that you should just do a proper
cleanup instead, so that is what I decided to do:
https://lore.kernel.org/linux-ide/20240209130307.39113-1-cassel@kernel.org/
So feel free to review that one instead :)
Kind regards,
Niklas
prev parent reply other threads:[~2024-02-09 13:26 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-07 9:10 [PATCH] ahci: document and clarify unconventional intel quirk Niklas Cassel
2024-02-08 16:09 ` Mika Westerberg
2024-02-08 23:33 ` Damien Le Moal
2024-02-09 0:58 ` Dan Williams
2024-02-09 13:26 ` Niklas Cassel [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZcYn8y5GYMJ+7tLF@x1-carbon \
--to=cassel@kernel.org \
--cc=dan.j.williams@intel.com \
--cc=dlemoal@kernel.org \
--cc=linux-ide@vger.kernel.org \
--cc=mika.westerberg@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).