Re: [PATCH 1/1] PCI: Add pci reset quirk for Nvidia GPUs

[Alex Williamson]

On Fri, 23 Apr 2021 16:45:15 -0500
Shanker R Donthineni <sdonthineni@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx/