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

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Keith Busch <kbusch at kernel.org>
> Sent: Tuesday, May 25, 2021 15:24
> To: Limonciello, Mario <Mario.Limonciello at amd.com>
> Cc: Hans de Goede <hdegoede at redhat.com>; Deucher, Alexander
> <Alexander.Deucher at amd.com>; Christoph Hellwig <hch at lst.de>; Liang, Prike
> <Prike.Liang at amd.com>; 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>
> Subject: Re: [PATCH] nvme-pci: set some AMD PCIe downstream storage device
> to D3 for s2idle
> 
> On Tue, May 25, 2021 at 08:09:02PM +0000, Limonciello, Mario wrote:
> > [Public]
> >
> > > On Tue, May 25, 2021 at 02:06:09PM +0000, Limonciello, Mario wrote:
> > > > "Then the NVMe device will be shutdown by SMU firmware in the s2idle
> > > entry
> > > > and then will lost the NVMe power context during s2idle resume. Finally,
> > > > the NVMe command queue request will be processed abnormally and
> > > result
> > > > in access timeout"
> > >
> > > The nvme driver explicitly checks pm_set_suspend_via_firmware() in order
> > > to know if firmware may manipulate our device after completing the idle
> > > suspend. That is returning false here, yet firmware will do something
> > > anyway.
> >
> > pm_set_suspend_via_firmware is not set during s2idle - from
> drivers/acpi/sleep.c
> > it means ACPI S3 or ACPI S4 and thus pm_suspend_via_firmware however
> would
> > not be used.
> >
> > Overloading this definition on these AMD platforms to solve this NVME
> problem
> > would have unintended consequences.  Just glancing through the kernel I
> notice
> > the following drivers make use of that for decisions, which I would suspect to
> be
> > problematic:
> >
> > * cros_ec/gsmi (on any AMD chromebook, EC might receive wrong event and
> logging wrong)
> > * tpm
> > * i8042
> > * amdgpu (would break DPM_FLAG_SMART_SUSPEND)
> 
> Would it be less problematic if we check pm_suspend_no_platform()
> instead? According to the kernel-doc, that returns 'true' when
> firmware will not touch our device's power state, so it should return
> 'false' in order to be accurate here.
> 

At least at a glance to me this does more accurately reflect what you're
wanting to accomplish.

> Note, this is always set for PM_SUSPEND_TO_IDLE, so we'd still need a
> quirk to override it on this platform, but maybe this check doesn't
> have the same clashes for you like pm_suspend_via_firmware?

The thing affected by it are ACPI EC and pci/pci-driver.c that skips bus
mastering PM.  I think we'll have to have a look whether or not that causes
problems for wakeup and downstream devices.

Another way we could consider to go about it to try to get all those other
drivers that were clashing to use pm_suspend_no_platform instead.