[PATCH v4 1/2] PCI: add AMD PCIe quirk for nvme shutdown opt

[Liang, Prike]

[AMD Public Use]

> From: Keith Busch <kbusch at kernel.org>
> Sent: Friday, April 16, 2021 6:26 AM
> To: Liang, Prike <Prike.Liang at amd.com>
> Cc: Christoph Hellwig <hch at infradead.org>; linux-nvme at lists.infradead.org;
> Chaitanya.Kulkarni at wdc.com; gregkh at linuxfoundation.org;
> stable at vger.kernel.org; Deucher, Alexander
> <Alexander.Deucher at amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-
> k at amd.com>
> Subject: Re: [PATCH v4 1/2] PCI: add AMD PCIe quirk for nvme shutdown opt
>
> On Thu, Apr 15, 2021 at 09:41:53AM +0000, Liang, Prike wrote:
> > > From: Christoph Hellwig <hch at infradead.org>
> >
> > > I'd also much prefer if the flag is used on every pci_dev that is
> > > affected by the broken behavior rather than requiring another lookup in
> the driver.
> > Sorry can't get the meaning, could you give more detail how to implement
> this?
>
> The suggestion is child devices of the pci bus inherit the quirk so drivers don't
> need to look up the parent device that requires it.
>
> That makes sense for a couple reasons. For one, your hard-coded 0:0.0
> probably aligns to actual implementations, but I did't find a spec requirement
> that the host bridge occupy that BDf, so not having to look up a fixed location
> is more flexible.
>
> If I understand the suggestion correctly, I think it's probably easier to thread
> the quirk through the pci_bus->bus_flags. Does the below
> (untested) make sense?
>
Thanks Busch for elaborate clarification. The PCIe RC bus flag can pass to NVMe device successfully and it works well for this case. I will update the patch accordingly.
> ---
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index
> d47bb18b976a..022ff6cf202f 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2834,6 +2834,9 @@ static bool nvme_acpi_storage_d3(struct pci_dev
> *dev)
>  acpi_status status;
>  u8 val;
>
> +if (dev->bus->bus_flags & PCI_BUS_FLAGS_DISABLE_ON_S2I)
> +return true;
> +
>  /*
>   * Look for _DSD property specifying that the storage device on the
> port
>   * must use D3 to support deep platform power savings during diff --
> git a/drivers/pci/probe.c b/drivers/pci/probe.c index
> 953f15abc850..34ba691ec545 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -558,10 +558,13 @@ static struct pci_bus *pci_alloc_bus(struct pci_bus
> *parent)
>  INIT_LIST_HEAD(&b->resources);
>  b->max_bus_speed = PCI_SPEED_UNKNOWN;
>  b->cur_bus_speed = PCI_SPEED_UNKNOWN;
> +if (parent) {
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> -if (parent)
>  b->domain_nr = parent->domain_nr;
>  #endif
> +if (parent->bus_flags & PCI_BUS_FLAGS_DISABLE_ON_S2I)
> +b->bus_flags |= PCI_BUS_FLAGS_DISABLE_ON_S2I;
> +}
>  return b;
>  }
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index
> 653660e3ba9e..e8f74661138a 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -312,6 +312,14 @@ static void quirk_nopciamd(struct pci_dev *dev)  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD,
> PCI_DEVICE_ID_AMD_8151_0,quirk_nopciamd);
>
> +static void quirk_amd_s2i_fixup(struct pci_dev *dev) {
> +dev->bus->bus_flags |= PCI_BUS_FLAGS_DISABLE_ON_S2I;
> +pci_info(dev, "AMD simple suspend opt enabled\n");
> +
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_AMD, 0x1630,
> +quirk_amd_s2i_fixup);
> +
>  /* Triton requires workarounds to be used by the drivers */  static void
> quirk_triton(struct pci_dev *dev)  { diff --git a/include/linux/pci.h
> b/include/linux/pci.h index 86c799c97b77..7072e2ec88a2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -240,6 +240,8 @@ enum pci_bus_flags {
>  PCI_BUS_FLAGS_NO_MMRBC= (__force pci_bus_flags_t) 2,
>  PCI_BUS_FLAGS_NO_AERSID= (__force pci_bus_flags_t) 4,
>  PCI_BUS_FLAGS_NO_EXTCFG= (__force pci_bus_flags_t) 8,
> +/* Driver must pci_disable_device() for suspend-to-idle */
> +PCI_BUS_FLAGS_DISABLE_ON_S2I= (__force pci_bus_flags_t) 16,
>  };
>
>  /* Values from Link Status register, PCIe r3.1, sec 7.8.8 */
> --