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

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

[Basavaraj Natikar]

One of the AMD USB controllers fails to maintain internal functional
context when transitioning from D3 to D0, desynchronizing MSI-X bits.
As a result, add a quirk to this controller to clear the MSI-X bits
on suspend.

Note: This quirk works in all scenarios, regardless of whether the
integrated GPU is disabled in the BIOS.

Cc: Mario Limonciello <mario.limonciello@amd.com>
Reported-by: Thomas Glanzmann <thomas@glanzmann.de>
Link: https://lore.kernel.org/linux-usb/Y%2Fz9GdHjPyF2rNG3@glanzmann.de/T/#u
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/pci/quirks.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 44cab813bf95..ddf7100227d3 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6023,3 +6023,13 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d, dpc_log_size);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f, dpc_log_size);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
 #endif
+
+static void quirk_clear_msix(struct pci_dev *dev)
+{
+	u16 ctrl;
+
+	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &ctrl);
+	ctrl &= ~(PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE);
+	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, ctrl);
+}
+DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x15b8, quirk_clear_msix);
-- 
2.25.1

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

[Thomas Glanzmann]

Hello,

* Basavaraj Natikar <Basavaraj.Natikar@amd.com> [2023-03-06 08:24]:
> One of the AMD USB controllers fails to maintain internal functional
> context when transitioning from D3 to D0, desynchronizing MSI-X bits.
> As a result, add a quirk to this controller to clear the MSI-X bits
> on suspend.

> Note: This quirk works in all scenarios, regardless of whether the
> integrated GPU is disabled in the BIOS.

I tested the patch on top of v6.2.2 and USB hotplug works. Dmesg is here:

https://pbot.rmdir.de/H1BcUKbZUQQJ7NB6UylFEQ

> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Reported-by: Thomas Glanzmann <thomas@glanzmann.de>

Tested-by: Thomas Glanzmann <thomas@glanzmann.de>

> Link: https://lore.kernel.org/linux-usb/Y%2Fz9GdHjPyF2rNG3@glanzmann.de/T/#u
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> ---
>  drivers/pci/quirks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 44cab813bf95..ddf7100227d3 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6023,3 +6023,13 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d, dpc_log_size);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f, dpc_log_size);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
>  #endif
> +
> +static void quirk_clear_msix(struct pci_dev *dev)
> +{
> +	u16 ctrl;
> +
> +	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &ctrl);
> +	ctrl &= ~(PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE);
> +	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, ctrl);
> +}
> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x15b8, quirk_clear_msix);

Thank you for the patch.

Cheers,
        Thomas

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

[Bjorn Helgaas]

Let's mention the vendor and device name in the subject to make the
log more useful.

On Mon, Mar 06, 2023 at 12:53:40PM +0530, Basavaraj Natikar wrote:
> One of the AMD USB controllers fails to maintain internal functional
> context when transitioning from D3 to D0, desynchronizing MSI-X bits.
> As a result, add a quirk to this controller to clear the MSI-X bits
> on suspend.

Is this a documented erratum?  Please include a citation if so.

Are there any other AMD USB devices with the same defect?

The quick clears the Function Mask bit, so the MSI-X vectors may be
*unmasked* depending on the state of each vectors Mask bit.  I assume
the potential unmasking is safe because you also clear the MSI-X
Enable bit, so the function can't use MSI-X at all.

All state is lost in D3cold, so I guess this problem must occur during
a D3hot to D0 transition, right?  I assume this device sets
No_Soft_Reset, so the function is supposed to return to D0active with
all internal state intact.  But this device returns to D0active with
the MSI-X internal state corrupted?

I assume this relies on pci_restore_state() to restore the MSI-X
state.  Seems like that might be enough to restore the internal state
even without this quirk, but I guess it must not be.

> Note: This quirk works in all scenarios, regardless of whether the
> integrated GPU is disabled in the BIOS.

I don't know how the integrated GPU is related to this USB controller,
but I assume this fact is important somehow?

> Cc: Mario Limonciello <mario.limonciello@amd.com>
> Reported-by: Thomas Glanzmann <thomas@glanzmann.de>
> Link: https://lore.kernel.org/linux-usb/Y%2Fz9GdHjPyF2rNG3@glanzmann.de/T/#u

Apparently the symptom is one of these:

  xhci_hcd 0000:0c:00.0: Error while assigning device slot ID: Command Aborted
  xhci_hcd 0000:0c:00.0: Max number of devices this xHCI host supports is 64.
  usb usb1-port1: couldn't allocate usb_device
  xhci_hcd 0000:0c:00.0: WARN: xHC save state timeout
  xhci_hcd 0000:0c:00.0: PM: suspend_common(): xhci_pci_suspend+0x0/0x150 [xhci_pci] returns -110
  xhci_hcd 0000:0c:00.0: can't suspend (hcd_pci_runtime_suspend [usbcore] returned -110)

We should include the critical line or two in the commit log to help
users find the fix.

I see this must be xhci_suspend() returning -ETIMEDOUT after
xhci_save_registers(), but I don't see the connection from there to a
PCI_FIXUP_SUSPEND.  Can you connect the dots for me?

> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> ---
>  drivers/pci/quirks.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 44cab813bf95..ddf7100227d3 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6023,3 +6023,13 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d, dpc_log_size);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f, dpc_log_size);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
>  #endif
> +
> +static void quirk_clear_msix(struct pci_dev *dev)
> +{
> +	u16 ctrl;
> +
> +	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, &ctrl);
> +	ctrl &= ~(PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE);
> +	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS, ctrl);
> +}
> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x15b8, quirk_clear_msix);
> -- 
> 2.25.1
> 

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

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Wednesday, March 8, 2023 16:44
> To: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>
> Cc: bhelgaas@google.com; linux-pci@vger.kernel.org; Limonciello, Mario
> <Mario.Limonciello@amd.com>; thomas@glanzmann.de
> 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.
> 
> On Mon, Mar 06, 2023 at 12:53:40PM +0530, Basavaraj Natikar wrote:
> > One of the AMD USB controllers fails to maintain internal functional
> > context when transitioning from D3 to D0, desynchronizing MSI-X bits.
> > As a result, add a quirk to this controller to clear the MSI-X bits
> > on suspend.
> 
> Is this a documented erratum?  Please include a citation if so.
> 
> Are there any other AMD USB devices with the same defect?

FYI - it's not a hardware defect, it's a BIOS defect.

> 
> The quick clears the Function Mask bit, so the MSI-X vectors may be
> *unmasked* depending on the state of each vectors Mask bit.  I assume
> the potential unmasking is safe because you also clear the MSI-X
> Enable bit, so the function can't use MSI-X at all.
> 
> All state is lost in D3cold, so I guess this problem must occur during
> a D3hot to D0 transition, right?  I assume this device sets
> No_Soft_Reset, so the function is supposed to return to D0active with
> all internal state intact.  But this device returns to D0active with
> the MSI-X internal state corrupted?
> 
> I assume this relies on pci_restore_state() to restore the MSI-X
> state.  Seems like that might be enough to restore the internal state
> even without this quirk, but I guess it must not be.

The important part is the register value changing to make
the internal hardware move.  Because it restores identically it doesn't change
the internal hardware.

> 
> > Note: This quirk works in all scenarios, regardless of whether the
> > integrated GPU is disabled in the BIOS.
> 
> I don't know how the integrated GPU is related to this USB controller,
> but I assume this fact is important somehow?

This bug is due to a BIOS bug with the initialization.  We also posted in
parallel a different workaround that fixes the initialization to match what
the BIOS should have set via the GPU driver.  

It should be going in for 6.3-rc2.
https://gitlab.freedesktop.org/agd5f/linux/-/commit/07494a25fc8881e122c242a46b5c53e0e4403139

But because these are desktop processors, users can decide in BIOS setup
whether the integrated GPU should be enabled or disabled and that
workaround won't work if it's disabled.

> 
> > Cc: Mario Limonciello <mario.limonciello@amd.com>
> > Reported-by: Thomas Glanzmann <thomas@glanzmann.de>
> > Link: https://lore.kernel.org/linux-
> usb/Y%2Fz9GdHjPyF2rNG3@glanzmann.de/T/#u
> 
> Apparently the symptom is one of these:
> 
>   xhci_hcd 0000:0c:00.0: Error while assigning device slot ID: Command
> Aborted
>   xhci_hcd 0000:0c:00.0: Max number of devices this xHCI host supports is 64.
>   usb usb1-port1: couldn't allocate usb_device
>   xhci_hcd 0000:0c:00.0: WARN: xHC save state timeout
>   xhci_hcd 0000:0c:00.0: PM: suspend_common():
> xhci_pci_suspend+0x0/0x150 [xhci_pci] returns -110
>   xhci_hcd 0000:0c:00.0: can't suspend (hcd_pci_runtime_suspend [usbcore]
> returned -110)
> 
> We should include the critical line or two in the commit log to help
> users find the fix.
> 
> I see this must be xhci_suspend() returning -ETIMEDOUT after
> xhci_save_registers(), but I don't see the connection from there to a
> PCI_FIXUP_SUSPEND.  Can you connect the dots for me?
> 
> > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> > ---
> >  drivers/pci/quirks.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 44cab813bf95..ddf7100227d3 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -6023,3 +6023,13 @@
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d,
> dpc_log_size);
> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f,
> dpc_log_size);
> >  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31,
> dpc_log_size);
> >  #endif
> > +
> > +static void quirk_clear_msix(struct pci_dev *dev)
> > +{
> > +	u16 ctrl;
> > +
> > +	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
> &ctrl);
> > +	ctrl &= ~(PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE);
> > +	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
> ctrl);
> > +}
> > +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x15b8,
> quirk_clear_msix);
> > --
> > 2.25.1
> >

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

[Basavaraj Natikar]

On 3/9/2023 4:34 AM, Limonciello, Mario wrote:
> [Public]
>
>
>
>> -----Original Message-----
>> From: Bjorn Helgaas <helgaas@kernel.org>
>> Sent: Wednesday, March 8, 2023 16:44
>> To: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>
>> Cc: bhelgaas@google.com; linux-pci@vger.kernel.org; Limonciello, Mario
>> <Mario.Limonciello@amd.com>; thomas@glanzmann.de
>> 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

