[PATCH 0/1][SRU][B][C][D][OEM-B]PC SN720 NVMe WDC 256GB consumes more power in S2Idle than during long idle

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/1][SRU][B][C][D][OEM-B]PC SN720 NVMe WDC 256GB consumes more power in S2Idle than during long idle

AceLan Kao
To avoid conflicts, this patch assumes below commits have been applied
on top of master-next branch on Bionic, Cosmic, and Disco kernel,
since both patches modify the same files on the same position for the
same issue.
https://lists.ubuntu.com/archives/kernel-team/2018-November/096832.html

BugLink: https://bugs.launchpad.net/bugs/1805775

[Impact]
On new systems that facilitate s2idle, we observed the power consumption
raises higher than long idle does during s2idle with Western Digital PC
SN720 NVMe SSD SDAPNTW-256G.

Short idle: 5.3
Long idle: 3.0
S2I: 5.07

[Fix]
Windows doesn't put nvme to D3 in modern standby, and uses its own APST
feature to do the power management. To leverage its APST feature during
s2idle, we can't disable nvme device while suspending, too.
So, here is what we did on the driver, 1. prevent nvme from entering D3,
2. prevent nvme from being disabled when suspending.

[Test]
Verified on the WD NVMe, it fixes the power consumption issue with no
regression. And the power consumption decreases to 1.66W during s2idle.

[Regression Potential]
Low, the patches only applied to specific nvme module, and from our test,
the system is still stable.

AceLan Kao (1):
  SAUCE: pci/nvme: prevent WDC PC SN720 NVMe from entering D3 and being
    disabled

 drivers/nvme/host/pci.c | 2 ++
 drivers/pci/quirks.c    | 1 +
 2 files changed, 3 insertions(+)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/1][SRU][B][OEM-B] SAUCE: pci/nvme: prevent WDC PC SN720 NVMe from entering D3 and being disabled

AceLan Kao
BugLink: https://bugs.launchpad.net/bugs/1805775

It leads to the power consumption increases 3.41W during s2idle, while it
consumes much less idle if forbidding put WDC NVMe to D3 and before
entering S2Idle.

Windows doesn't put NVMe to D3 in Modern Standby, and uses its own APST
feature to do the power management. To leverage its APST feature during
s2idle, we can't disable nvme device while suspending, too.

So, here is what we do to the driver:
- Prevent nvme from entering D3,
- Prevent nvme from being disabled when suspending.

Signed-off-by: AceLan Kao <[hidden email]>
---
 drivers/nvme/host/pci.c | 2 ++
 drivers/pci/quirks.c    | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 5cf91d071b1a..6b71875bac65 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2719,6 +2719,8 @@ static const struct pci_device_id nvme_id_table[] = {
  .driver_data = NVME_QUIRK_LIGHTNVM, },
  { PCI_VDEVICE(SK_HYNIX, 0x1527), /* Sk Hynix */
  .driver_data = NVME_QUIRK_NO_DISABLE, },
+ { PCI_DEVICE(0x15b7, 0x5002), /* Sandisk */
+ .driver_data = NVME_QUIRK_NO_DISABLE, },
  { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
  { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
  { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 858f2c5952e7..657d951e1bda 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1189,6 +1189,7 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_VIA, PCI_ANY_ID,
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SK_HYNIX, 0x1527, quirk_no_ata_d3);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0xf1a6, quirk_no_ata_d3);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SAMSUNG, 0xa808, quirk_no_ata_d3);
+DECLARE_PCI_FIXUP_EARLY(0x15b7, 0x5002, quirk_no_ata_d3);
 
 /* This was originally an Alpha specific thing, but it really fits here.
  * The i82375 PCI/EISA bridge appears as non-classified. Fix that.
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/1][SRU][C][D] SAUCE: pci/nvme: prevent WDC PC SN720 NVMe from entering D3 and being disabled

AceLan Kao
In reply to this post by AceLan Kao
BugLink: https://bugs.launchpad.net/bugs/1805775

It leads to the power consumption increases 3.41W during s2idle, while it
consumes much less idle if forbidding put WDC NVMe to D3 and before
entering S2Idle.

Windows doesn't put NVMe to D3 in Modern Standby, and uses its own APST
feature to do the power management. To leverage its APST feature during
s2idle, we can't disable nvme device while suspending, too.

So, here is what we do to the driver:
- Prevent nvme from entering D3,
- Prevent nvme from being disabled when suspending.

Signed-off-by: AceLan Kao <[hidden email]>
---
 drivers/nvme/host/pci.c | 2 ++
 drivers/pci/quirks.c    | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8c72ced501d7..fa320741f457 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2775,6 +2775,8 @@ static const struct pci_device_id nvme_id_table[] = {
  .driver_data = NVME_QUIRK_LIGHTNVM, },
  { PCI_VDEVICE(SK_HYNIX, 0x1527), /* Sk Hynix */
  .driver_data = NVME_QUIRK_NO_DISABLE, },
+ { PCI_DEVICE(0x15b7, 0x5002), /* Sandisk */
+ .driver_data = NVME_QUIRK_NO_DISABLE, },
  { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
  { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
  { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 232ed8ad2432..0a990b5b1264 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1340,6 +1340,7 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_VIA, PCI_ANY_ID,
  PCI_CLASS_STORAGE_IDE, 8, quirk_no_ata_d3);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SK_HYNIX, 0x1527, quirk_no_ata_d3);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0xf1a6, quirk_no_ata_d3);
+DECLARE_PCI_FIXUP_EARLY(0x15b7, 0x5002, quirk_no_ata_d3);
 
 /*
  * This was originally an Alpha-specific thing, but it really fits here.
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK/cmnt: [PATCH 1/1][SRU][B][OEM-B] SAUCE: pci/nvme: prevent WDC PC SN720 NVMe from entering D3 and being disabled

AceLan Kao
In reply to this post by AceLan Kao
Can't apply on bionic and oem kernel, will resend it later.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[RESEND][PATCH 1/1][B][OEM-B] SAUCE: pci/nvme: prevent WDC PC SN720 NVMe from entering D3 and being disabled

AceLan Kao
In reply to this post by AceLan Kao
BugLink: https://bugs.launchpad.net/bugs/1805775

It leads to the power consumption increases 3.41W during s2idle, while it
consumes much less idle if forbidding put WDC NVMe to D3 and before
entering S2Idle.

Windows doesn't put NVMe to D3 in Modern Standby, and uses its own APST
feature to do the power management. To leverage its APST feature during
s2idle, we can't disable nvme device while suspending, too.

So, here is what we do to the driver:
- Prevent nvme from entering D3,
- Prevent nvme from being disabled when suspending.

Signed-off-by: AceLan Kao <acelan.kao at canonical.com>
---
 drivers/nvme/host/pci.c | 2 ++
 drivers/pci/quirks.c    | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 63e8d3010d1c..eea4c99f3411 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2717,6 +2717,8 @@ static const struct pci_device_id nvme_id_table[] = {
  .driver_data = NVME_QUIRK_LIGHTNVM, },
  { PCI_VDEVICE(SK_HYNIX, 0x1527), /* Sk Hynix */
  .driver_data = NVME_QUIRK_NO_DISABLE, },
+ { PCI_DEVICE(0x15b7, 0x5002),   /* Sandisk */
+ .driver_data = NVME_QUIRK_NO_DISABLE, },
  { PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_EXPRESS, 0xffffff) },
  { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2001) },
  { PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x2003) },
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f81dea5bff71..54f68bb5e993 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1188,6 +1188,7 @@ DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_VIA, PCI_ANY_ID,
  PCI_CLASS_STORAGE_IDE, 8, quirk_no_ata_d3);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SK_HYNIX, 0x1527, quirk_no_ata_d3);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0xf1a6, quirk_no_ata_d3);
