Re: [PATCH] ahci: Add Intel Emmitsburg PCH RAID PCI IDs

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

On Mon, 2020-11-23 at 11:38 +0000, Christoph Hellwig wrote:
> On Mon, Nov 23, 2020 at 01:26:22PM +0200, Mika Westerberg wrote:
> > On Mon, Nov 23, 2020 at 10:09:17AM +0000, Christoph Hellwig wrote:
> > > On Fri, Nov 20, 2020 at 12:53:09PM +0200, Mika Westerberg wrote:
> > > > On Thu, Nov 19, 2020 at 04:50:22PM +0000, Christoph Hellwig wrote:
> > > > > On Thu, Nov 19, 2020 at 01:43:18PM +0300, Mika Westerberg wrote:
> > > > > > Add Intel Emmitsburg PCH RAID PCI IDs to the list of supported
> > > > > > controllers.
> > > > > 
> > > > > Stupid question: what would it to get Intel to finally report the
> > > > > correct classcode after all the time?  The amount of IDs we need to list
> > > > > is getting ridiculous.

It turns out the problem is $OTHER_OS wants to load an AHCI driver
based on class code and if RAID mode reused the class code it would
break legacy there, but see below, there is a path to stem the tide of
new ids flowing into this file...

> > > > 
> > > > What is the correct class code in this case that it works with the AHCI
> > > > driver?
> > > > 
> > > > I think (not 100% sure) it reports standard AHCI class code when it is
> > > > not in RAID mode but these PCI IDs are for the RAID mode.
> > > 
> > > The right class code is the AHCI one.  The so called RAID mode doesn't
> > > change the operation of the device at all (except for sometimes hiding
> > > NVMe devices that are a different PCIe function to start with).
> > 
> > Thanks. I looked at the AHCI spec (1.3.1) and it says this regarding the
> > class code (CC) field:
> > 
> >   Informative Note: For HBAs that support RAID, the Sub Class Code reset
> >   value should be 04h and the Programming Interface reset value should be
> >   00h.
> > 
> > I think this is what the controller is doing when in "RAID mode".

Mika, "RAID mode" is an unfortunate misnomer because the hardware is
just standard AHCI programming model, always.

> The point is: these AHCI controllers do not support RAID in form
> despite the confusing naming.

I did some digging on this to both get the history and to push for a
concession that Intel will fix the hardware development pipeline to
stop the pointless churn of "RAID" ids going forward. The history is
equal parts encouraging and unfortunate. The encouraging finding is
that the "RAID" device-ids on "server" platforms has been stable for
several generations and will continue to be stable:

{ PCI_VDEVICE(INTEL, 0x2826), board_ahci }, /* PBG/Lewisburg RAID*/
...
{ PCI_VDEVICE(INTEL, 0x2827), board_ahci }, /* Wellsburg/Lewisburg RAID */

I.e. those RAID-ids also work for Emmitsburg. The unfortunate part of
the story is that those ids only correspond to "SATA0" and "SATA1"
instances of AHCI controllers in "RAID" mode on those platforms. Yes,
the exact same hardware duplicated at different PCI DEVFNs has a
different PCI device-id. Why? Apparently, $OTHER_OS wanted per
controller instance identification. I can only guess to apply per
controller id quirks. However, and this is a subtle detail, that
legacy-requirement only applies to the native "AHCI" mode case. I.e. in
AHCI mode, $OTHER_OS expects per-controller ids in addition to the
class code. For RAID mode, that per-controller id policy was carried
over on server platforms, but that appears to be due only to inertia,
not a requirement that matters in practice.

So what this patch is actually asking is to add another generic server
controller RAID-id but for a third instance of a
 controller in RAID mode. I.e. something like this diff:

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index ab5811ef5a53..039f3211eddc 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -324,7 +324,6 @@ static const struct pci_device_id ahci_pci_tbl[] = {
        { PCI_VDEVICE(INTEL, 0x1d02), board_ahci }, /* PBG AHCI */
        { PCI_VDEVICE(INTEL, 0x1d04), board_ahci }, /* PBG RAID */
        { PCI_VDEVICE(INTEL, 0x1d06), board_ahci }, /* PBG RAID */
-       { PCI_VDEVICE(INTEL, 0x2826), board_ahci }, /* PBG/Lewisburg RAID*/
        { PCI_VDEVICE(INTEL, 0x2323), board_ahci }, /* DH89xxCC AHCI */
        { PCI_VDEVICE(INTEL, 0x1e02), board_ahci }, /* Panther Point AHCI */
        { PCI_VDEVICE(INTEL, 0x1e03), board_ahci_mobile }, /* Panther M AHCI */
@@ -367,7 +366,9 @@ static const struct pci_device_id ahci_pci_tbl[] = {
        { PCI_VDEVICE(INTEL, 0x1f3e), board_ahci_avn }, /* Avoton RAID */
        { PCI_VDEVICE(INTEL, 0x1f3f), board_ahci_avn }, /* Avoton RAID */
        { PCI_VDEVICE(INTEL, 0x2823), board_ahci }, /* Wellsburg/Lewisburg AHCI*/
-       { PCI_VDEVICE(INTEL, 0x2827), board_ahci }, /* Wellsburg/Lewisburg RAID*/
+       { PCI_VDEVICE(INTEL, 0x2826), board_ahci }, /* Server SATA0 "RAID" */
+       { PCI_VDEVICE(INTEL, 0x2827), board_ahci }, /* Server SATA1 "RAID" */
+       { PCI_VDEVICE(INTEL, 0x282f), board_ahci }, /* Server SATA2 "RAID" */
        { PCI_VDEVICE(INTEL, 0x43d4), board_ahci }, /* Rocket Lake PCH-H RAID */
        { PCI_VDEVICE(INTEL, 0x43d5), board_ahci }, /* Rocket Lake PCH-H RAID */
        { PCI_VDEVICE(INTEL, 0x43d6), board_ahci }, /* Rocket Lake PCH-H RAID */

I.e. ids that are compatible from one generation to the next.

While it is too late to intercept hardware shipping with this new SATA2
generic-id, the purely theoretical future SATA3+ instances are on
notice to avoid the need to come back and touch this file. I.e. any
additional controller(s) can just be 0x282f from here on out... for
server, for client platforms where there has not been a commitment to a
stable "RAID" id, the recommendation continues to be switch to AHCI
mode in the BIOS.

I hope this is sufficient to clarify that this SATA2 id should be the
last time a new RAID id needs to be added to this file, save for cases
where a quirk set beyond board_ahci needs to be applied (unlikely).