>>
>> On Mon, Mar 06, 2023 at 12:53:40PM +0530, Basavaraj Natikar wrote:
>>> One of the AMD USB controllers fails to maintain internal functional
>>> context when transitioning from D3 to D0, desynchronizing MSI-X bits.
>>> As a result, add a quirk to this controller to clear the MSI-X bits
>>> on suspend.
>> Is this a documented erratum?  Please include a citation if so.
>>
>> Are there any other AMD USB devices with the same defect?
> FYI - it's not a hardware defect, it's a BIOS defect.
>
>> The quick clears the Function Mask bit, so the MSI-X vectors may be
>> *unmasked* depending on the state of each vectors Mask bit.  I assume
>> the potential unmasking is safe because you also clear the MSI-X
>> Enable bit, so the function can't use MSI-X at all.

Sure, will remove Function Mask bit only clear MSI-X enable bit is enough,
actually MSI-X enable bit doesn't change the internal hardware and there
will be no interrupts after resume hence below command timeout and eventually
error observed more logs below:
[  418.572737] xhci_hcd 0000:03:00.0: xhci_hub_status_data: stopping usb5 port polling
*[ 423.724511] xhci_hcd 0000:03:00.0: Command timeout, USBSTS: 0x00000000****[ 423.724517] xhci_hcd 0000:03:00.0: Command timeout*
[  423.724519] xhci_hcd 0000:03:00.0: Abort command ring
[  425.740742] xhci_hcd 0000:03:00.0: No stop event for abort, ring start fail?
*[ 425.740771] xhci_hcd 0000:03:00.0: Error while assigning device slot ID****[ 425.740777] xhci_hcd 0000:03:00.0: Max number of devices this xHCI
host supports is 64*.
[  425.740782] usb usb5-port1: couldn't allocate usb_device
[  425.740794] xhci_hcd 0000:03:00.0: disable port 5-1, portsc: 0x6e1
[  425.740818] hub 5-0:1.0: hub_suspend
[  425.740826] usb usb5: bus auto-suspend, wakeup 1
[  425.740835] xhci_hcd 0000:03:00.0: xhci_hub_status_data: stopping usb5 port polling
[  425.740842] xhci_hcd 0000:03:00.0: xhci_suspend: stopping usb5 port polling.
[  425.756878] xhci_hcd 0000:03:00.0: // Setting command ring address to 0xffffe001
[  425.776898] xhci_hcd 0000:03:00.0: WARN: xHC save state timeout
[  425.776910] xhci_hcd 0000:03:00.0: PM: suspend_common(): xhci_pci_suspend+0x0/0x170 [xhci_pci] returns -110
[  425.776917] xhci_hcd 0000:03:00.0: hcd_pci_runtime_suspend: -110
[  425.776918] xhci_hcd 0000:03:00.0: can't suspend (hcd_pci_runtime_suspend returned -110)


will change function name accordingly quirk_clear_msix_en
and with only ctrl &= ~PCI_MSIX_FLAGS_ENABLE;

>>
>> All state is lost in D3cold, so I guess this problem must occur during
>> a D3hot to D0 transition, right?  I assume this device sets
>> No_Soft_Reset, so the function is supposed to return to D0active with
>> all internal state intact.  But this device returns to D0active with
>> the MSI-X internal state corrupted?
>>
>> I assume this relies on pci_restore_state() to restore the MSI-X
>> state.  Seems like that might be enough to restore the internal state
>> even without this quirk, but I guess it must not be.
> The important part is the register value changing to make
> the internal hardware move.  Because it restores identically it doesn't change
> the internal hardware.

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.

>
>>> Note: This quirk works in all scenarios, regardless of whether the
>>> integrated GPU is disabled in the BIOS.
>> I don't know how the integrated GPU is related to this USB controller,
>> but I assume this fact is important somehow?
> This bug is due to a BIOS bug with the initialization.  We also posted in
> parallel a different workaround that fixes the initialization to match what
> the BIOS should have set via the GPU driver.  
>
> It should be going in for 6.3-rc2.
> https://gitlab.freedesktop.org/agd5f/linux/-/commit/07494a25fc8881e122c242a46b5c53e0e4403139
>
> But because these are desktop processors, users can decide in BIOS setup
> whether the integrated GPU should be enabled or disabled and that
> workaround won't work if it's disabled.
>
>>> Cc: Mario Limonciello <mario.limonciello@amd.com>
>>> Reported-by: Thomas Glanzmann <thomas@glanzmann.de>
>>> Link: https://lore.kernel.org/linux-
>> usb/Y%2Fz9GdHjPyF2rNG3@glanzmann.de/T/#u
>>
>> Apparently the symptom is one of these:
>>
>>   xhci_hcd 0000:0c:00.0: Error while assigning device slot ID: Command
>> Aborted
>>   xhci_hcd 0000:0c:00.0: Max number of devices this xHCI host supports is 64.
>>   usb usb1-port1: couldn't allocate usb_device
>>   xhci_hcd 0000:0c:00.0: WARN: xHC save state timeout
>>   xhci_hcd 0000:0c:00.0: PM: suspend_common():
>> xhci_pci_suspend+0x0/0x150 [xhci_pci] returns -110
>>   xhci_hcd 0000:0c:00.0: can't suspend (hcd_pci_runtime_suspend [usbcore]
>> returned -110)
>>
>> We should include the critical line or two in the commit log to help
>> users find the fix.
>>
>> I see this must be xhci_suspend() returning -ETIMEDOUT after
>> xhci_save_registers(), but I don't see the connection from there to a
>> PCI_FIXUP_SUSPEND.  Can you connect the dots for me?

Enabled more verbose logs in above comment for actual timeout due to MSI-X 
interrupt disabled internally even though in register shows MSI-X enabled
due to internal hardware de-synchronizing MSI-X enable bits on suspend so
explicit clear of MSI-X during suspend help to maintain and restores both 
internal register in sync with MSI-X enable bit.

[  418.572737] xhci_hcd 0000:03:00.0: xhci_hub_status_data: stopping usb5 port polling
*[ 423.724511] xhci_hcd 0000:03:00.0: Command timeout, USBSTS: 0x00000000****[ 423.724517] xhci_hcd 0000:03:00.0: Command timeout*
[  423.724519] xhci_hcd 0000:03:00.0: Abort command ring

>>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>>> ---
>>>  drivers/pci/quirks.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>>> index 44cab813bf95..ddf7100227d3 100644
>>> --- a/drivers/pci/quirks.c
>>> +++ b/drivers/pci/quirks.c
>>> @@ -6023,3 +6023,13 @@
>> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d,
>> dpc_log_size);
>>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f,
>> dpc_log_size);
>>>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31,
>> dpc_log_size);
>>>  #endif
>>> +
>>> +static void quirk_clear_msix(struct pci_dev *dev)
>>> +{
>>> +	u16 ctrl;
>>> +
>>> +	pci_read_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
>> &ctrl);
>>> +	ctrl &= ~(PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE);
>>> +	pci_write_config_word(dev, dev->msix_cap + PCI_MSIX_FLAGS,
>> ctrl);
>>> +}
>>> +DECLARE_PCI_FIXUP_SUSPEND(PCI_VENDOR_ID_AMD, 0x15b8,
>> quirk_clear_msix);
>>> --
>>> 2.25.1
>>>

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

[Bjorn Helgaas]

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@kernel.org>
> >> Sent: Wednesday, March 8, 2023 16:44
> >> To: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>
> >> Cc: bhelgaas@google.com; linux-pci@vger.kernel.org; Limonciello, Mario
> >> <Mario.Limonciello@amd.com>; thomas@glanzmann.de
> >> 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.

> >> On Mon, Mar 06, 2023 at 12:53:40PM +0530, Basavaraj Natikar wrote:
> >>> One of the AMD USB controllers fails to maintain internal functional
> >>> context when transitioning from D3 to D0, desynchronizing MSI-X bits.
> >>> As a result, add a quirk to this controller to clear the MSI-X bits
> >>> on suspend.
> > ...
> > FYI - it's not a hardware defect, it's a BIOS defect.

Commit log ("controller fails to maintain") suggested hardware defect
to me; maybe could be clarified.  If it's a defect in the way BIOS
initialized something, maybe the workaround could be a one-time thing
instead of an every-resume quirk?

> >> The quick clears the Function Mask bit, so the MSI-X vectors may be
> >> *unmasked* depending on the state of each vectors Mask bit.  I assume
> >> the potential unmasking is safe because you also clear the MSI-X
> >> Enable bit, so the function can't use MSI-X at all.
> 
> Sure, will remove Function Mask bit only clear MSI-X enable bit is enough,
> actually MSI-X enable bit doesn't change the internal hardware and there
> will be no interrupts after resume hence below command timeout and eventually
> error observed more logs below:
>
> [  418.572737] xhci_hcd 0000:03:00.0: xhci_hub_status_data: stopping usb5 port polling
> *[ 423.724511] xhci_hcd 0000:03:00.0: Command timeout, USBSTS: 0x00000000****[ 423.724517] xhci_hcd 0000:03:00.0: Command timeout*
> [  423.724519] xhci_hcd 0000:03:00.0: Abort command ring
> [  425.740742] xhci_hcd 0000:03:00.0: No stop event for abort, ring start fail?
> *[ 425.740771] xhci_hcd 0000:03:00.0: Error while assigning device slot ID****[ 425.740777] xhci_hcd 0000:03:00.0: Max number of devices this xHCI
> host supports is 64*.
> [  425.740782] usb usb5-port1: couldn't allocate usb_device
> [  425.740794] xhci_hcd 0000:03:00.0: disable port 5-1, portsc: 0x6e1
> [  425.740818] hub 5-0:1.0: hub_suspend
> [  425.740826] usb usb5: bus auto-suspend, wakeup 1
> [  425.740835] xhci_hcd 0000:03:00.0: xhci_hub_status_data: stopping usb5 port polling
> [  425.740842] xhci_hcd 0000:03:00.0: xhci_suspend: stopping usb5 port polling.
> [  425.756878] xhci_hcd 0000:03:00.0: // Setting command ring address to 0xffffe001
> [  425.776898] xhci_hcd 0000:03:00.0: WARN: xHC save state timeout
> [  425.776910] xhci_hcd 0000:03:00.0: PM: suspend_common(): xhci_pci_suspend+0x0/0x170 [xhci_pci] returns -110
> [  425.776917] xhci_hcd 0000:03:00.0: hcd_pci_runtime_suspend: -110
> [  425.776918] xhci_hcd 0000:03:00.0: can't suspend (hcd_pci_runtime_suspend returned -110)
> 
> will change function name accordingly quirk_clear_msix_en
> and with only ctrl &= ~PCI_MSIX_FLAGS_ENABLE;
> 
> >> All state is lost in D3cold, so I guess this problem must occur during
> >> a D3hot to D0 transition, right?  I assume this device sets
> >> No_Soft_Reset, so the function is supposed to return to D0active with
> >> all internal state intact.  But this device returns to D0active with
> >> the MSI-X internal state corrupted?
> >>
> >> I assume this relies on pci_restore_state() to restore the MSI-X
> >> state.  Seems like that might be enough to restore the internal state
> >> even without this quirk, but I guess it must not be.
> >
> > The important part is the register value changing to make
> > the internal hardware move.  Because it restores identically it doesn't change
> > the internal hardware.
> 
> 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?

> >>> Note: This quirk works in all scenarios, regardless of whether the
> >>> integrated GPU is disabled in the BIOS.
> >>
> >> I don't know how the integrated GPU is related to this USB controller,
> >> but I assume this fact is important somehow?
> >
> > This bug is due to a BIOS bug with the initialization.  We also posted in
> > parallel a different workaround that fixes the initialization to match what
> > the BIOS should have set via the GPU driver.  
> >
> > It should be going in for 6.3-rc2.
> > https://gitlab.freedesktop.org/agd5f/linux/-/commit/07494a25fc8881e122c242a46b5c53e0e4403139

That nbio_v7.2.c patch and this patch don't look anything alike.  It
looks like the nbio_v7.2.c patch might run once?  Could *this* be done
once at enumeration-time, too?

Bjorn

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

[Limonciello, Mario]

On 3/9/2023 12:25, 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@kernel.org>
>>>> Sent: Wednesday, March 8, 2023 16:44
>>>> To: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>
>>>> Cc: bhelgaas@google.com; linux-pci@vger.kernel.org; Limonciello, Mario
>>>> <Mario.Limonciello@amd.com>; thomas@glanzmann.de
>>>> 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.
> 
>>>> On Mon, Mar 06, 2023 at 12:53:40PM +0530, Basavaraj Natikar wrote:
>>>>> One of the AMD USB controllers fails to maintain internal functional
>>>>> context when transitioning from D3 to D0, desynchronizing MSI-X bits.
>>>>> As a result, add a quirk to this controller to clear the MSI-X bits
>>>>> on suspend.
>>> ...
>>> FYI - it's not a hardware defect, it's a BIOS defect.
> 
> Commit log ("controller fails to maintain") suggested hardware defect
> to me; maybe could be clarified.  If it's a defect in the way BIOS
> initialized something, maybe the workaround could be a one-time thing
> instead of an every-resume quirk?
> 
>>>> The quick clears the Function Mask bit, so the MSI-X vectors may be
>>>> *unmasked* depending on the state of each vectors Mask bit.  I assume
>>>> the potential unmasking is safe because you also clear the MSI-X
>>>> Enable bit, so the function can't use MSI-X at all.
>>
>> Sure, will remove Function Mask bit only clear MSI-X enable bit is enough,
>> actually MSI-X enable bit doesn't change the internal hardware and there
>> will be no interrupts after resume hence below command timeout and eventually
>> error observed more logs below:
>>
>> [  418.572737] xhci_hcd 0000:03:00.0: xhci_hub_status_data: stopping usb5 port polling
>> *[ 423.724511] xhci_hcd 0000:03:00.0: Command timeout, USBSTS: 0x00000000****[ 423.724517] xhci_hcd 0000:03:00.0: Command timeout*
>> [  423.724519] xhci_hcd 0000:03:00.0: Abort command ring
>> [  425.740742] xhci_hcd 0000:03:00.0: No stop event for abort, ring start fail?
>> *[ 425.740771] xhci_hcd 0000:03:00.0: Error while assigning device slot ID****[ 425.740777] xhci_hcd 0000:03:00.0: Max number of devices this xHCI
>> host supports is 64*.
>> [  425.740782] usb usb5-port1: couldn't allocate usb_device
>> [  425.740794] xhci_hcd 0000:03:00.0: disable port 5-1, portsc: 0x6e1
>> [  425.740818] hub 5-0:1.0: hub_suspend
>> [  425.740826] usb usb5: bus auto-suspend, wakeup 1
>> [  425.740835] xhci_hcd 0000:03:00.0: xhci_hub_status_data: stopping usb5 port polling
>> [  425.740842] xhci_hcd 0000:03:00.0: xhci_suspend: stopping usb5 port polling.
>> [  425.756878] xhci_hcd 0000:03:00.0: // Setting command ring address to 0xffffe001
>> [  425.776898] xhci_hcd 0000:03:00.0: WARN: xHC save state timeout
>> [  425.776910] xhci_hcd 0000:03:00.0: PM: suspend_common(): xhci_pci_suspend+0x0/0x170 [xhci_pci] returns -110
>> [  425.776917] xhci_hcd 0000:03:00.0: hcd_pci_runtime_suspend: -110
>> [  425.776918] xhci_hcd 0000:03:00.0: can't suspend (hcd_pci_runtime_suspend returned -110)
>>
>> will change function name accordingly quirk_clear_msix_en
>> and with only ctrl &= ~PCI_MSIX_FLAGS_ENABLE;
>>
>>>> All state is lost in D3cold, so I guess this problem must occur during
>>>> a D3hot to D0 transition, right?  I assume this device sets
>>>> No_Soft_Reset, so the function is supposed to return to D0active with
>>>> all internal state intact.  But this device returns to D0active with
>>>> the MSI-X internal state corrupted?
>>>>
>>>> I assume this relies on pci_restore_state() to restore the MSI-X
>>>> state.  Seems like that might be enough to restore the internal state
>>>> even without this quirk, but I guess it must not be.
>>>
>>> The important part is the register value changing to make
>>> the internal hardware move.  Because it restores identically it doesn't change
>>> the internal hardware.
>>
>> 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?
> 
>>>>> Note: This quirk works in all scenarios, regardless of whether the
>>>>> integrated GPU is disabled in the BIOS.
>>>>
>>>> I don't know how the integrated GPU is related to this USB controller,
>>>> but I assume this fact is important somehow?
>>>
>>> This bug is due to a BIOS bug with the initialization.  We also posted in
>>> parallel a different workaround that fixes the initialization to match what
>>> the BIOS should have set via the GPU driver.
>>>
>>> It should be going in for 6.3-rc2.
>>> https://gitlab.freedesktop.org/agd5f/linux/-/commit/07494a25fc8881e122c242a46b5c53e0e4403139
> 
> That nbio_v7.2.c patch and this patch don't look anything alike.  It
> looks like the nbio_v7.2.c patch might run once?  Could *this* be done
> once at enumeration-time, too?
> 

They don't look anything alike because they're attacking the problem 
from different angles.

The NBIO patch fixes the initialization value for the internal 
registers.  This is what the BIOS "should" have done.  When the internal 
registers are configured properly then the behavior the kernel expects 
works as well.

The NBIO patch will run both at amdgpu startup as well as when resuming 
from suspend.

This patch we're discussing treats the symptoms of the deficiency and 
avoids the impact.
This patch runs any time the controller is runtime resumed.  So, yes it 
will run more frequently.  Because this patch is treating the symptoms 
it needs to be applied every single time the controller exits D3.

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

[Bjorn Helgaas]

On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
> On 3/9/2023 12:25, Bjorn Helgaas wrote:
> ...

> > > > https://gitlab.freedesktop.org/agd5f/linux/-/commit/07494a25fc8881e122c242a46b5c53e0e4403139
> > 
> > That nbio_v7.2.c patch and this patch don't look anything alike.  It
> > looks like the nbio_v7.2.c patch might run once?  Could *this* be done
> > once at enumeration-time, too?
> 
> They don't look anything alike because they're attacking the problem from
> different angles.

Why do we need different angles?

> The NBIO patch fixes the initialization value for the internal registers.
> This is what the BIOS "should" have done.  When the internal registers are
> configured properly then the behavior the kernel expects works as well.
> 
> The NBIO patch will run both at amdgpu startup as well as when resuming from
> suspend.

If initializing something as BIOS should have done makes the hardware
work correctly, isn't once enough?  Why does the NBIO patch need to
run at resume-time?

> This patch we're discussing treats the symptoms of the deficiency and avoids
> the impact.
> This patch runs any time the controller is runtime resumed.  So, yes it will
> run more frequently.  Because this patch is treating the symptoms it needs
> to be applied every single time the controller exits D3.

This patch runs at *suspend*-time (DECLARE_PCI_FIXUP_SUSPEND), not
resume-time.

The difference is important because with this broken BIOS, MSI-X is
disabled between the suspend quirk and some distant point in resume.
With non-broken BIOS, MSI-X remains *enabled* for at least part of
that period, and I don't want to have to figure out whether that
difference is important.

We have fragments of a coherent commit log, but it's not quite a
complete story yet.  I think so far we have:

  - Issue affects only the 1022:15b8 USB controller (well, I guess it
    also affects some GPU device?)
  - Only a problem when BIOS doesn't initialize controller correctly
  - Controller claims to preserve internal state on D3hot->D0
    transition, but it doesn't
  - D0->D3hot->D0 transitions do preserve external PCI_MSIX_FLAGS
    state; only internal state is lost
  - When MSI-X is enabled and controller transitions D0->D3hot->D0,
    MSI-X appears enabled per PCI_MSIX_FLAGS, but is actually
    *disabled* because the internal state was lost
  - MSI-X being disabled leads to xhci_hcd command timeouts because
    interrupts are missed
  - Not possible for an enumeration-time quirk to fix the controller
    initialization problem (why not?)
  - Writing PCI_MSIX_FLAGS with a *different* value fixes the internal
    state; writing the same value does nothing
  - A suspend- or resume-time quirk can work around this, and this is
    safe on *all* 1022:15b8 devices regardless of whether the BIOS is
    broken
  - The same approach can't be used for both 1022:15b8 and the GPU
    device because ...?

Bjorn

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

[Mario Limonciello]