+DECLARE_PCI_FIXUP_EARLY(0x15b7, 0x5002, quirk_no_ata_d3);
 
 /* This was originally an Alpha specific thing, but it really fits here.
  * The i82375 PCI/EISA bridge appears as non-classified. Fix that.
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 0/1][SRU][B][C][D][OEM-B]PC SN720 NVMe WDC 256GB consumes more power in S2Idle than during long idle

Kai-Heng Feng
In reply to this post by AceLan Kao


> On Nov 29, 2018, at 3:44 PM, AceLan Kao <[hidden email]> wrote:
>
> To avoid conflicts, this patch assumes below commits have been applied
> on top of master-next branch on Bionic, Cosmic, and Disco kernel,
> since both patches modify the same files on the same position for the
> same issue.
> https://lists.ubuntu.com/archives/kernel-team/2018-November/096832.html
>
> BugLink: https://bugs.launchpad.net/bugs/1805775
>
> [Impact]
> On new systems that facilitate s2idle, we observed the power consumption
> raises higher than long idle does during s2idle with Western Digital PC
> SN720 NVMe SSD SDAPNTW-256G.
>
> Short idle: 5.3
> Long idle: 3.0
> S2I: 5.07
>
> [Fix]
> Windows doesn't put nvme to D3 in modern standby, and uses its own APST
> feature to do the power management. To leverage its APST feature during
> s2idle, we can't disable nvme device while suspending, too.
> So, here is what we did on the driver, 1. prevent nvme from entering D3,
> 2. prevent nvme from being disabled when suspending.
>
> [Test]
> Verified on the WD NVMe, it fixes the power consumption issue with no
> regression. And the power consumption decreases to 1.66W during s2idle.
>
> [Regression Potential]
> Low, the patches only applied to specific nvme module, and from our test,
> the system is still stable.
>
> AceLan Kao (1):
>  SAUCE: pci/nvme: prevent WDC PC SN720 NVMe from entering D3 and being
>    disabled
>
> drivers/nvme/host/pci.c | 2 ++
> drivers/pci/quirks.c    | 1 +
> 2 files changed, 3 insertions(+)

Acked-by: Kai-Heng Feng <[hidden email]>

>
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED[OEM-B]: [PATCH 0/1][SRU][B][C][D][OEM-B]PC SN720 NVMe WDC 256GB consumes more power in S2Idle than during long idle

AceLan Kao
In reply to this post by AceLan Kao
Applied on oem kernel 4.15.0-1029.34

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/cmt / APPLIED[D]: [PATCH 0/1][SRU][B][C][D][OEM-B]PC SN720 NVMe WDC 256GB consumes more power in S2Idle than during long idle

Seth Forshee
In reply to this post by AceLan Kao
On Thu, Nov 29, 2018 at 03:44:23PM +0800, AceLan Kao wrote:

> To avoid conflicts, this patch assumes below commits have been applied
> on top of master-next branch on Bionic, Cosmic, and Disco kernel,
> since both patches modify the same files on the same position for the
> same issue.
> https://lists.ubuntu.com/archives/kernel-team/2018-November/096832.html
>
> BugLink: https://bugs.launchpad.net/bugs/1805775
>
> [Impact]
> On new systems that facilitate s2idle, we observed the power consumption
> raises higher than long idle does during s2idle with Western Digital PC
> SN720 NVMe SSD SDAPNTW-256G.
>
> Short idle: 5.3
> Long idle: 3.0
> S2I: 5.07
>
> [Fix]
> Windows doesn't put nvme to D3 in modern standby, and uses its own APST
> feature to do the power management. To leverage its APST feature during
> s2idle, we can't disable nvme device while suspending, too.
> So, here is what we did on the driver, 1. prevent nvme from entering D3,
> 2. prevent nvme from being disabled when suspending.
>
> [Test]
> Verified on the WD NVMe, it fixes the power consumption issue with no
> regression. And the power consumption decreases to 1.66W during s2idle.
>
> [Regression Potential]
> Low, the patches only applied to specific nvme module, and from our test,
> the system is still stable.

Scope limited to a single device, positive testing. Note though that the
patches should be prefixed with "UBUNTU: SAUCE:" and not just "SAUCE:".

Acked-by: Seth Forshee <[hidden email]>

Is this patch going upstream?

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: ACK/cmt / APPLIED[D]: [PATCH 0/1][SRU][B][C][D][OEM-B]PC SN720 NVMe WDC 256GB consumes more power in S2Idle than during long idle

AceLan Kao
This patch is not going upstream.

Maintainer doesn't like the quirks, doesn't agree this should be fixed
in drivers,
and think it should be a firmware issue that doesn't behave well during D3.

We've setup many meetings with nvme vendors, and one vendor says from the spec.
there are no words that describes they should enter low power mode when they are
put into suspended state, and their firmware design doesn't allow to
add code to enter
low power mode.

We have more meetings with nvme vendors to figure out a better solution,
but currently adding quirk and preserve them in ubuntu kernel is the
only feasible way
we can do.
Seth Forshee <[hidden email]> 於 2018年12月5日 週三 下午9:57寫道:

>
> On Thu, Nov 29, 2018 at 03:44:23PM +0800, AceLan Kao wrote:
> > To avoid conflicts, this patch assumes below commits have been applied
> > on top of master-next branch on Bionic, Cosmic, and Disco kernel,
> > since both patches modify the same files on the same position for the
> > same issue.
> > https://lists.ubuntu.com/archives/kernel-team/2018-November/096832.html
> >
> > BugLink: https://bugs.launchpad.net/bugs/1805775
> >
> > [Impact]
> > On new systems that facilitate s2idle, we observed the power consumption
> > raises higher than long idle does during s2idle with Western Digital PC
> > SN720 NVMe SSD SDAPNTW-256G.
> >
> > Short idle: 5.3
> > Long idle: 3.0
> > S2I: 5.07
> >
> > [Fix]
> > Windows doesn't put nvme to D3 in modern standby, and uses its own APST
> > feature to do the power management. To leverage its APST feature during
> > s2idle, we can't disable nvme device while suspending, too.
> > So, here is what we did on the driver, 1. prevent nvme from entering D3,
> > 2. prevent nvme from being disabled when suspending.
> >
> > [Test]
> > Verified on the WD NVMe, it fixes the power consumption issue with no
> > regression. And the power consumption decreases to 1.66W during s2idle.
> >
> > [Regression Potential]
> > Low, the patches only applied to specific nvme module, and from our test,
> > the system is still stable.
>
> Scope limited to a single device, positive testing. Note though that the
> patches should be prefixed with "UBUNTU: SAUCE:" and not just "SAUCE:".
>
> Acked-by: Seth Forshee <[hidden email]>
>
> Is this patch going upstream?

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team