[AMD Public Use]
From: Keith Busch kbusch@kernel.org Sent: Friday, April 16, 2021 6:26 AM To: Liang, Prike Prike.Liang@amd.com Cc: Christoph Hellwig hch@infradead.org; linux-nvme@lists.infradead.org; Chaitanya.Kulkarni@wdc.com; gregkh@linuxfoundation.org; stable@vger.kernel.org; Deucher, Alexander Alexander.Deucher@amd.com; S-k, Shyam-sundar <Shyam-sundar.S- k@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@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 */