[AMD Public Use]
> From: Keith Busch <kbusch@xxxxxxxxxx>
> Sent: Friday, April 16, 2021 6:26 AM
> To: Liang, Prike <Prike.Liang@xxxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>; linux-nvme@xxxxxxxxxxxxxxxxxxx;
> Chaitanya.Kulkarni@xxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx;
> stable@xxxxxxxxxxxxxxx; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; S-k, Shyam-sundar <Shyam-sundar.S-
> k@xxxxxxx>
> 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@xxxxxxxxxxxxx>
> >
> > > 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 */
> --
