[PATCH] nvme-pci: set some AMD PCIe downstream storage device to D3 for s2idle

[Deucher, Alexander]

[AMD Public Use]

> -----Original Message-----
> From: Hans de Goede <hdegoede at redhat.com>
> Sent: Tuesday, May 25, 2021 9:54 AM
> To: Deucher, Alexander <Alexander.Deucher at amd.com>; Christoph Hellwig
> <hch at lst.de>; Liang, Prike <Prike.Liang at amd.com>
> Cc: kbusch at kernel.org; axboe at fb.com; sagi at grimberg.me; linux-
> nvme at lists.infradead.org; S-k, Shyam-sundar <Shyam-sundar.S-
> k at amd.com>; Limonciello, Mario <Mario.Limonciello at amd.com>
> 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:
> > [AMD Public Use]
> >
> >> -----Original Message-----
> >> From: Christoph Hellwig <hch at lst.de>
> >> Sent: Tuesday, May 25, 2021 2:21 AM
> >> To: Liang, Prike <Prike.Liang at amd.com>
> >> Cc: kbusch at kernel.org; axboe at fb.com; hch at lst.de; sagi at grimberg.me;
> >> linux-nvme at lists.infradead.org; Deucher, Alexander
> >> <Alexander.Deucher at amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-
> >> k at amd.com>; Limonciello, Mario <Mario.Limonciello at amd.com>
> >> 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:
> >>> +#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*/
> >>> +	X86_MATCH_VENDOR_FAM_MODEL(AMD, 23, 96, NULL),
> >> /*Renoir*/
> >>> +	X86_MATCH_VENDOR_FAM_MODEL(AMD, 23, 104,
> >> NULL),/*Lucienne*/
> >>> +	{}
> >>> +};
> >>> +#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
> >> 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/
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
>