[RFC PATCH] PCI: Extend D3hot delay for NVIDIA HDA controllers

[RFC PATCH] PCI: Extend D3hot delay for NVIDIA HDA controllers

[Alex Williamson]

Assignment of NVIDIA Ampere-based GPUs have seen a regression since the
below referenced commit, where the reduced D3hot transition delay appears
to introduce a small window where a D3hot->D0 transition followed by a bus
reset can wedge the device.  The entire device is subsequently unavailable,
returning -1 on config space read and is unrecoverable without a host reset.

This has been observed with RTX A2000 and A5000 GPU and audio functions
assigned to a Windows VM, where shutdown of the VM places the devices in
D3hot prior to vfio-pci performing a bus reset when userspace releases the
devices.  The issue has roughly a 2-3% chance of occurring per shutdown.

Restoring the HDA controller d3hot_delay to the effective value before the
below commit has been shown to resolve the issue.

I'm looking for input from NVIDIA whether this issue is unique to
Ampere-based HDA controllers or should be assumed to linger in both older
and newer controllers as well.  Currently we've not been able to reproduce
the issue other than on Ampere HDA controllers, however the implementation
here includes all NVIDIA HDA controllers based on PCI vendor and device
class.

If we were to limit the quirk to Ampere HDA controllers, I think that would
include:

1aef	GA102 High Definition Audio Controller
228b	GA104 High Definition Audio Controller
228e	GA106 High Definition Audio Controller

Cc: Abhishek Sahu <abhsahu@nvidia.com>
Cc: Tarun Gupta <targupta@nvidia.com>
Fixes: 3e347969a577 ("PCI/PM: Reduce D3hot delay with usleep_range()")
Reported-by: Zhiyi Guo <zhguo@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/pci/quirks.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 44cab813bf95..f4e2a88729fd 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1939,6 +1939,19 @@ static void quirk_radeon_pm(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6741, quirk_radeon_pm);
 
