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

[Hans de Goede]

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.

Regards,

Hans