[PATCH 0/3] [SRU Y/Z/A] APST gets enabled against explicit kernel option

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 0/3] [SRU Y/Z/A] APST gets enabled against explicit kernel option

Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1699004

[Impact]
The NVMe driver doesn't set APSTE on APST quirked devices at initialization.
If the BIOS or NVMe enables APST before driver loading, APST will never be
disabled, it also ignores explicit kernel option as result.
So the faulty NVMe may not work as intended.

[Test Case]
$ sudo nvme get-feature -f 0x0c -H /dev/nvme0
...will show APST is "Disabled" instead of "Enabled"

[Fix]
Originally, APSTA is used to determined whether APST should be enabled or not,
but it actually means hardware support instead of APST enabled/disabled.
Separate the logic "apst is supported" and "to enable apst", so we can
use the latter one to explicitly disable APST at initialiaztion.

[Regression Potential]
Very low.
This SRU didn't change anything really - it just explicitly set APSTE at
initialization.

Andy Lutomirski (2):
  nvme: Display raw APST configuration via DYNAMIC_DEBUG
  nvme: Add nvme_core.force_apst to ignore the NO_APST quirk

Kai-Heng Feng (1):
  nvme: explicitly disable APST on quirked devices

 drivers/nvme/host/core.c | 47 +++++++++++++++++++++++++++++++++++++++++------
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 42 insertions(+), 6 deletions(-)

--
2.13.2


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

[PATCH 1/3] [SRU Y/Z/A] nvme: Display raw APST configuration via DYNAMIC_DEBUG

Kai-Heng Feng
From: Andy Lutomirski <[hidden email]>

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

Debugging APST is currently a bit of a pain.  This gives optional
simple log messages that describe the APST state.

The easiest way to use this is probably with the nvme_core.dyndbg=+p
module parameter.

Signed-off-by: Andy Lutomirski <[hidden email]>
Reviewed-by: Christoph Hellwig <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(cherry picked from commit fb0dc3993b537e12ce63511d535ff86efff13c8f)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0b81bbf635a6..0acb533e5069 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1222,6 +1222,8 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
 
  unsigned apste;
  struct nvme_feat_auto_pst *table;
+ u64 max_lat_us = 0;
+ int max_ps = -1;
  int ret;
 
  /*
@@ -1243,6 +1245,7 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
  if (ctrl->ps_max_latency_us == 0) {
  /* Turn off APST. */
  apste = 0;
+ dev_dbg(ctrl->device, "APST disabled\n");
  } else {
  __le64 target = cpu_to_le64(0);
  int state;
@@ -1292,9 +1295,22 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
 
  target = cpu_to_le64((state << 3) |
      (transition_ms << 8));
+
+ if (max_ps == -1)
+ max_ps = state;
+
+ if (total_latency_us > max_lat_us)
+ max_lat_us = total_latency_us;
  }
 
  apste = 1;
+
+ if (max_ps == -1) {
+ dev_dbg(ctrl->device, "APST enabled but no non-operational states are available\n");
+ } else {
+ dev_dbg(ctrl->device, "APST enabled: max PS = %d, max round-trip latency = %lluus, table = %*phN\n",
+ max_ps, max_lat_us, (int)sizeof(*table), table);
+ }
  }
 
  ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
--
2.13.2


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

[PATCH 2/3] [SRU A] nvme: Add nvme_core.force_apst to ignore the NO_APST quirk

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Andy Lutomirski <[hidden email]>

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

We're probably going to be stuck quirking APST off on an over-broad
range of devices for 4.11.  Let's make it easy to override the quirk
for testing.

Signed-off-by: Andy Lutomirski <[hidden email]>
Reviewed-by: Christoph Hellwig <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(cherry picked from commit c35e30b4727b390ce7a6dd7ead31335320c2b83e)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 11c34f495fb9..78876bc1d485 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -62,6 +62,10 @@ module_param(default_ps_max_latency_us, ulong, 0644);
 MODULE_PARM_DESC(default_ps_max_latency_us,
  "max power saving latency for new devices; use PM QOS to change per device");
 
+static bool force_apst;
+module_param(force_apst, bool, 0644);
+MODULE_PARM_DESC(force_apst, "allow APST for newly enumerated devices even if quirked off");
+
 static LIST_HEAD(nvme_ctrl_list);
 static DEFINE_SPINLOCK(dev_list_lock);
 
@@ -1504,6 +1508,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
  }
  }
 
+ if (force_apst && (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS)) {
+ dev_warn(ctrl->dev, "forcibly allowing all power states due to nvme_core.force_apst -- use at your own risk\n");
+ ctrl->quirks &= ~NVME_QUIRK_NO_DEEPEST_PS;
+ }
+
  ctrl->oacs = le16_to_cpu(id->oacs);
  ctrl->vid = le16_to_cpu(id->vid);
  ctrl->oncs = le16_to_cpup(&id->oncs);
@@ -1526,7 +1535,16 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
  ctrl->npss = id->npss;
  prev_apsta = ctrl->apsta;
