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

[Shanker R Donthineni <sdonthineni@nvidia.com>]

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
>