On 3/9/23 16:30, Bjorn Helgaas wrote:
> On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
>> On 3/9/2023 12:25, Bjorn Helgaas wrote:
>> ...
> 
>>>>> https://gitlab.freedesktop.org/agd5f/linux/-/commit/07494a25fc8881e122c242a46b5c53e0e4403139
>>>
>>> That nbio_v7.2.c patch and this patch don't look anything alike.  It
>>> looks like the nbio_v7.2.c patch might run once?  Could *this* be done
>>> once at enumeration-time, too?
>>
>> They don't look anything alike because they're attacking the problem from
>> different angles.
> 
> Why do we need different angles?

The GPU driver approach only works if the GPU is enabled.  If the GPU 
could never be disabled then it alone would be sufficient.

> 
>> The NBIO patch fixes the initialization value for the internal registers.
>> This is what the BIOS "should" have done.  When the internal registers are
>> configured properly then the behavior the kernel expects works as well.
>>
>> The NBIO patch will run both at amdgpu startup as well as when resuming from
>> suspend.
> 
> If initializing something as BIOS should have done makes the hardware
> work correctly, isn't once enough?  Why does the NBIO patch need to
> run at resume-time?

During suspend some internal registers are in a power domain that the 
state will be lost.  These are typically restored by the BIOS to the 
values defined in initialization tables before handing control back to 
the OS.


> 
>> This patch we're discussing treats the symptoms of the deficiency and avoids
>> the impact.
>> This patch runs any time the controller is runtime resumed.  So, yes it will
>> run more frequently.  Because this patch is treating the symptoms it needs
>> to be applied every single time the controller exits D3.
> 
> This patch runs at *suspend*-time (DECLARE_PCI_FIXUP_SUSPEND), not
> resume-time.
> 
> The difference is important because with this broken BIOS, MSI-X is
> disabled between the suspend quirk and some distant point in resume.
> With non-broken BIOS, MSI-X remains *enabled* for at least part of
> that period, and I don't want to have to figure out whether that
> difference is important.

I'll let Basavaraj comment on the timing here with the behavior 
workaround and sequence of events.

> 
> We have fragments of a coherent commit log, but it's not quite a
> complete story yet.  I think so far we have:
> 
>    - Issue affects only the 1022:15b8 USB controller (well, I guess it
>      also affects some GPU device?)

Same device.  It's just a way to access the internal registers.

>    - Only a problem when BIOS doesn't initialize controller correctly
>    - Controller claims to preserve internal state on D3hot->D0
>      transition, but it doesn't
>    - D0->D3hot->D0 transitions do preserve external PCI_MSIX_FLAGS
>      state; only internal state is lost
>    - When MSI-X is enabled and controller transitions D0->D3hot->D0,
>      MSI-X appears enabled per PCI_MSIX_FLAGS, but is actually
>      *disabled* because the internal state was lost
>    - MSI-X being disabled leads to xhci_hcd command timeouts because
>      interrupts are missed
>    - Not possible for an enumeration-time quirk to fix the controller
>      initialization problem (why not?)
>    - Writing PCI_MSIX_FLAGS with a *different* value fixes the internal
>      state; writing the same value does nothing
>    - A suspend- or resume-time quirk can work around this, and this is
>      safe on *all* 1022:15b8 devices regardless of whether the BIOS is
>      broken
>    - The same approach can't be used for both 1022:15b8 and the GPU
>      device because ...?
> 
> Bjorn

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

[Basavaraj Natikar]

On 3/10/2023 6:27 AM, Mario Limonciello wrote:
> On 3/9/23 16:30, Bjorn Helgaas wrote:
>> On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
>>> On 3/9/2023 12:25, Bjorn Helgaas wrote:
>>> ...
>>
>>>>>> https://gitlab.freedesktop.org/agd5f/linux/-/commit/07494a25fc8881e122c242a46b5c53e0e4403139
>>>>>>
>>>>
>>>> That nbio_v7.2.c patch and this patch don't look anything alike.  It
>>>> looks like the nbio_v7.2.c patch might run once?  Could *this* be done
>>>> once at enumeration-time, too?
>>>
>>> They don't look anything alike because they're attacking the problem
>>> from
>>> different angles.
>>
>> Why do we need different angles?
>
> The GPU driver approach only works if the GPU is enabled.  If the GPU
> could never be disabled then it alone would be sufficient.
>
>>
>>> The NBIO patch fixes the initialization value for the internal
>>> registers.
>>> This is what the BIOS "should" have done.  When the internal
>>> registers are
>>> configured properly then the behavior the kernel expects works as well.
>>>
>>> The NBIO patch will run both at amdgpu startup as well as when
>>> resuming from
>>> suspend.
>>
>> If initializing something as BIOS should have done makes the hardware
>> work correctly, isn't once enough?  Why does the NBIO patch need to
>> run at resume-time?
>
> During suspend some internal registers are in a power domain that the
> state will be lost.  These are typically restored by the BIOS to the
> values defined in initialization tables before handing control back to
> the OS.
>
>
>>
>>> This patch we're discussing treats the symptoms of the deficiency
>>> and avoids
>>> the impact.
>>> This patch runs any time the controller is runtime resumed.  So, yes
>>> it will
>>> run more frequently.  Because this patch is treating the symptoms it
>>> needs
>>> to be applied every single time the controller exits D3.
>>
>> This patch runs at *suspend*-time (DECLARE_PCI_FIXUP_SUSPEND), not
>> resume-time.
>>
>> The difference is important because with this broken BIOS, MSI-X is
>> disabled between the suspend quirk and some distant point in resume.
>> With non-broken BIOS, MSI-X remains *enabled* for at least part of
>> that period, and I don't want to have to figure out whether that
>> difference is important.
>
> I'll let Basavaraj comment on the timing here with the behavior
> workaround and sequence of events.

As replied in the previous mail, Bjron's suggestion works well and holds good so 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);


Will change the commit message accordingly.

I guess for the questions below we already answered.
Please let us know if you need more clarifications.


Thanks,
--
Basavaraj

>
>>
>> We have fragments of a coherent commit log, but it's not quite a
>> complete story yet.  I think so far we have:
>>
>>    - Issue affects only the 1022:15b8 USB controller (well, I guess it
>>      also affects some GPU device?)
>
> Same device.  It's just a way to access the internal registers.
>
>>    - Only a problem when BIOS doesn't initialize controller correctly
>>    - Controller claims to preserve internal state on D3hot->D0
>>      transition, but it doesn't
>>    - D0->D3hot->D0 transitions do preserve external PCI_MSIX_FLAGS
>>      state; only internal state is lost
>>    - When MSI-X is enabled and controller transitions D0->D3hot->D0,
>>      MSI-X appears enabled per PCI_MSIX_FLAGS, but is actually
>>      *disabled* because the internal state was lost
>>    - MSI-X being disabled leads to xhci_hcd command timeouts because
>>      interrupts are missed
>>    - Not possible for an enumeration-time quirk to fix the controller
>>      initialization problem (why not?)
>>    - Writing PCI_MSIX_FLAGS with a *different* value fixes the internal
>>      state; writing the same value does nothing
>>    - A suspend- or resume-time quirk can work around this, and this is
>>      safe on *all* 1022:15b8 devices regardless of whether the BIOS is
>>      broken
>>    - The same approach can't be used for both 1022:15b8 and the GPU
>>      device because ...?
>>
>> Bjorn
>

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

[Bjorn Helgaas]

On Thu, Mar 09, 2023 at 06:57:38PM -0600, Mario Limonciello wrote:
> On 3/9/23 16:30, Bjorn Helgaas wrote:
> > On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
> > > On 3/9/2023 12:25, Bjorn Helgaas wrote:
> > > ...
> > 
> > > > > > https://gitlab.freedesktop.org/agd5f/linux/-/commit/07494a25fc8881e122c242a46b5c53e0e4403139
> > > > 
> > > > That nbio_v7.2.c patch and this patch don't look anything alike.  It
> > > > looks like the nbio_v7.2.c patch might run once?  Could *this* be done
> > > > once at enumeration-time, too?
> > > 
> > > They don't look anything alike because they're attacking the problem from
> > > different angles.
> > 
> > Why do we need different angles?
> 
> The GPU driver approach only works if the GPU is enabled.  If the GPU could
> never be disabled then it alone would be sufficient.
> 
> > > The NBIO patch fixes the initialization value for the internal registers.
> > > This is what the BIOS "should" have done.  When the internal registers are
> > > configured properly then the behavior the kernel expects works as well.
> > > 
> > > The NBIO patch will run both at amdgpu startup as well as when resuming from
> > > suspend.
> > 
> > If initializing something as BIOS should have done makes the hardware
> > work correctly, isn't once enough?  Why does the NBIO patch need to
> > run at resume-time?
> 
> During suspend some internal registers are in a power domain that the state
> will be lost.  These are typically restored by the BIOS to the values
> defined in initialization tables before handing control back to the OS.

I don't quite get this.  I thought I read that if BIOS had initialized
the hardware correctly, a D0->D3hot->D0 transition would work without
any issues.  Linux can do this with PMCSR writes and BIOS isn't
involved at all.

Bjorn

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

[Limonciello, Mario]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Friday, March 10, 2023 16:14
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>; Natikar, Basavaraj
> <Basavaraj.Natikar@amd.com>; bhelgaas@google.com; linux-
> pci@vger.kernel.org; thomas@glanzmann.de
> Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X
> 
> On Thu, Mar 09, 2023 at 06:57:38PM -0600, Mario Limonciello wrote:
> > On 3/9/23 16:30, Bjorn Helgaas wrote:
> > > On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
> > > > On 3/9/2023 12:25, Bjorn Helgaas wrote:
> > > > ...
> > >
> > > > > > > https://gitlab.freedesktop.org/agd5f/linux/-
> /commit/07494a25fc8881e122c242a46b5c53e0e4403139
> > > > >
> > > > > That nbio_v7.2.c patch and this patch don't look anything alike.  It
> > > > > looks like the nbio_v7.2.c patch might run once?  Could *this* be
> done
> > > > > once at enumeration-time, too?
> > > >
> > > > They don't look anything alike because they're attacking the problem
> from
> > > > different angles.
> > >
> > > Why do we need different angles?
> >
> > The GPU driver approach only works if the GPU is enabled.  If the GPU
> could
> > never be disabled then it alone would be sufficient.
> >
> > > > The NBIO patch fixes the initialization value for the internal registers.
> > > > This is what the BIOS "should" have done.  When the internal registers
> are
> > > > configured properly then the behavior the kernel expects works as
> well.
> > > >
> > > > The NBIO patch will run both at amdgpu startup as well as when
> resuming from
> > > > suspend.
> > >
> > > If initializing something as BIOS should have done makes the hardware
> > > work correctly, isn't once enough?  Why does the NBIO patch need to
> > > run at resume-time?
> >
> > During suspend some internal registers are in a power domain that the
> state
> > will be lost.  These are typically restored by the BIOS to the values
> > defined in initialization tables before handing control back to the OS.
> 
> I don't quite get this.  I thought I read that if BIOS had initialized
> the hardware correctly, a D0->D3hot->D0 transition would work without
> any issues.  Linux can do this with PMCSR writes and BIOS isn't
> involved at all.
> 
> Bjorn

During a suspend transition not all registers are powered.  Firmware will 
capture some during the suspend transition and restore some of them
for the resume transition, but it's up to the firmware whether this one is
included.

Furthermore most IP blocks in amdgpu typically initialize the same during both
startup and resume to ensure that firmware couldn't have mucked with
the expected golden state.

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

[Bjorn Helgaas]

[+cc Rafael for RESUME_EARLY quirk question]

On Mon, Mar 20, 2023 at 01:32:16AM +0000, Limonciello, Mario wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: Friday, March 10, 2023 16:14
> > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>; Natikar, Basavaraj
> > <Basavaraj.Natikar@amd.com>; bhelgaas@google.com; linux-
> > pci@vger.kernel.org; thomas@glanzmann.de
> > Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X
> > 
> > On Thu, Mar 09, 2023 at 06:57:38PM -0600, Mario Limonciello wrote:
> > > On 3/9/23 16:30, Bjorn Helgaas wrote:
> > > > On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
> > > > > On 3/9/2023 12:25, Bjorn Helgaas wrote:
> > > > > ...
> > > >
> > > > > > > > https://gitlab.freedesktop.org/agd5f/linux/-
> > /commit/07494a25fc8881e122c242a46b5c53e0e4403139
> > > > > >
> > > > > > That nbio_v7.2.c patch and this patch don't look anything
> > > > > > alike.  It looks like the nbio_v7.2.c patch might run
> > > > > > once?  Could *this* be done once at enumeration-time, too?
> > > > >
> > > > > They don't look anything alike because they're attacking the
> > > > > problem from different angles.
> > > >
> > > > Why do we need different angles?
> > >
> > > The GPU driver approach only works if the GPU is enabled.  If
> > > the GPU could never be disabled then it alone would be
> > > sufficient.
> > >
> > > > > The NBIO patch fixes the initialization value for the
> > > > > internal registers.  This is what the BIOS "should" have
> > > > > done.  When the internal registers are configured properly
> > > > > then the behavior the kernel expects works as well.
> > > > >
> > > > > The NBIO patch will run both at amdgpu startup as well as
> > > > > when resuming from suspend.
> > > >
> > > > If initializing something as BIOS should have done makes the
> > > > hardware work correctly, isn't once enough?  Why does the NBIO
> > > > patch need to run at resume-time?
> > >
> > > During suspend some internal registers are in a power domain
> > > that the state will be lost.  These are typically restored by
> > > the BIOS to the values defined in initialization tables before
> > > handing control back to the OS.
> > 
> > I don't quite get this.  I thought I read that if BIOS had
> > initialized the hardware correctly, a D0->D3hot->D0 transition
> > would work without any issues.  Linux can do this with PMCSR
> > writes and BIOS isn't involved at all.
> 
> During a suspend transition not all registers are powered.  Firmware
> will capture some during the suspend transition and restore some of
> them for the resume transition, but it's up to the firmware whether
> this one is included.
> 
> Furthermore most IP blocks in amdgpu typically initialize the same
> during both startup and resume to ensure that firmware couldn't have
> mucked with the expected golden state.

We're spending way more time on this than makes sense, but I do think
it's important that the commit log is accurate and makes sense even to
people who don't know the internals of the device.

It *sounds* like what's happening is:

  - OS writes PMCSR to put device in D3hot
  - BIOS traps D0->D3hot transition via something like SMI and
    captures MSI-X state
  - Device enters D3hot
  - Device internal MSI-X state is lost
  - BIOS traps D3hot->D0 transition via SMI
  - Device enters D0
  - BIOS restores MSI-X state
  - OS resumes use of device

If that's what's happening, the fact that the device loses the
internal state in D3hot sounds like a *hardware* defect -- if you put
the device in a system without a BIOS, the D0->D3hot->D0 transitions
would not work as required by the PCIe spec.

We can call the fact that BIOS lacks the MSI-X save/restore a BIOS
defect, but the only reason BIOS would *need* that save/restore is
because of the underlying *hardware* defect.

If that's the case, I would expect a commit log something like this:

  The AMD [1022:15b8] USB controller loses some internal functional
  MSI-X context when transitioning from D0 to D3hot.  BIOS normally
  traps D0->D3hot and D3hot->D0 transitions so it can save and restore
  that internal context, but some firmware in the field lacks this
  workaround.

  If MSI-X is enabled, toggle the PCI_MSIX_FLAGS_ENABLE bit when
  resuming to D0, which resynchronizes the internal state that was
  lost in D3hot.

Rafael, do we run the DECLARE_PCI_FIXUP_RESUME_EARLY quirks for *all*
D3hot->D0 transitions?

I'm concerned about places like pci_pm_reset(), where we do
D0->D3hot->D0 to do the reset.  Or vfio_pm_config_write(), where it
looks like a guest could do that without running the quirk.

Current proposed patch is:
https://lore.kernel.org/r/ddbbfb50-24b6-202f-7452-c8959901c739@amd.com

Bjorn

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

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Monday, March 20, 2023 12:15
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>;
> bhelgaas@google.com; linux-pci@vger.kernel.org; thomas@glanzmann.de;
> Rafael J. Wysocki <rjw@rjwysocki.net>
> Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X
> 
> [+cc Rafael for RESUME_EARLY quirk question]
> 
> On Mon, Mar 20, 2023 at 01:32:16AM +0000, Limonciello, Mario wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > Sent: Friday, March 10, 2023 16:14
> > > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > > Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>; Natikar, Basavaraj
> > > <Basavaraj.Natikar@amd.com>; bhelgaas@google.com; linux-
> > > pci@vger.kernel.org; thomas@glanzmann.de
> > > Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X
> > >
> > > On Thu, Mar 09, 2023 at 06:57:38PM -0600, Mario Limonciello wrote:
> > > > On 3/9/23 16:30, Bjorn Helgaas wrote:
> > > > > On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
> > > > > > On 3/9/2023 12:25, Bjorn Helgaas wrote:
> > > > > > ...
> > > > >
> > > > > > > > > https://gitlab.freedesktop.org/agd5f/linux/-
> > > /commit/07494a25fc8881e122c242a46b5c53e0e4403139
> > > > > > >
> > > > > > > That nbio_v7.2.c patch and this patch don't look anything
> > > > > > > alike.  It looks like the nbio_v7.2.c patch might run
> > > > > > > once?  Could *this* be done once at enumeration-time, too?
> > > > > >
> > > > > > They don't look anything alike because they're attacking the
> > > > > > problem from different angles.
> > > > >
> > > > > Why do we need different angles?
> > > >
> > > > The GPU driver approach only works if the GPU is enabled.  If
> > > > the GPU could never be disabled then it alone would be
> > > > sufficient.
> > > >
> > > > > > The NBIO patch fixes the initialization value for the
> > > > > > internal registers.  This is what the BIOS "should" have
> > > > > > done.  When the internal registers are configured properly
> > > > > > then the behavior the kernel expects works as well.
> > > > > >
> > > > > > The NBIO patch will run both at amdgpu startup as well as
> > > > > > when resuming from suspend.
> > > > >
> > > > > If initializing something as BIOS should have done makes the
> > > > > hardware work correctly, isn't once enough?  Why does the NBIO
> > > > > patch need to run at resume-time?
> > > >
> > > > During suspend some internal registers are in a power domain
> > > > that the state will be lost.  These are typically restored by
> > > > the BIOS to the values defined in initialization tables before
> > > > handing control back to the OS.
> > >
> > > I don't quite get this.  I thought I read that if BIOS had
> > > initialized the hardware correctly, a D0->D3hot->D0 transition
> > > would work without any issues.  Linux can do this with PMCSR
> > > writes and BIOS isn't involved at all.
> >
> > During a suspend transition not all registers are powered.  Firmware
> > will capture some during the suspend transition and restore some of
> > them for the resume transition, but it's up to the firmware whether
> > this one is included.
> >
> > Furthermore most IP blocks in amdgpu typically initialize the same
> > during both startup and resume to ensure that firmware couldn't have
> > mucked with the expected golden state.
> 
> We're spending way more time on this than makes sense, but I do think

Yeah..

> it's important that the commit log is accurate and makes sense even to
> people who don't know the internals of the device.
> 
> It *sounds* like what's happening is:
> 
>   - OS writes PMCSR to put device in D3hot
>   - BIOS traps D0->D3hot transition via something like SMI and
>     captures MSI-X state
>   - Device enters D3hot
>   - Device internal MSI-X state is lost
>   - BIOS traps D3hot->D0 transition via SMI
>   - Device enters D0
>   - BIOS restores MSI-X state
>   - OS resumes use of device
> 
> If that's what's happening, the fact that the device loses the
> internal state in D3hot sounds like a *hardware* defect -- if you put
> the device in a system without a BIOS, the D0->D3hot->D0 transitions
> would not work as required by the PCIe spec.

Actually it's a controller integrated into the APU.

So any system you put this APU into has a BIOS.  Because it's a socketed
APU people can very easily move it from one motherboard to another and one
vendor may have the BIOS properly configuring but another might not.

> 
> We can call the fact that BIOS lacks the MSI-X save/restore a BIOS
> defect, but the only reason BIOS would *need* that save/restore is
> because of the underlying *hardware* defect.
> 
> If that's the case, I would expect a commit log something like this:
> 
>   The AMD [1022:15b8] USB controller loses some internal functional
>   MSI-X context when transitioning from D0 to D3hot.  BIOS normally
>   traps D0->D3hot and D3hot->D0 transitions so it can save and restore
>   that internal context, but some firmware in the field lacks this
>   workaround.

I wouldn't call it a workaround.  The hardware is doing exactly as it's
intended for how the firmware programmed.

> 
>   If MSI-X is enabled, toggle the PCI_MSIX_FLAGS_ENABLE bit when
>   resuming to D0, which resynchronizes the internal state that was
>   lost in D3hot.

Otherwise the commit message sounds good to me.

> 
> Rafael, do we run the DECLARE_PCI_FIXUP_RESUME_EARLY quirks for *all*
> D3hot->D0 transitions?
> 
> I'm concerned about places like pci_pm_reset(), where we do
> D0->D3hot->D0 to do the reset.  Or vfio_pm_config_write(), where it
> looks like a guest could do that without running the quirk.
> 
> Current proposed patch is:
> https://lore.kernel.org/r/ddbbfb50-24b6-202f-7452-c8959901c739@amd.com
> 
> Bjorn

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

[Bjorn Helgaas]

On Mon, Mar 20, 2023 at 05:20:55PM +0000, Limonciello, Mario wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > Sent: Monday, March 20, 2023 12:15
> > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>;
> > bhelgaas@google.com; linux-pci@vger.kernel.org; thomas@glanzmann.de;
> > Rafael J. Wysocki <rjw@rjwysocki.net>
> > Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X

Whatever you're using for email adds all these redundant headers to
every response...

> > On Mon, Mar 20, 2023 at 01:32:16AM +0000, Limonciello, Mario wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > Sent: Friday, March 10, 2023 16:14
> > > > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > > > Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>; Natikar, Basavaraj
> > > > <Basavaraj.Natikar@amd.com>; bhelgaas@google.com; linux-
> > > > pci@vger.kernel.org; thomas@glanzmann.de
> > > > Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X
> > > >
> > > > On Thu, Mar 09, 2023 at 06:57:38PM -0600, Mario Limonciello wrote:
> > > > > On 3/9/23 16:30, Bjorn Helgaas wrote:
> > > > > > On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
> > > > > > > On 3/9/2023 12:25, Bjorn Helgaas wrote:
> > > > > > > ...

> > it's important that the commit log is accurate and makes sense even to
> > people who don't know the internals of the device.
> > 
> > It *sounds* like what's happening is:
> > 
> >   - OS writes PMCSR to put device in D3hot
> >   - BIOS traps D0->D3hot transition via something like SMI and
> >     captures MSI-X state
> >   - Device enters D3hot
> >   - Device internal MSI-X state is lost
> >   - BIOS traps D3hot->D0 transition via SMI
> >   - Device enters D0
> >   - BIOS restores MSI-X state
> >   - OS resumes use of device
> > 
> > If that's what's happening, the fact that the device loses the
> > internal state in D3hot sounds like a *hardware* defect -- if you put
> > the device in a system without a BIOS, the D0->D3hot->D0 transitions
> > would not work as required by the PCIe spec.
> 
> Actually it's a controller integrated into the APU.
> 
> So any system you put this APU into has a BIOS.  Because it's a socketed
> APU people can very easily move it from one motherboard to another and one
> vendor may have the BIOS properly configuring but another might not.
>
> > We can call the fact that BIOS lacks the MSI-X save/restore a BIOS
> > defect, but the only reason BIOS would *need* that save/restore is
> > because of the underlying *hardware* defect.
> > 
> > If that's the case, I would expect a commit log something like this:
> > 
> >   The AMD [1022:15b8] USB controller loses some internal functional
> >   MSI-X context when transitioning from D0 to D3hot.  BIOS normally
> >   traps D0->D3hot and D3hot->D0 transitions so it can save and restore
> >   that internal context, but some firmware in the field lacks this
> >   workaround.
> 
> I wouldn't call it a workaround.  The hardware is doing exactly as it's
> intended for how the firmware programmed.

The whole point of the PCI spec is to build devices where standard
features like power management can be operated without device-specific
knowledge.  If we need device-specific code in BIOS or Linux, I'd say
that's a workaround.

Does this device set No_Soft_Reset?  If it does, when it receives a
config write to PMCSR that puts it in D3hot, followed by a config
write that puts it back in D0, it's supposed to return to D0 with no
additional software intervention required.  I think that also means no
BIOS intervention is required.

If it does not set No_Soft_Reset, pci_pm_reset() assumes that a
D0->D3hot->D0 transition resets the device.  We save and restore the
MSI-X Capability in that case, but we do NOT run the
DECLARE_PCI_FIXUP_RESUME_EARLY quirks.  I think that means MSI-X would
not work after a PM reset of this device.

Bjorn

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

[Limonciello, Mario]

> Whatever you're using for email adds all these redundant headers to
> every response...

Sorry about that, let me use a real email client this time.

> 
>>> On Mon, Mar 20, 2023 at 01:32:16AM +0000, Limonciello, Mario wrote:
>>>>> -----Original Message-----
>>>>> From: Bjorn Helgaas <helgaas@kernel.org>
>>>>> Sent: Friday, March 10, 2023 16:14
>>>>> To: Limonciello, Mario <Mario.Limonciello@amd.com>
>>>>> Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>; Natikar, Basavaraj
>>>>> <Basavaraj.Natikar@amd.com>; bhelgaas@google.com; linux-
>>>>> pci@vger.kernel.org; thomas@glanzmann.de
>>>>> Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X
>>>>>
>>>>> On Thu, Mar 09, 2023 at 06:57:38PM -0600, Mario Limonciello wrote:
>>>>>> On 3/9/23 16:30, Bjorn Helgaas wrote:
>>>>>>> On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
>>>>>>>> On 3/9/2023 12:25, Bjorn Helgaas wrote:
>>>>>>>> ...
> 
>>> it's important that the commit log is accurate and makes sense even to
>>> people who don't know the internals of the device.
>>>
>>> It *sounds* like what's happening is:
>>>
>>>    - OS writes PMCSR to put device in D3hot
>>>    - BIOS traps D0->D3hot transition via something like SMI and
>>>      captures MSI-X state
>>>    - Device enters D3hot
>>>    - Device internal MSI-X state is lost
>>>    - BIOS traps D3hot->D0 transition via SMI
>>>    - Device enters D0
>>>    - BIOS restores MSI-X state
>>>    - OS resumes use of device
>>>
>>> If that's what's happening, the fact that the device loses the
>>> internal state in D3hot sounds like a *hardware* defect -- if you put
>>> the device in a system without a BIOS, the D0->D3hot->D0 transitions
>>> would not work as required by the PCIe spec.
>>
>> Actually it's a controller integrated into the APU.
>>
>> So any system you put this APU into has a BIOS.  Because it's a socketed
>> APU people can very easily move it from one motherboard to another and one
>> vendor may have the BIOS properly configuring but another might not.
>>
>>> We can call the fact that BIOS lacks the MSI-X save/restore a BIOS
>>> defect, but the only reason BIOS would *need* that save/restore is
>>> because of the underlying *hardware* defect.
>>>
>>> If that's the case, I would expect a commit log something like this:
>>>
>>>    The AMD [1022:15b8] USB controller loses some internal functional
>>>    MSI-X context when transitioning from D0 to D3hot.  BIOS normally
>>>    traps D0->D3hot and D3hot->D0 transitions so it can save and restore
>>>    that internal context, but some firmware in the field lacks this
>>>    workaround.
>>
>> I wouldn't call it a workaround.  The hardware is doing exactly as it's
>> intended for how the firmware programmed.
> 
> The whole point of the PCI spec is to build devices where standard
> features like power management can be operated without device-specific
> knowledge.  

Right.  I "think" the confusion here might stem from the term "BIOS".

The initialization of the hardware happens from a series of 
microcontrollers in the APU before X86 cores are released from reset. 
By the time UEFI runs all of this should have already happened.

When I say "BIOS" I mean collectively "all" of this firmware.

> If we need device-specific code in BIOS or Linux, I'd say
> that's a workaround.

Something I'd like to point out in case it wasn't apparent is Windows 
doesn't actually hit this problem.  It doesn't matter if the correct 
hardware initialization code is included in "BIOS" or not.

So I wonder maybe we should be looking at this another way?

> 
> Does this device set No_Soft_Reset?  If it does, when it receives a
> config write to PMCSR that puts it in D3hot, followed by a config
> write that puts it back in D0, it's supposed to return to D0 with no
> additional software intervention required.  I think that also means no
> BIOS intervention is required.
> 
> If it does not set No_Soft_Reset, pci_pm_reset() assumes that a
> D0->D3hot->D0 transition resets the device.  We save and restore the
> MSI-X Capability in that case, but we do NOT run the
> DECLARE_PCI_FIXUP_RESUME_EARLY quirks.  I think that means MSI-X would
> not work after a PM reset of this device.
> 
> Bjorn

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

[Bjorn Helgaas]