- ctrl->apsta = (ctrl->quirks & NVME_QUIRK_NO_APST) ? 0 : id->apsta;
+ if (ctrl->quirks & NVME_QUIRK_NO_APST) {
+ if (force_apst && id->apsta) {
+ dev_warn(ctrl->dev, "forcibly allowing APST due to nvme_core.force_apst -- use at your own risk\n");
+ ctrl->apsta = 1;
+ } else {
+ ctrl->apsta = 0;
+ }
+ } else {
+ ctrl->apsta = id->apsta;
+ }
  memcpy(ctrl->psd, id->psd, sizeof(ctrl->psd));
 
  if (ctrl->ops->is_fabrics) {
--
2.13.2


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

[PATCH 2/3] [SRU Y/Z] nvme: Add nvme_core.force_apst to ignore the NO_APST quirk

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
From: Andy Lutomirski <[hidden email]>

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

We're probably going to be stuck quirking APST off on an over-broad
range of devices for 4.11.  Let's make it easy to override the quirk
for testing.

Signed-off-by: Andy Lutomirski <[hidden email]>
Reviewed-by: Christoph Hellwig <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(backported from commit c35e30b4727b390ce7a6dd7ead31335320c2b83e)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0acb533e5069..cdda12bd1676 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -62,6 +62,10 @@ module_param(default_ps_max_latency_us, ulong, 0644);
 MODULE_PARM_DESC(default_ps_max_latency_us,
  "max power saving latency for new devices; use PM QOS to change per device");
 
+static bool force_apst;
+module_param(force_apst, bool, 0644);
+MODULE_PARM_DESC(force_apst, "allow APST for newly enumerated devices even if quirked off");
+
 static LIST_HEAD(nvme_ctrl_list);
 static DEFINE_SPINLOCK(dev_list_lock);
 
@@ -1448,6 +1452,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
  }
  }
 
+ if (force_apst && (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS)) {
+ dev_warn(ctrl->dev, "forcibly allowing all power states due to nvme_core.force_apst -- use at your own risk\n");
+ ctrl->quirks &= ~NVME_QUIRK_NO_DEEPEST_PS;
+ }
+
  ctrl->vid = le16_to_cpu(id->vid);
  ctrl->oncs = le16_to_cpup(&id->oncs);
  atomic_set(&ctrl->abort_limit, id->acl + 1);
@@ -1469,7 +1478,16 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
  ctrl->npss = id->npss;
  prev_apsta = ctrl->apsta;
- ctrl->apsta = (ctrl->quirks & NVME_QUIRK_NO_APST) ? 0 : id->apsta;
+ if (ctrl->quirks & NVME_QUIRK_NO_APST) {
+ if (force_apst && id->apsta) {
+ dev_warn(ctrl->dev, "forcibly allowing APST due to nvme_core.force_apst -- use at your own risk\n");
+ ctrl->apsta = 1;
+ } else {
+ ctrl->apsta = 0;
+ }
+ } else {
+ ctrl->apsta = id->apsta;
+ }
  memcpy(ctrl->psd, id->psd, sizeof(ctrl->psd));
 
  if (ctrl->ops->is_fabrics) {
--
2.13.2


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

[PATCH 3/3] [SRU Y/Z/A] nvme: explicitly disable APST on quirked devices

Kai-Heng Feng
In reply to this post by Kai-Heng Feng
BugLink: https://bugs.launchpad.net/bugs/1699004

A user reports APST is enabled, even when the NVMe is quirked or with
option "default_ps_max_latency_us=0".

The current logic will not set APST if the device is quirked. But the
NVMe in question will enable APST automatically.

Separate the logic "apst is supported" and "to enable apst", so we can
use the latter one to explicitly disable APST at initialiaztion.

BugLink: https://bugs.launchpad.net/bugs/1699004
Signed-off-by: Kai-Heng Feng <[hidden email]>
Reviewed-by: Andy Lutomirski <[hidden email]>
Signed-off-by: Keith Busch <[hidden email]>
(backported from commit 71a8a57ce79c4f3b19c2a5c0ccecede32341cb22 git://git.infradead.org/nvme.git nvme-4.13)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 19 ++++++++++---------
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cdda12bd1676..c50e8cf92c8b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1246,7 +1246,7 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
  if (!table)
  return;
 
- if (ctrl->ps_max_latency_us == 0) {
+ if (!ctrl->apst_enabled || ctrl->ps_max_latency_us == 0) {
  /* Turn off APST. */
  apste = 0;
  dev_dbg(ctrl->device, "APST disabled\n");
@@ -1410,7 +1410,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
  u64 cap;
  int ret, page_shift;
  u32 max_hw_sectors;
- u8 prev_apsta;
+ bool prev_apst_enabled;
 
  ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
  if (ret) {
@@ -1477,16 +1477,17 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
  ctrl->kas = le16_to_cpu(id->kas);
 
  ctrl->npss = id->npss;
- prev_apsta = ctrl->apsta;
+ ctrl->apsta = id->apsta;
+ prev_apst_enabled = ctrl->apst_enabled;
  if (ctrl->quirks & NVME_QUIRK_NO_APST) {
  if (force_apst && id->apsta) {
- dev_warn(ctrl->dev, "forcibly allowing APST due to nvme_core.force_apst -- use at your own risk\n");
- ctrl->apsta = 1;
+ dev_warn(ctrl->device, "forcibly allowing APST due to nvme_core.force_apst -- use at your own risk\n");
+ ctrl->apst_enabled = true;
  } else {
- ctrl->apsta = 0;
+ ctrl->apst_enabled = false;
  }
  } else {
- ctrl->apsta = id->apsta;
+ ctrl->apst_enabled = id->apsta;
  }
  memcpy(ctrl->psd, id->psd, sizeof(ctrl->psd));
 
@@ -1514,9 +1515,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
  kfree(id);
 
- if (ctrl->apsta && !prev_apsta)
+ if (ctrl->apst_enabled && !prev_apst_enabled)
  dev_pm_qos_expose_latency_tolerance(ctrl->device);
- else if (!ctrl->apsta && prev_apsta)
+ else if (!ctrl->apst_enabled && prev_apst_enabled)
  dev_pm_qos_hide_latency_tolerance(ctrl->device);
 
  nvme_configure_apst(ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 6ace64f855f4..7596ae072b5c 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -166,6 +166,7 @@ struct nvme_ctrl {
 
  /* Power saving configuration */
  u64 ps_max_latency_us;
+ bool apst_enabled;
 
  /* Fabrics only */
  u16 sqsize;
--
2.13.2


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

ACK / APPLIED[artful]: [PATCH 0/3] [SRU Y/Z/A] APST gets enabled against explicit kernel option

Seth Forshee
In reply to this post by Kai-Heng Feng
On Thu, Jun 29, 2017 at 05:59:18PM +0800, Kai-Heng Feng wrote:

> BugLink: https://bugs.launchpad.net/bugs/1699004
>
> [Impact]
> The NVMe driver doesn't set APSTE on APST quirked devices at initialization.
> If the BIOS or NVMe enables APST before driver loading, APST will never be
> disabled, it also ignores explicit kernel option as result.
> So the faulty NVMe may not work as intended.
>
> [Test Case]
> $ sudo nvme get-feature -f 0x0c -H /dev/nvme0
> ...will show APST is "Disabled" instead of "Enabled"
>
> [Fix]
> Originally, APSTA is used to determined whether APST should be enabled or not,
> but it actually means hardware support instead of APST enabled/disabled.
> Separate the logic "apst is supported" and "to enable apst", so we can
> use the latter one to explicitly disable APST at initialiaztion.
>
> [Regression Potential]
> Very low.
> This SRU didn't change anything really - it just explicitly set APSTE at
> initialization.
>
> Andy Lutomirski (2):
>   nvme: Display raw APST configuration via DYNAMIC_DEBUG
>   nvme: Add nvme_core.force_apst to ignore the NO_APST quirk
>
> Kai-Heng Feng (1):
>   nvme: explicitly disable APST on quirked devices

Note that this is now commit 76a5af841755a0427229a6a77ca83781d61e5b2a
upstream.

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

Applied to artful/master-next and unstable/master, thanks.

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

ACK: [PATCH 3/3] [SRU Y/Z] APST gets enabled against explicit kernel option

Stefan Bader-2
In reply to this post by Kai-Heng Feng
On 29.06.2017 11:59, Kai-Heng Feng wrote:

> BugLink: https://bugs.launchpad.net/bugs/1699004
>
> [Impact]
> The NVMe driver doesn't set APSTE on APST quirked devices at initialization.
> If the BIOS or NVMe enables APST before driver loading, APST will never be
> disabled, it also ignores explicit kernel option as result.
> So the faulty NVMe may not work as intended.
>
> [Test Case]
> $ sudo nvme get-feature -f 0x0c -H /dev/nvme0
> ...will show APST is "Disabled" instead of "Enabled"
>
> [Fix]
> Originally, APSTA is used to determined whether APST should be enabled or not,
> but it actually means hardware support instead of APST enabled/disabled.
> Separate the logic "apst is supported" and "to enable apst", so we can
> use the latter one to explicitly disable APST at initialiaztion.
>
> [Regression Potential]
> Very low.
> This SRU didn't change anything really - it just explicitly set APSTE at
> initialization.
>
> Andy Lutomirski (2):
>   nvme: Display raw APST configuration via DYNAMIC_DEBUG
>   nvme: Add nvme_core.force_apst to ignore the NO_APST quirk
>
> Kai-Heng Feng (1):
>   nvme: explicitly disable APST on quirked devices
>
>  drivers/nvme/host/core.c | 47 +++++++++++++++++++++++++++++++++++++++++------
>  drivers/nvme/host/nvme.h |  1 +
>  2 files changed, 42 insertions(+), 6 deletions(-)
>
Acked-by: Stefan Bader <[hidden email]>



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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

APPLIED: [PATCH 0/3] [SRU Y/Z/A] APST gets enabled against explicit kernel option

Thadeu Lima de Souza Cascardo-3
In reply to this post by Kai-Heng Feng
Applied to yakkety and zesty master-next branches.

Fixed the last patch to refer to the commit upstream.

Thanks.
Cascardo.

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