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

[Shanker R Donthineni]

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;
   ...
}

>    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