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

[Hans de Goede]

Hi,

On 5/25/21 5:18 PM, Limonciello, Mario wrote:
> [Public]
> 
>>
>> I think what we're all missing here is that the concept of requring devices
>> to go to D3 for suspend to idle is a higher level concept.  
> 
> Ah.. so your argument being we should keep it a higher level concept in Linux
> kernel too.  IOW maybe even nvme_acpi_storage_d3 shouldn't be living 
> in drivers/nvme/host/pci.c, but somewhere else more acpi platform oriented.
> 
>> AFAIK this
>> comes from this microsoft document:
>>
>> https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro
>>
>> and spread from there.  Note that this document explicitly mentions AHCI
>> in addition to NVMe.  It also has some issues that I can spot:
>>
>>  - PCIe slots are not specific to storage device, so this really needs to
>>    apply to all devices
> 
> I don't disagree here but I'll point out that on the Windows side that page
> mentions that there is also:
> 1) A "global" registry key option
> 2) A hardcoded allowlist
> 
>>  - it generall is a rather bad idea to start with as each shutdown not
>>    only causes media progam/erase cycles, but also is not very power
>>    efficient.
>>
>> So what we need is a way for a driver to figure out if for a given
>> device it should shut down the device fully or just do something that
>> is efficient for saving as much as possible power.  That can be either
>> in form of a flag 
> 
> So how about a publishing a notification chain that a platform driver can
> optionally pick up and set that flag when the device is probed?  Coming
> back to my idea to throw this in amd-pmc, that could also potentially
> mean moving out the Lenovo DMI quirk and let something like
> thinkpad-acpi behave as a notifier and handle it too.
> 
> Hans, would appreciate your thoughts here.

I see that the discussion has already continued without my thoughts (good),
reading the further discussion I see that pm_suspend_via_firmware() and
pm_suspend_no_platform() have already been mentioned. Using these (or
introducing something similar for this use-case) was also my first
thought on this (after the previous options were shot down).

So I think that looking at those is going in the right direction. I notice
that Rafael Wysocki is missing from the Cc (I've added him now) I believe
that this is something which is right up his alley and he might have some
ideas on this.

Mario, can you provide a summary of the discussion so far for Rafael
(I believe you have a better picture of this then I do).

Regards,

Hans