[AMD Public Use]

-----Original Message-----
From: Hans de Goede <redacted>
Sent: Tuesday, May 25, 2021 9:54 AM
To: Deucher, Alexander <Alexander.Deucher@amd.com>; Christoph Hellwig
[off-list ref]; Liang, Prike [off-list ref]
Cc: kbusch@kernel.org; axboe@fb.com; sagi@grimberg.me; linux-
nvme@lists.infradead.org; S-k, Shyam-sundar <Shyam-sundar.S-
k@amd.com>; Limonciello, Mario [off-list ref]
Subject: Re: [PATCH] nvme-pci: set some AMD PCIe downstream storage
device to D3 for s2idle

Hi,

On 5/25/21 3:39 PM, Deucher, Alexander wrote:

quoted

[AMD Public Use]

quoted

-----Original Message-----
From: Christoph Hellwig <hch@lst.de>
Sent: Tuesday, May 25, 2021 2:21 AM
To: Liang, Prike <redacted>
Cc: kbusch@kernel.org; axboe@fb.com; hch@lst.de; sagi@grimberg.me;
linux-nvme@lists.infradead.org; Deucher, Alexander
[off-list ref]; S-k, Shyam-sundar <Shyam-sundar.S-
k@amd.com>; Limonciello, Mario [off-list ref]
Subject: Re: [PATCH] nvme-pci: set some AMD PCIe downstream storage
device to D3 for s2idle

On Tue, May 25, 2021 at 10:48:59AM +0800, Prike Liang wrote:

quoted

+#ifdef CONFIG_X86
+#include <asm/cpu_device_id.h>
+#endif

 #include "trace.h"
 #include "nvme.h"

@@ -2828,6 +2831,16 @@ static unsigned long

check_vendor_combination_bug(struct pci_dev *pdev)  }

 #ifdef CONFIG_ACPI
+
+#ifdef CONFIG_X86
+static const struct x86_cpu_id storage_d3_cpu_ids[] = {
+	X86_MATCH_VENDOR_FAM_MODEL(AMD, 25, 80, NULL),

/*Cezanne*/

quoted

+	X86_MATCH_VENDOR_FAM_MODEL(AMD, 23, 96, NULL),

/*Renoir*/

quoted

+	X86_MATCH_VENDOR_FAM_MODEL(AMD, 23, 104,

NULL),/*Lucienne*/

quoted

+	{}
+};
+#endif

This is completely unacceptable.  The NVMe driver could not care less
what CPU we on.  We need information from the PCI or power

managment

quoted

quoted

core on how broken the power management of the root port is, not this
kind of crap in a low-level driver, with potentially many more
needing the same kind of quirk in the future.

Hans,

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.

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.

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.

We tried something like that:
https://lore.kernel.org/stable/20210416155653.GA31818@redsun51.ssa.fujisawa.hgst.com/T/ (local)
But the issue isn't with the pcie root port, it's the platform firmware behavior.  Basically this requirement was standardized with the ACPI "StorageD3Enable" flag, but these platforms unfortunately don't implement that.

Alex

Regards,

Hans

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