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

From: Keith Busch <kbusch@kernel.org>
To: Hans de Goede <hdegoede@redhat.com>
Cc: "Limonciello, Mario" <Mario.Limonciello@amd.com>,
	Christoph Hellwig <hch@lst.de>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>,
	"Deucher, Alexander" <Alexander.Deucher@amd.com>,
	"Liang, Prike" <Prike.Liang@amd.com>,
	"axboe@fb.com" <axboe@fb.com>,
	"sagi@grimberg.me" <sagi@grimberg.me>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
	"S-k, Shyam-sundar" <Shyam-sundar.S-k@amd.com>
Subject: Re: [PATCH] nvme-pci: set some AMD PCIe downstream storage device to D3 for s2idle
Date: Wed, 26 May 2021 07:45:28 -0700	[thread overview]
Message-ID: <20210526144528.GC3704949@dhcp-10-100-145-180.wdc.com> (raw)
In-Reply-To: <8eb98344-cee8-df6b-8e72-1771b16702b6@redhat.com>

On Wed, May 26, 2021 at 10:52:35AM +0200, Hans de Goede wrote:
> On 5/25/21 5:18 PM, Limonciello, Mario wrote:
> > 
> > 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).

Rafael,

For context, here's the summary from my understanding:

We (linux-nvme) received a bug report that a platform fails to resume
after idle suspend due to mismatched behavior with the nvme driver.

When suspending, the nvme driver checks pm_suspend_via_firmware(). If
false, the driver assumes platform firmware will not alter our device's
power state after the kernel completes its suspend process.

But this platform's SMU firmware will remove power from the device.
Since the driver believed that wouldn't happen, the driver did not
prepare the device for this powerloss event.

It seems that the kernel's assumptions around pm_suspend_via_firmware()
and pm_suspend_no_platform() may not accurately reflect what the
platform's firmware actually does.

I do not know of a better way to detect if the platform will remove power,
so I'm looking at quirks to suppress PM_SUSPEND_FLAG_NO_PLATFORM for
this platform. I'm hoping there's a better option, though :)

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