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

[Stephen Douthit]

On 8/30/19 12:00 PM, Dan Williams wrote:
> 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.

Got it.  I just assumed (incorrectly) the vendor ID was from the device
config space and not the match table.

In that case:
Reviewed-by: Stephen Douthit <stephend@xxxxxxxxxxxxxxx>