On select platforms, some Nvidia GPU devices require platform-specific
quirks around device reset, and these GPUs do not work with FLR/SBR.
For these devices, add a quirk to handle the device reset in firmware.
Platforms that need the device reset quirk expose the firmware reset
method for the affected devices and the GPUs in these platforms have
a unique device ID range.
This reset issue will be fixed in the next generation of hardware.
Signed-off-by: Shanker Donthineni <[email protected]>
---
drivers/pci/quirks.c | 54 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 653660e3ba9e..23fc90d209c2 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3913,6 +3913,59 @@ static int delay_250ms_after_flr(struct pci_dev *dev, int probe)
return 0;
}
+/*
+ * Some Nvidia GPU devices do not work with bus reset, SBR needs to be
+ * prevented for those affected devices.
+ */
+static void quirk_nvidia_no_bus_reset(struct pci_dev *dev)
+{
+ if ((dev->device & 0xffc0) == 0x2340)
+ dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
+ quirk_nvidia_no_bus_reset);
+
+/*
+ * Some Nvidia GPU devices do not work with standard resets. These GPU
+ * devices are only in select systems and those systems have _RST method
+ * defined in the firmware. This quirk invokes a _RST() on the associated
+ * device to fix the reset issue.
+ */
+static int reset_nvidia_gpu_quirk(struct pci_dev *dev, int probe)
+{
+#ifdef CONFIG_ACPI
+ acpi_handle handle = ACPI_HANDLE(&dev->dev);
+
+ /*
+ * Check for the affected devices' ID range. If device is not in
+ * the affected range, return -ENOTTY indicating no device
+ * specific reset method is available.
+ */
+ if ((dev->device & 0xffc0) != 0x2340)
+ return -ENOTTY;
+
+ /*
+ * Return -ENOTTY indicating no device-specific reset method if _RST
+ * method is not defined
+ */
+ if (!handle || !acpi_has_method(handle, "_RST"))
+ return -ENOTTY;
+
+ /* Return 0 for probe phase indicating that we can reset this device */
+ if (probe)
+ return 0;
+
+ /* Invoke _RST() method to perform the device-specific reset */
+ if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
+ pci_warn(dev, "Failed to reset the device\n");
+ return -EINVAL;
+ }
+ return 0;
+#else
+ return -ENOTTY;
+#endif
+}
+
static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82599_SFP_VF,
reset_intel_82599_sfp_virtfn },
@@ -3924,6 +3977,7 @@ static const struct pci_dev_reset_methods pci_dev_reset_methods[] = {
{ PCI_VENDOR_ID_INTEL, 0x0953, delay_250ms_after_flr },
{ PCI_VENDOR_ID_CHELSIO, PCI_ANY_ID,
reset_chelsio_generic_dev },
+ { PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, reset_nvidia_gpu_quirk },
{ 0 }
};
--
2.17.1
+Alex,
On 4/23/2021 10:54 AM, Shanker Donthineni wrote:
> +static int reset_nvidia_gpu_quirk(struct pci_dev *dev, int probe)
> +{
> +#ifdef CONFIG_ACPI
> + acpi_handle handle = ACPI_HANDLE(&dev->dev);
> +
> + /*
> + * Check for the affected devices' ID range. If device is not in
> + * the affected range, return -ENOTTY indicating no device
> + * specific reset method is available.
> + */
> + if ((dev->device & 0xffc0) != 0x2340)
> + return -ENOTTY;
> +
> + /*
> + * Return -ENOTTY indicating no device-specific reset method if _RST
> + * method is not defined
> + */
> + if (!handle || !acpi_has_method(handle, "_RST"))
> + return -ENOTTY;
> +
> + /* Return 0 for probe phase indicating that we can reset this device */
> + if (probe)
> + return 0;
> +
> + /* Invoke _RST() method to perform the device-specific reset */
> + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
> + pci_warn(dev, "Failed to reset the device\n");
> + return -EINVAL;
> + }
> + return 0;
> +#else
> + return -ENOTTY;
> +#endif
> +}
Interesting, some pieces of this function (especially the ACPI _RST)
could be generalized.
On Fri, 23 Apr 2021 11:12:05 -0400
Sinan Kaya <[email protected]> wrote:
> +Alex,
>
> On 4/23/2021 10:54 AM, Shanker Donthineni wrote:
> > +static int reset_nvidia_gpu_quirk(struct pci_dev *dev, int probe)
> > +{
> > +#ifdef CONFIG_ACPI
> > + acpi_handle handle = ACPI_HANDLE(&dev->dev);
> > +
> > + /*
> > + * Check for the affected devices' ID range. If device is not in
> > + * the affected range, return -ENOTTY indicating no device
> > + * specific reset method is available.
> > + */
> > + if ((dev->device & 0xffc0) != 0x2340)
> > + return -ENOTTY;
> > +
> > + /*
> > + * Return -ENOTTY indicating no device-specific reset method if _RST
> > + * method is not defined
> > + */
> > + if (!handle || !acpi_has_method(handle, "_RST"))
> > + return -ENOTTY;
> > +
> > + /* Return 0 for probe phase indicating that we can reset this device */
> > + if (probe)
> > + return 0;
> > +
> > + /* Invoke _RST() method to perform the device-specific reset */
> > + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
> > + pci_warn(dev, "Failed to reset the device\n");
> > + return -EINVAL;
> > + }
> > + return 0;
> > +#else
> > + return -ENOTTY;
> > +#endif
> > +}
>
> Interesting, some pieces of this function (especially the ACPI _RST)
> could be generalized.
Agreed, we should add a new function level reset method for this rather
than a device specific reset. At that point the extent of the device
specific quirk could be to restrict SBR. It'd be useful to know what
these devices are (not in pciids yet), why we expect to only see them in
specific platforms (embedded device?), and the failure mode of the SBR.
Thanks,
Alex
On 4/23/21 10:37 AM, Alex Williamson wrote:
> External email: Use caution opening links or attachments
>
>
> On Fri, 23 Apr 2021 11:12:05 -0400
> Sinan Kaya <[email protected]> wrote:
>
>> +Alex,
>>
>> On 4/23/2021 10:54 AM, Shanker Donthineni wrote:
>>> +static int reset_nvidia_gpu_quirk(struct pci_dev *dev, int probe)
>>> +{
>>> +#ifdef CONFIG_ACPI
>>> + acpi_handle handle = ACPI_HANDLE(&dev->dev);
>>> +
>>> + /*
>>> + * Check for the affected devices' ID range. If device is not in
>>> + * the affected range, return -ENOTTY indicating no device
>>> + * specific reset method is available.
>>> + */
>>> + if ((dev->device & 0xffc0) != 0x2340)
>>> + return -ENOTTY;
>>> +
>>> + /*
>>> + * Return -ENOTTY indicating no device-specific reset method if _RST
>>> + * method is not defined
>>> + */
>>> + if (!handle || !acpi_has_method(handle, "_RST"))
>>> + return -ENOTTY;
>>> +
>>> + /* Return 0 for probe phase indicating that we can reset this device */
>>> + if (probe)
>>> + return 0;
>>> +
>>> + /* Invoke _RST() method to perform the device-specific reset */
>>> + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
>>> + pci_warn(dev, "Failed to reset the device\n");
>>> + return -EINVAL;
>>> + }
>>> + return 0;
>>> +#else
>>> + return -ENOTTY;
>>> +#endif
>>> +}
>> Interesting, some pieces of this function (especially the ACPI _RST)
>> could be generalized.
> Agreed, we should add a new function level reset method for this rather
> than a device specific reset. At that point the extent of the device
> specific quirk could be to restrict SBR.
Thanks Sinan/Alex, Agree ACPI _RST is a generic method applicable
to all PCI-ACPI-DEVICE objects. I'll define a new helper function
pci_dev_acpi_reset() and move common code to it. I've one question
before posting a v2 patch, should I call pci_dev_acpi_reset() from
the reset_nvidia_gpu_quirk() or always apply _RST method if exists?
Option-1:
static int reset_nvidia_gpu_quirk(struct pci_dev *dev, int probe)
{
/* Check for the affected devices' ID range */
if ((dev->device & 0xffc0) != 0x2340)
return -ENOTTY;
return pci_dev_acpi_reset(dev, probe);
}
OR
Option-2
int pci_dev_specific_reset(struct pci_dev *dev, int probe)
{
const struct pci_dev_reset_methods *i;
if (!pci_dev_acpi_reset(dev, probe))
return 0;
...
}
> It'd be useful to know what
> these devices are (not in pciids yet), why we expect to only see them in
> specific platforms (embedded device?), and the failure mode of the SBR.
These are not plug-in PCIe GPU cards, will exist on upcoming
server baseboards. Triggering SBR without firmware notification
would leave the device inoperable for the current system boot.
It requires a system hard-reboot to get the GPU device back to
normal operating condition post-SBR.
> Thanks,
>
> Alex
>
On Fri, Apr 23, 2021 at 04:45:15PM -0500, Shanker R Donthineni wrote:
> > specific platforms (embedded device?), and the failure mode of the SBR.
> These are not plug-in PCIe GPU cards, will exist on upcoming
> server baseboards. Triggering SBR without firmware notification
Please submit the quirks together with the actual support for the GPUs
in the nouveau driver, as they are completely useless without that.
On Fri, 23 Apr 2021 16:45:15 -0500
Shanker R Donthineni <[email protected]> wrote:
> On 4/23/21 10:37 AM, Alex Williamson wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Fri, 23 Apr 2021 11:12:05 -0400
> > Sinan Kaya <[email protected]> wrote:
> >
> >> +Alex,
> >>
> >> On 4/23/2021 10:54 AM, Shanker Donthineni wrote:
> >>> +static int reset_nvidia_gpu_quirk(struct pci_dev *dev, int probe)
> >>> +{
> >>> +#ifdef CONFIG_ACPI
> >>> + acpi_handle handle = ACPI_HANDLE(&dev->dev);
> >>> +
> >>> + /*
> >>> + * Check for the affected devices' ID range. If device is not in
> >>> + * the affected range, return -ENOTTY indicating no device
> >>> + * specific reset method is available.
> >>> + */
> >>> + if ((dev->device & 0xffc0) != 0x2340)
> >>> + return -ENOTTY;
> >>> +
> >>> + /*
> >>> + * Return -ENOTTY indicating no device-specific reset method if _RST
> >>> + * method is not defined
> >>> + */
> >>> + if (!handle || !acpi_has_method(handle, "_RST"))
> >>> + return -ENOTTY;
> >>> +
> >>> + /* Return 0 for probe phase indicating that we can reset this device */
> >>> + if (probe)
> >>> + return 0;
> >>> +
> >>> + /* Invoke _RST() method to perform the device-specific reset */
> >>> + if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
> >>> + pci_warn(dev, "Failed to reset the device\n");
> >>> + return -EINVAL;
> >>> + }
> >>> + return 0;
> >>> +#else
> >>> + return -ENOTTY;
> >>> +#endif
> >>> +}
> >> Interesting, some pieces of this function (especially the ACPI _RST)
> >> could be generalized.
> > Agreed, we should add a new function level reset method for this rather
> > than a device specific reset. At that point the extent of the device
> > specific quirk could be to restrict SBR.
> Thanks Sinan/Alex, Agree ACPI _RST is a generic method applicable
> to all PCI-ACPI-DEVICE objects. I'll define a new helper function
> pci_dev_acpi_reset() and move common code to it. I've one question
> before posting a v2 patch, should I call pci_dev_acpi_reset() from
> the reset_nvidia_gpu_quirk() or always apply _RST method if exists?
>
> Option-1:
> static int reset_nvidia_gpu_quirk(struct pci_dev *dev, int probe)
> {
> /* Check for the affected devices' ID range */
> if ((dev->device & 0xffc0) != 0x2340)
> return -ENOTTY;
> return pci_dev_acpi_reset(dev, probe);
> }
>
> OR
>
> Option-2
> int pci_dev_specific_reset(struct pci_dev *dev, int probe)
> {
> const struct pci_dev_reset_methods *i;
>
> if (!pci_dev_acpi_reset(dev, probe))
> return 0;
> ...
> }
Not quite either actually. I think this is a standard mechanism for
firmware to provide a reset method for a device, so it should be called
as a first-class mechanism from __pci_reset_function_locked() and
pci_probe_reset_function() rather than from within the device specific
callout. pci_dev_specific_reset() should only handle our own software
defined reset quirks for devices. It seems like we should be able to
safely probe any device for an ACPI device handle with _RST method
support.
I'd likely set the default priority of a a new acpi reset mechanism
below our own software defined resets, but above hardware resets. We
should only need the PCI header fixup quirk for this device to set the
NO_BUS_RESET flag, which would prevent userspace from re-prioritizing
SBR reset when we consider proposals like the one from Amey to allow
userspace policy management of reset mechanisms[1].
> > It'd be useful to know what
> > these devices are (not in pciids yet), why we expect to only see them in
> > specific platforms (embedded device?), and the failure mode of the SBR.
> These are not plug-in PCIe GPU cards, will exist on upcoming
> server baseboards. Triggering SBR without firmware notification
> would leave the device inoperable for the current system boot.
> It requires a system hard-reboot to get the GPU device back to
> normal operating condition post-SBR.
Any such descriptions you can include in the quirk to disable SBR for
this device, especially if you can share public code names, seems like
it would only help people associate this support to the hardware that
requires it. Thanks,
Alex
[1]https://lore.kernel.org/linux-pci/[email protected]/
On Mon, 26 Apr 2021 19:19:43 +0100
Christoph Hellwig <[email protected]> wrote:
> On Fri, Apr 23, 2021 at 04:45:15PM -0500, Shanker R Donthineni wrote:
> > > specific platforms (embedded device?), and the failure mode of the SBR.
> > These are not plug-in PCIe GPU cards, will exist on upcoming
> > server baseboards. Triggering SBR without firmware notification
>
> Please submit the quirks together with the actual support for the GPUs
> in the nouveau driver, as they are completely useless without that.
My default assumption would be that this resolves an issue with
assigning this device to a userspace or VM driver through vfio-pci, as
most in-kernel drivers don't make use of this interface themselves;
they often know more device specific ways to re-initialize hardware.
This reset path is also trivially accessible through pci-sysfs. I
don't expect nouveau would have much use for this even if it did
include support for these devices. Thanks,
Alex