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

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

[Prike Liang]

In the NVMe controller default suspend mode use APST do the power state
suspend and resume and the NVMe remains in D0 during s2idle entry.Then the
NVMe device will be shutdown by firmware in the s0ix entry and will not
restore the third-party NVMe device power context in the firmware s0ix
resume. Finally,the NVMe will lost the power state during s2idle resume
and result in request queue timeout. So far,this issue only found on the
Renoir/Lucienne/Cezanne series and can be addressed by shutdown the NVMe
device in the s2idle entry.

Link:https://lore.kernel.org/stable/20210416155653.GA31818@redsun51.ssa.fujisawa.hgst.com/T/

Suggested-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/nvme/host/pci.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6bad4d4..49cd24e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -26,6 +26,9 @@
 #include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/sed-opal.h>
 #include <linux/pci-p2pdma.h>
+#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
+
 static bool nvme_acpi_storage_d3(struct pci_dev *dev)
 {
 	struct acpi_device *adev;
@@ -2836,6 +2849,13 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
 	acpi_status status;
 	u8 val;
 
+#ifdef CONFIG_X86
+	/*
+	 *  Set the NVMe on the target platform to D3 directly by kernel power management.
+	 */
+	if (x86_match_cpu(storage_d3_cpu_ids) && pm_suspend_default_s2idle())
+		return true;
+#endif
 	/*
 	 * Look for _DSD property specifying that the storage device on the port
 	 * must use D3 to support deep platform power savings during
-- 
2.7.4


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

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

[Christoph Hellwig]

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.

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

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

[Liang, Prike]

[Public]

> -----Original Message-----
> From: Christoph Hellwig <hch@lst.de>
> Sent: Tuesday, May 25, 2021 2:21 PM
> To: Liang, Prike <Prike.Liang@amd.com>
> Cc: kbusch@kernel.org; axboe@fb.com; hch@lst.de; sagi@grimberg.me;
> linux-nvme@lists.infradead.org; Deucher, Alexander
> <Alexander.Deucher@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-
> k@amd.com>; Limonciello, Mario <Mario.Limonciello@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.
This solution NAK is reasonable from software decouple perspective. As to this issue
seems only take care the NVMe D3 support during s2idle and the StorageD3Enable property
is defined for this purpose. How about approach this issue directly like as following fix?

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6bad4d4..8d89c84 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2856,8 +2856,15 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
        status = acpi_get_handle(adev->handle, "PXSX", &handle);
        if (ACPI_FAILURE(status)) {
                status = acpi_get_handle(adev->handle, "PEGP", &handle);
-               if (ACPI_FAILURE(status))
-                       return false;
+               if (ACPI_FAILURE(status)) {
+                       /*
+                        * In order to support NVMe D3 during s2idle, the property of AMD platform
+                        * is defined in the GPP6.NVME device.
+                        */
+                       status = acpi_get_handle(adev->handle, "\\_SB.PCI0.GPP6.NVME", &handle);
+                       if (ACPI_FAILURE(status))
+                               return false;
+               }
        }




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

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

[Christoph Hellwig]

