| Message ID | 20230306072340.172306-1-Basavaraj.Natikar@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Add quirk to clear MSI-X | expand |
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
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 >
[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 > >
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 >>>
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
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.
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
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
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
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 >
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
[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.
[+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
[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
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
> 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
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
>> 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?
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
>> 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.
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
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); >
[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.
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); > > > > > >
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);
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(+)