Re: [PATCH v4 2/2] PCI: Enable NO_BUS_RESET quirk for Nvidia GPUs

[Bjorn Helgaas]

On Mon, May 03, 2021 at 09:07:11PM -0500, Shanker R Donthineni wrote:
> On 5/3/21 5:42 PM, Bjorn Helgaas wrote:
> > Obviously _RST only works for built-in devices, since there's no AML
> > for plug-in devices, right?  So if there's a plug-in card with this
> > GPU, neither SBR nor _RST will work?
> These are not plug-in PCIe GPU cards, will exist on upcoming server
> baseboards. ACPI-reset should wok for plug-in devices as well as long
> as firmware has _RST method defined in ACPI-device associated with
> the PCIe hot-plug slot.

Maybe I'm missing something, but I don't see how _RST can work for
plug-in devices.  _RST is part of the system firmware, and that
firmware knows nothing about what will be plugged into the slot.  So
if system firmware supplies _RST that knows how to reset the Nvidia
GPU, it's not going to do the right thing if you plug in an NVMe
device instead.

Can you elaborate on how _RST would work for plug-in devices?  My only
point here is that IF this GPU is ever on a plug-in card, neither _RST
nor SBR would work, so we'd have to use whatever other reset methods
*do* work (I guess only FLR?)

> I've verified PCIe plug-in feature using SYSFS interface.
> 
> 1) Remove device using sysfs interface
>   root@test:/sys/bus/pci# echo 1 > devices/0005:01:00.0/remove
>   root@test:/sys/bus/pci# lspci -s 0005:01:00.0
>  
> 2) Rescan PCI bus using sysfs interface
>   root@test:/sys/bus/pci# echo 1 > devices/0005:00:00.0/rescan
>   root@test:/sys/bus/pci# lspci -s 0005:01:00.0
>   0005:01:00.0 3D controller: NVIDIA Corporation Device 2341 (rev a1)
> 
> 3) List current reset methods
>   root@jetson:/sys/bus/pci# cat devices/0005:01:00.0/reset_method
>   acpi,flr
> 
> Example AML code:
>  // Device definition for slot/devfn
>   Device(GPU0) {
>      Name(_ADR,0x00000000)
>      Method (_RST, 0)
>      {
>         printf("Entering ACPI _RST method")
>         // RESET code
>         printf("Exiting ACPI _RST method")
>      }
>   }
> 
> 4) Issue device reset from the userspace
>  root@test:/sys/bus/pci# echo 1 > devices/0005:01:00.0/reset
> 
> dmesg:
>  [ 6156.426303] ACPI Debug:  "Entering PCI9 _RST method"
>  [ 6156.427007] ACPI Debug:  "Exiting PCI9 _RST method"
> 
> > I'm wondering if we should log something to dmesg in
> > quirk_no_bus_reset(), quirk_no_pm_reset(), quirk_no_flr(), etc., just
> > so we have a hint about the fact that resets won't work quite as
> > expected on these devices.
> Yes, it would be very useful to know what PCI quirks were applied
> during boot.  Should I create a separate patch for adding pci_info()
> or include as part of this patch?

Don't include it as part of this patch.  It's a separate logical
change so should be a separate patch.  We can worry about that later.

>  --- a/drivers/pci/quirks.c
>  +++ b/drivers/pci/quirks.c
>  @@ -3556,6 +3556,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX, PCI_ANY_ID,
>   static void quirk_no_bus_reset(struct pci_dev *dev)
>   {
>          dev->dev_flags |= PCI_DEV_FLAGS_NO_BUS_RESET;
>        +pci_info(dev, "Applied NO_BUS_RESET quirk\n");
>   }
> 
>   /*
>  @@ -3598,6 +3599,7 @@ static void quirk_no_pm_reset(struct pci_dev *dev)
>           */
>          if (!pci_is_root_bus(dev->bus))
>                  dev->dev_flags |= PCI_DEV_FLAGS_NO_PM_RESET;
>         +pci_info(dev, "Applied NO_PM_RESET quirk\n");
>   }
> 
>   /*
>  @@ -5138,6 +5140,7 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x443, quirk_intel_qat_vf_cap);
>   static void quirk_no_flr(struct pci_dev *dev)
>   {
>          dev->dev_flags |= PCI_DEV_FLAGS_NO_FLR_RESET;
>         +pci_info(dev, "Applied NO_FLR_RESET quirk\n");
>   }
> 
>