Re: [PATCH v3] libata/ahci: Drop PCS quirk for Denverton and beyond

[Dan Williams]

On Fri, Aug 30, 2019 at 8:47 AM Stephen Douthit
<stephend@xxxxxxxxxxxxxxx> wrote:
>
> On 8/29/19 7:30 PM, Dan Williams wrote:
> > The Linux ahci driver has historically implemented a configuration fixup
> > for platforms / platform-firmware that fails to enable the ports prior
> > to OS hand-off at boot. The fixup was originally implemented way back
> > before ahci moved from drivers/scsi/ to drivers/ata/, and was updated in
> > 2007 via commit 49f290903935 "ahci: update PCS programming". The quirk
> > sets a port-enable bitmap in the PCS register at offset 0x92.
> >
> > This quirk could be applied generically up until the arrival of the
> > Denverton (DNV) platform. The DNV AHCI controller architecture supports
> > more than 6 ports and along with that the PCS register location and
> > format were updated to allow for more possible ports in the bitmap. DNV
> > AHCI expands the register to 32-bits and moves it to offset 0x94.
> >
> > As it stands there are no known problem reports with existing Linux
> > trying to set bits at offset 0x92 which indicates that the quirk is not
> > applicable. Likely it is not applicable on a wider range of platforms,
> > but it is difficult to discern which platforms if any still depend on
> > the quirk.
> >
> > Rather than try to fix the PCS quirk to consider the DNV register layout
> > instead require explicit opt-in. The assumption is that the OS driver
> > need not touch this register, and platforms can be added with a new
> > boad_ahci_pcs7 board-id when / if problematic platforms are found in the
> > future. The logic in ahci_intel_pcs_quirk() looks for all Intel AHCI
> > instances with "legacy" board-ids and otherwise skips the quirk if the
> > board was matched by class-code.
> >
> > Reported-by: Stephen Douthit <stephend@xxxxxxxxxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > ---
> > Changes since v2:
> > - Use board_ahci_pcs7 to opt Denverton out of the PCS quirk.
> >
> >   drivers/ata/ahci.c |  116 +++++++++++++++++++++++++++++++---------------------
> >   drivers/ata/ahci.h |    2 +
> >   2 files changed, 71 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> > index f7652baa6337..3e63294304c7 100644
> > --- a/drivers/ata/ahci.c
> > +++ b/drivers/ata/ahci.c
> > @@ -65,6 +65,12 @@ enum board_ids {
> >       board_ahci_sb700,       /* for SB700 and SB800 */
> >       board_ahci_vt8251,
> >
> > +     /*
> > +      * board IDs for Intel chipsets that support more than 6 ports
> > +      * *and* end up needing the PCS quirk.
> > +      */
> > +     board_ahci_pcs7,
> > +
> >       /* aliases */
> >       board_ahci_mcp_linux    = board_ahci_mcp65,
> >       board_ahci_mcp67        = board_ahci_mcp65,
> > @@ -220,6 +226,12 @@ static const struct ata_port_info ahci_port_info[] = {
> >               .udma_mask      = ATA_UDMA6,
> >               .port_ops       = &ahci_vt8251_ops,
> >       },
> > +     [board_ahci_pcs7] = {
> > +             .flags          = AHCI_FLAG_COMMON,
> > +             .pio_mask       = ATA_PIO4,
> > +             .udma_mask      = ATA_UDMA6,
> > +             .port_ops       = &ahci_ops,
> > +     },
> >   };
> >
> >   static const struct pci_device_id ahci_pci_tbl[] = {
> > @@ -264,26 +276,26 @@ static const struct pci_device_id ahci_pci_tbl[] = {
> >       { PCI_VDEVICE(INTEL, 0x3b2b), board_ahci }, /* PCH RAID */
> >       { PCI_VDEVICE(INTEL, 0x3b2c), board_ahci_mobile }, /* PCH M RAID */
> >       { PCI_VDEVICE(INTEL, 0x3b2f), board_ahci }, /* PCH AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19b0), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19b1), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19b2), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19b3), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19b4), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19b5), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19b6), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19b7), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19bE), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19bF), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19c0), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19c1), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19c2), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19c3), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19c4), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19c5), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19c6), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19c7), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19cE), board_ahci }, /* DNV AHCI */
> > -     { PCI_VDEVICE(INTEL, 0x19cF), board_ahci }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19b0), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19b1), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19b2), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19b3), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19b4), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19b5), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19b6), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19b7), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19bE), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19bF), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19c0), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19c1), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19c2), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19c3), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19c4), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19c5), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19c6), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19c7), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19cE), board_ahci_pcs7 }, /* DNV AHCI */
> > +     { PCI_VDEVICE(INTEL, 0x19cF), board_ahci_pcs7 }, /* DNV AHCI */
>
> I think you want to invert this scheme and mark the old controllers as
> board_ahci_pcs6 and leave Denverton/newer controllers as board_ahci.
>
> Otherwise the quirk below runs for any Intel controllers that matched
> based on the AHCI class code, since their board_id will be board_ahci
> which is < board_ahci_pcs7.

It shouldn't apply for the class code because the id->vendor in that
case will be PCI_ANY_ID not PCI_VENDOR_ID_INTEL.