+/*
+ * NVIDIA Ampere-based HDA controllers can wedge the whole device if a bus
+ * reset is performed too soon after transition to D0, extend d3hot_delay
+ * to previous effective default for all NVIDIA HDA controllers.
+ */
+static void quirk_nvidia_hda_pm(struct pci_dev *dev)
+{
+	quirk_d3hot_delay(dev, 20);
+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
+			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8,
+			      quirk_nvidia_hda_pm);
+
 /*
  * Ryzen5/7 XHCI controllers fail upon resume from runtime suspend or s2idle.
  * https://bugzilla.kernel.org/show_bug.cgi?id=205587

Re: [RFC PATCH] PCI: Extend D3hot delay for NVIDIA HDA controllers

[Bjorn Helgaas]

[+cc Sajid, author of 3e347969a577]

On Tue, Mar 28, 2023 at 04:59:30PM -0600, Alex Williamson wrote:
> Assignment of NVIDIA Ampere-based GPUs have seen a regression since the
> below referenced commit, where the reduced D3hot transition delay appears
> to introduce a small window where a D3hot->D0 transition followed by a bus
> reset can wedge the device.  The entire device is subsequently unavailable,
> returning -1 on config space read and is unrecoverable without a host reset.
> 
> This has been observed with RTX A2000 and A5000 GPU and audio functions
> assigned to a Windows VM, where shutdown of the VM places the devices in
> D3hot prior to vfio-pci performing a bus reset when userspace releases the
> devices.  The issue has roughly a 2-3% chance of occurring per shutdown.
> 
> Restoring the HDA controller d3hot_delay to the effective value before the
> below commit has been shown to resolve the issue.

Interesting.  This sounds like it was a hassle to track down.  I guess
we knew there was some risk in reducing those delays.

Did you by chance notice whether the actual delay when the device gets
wedged is sufficient per spec?

If there's a case where the usleep_range() doesn't quite wait the
spec-mandated time, we should adjust that in case we have the same
problem with other devices.

> I'm looking for input from NVIDIA whether this issue is unique to
> Ampere-based HDA controllers or should be assumed to linger in both older
> and newer controllers as well.  Currently we've not been able to reproduce
> the issue other than on Ampere HDA controllers, however the implementation
> here includes all NVIDIA HDA controllers based on PCI vendor and device
> class.
> 
> If we were to limit the quirk to Ampere HDA controllers, I think that would
> include:
> 
> 1aef	GA102 High Definition Audio Controller
> 228b	GA104 High Definition Audio Controller
> 228e	GA106 High Definition Audio Controller
> 
> Cc: Abhishek Sahu <abhsahu@nvidia.com>
> Cc: Tarun Gupta <targupta@nvidia.com>
> Fixes: 3e347969a577 ("PCI/PM: Reduce D3hot delay with usleep_range()")
> Reported-by: Zhiyi Guo <zhguo@redhat.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/pci/quirks.c |   13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 44cab813bf95..f4e2a88729fd 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -1939,6 +1939,19 @@ static void quirk_radeon_pm(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6741, quirk_radeon_pm);
>  
> +/*
> + * NVIDIA Ampere-based HDA controllers can wedge the whole device if a bus
> + * reset is performed too soon after transition to D0, extend d3hot_delay
> + * to previous effective default for all NVIDIA HDA controllers.
> + */
> +static void quirk_nvidia_hda_pm(struct pci_dev *dev)
> +{
> +	quirk_d3hot_delay(dev, 20);
> +}
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
> +			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8,
> +			      quirk_nvidia_hda_pm);
> +
>  /*
>   * Ryzen5/7 XHCI controllers fail upon resume from runtime suspend or s2idle.
>   * https://bugzilla.kernel.org/show_bug.cgi?id=205587
> 
> 

Re: [RFC PATCH] PCI: Extend D3hot delay for NVIDIA HDA controllers

[Alex Williamson]

On Thu, 6 Apr 2023 16:50:49 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> [+cc Sajid, author of 3e347969a577]
> 
> On Tue, Mar 28, 2023 at 04:59:30PM -0600, Alex Williamson wrote:
> > Assignment of NVIDIA Ampere-based GPUs have seen a regression since the
> > below referenced commit, where the reduced D3hot transition delay appears
> > to introduce a small window where a D3hot->D0 transition followed by a bus
> > reset can wedge the device.  The entire device is subsequently unavailable,
> > returning -1 on config space read and is unrecoverable without a host reset.
> > 
> > This has been observed with RTX A2000 and A5000 GPU and audio functions
> > assigned to a Windows VM, where shutdown of the VM places the devices in
> > D3hot prior to vfio-pci performing a bus reset when userspace releases the
> > devices.  The issue has roughly a 2-3% chance of occurring per shutdown.
> > 
> > Restoring the HDA controller d3hot_delay to the effective value before the
> > below commit has been shown to resolve the issue.  
> 
> Interesting.  This sounds like it was a hassle to track down.  I guess
> we knew there was some risk in reducing those delays.
> 
> Did you by chance notice whether the actual delay when the device gets
> wedged is sufficient per spec?
> 
> If there's a case where the usleep_range() doesn't quite wait the
> spec-mandated time, we should adjust that in case we have the same
> problem with other devices.

That would have been a good test, unfortunately I didn't check and
don't currently have access to the system anymore.  Perhaps this is
something the NVIDIA folks can check as they're investigating the scope
of affected hardware.  Thanks,

Alex

> > I'm looking for input from NVIDIA whether this issue is unique to
> > Ampere-based HDA controllers or should be assumed to linger in both older
> > and newer controllers as well.  Currently we've not been able to reproduce
> > the issue other than on Ampere HDA controllers, however the implementation
> > here includes all NVIDIA HDA controllers based on PCI vendor and device
> > class.
> > 
> > If we were to limit the quirk to Ampere HDA controllers, I think that would
> > include:
> > 
> > 1aef	GA102 High Definition Audio Controller
> > 228b	GA104 High Definition Audio Controller
> > 228e	GA106 High Definition Audio Controller
> > 
> > Cc: Abhishek Sahu <abhsahu@nvidia.com>
> > Cc: Tarun Gupta <targupta@nvidia.com>
> > Fixes: 3e347969a577 ("PCI/PM: Reduce D3hot delay with usleep_range()")
> > Reported-by: Zhiyi Guo <zhguo@redhat.com>
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  drivers/pci/quirks.c |   13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index 44cab813bf95..f4e2a88729fd 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -1939,6 +1939,19 @@ static void quirk_radeon_pm(struct pci_dev *dev)
> >  }
> >  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6741, quirk_radeon_pm);
> >  
> > +/*
> > + * NVIDIA Ampere-based HDA controllers can wedge the whole device if a bus
> > + * reset is performed too soon after transition to D0, extend d3hot_delay
> > + * to previous effective default for all NVIDIA HDA controllers.
> > + */
> > +static void quirk_nvidia_hda_pm(struct pci_dev *dev)
> > +{
> > +	quirk_d3hot_delay(dev, 20);
> > +}
> > +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
> > +			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8,
> > +			      quirk_nvidia_hda_pm);
> > +
> >  /*
> >   * Ryzen5/7 XHCI controllers fail upon resume from runtime suspend or s2idle.
> >   * https://bugzilla.kernel.org/show_bug.cgi?id=205587
> > 
> >   
> 

Re: [RFC PATCH] PCI: Extend D3hot delay for NVIDIA HDA controllers

[Alex Williamson]

On Thu, 13 Apr 2023 00:40:17 +0530
"Tarun Gupta (SW-GPU)" <targupta@nvidia.com> wrote:

> On 4/7/2023 3:31 AM, Alex Williamson wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Thu, 6 Apr 2023 16:50:49 -0500
> > Bjorn Helgaas<helgaas@kernel.org>  wrote:
> >  
> >> [+cc Sajid, author of 3e347969a577]
> >>
> >> On Tue, Mar 28, 2023 at 04:59:30PM -0600, Alex Williamson wrote:  
> >>> Assignment of NVIDIA Ampere-based GPUs have seen a regression since the
> >>> below referenced commit, where the reduced D3hot transition delay appears
> >>> to introduce a small window where a D3hot->D0 transition followed by a bus
> >>> reset can wedge the device.  The entire device is subsequently unavailable,
> >>> returning -1 on config space read and is unrecoverable without a host reset.
> >>>
> >>> This has been observed with RTX A2000 and A5000 GPU and audio functions
> >>> assigned to a Windows VM, where shutdown of the VM places the devices in
> >>> D3hot prior to vfio-pci performing a bus reset when userspace releases the
> >>> devices.  The issue has roughly a 2-3% chance of occurring per shutdown.
> >>>
> >>> Restoring the HDA controller d3hot_delay to the effective value before the
> >>> below commit has been shown to resolve the issue.  
> >> Interesting.  This sounds like it was a hassle to track down.  I guess
> >> we knew there was some risk in reducing those delays.
> >>
> >> Did you by chance notice whether the actual delay when the device gets
> >> wedged is sufficient per spec?
> >>
> >> If there's a case where the usleep_range() doesn't quite wait the
> >> spec-mandated time, we should adjust that in case we have the same
> >> problem with other devices.  
> > That would have been a good test, unfortunately I didn't check and
> > don't currently have access to the system anymore.  Perhaps this is
> > something the NVIDIA folks can check as they're investigating the scope
> > of affected hardware.  Thanks,
> >
> > Alex
> >  
> >>> I'm looking for input from NVIDIA whether this issue is unique to
> >>> Ampere-based HDA controllers or should be assumed to linger in both older
> >>> and newer controllers as well.  Currently we've not been able to reproduce
> >>> the issue other than on Ampere HDA controllers, however the implementation
> >>> here includes all NVIDIA HDA controllers based on PCI vendor and device
> >>> class.  
> 
> Hi, I believe it is safe to include this patch for all HDA controllers and not
> limit to only Ampere-based HDA controllers. This patch looks fine to me.
> 
> Haven't explicitly tested whether usleep_range() performs as mandated, but
> from our testing it doesn't seem to be the problem. On the same platform,
> the problem reproduces on certain GPU configs but doesn't reproduce on other.

Thanks, Tarun.  Feel free to also send an explicit Acked-by: or
Reviewed-by: if you'd like.

Bjorn, I can resend this as a non-RFC if you prefer, the only change
would be deleting "I'm looking for input..." through to the Cc:s.
Thanks,

Alex

> >>>
> >>> If we were to limit the quirk to Ampere HDA controllers, I think that would
> >>> include:
> >>>
> >>> 1aef        GA102 High Definition Audio Controller
> >>> 228b        GA104 High Definition Audio Controller
> >>> 228e        GA106 High Definition Audio Controller
> >>>
> >>> Cc: Abhishek Sahu<abhsahu@nvidia.com>
> >>> Cc: Tarun Gupta<targupta@nvidia.com>
> >>> Fixes: 3e347969a577 ("PCI/PM: Reduce D3hot delay with usleep_range()")
> >>> Reported-by: Zhiyi Guo<zhguo@redhat.com>
> >>> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> >>> ---
> >>>   drivers/pci/quirks.c |   13 +++++++++++++
> >>>   1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> >>> index 44cab813bf95..f4e2a88729fd 100644
> >>> --- a/drivers/pci/quirks.c
> >>> +++ b/drivers/pci/quirks.c
> >>> @@ -1939,6 +1939,19 @@ static void quirk_radeon_pm(struct pci_dev *dev)
> >>>   }
> >>>   DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6741, quirk_radeon_pm);
> >>>
> >>> +/*
> >>> + * NVIDIA Ampere-based HDA controllers can wedge the whole device if a bus
> >>> + * reset is performed too soon after transition to D0, extend d3hot_delay
> >>> + * to previous effective default for all NVIDIA HDA controllers.
> >>> + */
> >>> +static void quirk_nvidia_hda_pm(struct pci_dev *dev)
> >>> +{
> >>> +   quirk_d3hot_delay(dev, 20);
> >>> +}
> >>> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
> >>> +                         PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8,
> >>> +                         quirk_nvidia_hda_pm);
> >>> +
> >>>   /*
> >>>    * Ryzen5/7 XHCI controllers fail upon resume from runtime suspend or s2idle.
> >>>    *https://bugzilla.kernel.org/show_bug.cgi?id=205587
> >>>
> >>>  

[PATCH] PCI: Extend D3hot delay for NVIDIA HDA controllers

[Alex Williamson]

Assignment of NVIDIA Ampere-based GPUs have seen a regression since the
below referenced commit, where the reduced D3hot transition delay appears
to introduce a small window where a D3hot->D0 transition followed by a bus
reset can wedge the device.  The entire device is subsequently unavailable,
returning -1 on config space read and is unrecoverable without a host reset.

This has been observed with RTX A2000 and A5000 GPU and audio functions
assigned to a Windows VM, where shutdown of the VM places the devices in
D3hot prior to vfio-pci performing a bus reset when userspace releases the
devices.  The issue has roughly a 2-3% chance of occurring per shutdown.

Restoring the HDA controller d3hot_delay to the effective value before the
below commit has been shown to resolve the issue.  NVIDIA confirms this
change should be safe for all of their HDA controllers.

Cc: Abhishek Sahu <abhsahu@nvidia.com>
Cc: Tarun Gupta <targupta@nvidia.com>
Fixes: 3e347969a577 ("PCI/PM: Reduce D3hot delay with usleep_range()")
Reported-by: Zhiyi Guo <zhguo@redhat.com>
Reviewed-by: Tarun Gupta <targupta@nvidia.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

Unfortunately Tarun's reply with confirmation doesn't show up on lore,
possibly due to html email, or else I'd provide that as a Link:.

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

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 44cab813bf95..f4e2a88729fd 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1939,6 +1939,19 @@ static void quirk_radeon_pm(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6741, quirk_radeon_pm);
 
+/*
+ * NVIDIA Ampere-based HDA controllers can wedge the whole device if a bus
+ * reset is performed too soon after transition to D0, extend d3hot_delay
+ * to previous effective default for all NVIDIA HDA controllers.
+ */
+static void quirk_nvidia_hda_pm(struct pci_dev *dev)
+{
+	quirk_d3hot_delay(dev, 20);
+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
+			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8,
+			      quirk_nvidia_hda_pm);
+
 /*
  * Ryzen5/7 XHCI controllers fail upon resume from runtime suspend or s2idle.
  * https://bugzilla.kernel.org/show_bug.cgi?id=205587
-- 
2.39.2

Re: [PATCH] PCI: Extend D3hot delay for NVIDIA HDA controllers

[Bjorn Helgaas]

[+cc Mika, Sathy, Lukas since they've been looking at similar delays]

On Thu, Apr 13, 2023 at 01:40:42PM -0600, Alex Williamson wrote:
> Assignment of NVIDIA Ampere-based GPUs have seen a regression since the
> below referenced commit, where the reduced D3hot transition delay appears
> to introduce a small window where a D3hot->D0 transition followed by a bus
> reset can wedge the device.  The entire device is subsequently unavailable,
> returning -1 on config space read and is unrecoverable without a host reset.
> 
> This has been observed with RTX A2000 and A5000 GPU and audio functions
> assigned to a Windows VM, where shutdown of the VM places the devices in
> D3hot prior to vfio-pci performing a bus reset when userspace releases the
> devices.  The issue has roughly a 2-3% chance of occurring per shutdown.
> 
> Restoring the HDA controller d3hot_delay to the effective value before the
> below commit has been shown to resolve the issue.  NVIDIA confirms this
> change should be safe for all of their HDA controllers.
> 
> Cc: Abhishek Sahu <abhsahu@nvidia.com>
> Cc: Tarun Gupta <targupta@nvidia.com>
> Fixes: 3e347969a577 ("PCI/PM: Reduce D3hot delay with usleep_range()")
> Reported-by: Zhiyi Guo <zhguo@redhat.com>
> Reviewed-by: Tarun Gupta <targupta@nvidia.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Applied to pci/reset for v6.4, thanks, Alex!

I guess there's no real risk here since we're waiting *longer*.  It
only makes NVIDIA GPU resets take longer.

Mika has some patches in flight that increase delays generically in
some cases, but I think that applies to D3cold -> D0 transitions,
which I don't *think* you're doing here.

> ---
> 
> Unfortunately Tarun's reply with confirmation doesn't show up on lore,
> possibly due to html email, or else I'd provide that as a Link:.
> 
>  drivers/pci/quirks.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 44cab813bf95..f4e2a88729fd 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -1939,6 +1939,19 @@ static void quirk_radeon_pm(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_ATI, 0x6741, quirk_radeon_pm);
>  
> +/*
> + * NVIDIA Ampere-based HDA controllers can wedge the whole device if a bus
> + * reset is performed too soon after transition to D0, extend d3hot_delay
> + * to previous effective default for all NVIDIA HDA controllers.
> + */
> +static void quirk_nvidia_hda_pm(struct pci_dev *dev)
> +{
> +	quirk_d3hot_delay(dev, 20);
> +}
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
> +			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8,
> +			      quirk_nvidia_hda_pm);
> +
>  /*
>   * Ryzen5/7 XHCI controllers fail upon resume from runtime suspend or s2idle.
>   * https://bugzilla.kernel.org/show_bug.cgi?id=205587
> -- 
> 2.39.2
>