On Mon, Mar 20, 2023 at 02:47:48PM -0500, Limonciello, Mario wrote:
> > > > On Mon, Mar 20, 2023 at 01:32:16AM +0000, Limonciello, Mario wrote:
> > > > > > -----Original Message-----
> > > > > > From: Bjorn Helgaas <helgaas@kernel.org>
> > > > > > Sent: Friday, March 10, 2023 16:14
> > > > > > To: Limonciello, Mario <Mario.Limonciello@amd.com>
> > > > > > Cc: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>; Natikar, Basavaraj
> > > > > > <Basavaraj.Natikar@amd.com>; bhelgaas@google.com; linux-
> > > > > > pci@vger.kernel.org; thomas@glanzmann.de
> > > > > > Subject: Re: [PATCH] PCI: Add quirk to clear MSI-X
> > > > > > 
> > > > > > On Thu, Mar 09, 2023 at 06:57:38PM -0600, Mario Limonciello wrote:
> > > > > > > On 3/9/23 16:30, Bjorn Helgaas wrote:
> > > > > > > > On Thu, Mar 09, 2023 at 12:32:41PM -0600, Limonciello, Mario wrote:
> > > > > > > > > On 3/9/2023 12:25, Bjorn Helgaas wrote:
> > > > > > > > > ...
> > 
> > > > it's important that the commit log is accurate and makes sense even to
> > > > people who don't know the internals of the device.
> > > > 
> > > > It *sounds* like what's happening is:
> > > > 
> > > >    - OS writes PMCSR to put device in D3hot
> > > >    - BIOS traps D0->D3hot transition via something like SMI and
> > > >      captures MSI-X state
> > > >    - Device enters D3hot
> > > >    - Device internal MSI-X state is lost
> > > >    - BIOS traps D3hot->D0 transition via SMI
> > > >    - Device enters D0
> > > >    - BIOS restores MSI-X state
> > > >    - OS resumes use of device
> > > > 
> > > > If that's what's happening, the fact that the device loses the
> > > > internal state in D3hot sounds like a *hardware* defect -- if you put
> > > > the device in a system without a BIOS, the D0->D3hot->D0 transitions
> > > > would not work as required by the PCIe spec.
> > > 
> > > Actually it's a controller integrated into the APU.
> > > 
> > > So any system you put this APU into has a BIOS.  Because it's a socketed
> > > APU people can very easily move it from one motherboard to another and one
> > > vendor may have the BIOS properly configuring but another might not.
> > > 
> > > > We can call the fact that BIOS lacks the MSI-X save/restore a BIOS
> > > > defect, but the only reason BIOS would *need* that save/restore is
> > > > because of the underlying *hardware* defect.
> > > > 
> > > > If that's the case, I would expect a commit log something like this:
> > > > 
> > > >    The AMD [1022:15b8] USB controller loses some internal functional
> > > >    MSI-X context when transitioning from D0 to D3hot.  BIOS normally
> > > >    traps D0->D3hot and D3hot->D0 transitions so it can save and restore
> > > >    that internal context, but some firmware in the field lacks this
> > > >    workaround.
> > > 
> > > I wouldn't call it a workaround.  The hardware is doing exactly as it's
> > > intended for how the firmware programmed.
> > 
> > The whole point of the PCI spec is to build devices where standard
> > features like power management can be operated without device-specific
> > knowledge.
> 
> Right.  I "think" the confusion here might stem from the term "BIOS".
> 
> The initialization of the hardware happens from a series of microcontrollers
> in the APU before X86 cores are released from reset. By the time UEFI runs
> all of this should have already happened.
> 
> When I say "BIOS" I mean collectively "all" of this firmware.

I don't understand the point you're making here.  I don't think it
matters whether this device-specific knowledge is in APU
microcontroller firmware, BIOS, Linux, etc.

I'm trying to suggest that if we zoom all the way in and look just at
the PCIe TLPs, we would see two config writes that put the device in
D3hot, then back in D0.  A working device should end up either back in
D0active with MSI-X fully enabled (if No_Soft_Reset is set and MSI-X
was originally enabled), or in D0uninitialized (if No_Soft_Reset is
clear).

But with this device, apparently some additional software intervention
is required, i.e., after the config write to go back to D0, we need
two more writes to clear and set the MSI-X enable bit.

Let's say somebody runs coreboot on this platform.  Does coreboot need
this device-specific knowledge?

> > If we need device-specific code in BIOS or Linux, I'd say
> > that's a workaround.
> 
> Something I'd like to point out in case it wasn't apparent is Windows
> doesn't actually hit this problem.  It doesn't matter if the correct
> hardware initialization code is included in "BIOS" or not.

That's interesting because it means there's something we (I, at least)
don't understand about what's going on here.  Maybe Windows never puts
the device in D3hot at runtime?  Or maybe Windows disables MSI-X
before putting it into D3 and re-enables it after going to D0?  I
don't know how Windows power management works, so I'm not sure this
tells us anything useful about Linux.

> > Does this device set No_Soft_Reset?  If it does, when it receives a
> > config write to PMCSR that puts it in D3hot, followed by a config
> > write that puts it back in D0, it's supposed to return to D0 with no
> > additional software intervention required.  I think that also means no
> > BIOS intervention is required.
> > 
> > If it does not set No_Soft_Reset, pci_pm_reset() assumes that a
> > D0->D3hot->D0 transition resets the device.  We save and restore the
> > MSI-X Capability in that case, but we do NOT run the
> > DECLARE_PCI_FIXUP_RESUME_EARLY quirks.  I think that means MSI-X would
> > not work after a PM reset of this device.
> > 
> > Bjorn

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

[Limonciello, Mario]

>> When I say "BIOS" I mean collectively "all" of this firmware.
> 
> I don't understand the point you're making here.  I don't think it
> matters whether this device-specific knowledge is in APU
> microcontroller firmware, BIOS, Linux, etc.
> 
> I'm trying to suggest that if we zoom all the way in and look just at
> the PCIe TLPs, we would see two config writes that put the device in
> D3hot, then back in D0.  A working device should end up either back in
> D0active with MSI-X fully enabled (if No_Soft_Reset is set and MSI-X
> was originally enabled), or in D0uninitialized (if No_Soft_Reset is
> clear).
> 
> But with this device, apparently some additional software intervention
> is required, i.e., after the config write to go back to D0, we need
> two more writes to clear and set the MSI-X enable bit.

My point is that's only needed if the hardware wasn't initialized 
correctly.  If it's initialized properly then it behaves like you expect.

> 
> Let's say somebody runs coreboot on this platform.  Does coreboot need
> this device-specific knowledge?
> 

Yes; the exact same bug will happen with a coreboot implementation that 
had the initialization done improperly.


>>> If we need device-specific code in BIOS or Linux, I'd say
>>> that's a workaround.
>>
>> Something I'd like to point out in case it wasn't apparent is Windows
>> doesn't actually hit this problem.  It doesn't matter if the correct
>> hardware initialization code is included in "BIOS" or not.
> 
> That's interesting because it means there's something we (I, at least)
> don't understand about what's going on here.  Maybe Windows never puts
> the device in D3hot at runtime?  Or maybe Windows disables MSI-X
> before putting it into D3 and re-enables it after going to D0?  I
> don't know how Windows power management works, so I'm not sure this
> tells us anything useful about Linux.

Windows does put it into D3hot at runtime when nothing is plugged in, 
just like Linux does.

I believe you're right the difference is that Windows turns off MSI-X 
before putting it into D3 and re-enables it after going to D0.  So this 
behavior doesn't happen there.

Basavaraj - can you please confirm that?

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

[Bjorn Helgaas]

On Mon, Mar 20, 2023 at 04:37:17PM -0500, Limonciello, Mario wrote:
> > > When I say "BIOS" I mean collectively "all" of this firmware.
> > 
> > I don't understand the point you're making here.  I don't think it
> > matters whether this device-specific knowledge is in APU
> > microcontroller firmware, BIOS, Linux, etc.
> > 
> > I'm trying to suggest that if we zoom all the way in and look just at
> > the PCIe TLPs, we would see two config writes that put the device in
> > D3hot, then back in D0.  A working device should end up either back in
> > D0active with MSI-X fully enabled (if No_Soft_Reset is set and MSI-X
> > was originally enabled), or in D0uninitialized (if No_Soft_Reset is
> > clear).
> > 
> > But with this device, apparently some additional software intervention
> > is required, i.e., after the config write to go back to D0, we need
> > two more writes to clear and set the MSI-X enable bit.
> 
> My point is that's only needed if the hardware wasn't initialized correctly.
> If it's initialized properly then it behaves like you expect.

So is this something that BIOS must initialize, and then it's locked
so that by the time Linux shows up, this one-time initialization can
no longer be done?

If Linux *could* do this one-time initialization, and subsequent
D0/D3hot transitions worked per spec, that would be awesome because we
wouldn't have to worry about making sure we run the quirk at every
possible transition.

> > Let's say somebody runs coreboot on this platform.  Does coreboot need
> > this device-specific knowledge?
> 
> Yes; the exact same bug will happen with a coreboot implementation that had
> the initialization done improperly.

My claim is that this means the device doesn't conform to the spec.

If we add a conforming PCI device that neither the OS nor the firmware
has ever seen before, standard generic functionality like power
management should just work.

Bjorn

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

[Mario Limonciello]

>> My point is that's only needed if the hardware wasn't initialized correctly.
>> If it's initialized properly then it behaves like you expect.
> So is this something that BIOS must initialize, and then it's locked
> so that by the time Linux shows up, this one-time initialization can
> no longer be done?
>
> If Linux *could* do this one-time initialization, and subsequent
> D0/D3hot transitions worked per spec, that would be awesome because we
> wouldn't have to worry about making sure we run the quirk at every
> possible transition.

It can be changed again at runtime.

That's exactly what we did in amdgpu for the case that the user didn't 
disable integrated GPU.

We did the init for the IP block during amdgpu's HW init phase.

I see 3 ways to address this:

1) As submitted or similar (on every D state transition work around the 
issue).
2) Mimic the Windows behavior in Linux by disabling MSI-X during D3 
entry and re-enabling on D0.
3) Look for a way to get to and program that register outside of amdgpu.

There are merits to all those approaches, what do you think?
>>> Let's say somebody runs coreboot on this platform.  Does coreboot need
>>> this device-specific knowledge?
>> Yes; the exact same bug will happen with a coreboot implementation that had
>> the initialization done improperly.
> My claim is that this means the device doesn't conform to the spec.
> If we add a conforming PCI device that neither the OS nor the firmware
> has ever seen before, standard generic functionality like power
> management should just work.
>
> Bjorn

Yeah as it's configured here I agree with you.

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

[Bjorn Helgaas]

On Mon, Mar 20, 2023 at 05:52:58PM -0500, Mario Limonciello wrote:
> > If Linux *could* do this one-time initialization, and subsequent
> > D0/D3hot transitions worked per spec, that would be awesome
> > because we wouldn't have to worry about making sure we run the
> > quirk at every possible transition.
> 
> It can be changed again at runtime.
> 
> That's exactly what we did in amdgpu for the case that the user
> didn't disable integrated GPU.
> 
> We did the init for the IP block during amdgpu's HW init phase.
> 
> I see 3 ways to address this:
> 
> 1) As submitted or similar (on every D state transition work around
> the issue).
>
> 2) Mimic the Windows behavior in Linux by disabling MSI-X during D3
> entry and re-enabling on D0.
>
> 3) Look for a way to get to and program that register outside of
> amdgpu.
> 
> There are merits to all those approaches, what do you think?

Without knowing anything about the difficulty of 3), that seems the
most attractive to me because we never have to worry about catching
every D state transition.

Bjorn

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

[Basavaraj Natikar]

On 3/21/2023 4:37 PM, Bjorn Helgaas wrote:
> On Mon, Mar 20, 2023 at 05:52:58PM -0500, Mario Limonciello wrote:
>>> If Linux *could* do this one-time initialization, and subsequent
>>> D0/D3hot transitions worked per spec, that would be awesome
>>> because we wouldn't have to worry about making sure we run the
>>> quirk at every possible transition.
>> It can be changed again at runtime.
>>
>> That's exactly what we did in amdgpu for the case that the user
>> didn't disable integrated GPU.
>>
>> We did the init for the IP block during amdgpu's HW init phase.
>>
>> I see 3 ways to address this:
>>
>> 1) As submitted or similar (on every D state transition work around
>> the issue).
>>
>> 2) Mimic the Windows behavior in Linux by disabling MSI-X during D3
>> entry and re-enabling on D0.
>>
>> 3) Look for a way to get to and program that register outside of
>> amdgpu.
>>
>> There are merits to all those approaches, what do you think?
> Without knowing anything about the difficulty of 3), that seems the
> most attractive to me because we never have to worry about catching
> every D state transition.

Sure Bjron we came up with below solution without worrying much on very D state
transition and works perfectly fine.

PCI: Add quirk to clear AMD strap_15B8 NO_SOFT_RESET dev 2 f0

The AMD [1022:15b8] USB controller loses some internal functional
MSI-X context when transitioning from D0 to D3hot. BIOS normally
traps D0->D3hot and D3hot->D0 transitions so it can save and restore
that internal context, but some firmware in the field lacks due to
AMD_15B8_RCC_DEV2_EPF0_STRAP2 NO_SOFT_RESET bit is set.

Hence add quirk to clear AMD_15B8_RCC_DEV2_EPF0_STRAP2 NO_SOFT_RESET
bit before USB controller initialization during boot.

---
 drivers/pci/quirks.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 44cab813bf95..0c088fa58ad7 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -12,6 +12,7 @@
  * file, where their drivers can use them.
  */
 
+#include <asm/amd_nb.h>
 #include <linux/bitfield.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -6023,3 +6024,21 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d, dpc_log_size);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f, dpc_log_size);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
 #endif
+
+#define AMD_15B8_RCC_DEV2_EPF0_STRAP2                                  0x10136008
+#define AMD_15B8_RCC_DEV2_EPF0_STRAP2_NO_SOFT_RESET_DEV2_F0_MASK       0x00000080L
+
+static void quirk_clear_strap_no_soft_reset_dev2_f0(struct pci_dev *dev)
+{
+	u32 data;
+
+	if (!amd_smn_read(0 , AMD_15B8_RCC_DEV2_EPF0_STRAP2, &data)) {
+		data &= ~AMD_15B8_RCC_DEV2_EPF0_STRAP2_NO_SOFT_RESET_DEV2_F0_MASK;
+		if (amd_smn_write(0, AMD_15B8_RCC_DEV2_EPF0_STRAP2, data))
+			pci_err(dev, "Failed to write data 0x%x\n", data);
+	} else {
+		pci_err(dev, "Failed to read data 0x%x\n", data);
+	}
+
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x15b8, quirk_clear_strap_no_soft_reset_dev2_f0);

 

>

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

[Limonciello, Mario]

[AMD Official Use Only - General]

> Without knowing anything about the difficulty of 3), that seems the
> > most attractive to me because we never have to worry about catching
> > every D state transition.
> 
> Sure Bjron we came up with below solution without worrying much on very D
> state
> transition and works perfectly fine.
> 
> PCI: Add quirk to clear AMD strap_15B8 NO_SOFT_RESET dev 2 f0
> 
> The AMD [1022:15b8] USB controller loses some internal functional
> MSI-X context when transitioning from D0 to D3hot. BIOS normally
> traps D0->D3hot and D3hot->D0 transitions so it can save and restore
> that internal context, but some firmware in the field lacks due to
> AMD_15B8_RCC_DEV2_EPF0_STRAP2 NO_SOFT_RESET bit is set.
> 
> Hence add quirk to clear AMD_15B8_RCC_DEV2_EPF0_STRAP2 NO_SOFT_RESET
> bit before USB controller initialization during boot.
> 
> ---
>  drivers/pci/quirks.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 44cab813bf95..0c088fa58ad7 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -12,6 +12,7 @@
>   * file, where their drivers can use them.
>   */
> 
> +#include <asm/amd_nb.h>
>  #include <linux/bitfield.h>
>  #include <linux/types.h>
>  #include <linux/kernel.h>
> @@ -6023,3 +6024,21 @@
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d, dpc_log_size);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f, dpc_log_size);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
>  #endif
> +
> +#define AMD_15B8_RCC_DEV2_EPF0_STRAP2                                  0x10136008
> +#define
> AMD_15B8_RCC_DEV2_EPF0_STRAP2_NO_SOFT_RESET_DEV2_F0_MASK
> 0x00000080L
> +
> +static void quirk_clear_strap_no_soft_reset_dev2_f0(struct pci_dev *dev)
> +{
> +	u32 data;
> +
> +	if (!amd_smn_read(0 , AMD_15B8_RCC_DEV2_EPF0_STRAP2, &data)) {
> +		data &=
> ~AMD_15B8_RCC_DEV2_EPF0_STRAP2_NO_SOFT_RESET_DEV2_F0_MASK;
> +		if (amd_smn_write(0, AMD_15B8_RCC_DEV2_EPF0_STRAP2,
> data))
> +			pci_err(dev, "Failed to write data 0x%x\n", data);
> +	} else {
> +		pci_err(dev, "Failed to read data 0x%x\n", data);
> +	}
> +
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x15b8,
> quirk_clear_strap_no_soft_reset_dev2_f0);
> 
> 
> 
> >

That patch looks good to me.  Bjorn may want you to send as a dedicated patch
rather at end of this thread.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Suggest to also add a Link: tag to the original report.

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

[Bjorn Helgaas]

On Tue, Mar 28, 2023 at 06:45:48PM +0530, Basavaraj Natikar wrote:
> 
> On 3/21/2023 4:37 PM, Bjorn Helgaas wrote:
> > On Mon, Mar 20, 2023 at 05:52:58PM -0500, Mario Limonciello wrote:
> >>> If Linux *could* do this one-time initialization, and subsequent
> >>> D0/D3hot transitions worked per spec, that would be awesome
> >>> because we wouldn't have to worry about making sure we run the
> >>> quirk at every possible transition.
> >> It can be changed again at runtime.
> >>
> >> That's exactly what we did in amdgpu for the case that the user
> >> didn't disable integrated GPU.
> >>
> >> We did the init for the IP block during amdgpu's HW init phase.
> >>
> >> I see 3 ways to address this:
> >>
> >> 1) As submitted or similar (on every D state transition work around
> >> the issue).
> >>
> >> 2) Mimic the Windows behavior in Linux by disabling MSI-X during D3
> >> entry and re-enabling on D0.
> >>
> >> 3) Look for a way to get to and program that register outside of
> >> amdgpu.
> >>
> >> There are merits to all those approaches, what do you think?
> > Without knowing anything about the difficulty of 3), that seems the
> > most attractive to me because we never have to worry about catching
> > every D state transition.
> 
> Sure Bjron we came up with below solution without worrying much on very D state
> transition and works perfectly fine.
> 
> PCI: Add quirk to clear AMD strap_15B8 NO_SOFT_RESET dev 2 f0
> 
> The AMD [1022:15b8] USB controller loses some internal functional
> MSI-X context when transitioning from D0 to D3hot. BIOS normally
> traps D0->D3hot and D3hot->D0 transitions so it can save and restore
> that internal context, but some firmware in the field lacks due to
> AMD_15B8_RCC_DEV2_EPF0_STRAP2 NO_SOFT_RESET bit is set.
> 
> Hence add quirk to clear AMD_15B8_RCC_DEV2_EPF0_STRAP2 NO_SOFT_RESET
> bit before USB controller initialization during boot.

I LOVE this, thank you so much for working this out!  This is SO much
better than something we have to do on every power state transition.

Can you please post again with your Signed-off-by?

I suspect this should also be moved to arch/x86/pci/fixup.c since only
x86 has <asm/amd_nb.h>.  Similarly, you might need some #ifdefs or
stubs for amd_smn_read()/amd_smn_write(), because it looks like those
only exist when CONFIG_AMD_NB=y so amd_nb.c is compiled.

> ---
>  drivers/pci/quirks.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 44cab813bf95..0c088fa58ad7 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -12,6 +12,7 @@
>   * file, where their drivers can use them.
>   */
>  
> +#include <asm/amd_nb.h>
>  #include <linux/bitfield.h>
>  #include <linux/types.h>
>  #include <linux/kernel.h>
> @@ -6023,3 +6024,21 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2d, dpc_log_size);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a2f, dpc_log_size);
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x9a31, dpc_log_size);
>  #endif
> +
> +#define AMD_15B8_RCC_DEV2_EPF0_STRAP2                                  0x10136008
> +#define AMD_15B8_RCC_DEV2_EPF0_STRAP2_NO_SOFT_RESET_DEV2_F0_MASK       0x00000080L
> +
> +static void quirk_clear_strap_no_soft_reset_dev2_f0(struct pci_dev *dev)
> +{
> +	u32 data;
> +
> +	if (!amd_smn_read(0 , AMD_15B8_RCC_DEV2_EPF0_STRAP2, &data)) {
> +		data &= ~AMD_15B8_RCC_DEV2_EPF0_STRAP2_NO_SOFT_RESET_DEV2_F0_MASK;
> +		if (amd_smn_write(0, AMD_15B8_RCC_DEV2_EPF0_STRAP2, data))
> +			pci_err(dev, "Failed to write data 0x%x\n", data);
> +	} else {
> +		pci_err(dev, "Failed to read data 0x%x\n", data);
> +	}
> +
> +}
> +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, 0x15b8, quirk_clear_strap_no_soft_reset_dev2_f0);
> 
>  
> 
> >
> 

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

[Basavaraj Natikar]

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@kernel.org>
>>>> Sent: Wednesday, March 8, 2023 16:44
>>>> To: Natikar, Basavaraj <Basavaraj.Natikar@amd.com>
>>>> Cc: bhelgaas@google.com; linux-pci@vger.kernel.org; Limonciello, Mario
>>>> <Mario.Limonciello@amd.com>; thomas@glanzmann.de
>>>> 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