Re: [PATCH] PCI: Add quirk to clear MSI-X

[Basavaraj Natikar <bnatikar@xxxxxxx>]

On 3/9/2023 11:55 PM, Bjorn Helgaas wrote:
> On Thu, Mar 09, 2023 at 01:04:17PM +0530, Basavaraj Natikar wrote:
>> On 3/9/2023 4:34 AM, Limonciello, Mario wrote:
>>>> -----Original Message-----
>>>> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
>>>> Sent: Wednesday, March 8, 2023 16:44
>>>> To: Natikar, Basavaraj <Basavaraj.Natikar@xxxxxxx>
>>>> Cc: bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Limonciello, Mario
>>>> <Mario.Limonciello@xxxxxxx>; thomas@xxxxxxxxxxxx
>>>> Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X
>>>>
>>>> Let's mention the vendor and device name in the subject to make the
>>>> log more useful.
>> Sure will change subject as below.
>> Add quirk on AMD 0x15b8 device to clear MSI-X enable bit
> "0x15b8" is not really useful in a subject line.  Use a name
> meaningful to users, like something "lspci" reports (I don't see
> "1002:15b8" in https://pci-ids.ucw.cz/read/PC/1002; it would be nice
> to add it) or at least something like "USB controller".   You can look
> at the history of drivers/pci/quirks.c to see examples.

Thank you Bjorn for the reference , "lscpi" output

03:00.0 USB controller [0c03]: Advanced Micro Devices, Inc. [AMD] Device [1022:15b8] (prog-if 30 [XHCI])
        Subsystem: Advanced Micro Devices, Inc. [AMD] Device [1022:d601
..

will change subject as below then:

Add quirk on AMD USB Controller 15b8 to restore MSI-X enable bit
and change commit message accordingly

>> Yes correct, even though pci_restore_state restores all pci registers states
>> including MSI-X bits __pci_restore_msix_state after resume but internal AMD
>> controller's MSI_X enable bit is out of sync and AMD controller fails to maintain 
>> internal MSI-X enable bits.
> So the register value *change* is important, and you force a different
> value by writing something different at suspend-time so the value at
> restore-time will be different.  That's a little obscure since those
> points are far separated.
>
> Also it changes the behavior (masking MSI-X at suspend-time), which
> complicates the analysis since we have to verify that we don't need
> MSI-X after the quirk runs.  And the current quirk relies on the fact
> that PCI_MSIX_FLAGS_ENABLE is set, which again complicates the
> analysis (I guess if MSI-X is *not* enabled, you might not need the
> quirk at all?)
>
> Is there any way you could do the quirk at resume-time, e.g., if MSI-X
> is supposed to be enabled, first disable it and immediately re-enable
> it?

Yes agreed , I will change the quirk to apply in resume instead of
suspend which also resolves the issue as below i.e. restoring during
resume if MSI-X is enabled works.

static void quirk_restore_msix_en(struct pci_dev *dev)
{
        u16 ctrl;

        pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &ctrl);
        if (!(ctrl & PCI_MSIX_FLAGS_ENABLE))
                return;

        ctrl &= ~PCI_MSIX_FLAGS_ENABLE;
        pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, ctrl);
        ctrl |= PCI_MSIX_FLAGS_ENABLE;
        pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, ctrl);
}
DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_AMD, 0x15b8, quirk_restore_msix_en);

Thanks,
--
Basavaraj