On Tue, May 25, 2021 at 12:11:25PM +0000, Liang, Prike wrote:
> @@ -2856,8 +2856,15 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
>         status = acpi_get_handle(adev->handle, "PXSX", &handle);
>         if (ACPI_FAILURE(status)) {
>                 status = acpi_get_handle(adev->handle, "PEGP", &handle);
> -               if (ACPI_FAILURE(status))
> -                       return false;
> +               if (ACPI_FAILURE(status)) {
> +                       /*
> +                        * In order to support NVMe D3 during s2idle, the property of AMD platform
> +                        * is defined in the GPP6.NVME device.
> +                        */
> +                       status = acpi_get_handle(adev->handle, "\\_SB.PCI0.GPP6.NVME", &handle);
> +                       if (ACPI_FAILURE(status))
> +                               return false;
> +               }

This whole chink also really needs to move out of the NVMe driver.  We
can't accumulate tons of crap in a generic PCIe driver to work around
all kinds of broken x86 platforms.

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

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

[Deucher, Alexander]

[AMD Public Use]

> -----Original Message-----
> From: Christoph Hellwig <hch@lst.de>
> Sent: Tuesday, May 25, 2021 2:21 AM
> To: Liang, Prike <Prike.Liang@amd.com>
> Cc: kbusch@kernel.org; axboe@fb.com; hch@lst.de; sagi@grimberg.me;
> linux-nvme@lists.infradead.org; Deucher, Alexander
> <Alexander.Deucher@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-
> k@amd.com>; Limonciello, Mario <Mario.Limonciello@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.

Thanks,

Alex

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

Re: [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@lst.de>
>> Sent: Tuesday, May 25, 2021 2:21 AM
>> To: Liang, Prike <Prike.Liang@amd.com>
>> Cc: kbusch@kernel.org; axboe@fb.com; hch@lst.de; sagi@grimberg.me;
>> linux-nvme@lists.infradead.org; Deucher, Alexander
>> <Alexander.Deucher@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-
>> k@amd.com>; Limonciello, Mario <Mario.Limonciello@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



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

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

[Limonciello, Mario]

[Public]

> > 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.
> 

Yes - that was my point mentioned in the previous thread that led to this 
patch.

> 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.

Actually that's what was proposed in earlier patch series, but Bjorn
wasn't happy with introducing a new quirk just for it, especially with it
being an issue stemming from a particular silicon generation only and was
pushing that it should be included in NVME driver directly.

> 
> 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.
> 

The back story here is that it's nothing to do with PCIe or NVME, but
an issue with what happens with the SMU in the SOC.

Quoting an earlier version commit message:

"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"

Which I think begs the question - how about if we keep the quirks list and logic
outside of NVME and also outside of PCI but instead in an AMD owned platform
driver?  Something like this:

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index a29b170701fc..ea3c8772cb11 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2837,6 +2837,12 @@ static bool nvme_acpi_storage_d3(struct pci_dev *dev)
        acpi_status status;
        u8 val;

+       /* Some AMD platforms need to use D3 due to SMU behavior */
+#if IS_REACHABLE(CONFIG_AMD_PMC)
+       if (amd_pmc_should_use_d3())
+               return true;
+#endif
+
        /*
         * Look for _DSD property specifying that the storage device on the port
         * must use D3 to support deep platform power savings during
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Christoph Hellwig]

On Tue, May 25, 2021 at 02:06:09PM +0000, Limonciello, Mario wrote:
> Quoting an earlier version commit message:
> 
> "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"

Where shutdown means power is cut off, right?  NVMe also has the concept
of shutting down the controller using a sequence of register writes and
reads, but if the SMU firmware messes with that we'd have deeper problems
than this.

> Which I think begs the question - how about if we keep the quirks list and logic
> outside of NVME and also outside of PCI but instead in an AMD owned platform
> driver?  Something like this:

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.  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
 - 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 or by splitting the suspend method in different ones
for different use cases.  Platform-specific code (right now for Intel
and AMD) can then make sure drivers do get the right requests instead of
hardcoding platform information in every driver that wants to be able
to implement intelligent suspend behavior.

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

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

[Limonciello, Mario]

[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.

> or by splitting the suspend method in different ones
> for different use cases.  Platform-specific code (right now for Intel
> and AMD) can then make sure drivers do get the right requests instead of
> hardcoding platform information in every driver that wants to be able
> to implement intelligent suspend behavior.

This seems like a gross assumption though that evicting the quirks into a
central place that every driver needs to behave the same.  AMD's case is
specific to NVME, particularly because APST will be used otherwise.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Keith Busch]

On Tue, May 25, 2021 at 03:18:46PM +0000, Limonciello, Mario wrote:
> > or by splitting the suspend method in different ones
> > for different use cases.  Platform-specific code (right now for Intel
> > and AMD) can then make sure drivers do get the right requests instead of
> > hardcoding platform information in every driver that wants to be able
> > to implement intelligent suspend behavior.
> 
> This seems like a gross assumption though that evicting the quirks into a
> central place that every driver needs to behave the same.  AMD's case is
> specific to NVME, particularly because APST will be used otherwise.

I don't think you mean APST. That feature enables controller power state
autonomy, and we do not messed with that on the driver's suspend path.

We do use the explicit nvme specific host managed power state feature,
though.

But I also don't see why this is specific to nvme. Are you saying that
if a some other protocol device was in the same slot, it would be okay
to use its protocol specific power settings? That doesn't sound right.

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

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

[Limonciello, Mario]

[AMD Official Use Only]

> > This seems like a gross assumption though that evicting the quirks into a
> > central place that every driver needs to behave the same.  AMD's case is
> > specific to NVME, particularly because APST will be used otherwise.
> 
> I don't think you mean APST. That feature enables controller power state
> autonomy, and we do not messed with that on the driver's suspend path.

Relying upon the drive to go into the appropriate low power state via APST is one
aspect of it, and then ASPM for the PCIe link state.

> 
> We do use the explicit nvme specific host managed power state feature,
> though.
> 
> But I also don't see why this is specific to nvme. Are you saying that
> if a some other protocol device was in the same slot, it would be okay
> to use its protocol specific power settings? That doesn't sound right

Maybe Prike can comment more here, but all of these s2idle designs we're talking
about are mobile designs without external PCIe.  OEMs make the decisions on
what PCIe devices are chosen and how they go into the lowest state. 

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

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

[Keith Busch]

On Tue, May 25, 2021 at 06:27:52PM +0000, Limonciello, Mario wrote:
> [AMD Official Use Only]
> 
> > > This seems like a gross assumption though that evicting the quirks into a
> > > central place that every driver needs to behave the same.  AMD's case is
> > > specific to NVME, particularly because APST will be used otherwise.
> > 
> > I don't think you mean APST. That feature enables controller power state
> > autonomy, and we do not messed with that on the driver's suspend path.
> 
> Relying upon the drive to go into the appropriate low power state via APST is one
> aspect of it, and then ASPM for the PCIe link state.

That's not quite accurate. The driver has no use for APST in the s2idle
path. We use explicit host managed power settings here, and this works
the same with controllers that don't even support the optional APST
feature.

> > We do use the explicit nvme specific host managed power state feature,
> > though.
> > 
> > But I also don't see why this is specific to nvme. Are you saying that
> > if a some other protocol device was in the same slot, it would be okay
> > to use its protocol specific power settings? That doesn't sound right
> 
> Maybe Prike can comment more here, but all of these s2idle designs we're talking
> about are mobile designs without external PCIe.  OEMs make the decisions on
> what PCIe devices are chosen and how they go into the lowest state. 

While assuming nvme might be a plausible design choice, for any device
type occupying the slot, any driver bound to it needs to make the device
ready for cold before letting the s2idle process proceed. That's why the
quirk shouldn't be nvme driver specific.

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

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

[Chaitanya Kulkarni]

Limonciello,

On 5/25/21 11:39, Limonciello, Mario wrote:
> [AMD Official Use Only]
>

Please avoid using this tag on the mailing list.



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

Re: [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


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

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

[Christoph Hellwig]

On Wed, May 26, 2021 at 10:52:35AM +0200, Hans de Goede wrote:
> 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.

I think providing some information to the driver that the platform
expects to power off the PCIe slots after ->suspend is called would
be extremely useful.  We already had to disable the intelligent suspend
support for all devices with a HMB because of it, which then caused another
regression because some platform/device combination get hickups when
put into D3cold.  I wish PM wasn't such a mess..

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

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

[Keith Busch]

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

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

[Rafael J. Wysocki]

On Wednesday, May 26, 2021 4:45:28 PM CEST Keith Busch wrote:
> 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.

How exactly does it do that?

In particular, how does it get a chance to run?

> 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.

Note that this is not about whether or not AML will remove power from devices. 

It is about passing control entirely to the platform firmware at the end of the
suspend transition.

If instead the kernel executes AML that happens to remove power from some
devices, that is a totally different case which should not be confused with
the above.

> 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 :)

Honestly, I'm not sure about the clear understanding of what's really going on
here.




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

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

[Limonciello, Mario]

[Public]


> >
> > 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.
> 
> How exactly does it do that?

It's running as a result of a platform driver notifying it to run (amd-pmc).

> 
> In particular, how does it get a chance to run?
> 
> > 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.
> 
> Note that this is not about whether or not AML will remove power from devices.
> 
> It is about passing control entirely to the platform firmware at the end of the
> suspend transition.
> 
> If instead the kernel executes AML that happens to remove power from some
> devices, that is a totally different case which should not be confused with
> the above.
> 
> > 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 :)
> 
> Honestly, I'm not sure about the clear understanding of what's really going on
> here.
> 
> 

We'll discuss internally and come back with a different proposal.
Thanks all for your feedback.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Rafael J. Wysocki]

On Wednesday, May 26, 2021 7:02:08 PM CEST Limonciello, Mario wrote:
> [Public]
> 
> 
> > >
> > > 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.
> > 
> > How exactly does it do that?
> 
> It's running as a result of a platform driver notifying it to run (amd-pmc).

I guess this happens in one of the amd-pmc driver's system-wide suspend
callbacks.  Which one?

> > 
> > In particular, how does it get a chance to run?
> > 
> > > 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.
> > 
> > Note that this is not about whether or not AML will remove power from devices.
> > 
> > It is about passing control entirely to the platform firmware at the end of the
> > suspend transition.
> > 
> > If instead the kernel executes AML that happens to remove power from some
> > devices, that is a totally different case which should not be confused with
> > the above.
> > 
> > > 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 :)
> > 
> > Honestly, I'm not sure about the clear understanding of what's really going on
> > here.
> > 
> > 
> 
> We'll discuss internally and come back with a different proposal.
> Thanks all for your feedback.
> 

OK




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

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

[Limonciello, Mario]

[AMD Official Use Only]

> 
> I guess this happens in one of the amd-pmc driver's system-wide suspend
> callbacks.  Which one?

IIRC it should be caused by:
.suspend_noirq

> 
> > >
> > > In particular, how does it get a chance to run?
> > >
> > > > 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.
> > >
> > > Note that this is not about whether or not AML will remove power from
> devices.
> > >
> > > It is about passing control entirely to the platform firmware at the end of the
> > > suspend transition.
> > >
> > > If instead the kernel executes AML that happens to remove power from
> some
> > > devices, that is a totally different case which should not be confused with
> > > the above.
> > >
> > > > 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 :)
> > >
> > > Honestly, I'm not sure about the clear understanding of what's really going
> on
> > > here.
> > >
> > >
> >
> > We'll discuss internally and come back with a different proposal.
> > Thanks all for your feedback.
> >
> 
> OK
> 
> 
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Limonciello, Mario]

[Public]

[resend without AMD official use tags; sorry my email client likes to change this constantly]

> -----Original Message-----
> From: Rafael J. Wysocki <rjw@rjwysocki.net>
> Sent: Wednesday, May 26, 2021 12:28
> To: Hans de Goede <hdegoede@redhat.com>; Keith Busch
> <kbusch@kernel.org>; Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Christoph Hellwig <hch@lst.de>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Liang, Prike <Prike.Liang@amd.com>;
> axboe@fb.com; sagi@grimberg.me; 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
> 
> On Wednesday, May 26, 2021 7:02:08 PM CEST Limonciello, Mario wrote:
> > [Public]
> >
> >
> > > >
> > > > 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.
> > >
> > > How exactly does it do that?
> >
> > It's running as a result of a platform driver notifying it to run (amd-pmc).
> 
> I guess this happens in one of the amd-pmc driver's system-wide suspend
> callbacks.  Which one?

IIRC it should be caused by:
.suspend_noirq

> 
> > >
> > > In particular, how does it get a chance to run?
> > >
> > > > 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.
> > >
> > > Note that this is not about whether or not AML will remove power from
> devices.
> > >
> > > It is about passing control entirely to the platform firmware at the end of the
> > > suspend transition.
> > >
> > > If instead the kernel executes AML that happens to remove power from
> some
> > > devices, that is a totally different case which should not be confused with
> > > the above.
> > >
> > > > 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 :)
> > >
> > > Honestly, I'm not sure about the clear understanding of what's really going
> on
> > > here.
> > >
> > >
> >
> > We'll discuss internally and come back with a different proposal.
> > Thanks all for your feedback.
> >
> 
> OK
> 
> 
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

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

[Keith Busch]

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.

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

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

[Limonciello, Mario]

[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)

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

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

[Keith Busch]

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.

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?

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

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

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Keith Busch <kbusch@kernel.org>
> Sent: Tuesday, May 25, 2021 15:24
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Christoph Hellwig <hch@lst.de>; Liang, Prike
> <Prike.Liang@amd.com>; axboe@fb.com; sagi@grimberg.me; 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
> 
> 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.

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

RE: [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@redhat.com>
> Sent: Tuesday, May 25, 2021 9:54 AM
> To: Deucher, Alexander <Alexander.Deucher@amd.com>; Christoph Hellwig
> <hch@lst.de>; Liang, Prike <Prike.Liang@amd.com>
> Cc: kbusch@kernel.org; axboe@fb.com; sagi@grimberg.me; linux-
> nvme@lists.infradead.org; S-k, Shyam-sundar <Shyam-sundar.S-
> k@amd.com>; Limonciello, Mario <Mario.Limonciello@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@lst.de>
> >> Sent: Tuesday, May 25, 2021 2:21 AM
> >> To: Liang, Prike <Prike.Liang@amd.com>
> >> Cc: kbusch@kernel.org; axboe@fb.com; hch@lst.de; sagi@grimberg.me;
> >> linux-nvme@lists.infradead.org; Deucher, Alexander
> >> <Alexander.Deucher@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-
> >> k@amd.com>; Limonciello, Mario <Mario.Limonciello@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
> 
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme