| Message ID | 20210423145402.14559-1-sdonthineni@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] PCI: Add pci reset quirk for Nvidia GPUs | expand |
+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 <okaya@kernel.org> 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 <okaya@kernel.org> 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 <sdonthineni@nvidia.com> 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 <okaya@kernel.org> 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/20210409192324.30080-1-ameynarkhede03@gmail.com/
On Mon, 26 Apr 2021 19:19:43 +0100 Christoph Hellwig <hch@infradead.org> 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
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 } };
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 <sdonthineni@nvidia.com> --- drivers/pci/quirks.c | 54 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)