Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.112.34 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.112.34; helo=mail.nvidia.com;
Subject: Re: [PATCH 1/1] PCI: Add pci reset quirk for Nvidia GPUs
To:     Alex Williamson <alex.williamson@redhat.com>,
        Sinan Kaya <okaya@kernel.org>
CC:     Bjorn Helgaas <bhelgaas@google.com>, <linux-pci@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, Vikram Sethi <vsethi@nvidia.com>
References: <20210423145402.14559-1-sdonthineni@nvidia.com>
 <ff4812ba-ec1d-9462-0cbd-029635af3267@kernel.org>
 <20210423093701.594efd86@redhat.com>
From:   Shanker R Donthineni <sdonthineni@nvidia.com>
Message-ID: <c758d8a8-4f8b-c505-118e-b364e93ae539@nvidia.com>
Date:   Fri, 23 Apr 2021 16:45:15 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20210423093701.594efd86@redhat.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Apr 2021 21:46:18.4623
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 50ec5824-b968-4f28-728a-08d906a13a34
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.112.34];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CO1NAM11FT026.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4748
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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
>