[Public]

quoted

Any ideas on how to handle this at that platform level?  We need to select the

NVME_QUIRK_SIMPLE_SUSPEND flag on certain AMD platforms.  This is a
platform firmware requirement.  It's not an NVME specific requirement, it's not
a PCIe specific requirement, it's a platform specific requirement.  DMI matching
doesn't really make sense because it affects all AMD platforms of a certain
generation.

Honestly I can understand that Christoph is a bit unhappy about this, but
the nvme driver seems like the right place for this to me and it is
already doing DMI based quirking for 1 specific Lenovo model, I don't
see why that would be ok, but a CPU-id based check would not be ok.

Yes - that was my point mentioned in the previous thread that led to this 
patch.

Both are ugly, yet both are unfortunately sometimes necessary.

Reading Christoph's reply a second time though, I believe that what
Christoph is trying to say, that this seems to be related to some
special suspend demands stemming from using the pci-e root ports
from the CPU, if instead the NVME device where situated behind
some pci-e switch, then the link to that switch would need to
power-down for the CPU's deep-sleep demands to be met, so this
really should be a (probably new?) flag on the pci-e parent of the
NVME device and then the nvme/host/pci.c code would set the
NVME_QUIRK_SIMPLE_SUSPEND flag based on the flag on its pci-e
parent, rather then having the CPU-id check inside the nvme code.

IOW I believe what Christoph is suggesting is the following:

1. Add some new flag (or some-such) to pci-e ports/links inside
Linux to signal the requirement for the link to be turned off
during suspend (or whatever the requirement actually is).

2. Make the drivers/pci code set that flag on the pci-e root
ports of the AMD CPUs which need this (based on the CPU-id as
done in the original patch).

3. Have the nvme/host/pci.c code would set the
NVME_QUIRK_SIMPLE_SUSPEND flag based on the new pci-e port/link
flag.

Actually that's what was proposed in earlier patch series, but Bjorn
wasn't happy with introducing a new quirk just for it, especially with it
being an issue stemming from a particular silicon generation only and was
pushing that it should be included in NVME driver directly.

Christoph have I understood correctly that this is more or less
what you are asking for ?

Note I don't know it this actually makes sense, because I don't
know all the details here. I believe that AMD is in the best
position to decide if this makes sense or not.

The back story here is that it's nothing to do with PCIe or NVME, but
an issue with what happens with the SMU in the SOC.

Quoting an earlier version commit message:

"Then the NVMe device will be shutdown by SMU firmware in the s2idle entry
and then will lost the NVMe power context during s2idle resume. Finally,
the NVMe command queue request will be processed abnormally and result
in access timeout"

Which I think begs the question - how about if we keep the quirks list and logic
outside of NVME and also outside of PCI but instead in an AMD owned platform
driver?  Something like this:

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a29b170701fc..ea3c8772cb11 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c

@@ -2837,6 +2837,12 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
        acpi_status status;
        u8 val;

+       /* Some AMD platforms need to use D3 due to SMU behavior */
+#if IS_REACHABLE(CONFIG_AMD_PMC)
+       if (amd_pmc_should_use_d3())
+               return true;
+#endif
+
        /*
         * Look for _DSD property specifying that the storage device on the port
         * must use D3 to support deep platform power savings during

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme