[SRU] [Xenial] [PATCH 00/14] Enable NVMe APST for Xenial, take two

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU] [Xenial] [PATCH 00/14] Enable NVMe APST for Xenial, take two

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

[Impact]
NVME APST feature is not enabled, so the power consumption is higher.

[Fix]
Enable NVME APST to conserve energy.

[Test]
Use 'nvme get-feature -f 0x0c -H /dev/nvme0' from nvme-cli to query the feature.

Before APST enabled:
get-feature:0xc (Autonomous Power State Transition), Current value:00000000
       Autonomous Power State Transition Enable (APSTE): Disabled

After APST enabled:
get-feature:0xc (Autonomous Power State Transition), Current value:0x000001
       Autonomous Power State Transition Enable (APSTE): Enabled

[Regression Potential]
Low. Now Artful is release for some time now, we only have one APST
issue, which is fixed by a new workaround.

Andy Lutomirski (8):
  nvme/scsi: Remove power management support
  nvme: Fix nvme_get/set_features() with a NULL result pointer
  nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
  nvme: Add a quirk mechanism that uses identify_ctrl
  nvme: Enable autonomous power state transitions
  nvme: Adjust the Samsung APST quirk
  nvme: Quirk APST off on "THNSF5256GPUK TOSHIBA"
  nvme: Quirk APST on Intel 600P/P3100 devices

Christoph Hellwig (3):
  nvme: return the whole CQE through the request passthrough interface
  nvme: factor out a add nvme_is_write helper
  nvme: Modify and export sync command submission for fabrics

Kai-Heng Feng (3):
  nvme: only consider exit latency when choosing useful non-op power
    states
  nvme: relax APST default max latency to 100ms
  nvme-pci: disable APST on Samsung SSD 960 EVO + ASUS PRIME B350M-A

 drivers/nvme/host/core.c | 287 +++++++++++++++++++++++++++++++++++++++++++----
 drivers/nvme/host/nvme.h |  27 ++++-
 drivers/nvme/host/pci.c  |  50 +++++++--
 drivers/nvme/host/scsi.c |  80 +------------
 include/linux/nvme.h     |   6 +
 5 files changed, 341 insertions(+), 109 deletions(-)

--
2.14.1


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

[SRU] [Xenial] [PATCH 01/14] nvme/scsi: Remove power management support

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

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

As far as I can tell, there is basically nothing correct about this
code.  It misinterprets npss (off-by-one).  It hardcodes a bunch of
power states, which is nonsense, because they're all just indices
into a table that software needs to parse.  It completely ignores
the distinction between operational and non-operational states.
And, until 4.8, if all of the above magically succeeded, it would
dereference a NULL pointer and OOPS.

Since this code appears to be useless, just delete it.

Signed-off-by: Andy Lutomirski <[hidden email]>
Reviewed-by: Christoph Hellwig <[hidden email]>
Acked-by: Jay Freyensee <[hidden email]>
Tested-by: Jay Freyensee <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(cherry picked from commit 26501db8dcbc3c63c0d8fb6c5bb098bc7d35d741)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/scsi.c | 74 ++----------------------------------------------
 1 file changed, 3 insertions(+), 71 deletions(-)

diff --git a/drivers/nvme/host/scsi.c b/drivers/nvme/host/scsi.c
index e947e298a737..44009105f8c8 100644
--- a/drivers/nvme/host/scsi.c
+++ b/drivers/nvme/host/scsi.c
@@ -72,15 +72,6 @@ static int sg_version_num = 30534; /* 2 digits for each component */
 #define ALL_LUNS_RETURNED 0x02
 #define ALL_WELL_KNOWN_LUNS_RETURNED 0x01
 #define RESTRICTED_LUNS_RETURNED 0x00
-#define NVME_POWER_STATE_START_VALID 0x00
-#define NVME_POWER_STATE_ACTIVE 0x01
-#define NVME_POWER_STATE_IDLE 0x02
-#define NVME_POWER_STATE_STANDBY 0x03
-#define NVME_POWER_STATE_LU_CONTROL 0x07
-#define POWER_STATE_0 0
-#define POWER_STATE_1 1
-#define POWER_STATE_2 2
-#define POWER_STATE_3 3
 #define DOWNLOAD_SAVE_ACTIVATE 0x05
 #define DOWNLOAD_SAVE_DEFER_ACTIVATE 0x0E
 #define ACTIVATE_DEFERRED_MICROCODE 0x0F
@@ -1229,64 +1220,6 @@ static void nvme_trans_fill_read_cap(u8 *response, struct nvme_id_ns *id_ns,
 
 /* Start Stop Unit Helper Functions */
 
-static int nvme_trans_power_state(struct nvme_ns *ns, struct sg_io_hdr *hdr,
- u8 pc, u8 pcmod, u8 start)
-{
- int res;
- int nvme_sc;
- struct nvme_id_ctrl *id_ctrl;
- int lowest_pow_st; /* max npss = lowest power consumption */
- unsigned ps_desired = 0;
-
- nvme_sc = nvme_identify_ctrl(ns->ctrl, &id_ctrl);
- res = nvme_trans_status_code(hdr, nvme_sc);
- if (res)
- return res;
-
- lowest_pow_st = max(POWER_STATE_0, (int)(id_ctrl->npss - 1));
- kfree(id_ctrl);
-
- switch (pc) {
- case NVME_POWER_STATE_START_VALID:
- /* Action unspecified if POWER CONDITION MODIFIER != 0 */
- if (pcmod == 0 && start == 0x1)
- ps_desired = POWER_STATE_0;
- if (pcmod == 0 && start == 0x0)
- ps_desired = lowest_pow_st;
- break;
- case NVME_POWER_STATE_ACTIVE:
- /* Action unspecified if POWER CONDITION MODIFIER != 0 */
- if (pcmod == 0)
- ps_desired = POWER_STATE_0;
- break;
- case NVME_POWER_STATE_IDLE:
- /* Action unspecified if POWER CONDITION MODIFIER != [0,1,2] */
- if (pcmod == 0x0)
- ps_desired = POWER_STATE_1;
- else if (pcmod == 0x1)
- ps_desired = POWER_STATE_2;
- else if (pcmod == 0x2)
- ps_desired = POWER_STATE_3;
- break;
- case NVME_POWER_STATE_STANDBY:
- /* Action unspecified if POWER CONDITION MODIFIER != [0,1] */
- if (pcmod == 0x0)
- ps_desired = max(POWER_STATE_0, (lowest_pow_st - 2));
- else if (pcmod == 0x1)
- ps_desired = max(POWER_STATE_0, (lowest_pow_st - 1));
- break;
- case NVME_POWER_STATE_LU_CONTROL:
- default:
- res = nvme_trans_completion(hdr, SAM_STAT_CHECK_CONDITION,
- ILLEGAL_REQUEST, SCSI_ASC_INVALID_CDB,
- SCSI_ASCQ_CAUSE_NOT_REPORTABLE);
- break;
- }
- nvme_sc = nvme_set_features(ns->ctrl, NVME_FEAT_POWER_MGMT, ps_desired, 0,
-    NULL);
- return nvme_trans_status_code(hdr, nvme_sc);
-}
-
 static int nvme_trans_send_activate_fw_cmd(struct nvme_ns *ns, struct sg_io_hdr *hdr,
  u8 buffer_id)
 {
@@ -2235,11 +2168,10 @@ static int nvme_trans_synchronize_cache(struct nvme_ns *ns,
 static int nvme_trans_start_stop(struct nvme_ns *ns, struct sg_io_hdr *hdr,
  u8 *cmd)
 {
- u8 immed, pcmod, pc, no_flush, start;
+ u8 immed, pcmod, no_flush, start;
 
  immed = cmd[1] & 0x01;
  pcmod = cmd[3] & 0x0f;
- pc = (cmd[4] & 0xf0) >> 4;
  no_flush = cmd[4] & 0x04;
  start = cmd[4] & 0x01;
 
@@ -2254,8 +2186,8 @@ static int nvme_trans_start_stop(struct nvme_ns *ns, struct sg_io_hdr *hdr,
  if (res)
  return res;
  }
- /* Setup the expected power state transition */
- return nvme_trans_power_state(ns, hdr, pc, pcmod, start);
+
+ return 0;
  }
 }
 
--
2.14.1


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

[SRU] [Xenial] [PATCH 02/14] nvme: return the whole CQE through the request passthrough interface

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

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

Both LighNVM and NVMe over Fabrics need to look at more than just the
status and result field.

Signed-off-by: Christoph Hellwig <[hidden email]>
Reviewed-by: Matias Bj?rling <[hidden email]>
Reviewed-by: Jay Freyensee <[hidden email]>
Reviewed-by: Sagi Grimberg <[hidden email]>
Signed-off-by: Sagi Grimberg <[hidden email]>
Reviewed-by: Keith Busch <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(backported from commit 1cb3cce5eb9de335330c8a147e47e3359a51a8b5)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 27 +++++++++++++++++++--------
 drivers/nvme/host/nvme.h |  3 ++-
 drivers/nvme/host/pci.c  | 10 +++-------
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 02598dd9633a..b9c6f98fd109 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -107,7 +107,6 @@ struct request *nvme_alloc_request(struct request_queue *q,
 
  req->cmd = (unsigned char *)cmd;
  req->cmd_len = sizeof(struct nvme_command);
- req->special = (void *)0;
 
  return req;
 }
@@ -117,7 +116,8 @@ struct request *nvme_alloc_request(struct request_queue *q,
  * if the result is positive, it's an NVM Express status code
  */
 int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
- void *buffer, unsigned bufflen, u32 *result, unsigned timeout)
+ struct nvme_completion *cqe, void *buffer, unsigned bufflen,
+ unsigned timeout)
 {
  struct request *req;
  int ret;
@@ -127,6 +127,7 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
  return PTR_ERR(req);
 
  req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
+ req->special = cqe;
 
  if (buffer && bufflen) {
  ret = blk_rq_map_kern(q, req, buffer, bufflen, GFP_KERNEL);
@@ -135,8 +136,6 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
  }
 
  blk_execute_rq(req->q, NULL, req, 0);
- if (result)
- *result = (u32)(uintptr_t)req->special;
  ret = req->errors;
  out:
  blk_mq_free_request(req);
@@ -146,7 +145,7 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
  void *buffer, unsigned bufflen)
 {
- return __nvme_submit_sync_cmd(q, cmd, buffer, bufflen, NULL, 0);
+ return __nvme_submit_sync_cmd(q, cmd, NULL, buffer, bufflen, 0);
 }
 
 int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
@@ -155,6 +154,7 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
  u32 *result, unsigned timeout)
 {
  bool write = cmd->common.opcode & 1;
+ struct nvme_completion cqe;
  struct nvme_ns *ns = q->queuedata;
  struct gendisk *disk = ns ? ns->disk : NULL;
  struct request *req;
@@ -167,6 +167,7 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
  return PTR_ERR(req);
 
  req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
+ req->special = &cqe;
 
  if (ubuffer && bufflen) {
  ret = blk_rq_map_user(q, req, NULL, ubuffer, bufflen,
@@ -221,7 +222,7 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
  blk_execute_rq(req->q, disk, req, 0);
  ret = req->errors;
  if (result)
- *result = (u32)(uintptr_t)req->special;
+ *result = le32_to_cpu(cqe.result);
  if (meta && !ret && !write) {
  if (copy_to_user(meta_buffer, meta, meta_len))
  ret = -EFAULT;
@@ -302,6 +303,8 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
  dma_addr_t dma_addr, u32 *result)
 {
  struct nvme_command c;
+ struct nvme_completion cqe;
+ int ret;
 
  memset(&c, 0, sizeof(c));
  c.features.opcode = nvme_admin_get_features;
@@ -309,13 +312,18 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
  c.features.dptr.prp1 = cpu_to_le64(dma_addr);
  c.features.fid = cpu_to_le32(fid);
 
- return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, 0, result, 0);
+ ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0);
+ if (ret >= 0)
+ *result = le32_to_cpu(cqe.result);
+ return ret;
 }
 
 int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
  dma_addr_t dma_addr, u32 *result)
 {
  struct nvme_command c;
+ struct nvme_completion cqe;
+ int ret;
 
  memset(&c, 0, sizeof(c));
  c.features.opcode = nvme_admin_set_features;
@@ -323,7 +331,10 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
  c.features.fid = cpu_to_le32(fid);
  c.features.dword11 = cpu_to_le32(dword11);
 
- return __nvme_submit_sync_cmd(dev->admin_q, &c, NULL, 0, result, 0);
+ ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0);
+ if (ret >= 0)
+ *result = le32_to_cpu(cqe.result);
+ return ret;
 }
 
 int nvme_get_log_page(struct nvme_ctrl *dev, struct nvme_smart_log **log)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index ff66714d624e..099f2385a890 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -272,7 +272,8 @@ void nvme_requeue_req(struct request *req);
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
  void *buf, unsigned bufflen);
 int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
- void *buffer, unsigned bufflen,  u32 *result, unsigned timeout);
+ struct nvme_completion *cqe, void *buffer, unsigned bufflen,
+ unsigned timeout);
 int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
  void __user *ubuffer, unsigned bufflen, u32 *result,
  unsigned timeout);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 1d6036286c5d..299c04e9a82f 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -863,10 +863,8 @@ static void __nvme_process_cq(struct nvme_queue *nvmeq, unsigned int *tag)
  }
 
  req = blk_mq_tag_to_rq(*nvmeq->tags, cqe.command_id);
- if (req->cmd_type == REQ_TYPE_DRV_PRIV) {
- u32 result = le32_to_cpu(cqe.result);
- req->special = (void *)(uintptr_t)result;
- }
+ if (req->cmd_type == REQ_TYPE_DRV_PRIV && req->special)
+ memcpy(req->special, &cqe, sizeof(cqe));
  blk_mq_complete_request(req, status >> 1);
 
  }
@@ -1023,12 +1021,10 @@ static void abort_endio(struct request *req, int error)
 {
  struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
  struct nvme_queue *nvmeq = iod->nvmeq;
- u32 result = (u32)(uintptr_t)req->special;
  u16 status = req->errors;
 
- dev_warn(nvmeq->q_dmadev, "Abort status:%x result:%x", status, result);
+ dev_warn(nvmeq->dev->ctrl.device, "Abort status: 0x%x", status);
  atomic_inc(&nvmeq->dev->ctrl.abort_limit);
-
  blk_mq_free_request(req);
 }
 
--
2.14.1


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

[SRU] [Xenial] [PATCH 03/14] nvme: factor out a add nvme_is_write helper

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

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

Centralize the check if a given NVMe command reads or writes data.

Reviewed-by: Sagi Grimberg <[hidden email]>
Reviewed-by: Jay Freyensee <[hidden email]>
Signed-off-by: Christoph Hellwig <[hidden email]>
Reviewed-by: Keith Busch <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(backported from commit 7a5abb4b48570c3552e33ff4c72ae1e8dac3ba15)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index b9c6f98fd109..455b1c91fd3e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -92,10 +92,9 @@ void nvme_requeue_req(struct request *req)
 struct request *nvme_alloc_request(struct request_queue *q,
  struct nvme_command *cmd, unsigned int flags)
 {
- bool write = cmd->common.opcode & 1;
  struct request *req;
 
- req = blk_mq_alloc_request(q, write, flags);
+ req = blk_mq_alloc_request(q, nvme_is_write(cmd), flags);
  if (IS_ERR(req))
  return req;
 
@@ -153,7 +152,7 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
  void __user *meta_buffer, unsigned meta_len, u32 meta_seed,
  u32 *result, unsigned timeout)
 {
- bool write = cmd->common.opcode & 1;
+ bool write = nvme_is_write(cmd);
  struct nvme_completion cqe;
  struct nvme_ns *ns = q->queuedata;
  struct gendisk *disk = ns ? ns->disk : NULL;
--
2.14.1


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

[SRU] [Xenial] [PATCH 04/14] nvme: Modify and export sync command submission for fabrics

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

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

NVMe over fabrics will use __nvme_submit_sync_cmd in the the
transport and require a few tweaks to it.  For that we export it
and add a few more paramters:

1. allow passing a queue ID to the block layer

   For the NVMe over Fabrics connect command we need to able to specify a
   queue ID that we want to send the command on.  Add a qid parameter to
   the relevant functions to enable this behavior.

2. allow submitting at_head commands

   In cases where we want to (re)connect to a controller
   where we have inflight queued commands we want to first
   connect and only then allow the other queued commands to
   be kicked. This will prevents failures in controller resets
   and reconnects.

3. allow passing flags to blk_mq_allocate_request

   Both for Fabrics connect the the keep-alive feature in NVMe 1.2.1 we
   want to be able to use reserved requests.

Reviewed-by: Jay Freyensee <[hidden email]>
Reviewed-by: Sagi Grimberg <[hidden email]>
Tested-by: Ming Lin <[hidden email]>
Signed-off-by: Christoph Hellwig <[hidden email]>
Reviewed-by: Keith Busch <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(cherry picked from commit eb71f435579ff61f342114ffaa662af163676753)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 27 ++++++++++++++++++---------
 drivers/nvme/host/nvme.h |  5 +++--
 drivers/nvme/host/pci.c  |  4 ++--
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 455b1c91fd3e..6e2c730ff5f4 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -90,11 +90,16 @@ void nvme_requeue_req(struct request *req)
 }
 
 struct request *nvme_alloc_request(struct request_queue *q,
- struct nvme_command *cmd, unsigned int flags)
+ struct nvme_command *cmd, unsigned int flags, int qid)
 {
  struct request *req;
 
- req = blk_mq_alloc_request(q, nvme_is_write(cmd), flags);
+ if (qid == NVME_QID_ANY) {
+ req = blk_mq_alloc_request(q, nvme_is_write(cmd), flags);
+ } else {
+ req = blk_mq_alloc_request_hctx(q, nvme_is_write(cmd), flags,
+ qid ? qid - 1 : 0);
+ }
  if (IS_ERR(req))
  return req;
 
@@ -116,12 +121,12 @@ struct request *nvme_alloc_request(struct request_queue *q,
  */
 int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
  struct nvme_completion *cqe, void *buffer, unsigned bufflen,
- unsigned timeout)
+ unsigned timeout, int qid, int at_head, int flags)
 {
  struct request *req;
  int ret;
 
- req = nvme_alloc_request(q, cmd, 0);
+ req = nvme_alloc_request(q, cmd, flags, qid);
  if (IS_ERR(req))
  return PTR_ERR(req);
 
@@ -134,17 +139,19 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
  goto out;
  }
 
- blk_execute_rq(req->q, NULL, req, 0);
+ blk_execute_rq(req->q, NULL, req, at_head);
  ret = req->errors;
  out:
  blk_mq_free_request(req);
  return ret;
 }
+EXPORT_SYMBOL_GPL(__nvme_submit_sync_cmd);
 
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
  void *buffer, unsigned bufflen)
 {
- return __nvme_submit_sync_cmd(q, cmd, NULL, buffer, bufflen, 0);
+ return __nvme_submit_sync_cmd(q, cmd, NULL, buffer, bufflen, 0,
+ NVME_QID_ANY, 0, 0);
 }
 
 int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
@@ -161,7 +168,7 @@ int __nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
  void *meta = NULL;
  int ret;
 
- req = nvme_alloc_request(q, cmd, 0);
+ req = nvme_alloc_request(q, cmd, 0, NVME_QID_ANY);
  if (IS_ERR(req))
  return PTR_ERR(req);
 
@@ -311,7 +318,8 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
  c.features.dptr.prp1 = cpu_to_le64(dma_addr);
  c.features.fid = cpu_to_le32(fid);
 
- ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0);
+ ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0,
+ NVME_QID_ANY, 0, 0);
  if (ret >= 0)
  *result = le32_to_cpu(cqe.result);
  return ret;
@@ -330,7 +338,8 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
  c.features.fid = cpu_to_le32(fid);
  c.features.dword11 = cpu_to_le32(dword11);
 
- ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0);
+ ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0,
+ NVME_QID_ANY, 0, 0);
  if (ret >= 0)
  *result = le32_to_cpu(cqe.result);
  return ret;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 099f2385a890..8198c96325e4 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -266,14 +266,15 @@ void nvme_stop_queues(struct nvme_ctrl *ctrl);
 void nvme_start_queues(struct nvme_ctrl *ctrl);
 void nvme_kill_queues(struct nvme_ctrl *ctrl);
 
+#define NVME_QID_ANY -1
 struct request *nvme_alloc_request(struct request_queue *q,
- struct nvme_command *cmd, unsigned int flags);
+ struct nvme_command *cmd, unsigned int flags, int qid);
 void nvme_requeue_req(struct request *req);
 int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
  void *buf, unsigned bufflen);
 int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
  struct nvme_completion *cqe, void *buffer, unsigned bufflen,
- unsigned timeout);
+ unsigned timeout, int qid, int at_head, int flags);
 int nvme_submit_user_cmd(struct request_queue *q, struct nvme_command *cmd,
  void __user *ubuffer, unsigned bufflen, u32 *result,
  unsigned timeout);
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 299c04e9a82f..8cbc4c083322 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1087,7 +1087,7 @@ static enum blk_eh_timer_return nvme_timeout(struct request *req, bool reserved)
  req->tag, nvmeq->qid);
 
  abort_req = nvme_alloc_request(dev->ctrl.admin_q, &cmd,
- BLK_MQ_REQ_NOWAIT);
+ BLK_MQ_REQ_NOWAIT, NVME_QID_ANY);
  if (IS_ERR(abort_req)) {
  atomic_inc(&dev->ctrl.abort_limit);
  return BLK_EH_RESET_TIMER;
@@ -1757,7 +1757,7 @@ static int nvme_delete_queue(struct nvme_queue *nvmeq, u8 opcode)
  cmd.delete_queue.opcode = opcode;
  cmd.delete_queue.qid = cpu_to_le16(nvmeq->qid);
 
- req = nvme_alloc_request(q, &cmd, BLK_MQ_REQ_NOWAIT);
+ req = nvme_alloc_request(q, &cmd, BLK_MQ_REQ_NOWAIT, NVME_QID_ANY);
  if (IS_ERR(req))
  return PTR_ERR(req);
 
--
2.14.1


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

[SRU] [Xenial] [PATCH 05/14] nvme: Fix nvme_get/set_features() with a NULL result pointer

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

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

nvme_set_features() callers seem to expect that passing NULL as the
result pointer is acceptable.  Teach nvme_set_features() not to try to
write to the NULL address.

For symmetry, make the same change to nvme_get_features(), despite the
fact that all current callers pass a valid result pointer.

I assume that this bug hasn't been reported in practice because
the callers that pass NULL are all in the SCSI translation layer
and no one uses the relevant operations.

Cc: [hidden email]
Signed-off-by: Andy Lutomirski <[hidden email]>
Reviewed-by: Sagi Grimberg <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(cherry picked from commit 9b47f77a680447e0132b2cf7fb82374e014bec1c)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 6e2c730ff5f4..ca2d8bbc99b7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -320,7 +320,7 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
 
  ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0,
  NVME_QID_ANY, 0, 0);
- if (ret >= 0)
+ if (ret >= 0 && result)
  *result = le32_to_cpu(cqe.result);
  return ret;
 }
@@ -340,7 +340,7 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 
  ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0,
  NVME_QID_ANY, 0, 0);
- if (ret >= 0)
+ if (ret >= 0 && result)
  *result = le32_to_cpu(cqe.result);
  return ret;
 }
--
2.14.1


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

[SRU] [Xenial] [PATCH 06/14] nvme: Pass pointers, not dma addresses, to nvme_get/set_features()

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

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

Any user I can imagine that needs a buffer at all will want to pass
a pointer directly.  There are no currently callers that use
buffers, so this change is painless, and it will make it much easier
to start using features that use buffers (e.g. APST).

Signed-off-by: Andy Lutomirski <[hidden email]>
Reviewed-by: Christoph Hellwig <[hidden email]>
Acked-by: Jay Freyensee <[hidden email]>
Tested-by: Jay Freyensee <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(cherry picked from commit 1a6fe74dfd1bb10afb41cbbbdc14890604be42a6)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 14 ++++++--------
 drivers/nvme/host/nvme.h |  4 ++--
 drivers/nvme/host/scsi.c |  6 +++---
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ca2d8bbc99b7..102eb424644e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -306,7 +306,7 @@ int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,
 }
 
 int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
- dma_addr_t dma_addr, u32 *result)
+      void *buffer, size_t buflen, u32 *result)
 {
  struct nvme_command c;
  struct nvme_completion cqe;
@@ -315,10 +315,9 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
  memset(&c, 0, sizeof(c));
  c.features.opcode = nvme_admin_get_features;
  c.features.nsid = cpu_to_le32(nsid);
- c.features.dptr.prp1 = cpu_to_le64(dma_addr);
  c.features.fid = cpu_to_le32(fid);
 
- ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0,
+ ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, buffer, buflen, 0,
  NVME_QID_ANY, 0, 0);
  if (ret >= 0 && result)
  *result = le32_to_cpu(cqe.result);
@@ -326,7 +325,7 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
 }
 
 int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
- dma_addr_t dma_addr, u32 *result)
+      void *buffer, size_t buflen, u32 *result)
 {
  struct nvme_command c;
  struct nvme_completion cqe;
@@ -334,12 +333,11 @@ int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
 
  memset(&c, 0, sizeof(c));
  c.features.opcode = nvme_admin_set_features;
- c.features.dptr.prp1 = cpu_to_le64(dma_addr);
  c.features.fid = cpu_to_le32(fid);
  c.features.dword11 = cpu_to_le32(dword11);
 
- ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe, NULL, 0, 0,
- NVME_QID_ANY, 0, 0);
+ ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &cqe,
+ buffer, buflen, 0, NVME_QID_ANY, 0, 0);
  if (ret >= 0 && result)
  *result = le32_to_cpu(cqe.result);
  return ret;
@@ -373,7 +371,7 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
  u32 result;
  int status, nr_io_queues;
 
- status = nvme_set_features(ctrl, NVME_FEAT_NUM_QUEUES, q_count, 0,
+ status = nvme_set_features(ctrl, NVME_FEAT_NUM_QUEUES, q_count, NULL, 0,
  &result);
  if (status)
  return status;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 8198c96325e4..bd93f40695ff 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -287,9 +287,9 @@ int nvme_identify_ns(struct nvme_ctrl *dev, unsigned nsid,
  struct nvme_id_ns **id);
 int nvme_get_log_page(struct nvme_ctrl *dev, struct nvme_smart_log **log);
 int nvme_get_features(struct nvme_ctrl *dev, unsigned fid, unsigned nsid,
- dma_addr_t dma_addr, u32 *result);
+      void *buffer, size_t buflen, u32 *result);
 int nvme_set_features(struct nvme_ctrl *dev, unsigned fid, unsigned dword11,
- dma_addr_t dma_addr, u32 *result);
+      void *buffer, size_t buflen, u32 *result);
 int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count);
 
 extern spinlock_t dev_list_lock;
diff --git a/drivers/nvme/host/scsi.c b/drivers/nvme/host/scsi.c
index 44009105f8c8..c2a0a1c7d05d 100644
--- a/drivers/nvme/host/scsi.c
+++ b/drivers/nvme/host/scsi.c
@@ -906,7 +906,7 @@ static int nvme_trans_log_temperature(struct nvme_ns *ns, struct sg_io_hdr *hdr,
  kfree(smart_log);
 
  /* Get Features for Temp Threshold */
- res = nvme_get_features(ns->ctrl, NVME_FEAT_TEMP_THRESH, 0, 0,
+ res = nvme_get_features(ns->ctrl, NVME_FEAT_TEMP_THRESH, 0, NULL, 0,
  &feature_resp);
  if (res != NVME_SC_SUCCESS)
  temp_c_thresh = LOG_TEMP_UNKNOWN;
@@ -1039,7 +1039,7 @@ static int nvme_trans_fill_caching_page(struct nvme_ns *ns,
  if (len < MODE_PAGE_CACHING_LEN)
  return -EINVAL;
 
- nvme_sc = nvme_get_features(ns->ctrl, NVME_FEAT_VOLATILE_WC, 0, 0,
+ nvme_sc = nvme_get_features(ns->ctrl, NVME_FEAT_VOLATILE_WC, 0, NULL, 0,
  &feature_resp);
  res = nvme_trans_status_code(hdr, nvme_sc);
  if (res)
@@ -1328,7 +1328,7 @@ static int nvme_trans_modesel_get_mp(struct nvme_ns *ns, struct sg_io_hdr *hdr,
  case MODE_PAGE_CACHING:
  dword11 = ((mode_page[2] & CACHING_MODE_PAGE_WCE_MASK) ? 1 : 0);
  nvme_sc = nvme_set_features(ns->ctrl, NVME_FEAT_VOLATILE_WC,
-    dword11, 0, NULL);
+    dword11, NULL, 0, NULL);
  res = nvme_trans_status_code(hdr, nvme_sc);
  break;
  case MODE_PAGE_CONTROL:
--
2.14.1


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

[SRU] [Xenial] [PATCH 07/14] nvme: Add a quirk mechanism that uses identify_ctrl

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

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

Currently, all NVMe quirks are based on PCI IDs.  Add a mechanism to
define quirks based on identify_ctrl's vendor id, model number,
and/or firmware revision.

Reviewed-by: Christoph Hellwig <[hidden email]>
Signed-off-by: Andy Lutomirski <[hidden email]>
Signed-off-by: Sagi Grimberg <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(backported from commit bd4da3abaabffdd2472fb7085fcadd5d1d8c2153)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 102eb424644e..c0f6b21a9a75 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -879,6 +879,50 @@ static void nvme_set_queue_limits(struct nvme_ctrl *ctrl,
  blk_queue_virt_boundary(q, ctrl->page_size - 1);
 }
 
+struct nvme_core_quirk_entry {
+ /*
+ * NVMe model and firmware strings are padded with spaces.  For
+ * simplicity, strings in the quirk table are padded with NULLs
+ * instead.
+ */
+ u16 vid;
+ const char *mn;
+ const char *fr;
+ unsigned long quirks;
+};
+
+static const struct nvme_core_quirk_entry core_quirks[] = {
+};
+
+/* match is null-terminated but idstr is space-padded. */
+static bool string_matches(const char *idstr, const char *match, size_t len)
+{
+ size_t matchlen;
+
+ if (!match)
+ return true;
+
+ matchlen = strlen(match);
+ WARN_ON_ONCE(matchlen > len);
+
+ if (memcmp(idstr, match, matchlen))
+ return false;
+
+ for (; matchlen < len; matchlen++)
+ if (idstr[matchlen] != ' ')
+ return false;
+
+ return true;
+}
+
+static bool quirk_matches(const struct nvme_id_ctrl *id,
+  const struct nvme_core_quirk_entry *q)
+{
+ return q->vid == le16_to_cpu(id->vid) &&
+ string_matches(id->mn, q->mn, sizeof(id->mn)) &&
+ string_matches(id->fr, q->fr, sizeof(id->fr));
+}
+
 /*
  * Initialize the cached copies of the Identify data and various controller
  * register in our nvme_ctrl structure.  This should be called as soon as
@@ -912,6 +956,24 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
  return -EIO;
  }
 
+ if (!ctrl->identified) {
+ /*
+ * Check for quirks.  Quirk can depend on firmware version,
+ * so, in principle, the set of quirks present can change
+ * across a reset.  As a possible future enhancement, we
+ * could re-scan for quirks every time we reinitialize
+ * the device, but we'd have to make sure that the driver
+ * behaves intelligently if the quirks change.
+ */
+
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(core_quirks); i++) {
+ if (quirk_matches(id, &core_quirks[i]))
+ ctrl->quirks |= core_quirks[i].quirks;
+ }
+ }
+
  ctrl->oncs = le16_to_cpup(&id->oncs);
  atomic_set(&ctrl->abort_limit, id->acl + 1);
  ctrl->vwc = id->vwc;
@@ -939,7 +1001,9 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
  nvme_set_queue_limits(ctrl, ctrl->admin_q);
 
  kfree(id);
- return 0;
+
+ ctrl->identified = true;
+ return ret;
 }
 
 static int nvme_dev_open(struct inode *inode, struct file *file)
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bd93f40695ff..e8e84aa65c28 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -81,6 +81,7 @@ enum nvme_quirks {
 #define NVME_QUIRK_DELAY_AMOUNT 2000
 
 struct nvme_ctrl {
+ bool identified;
  const struct nvme_ctrl_ops *ops;
  struct request_queue *admin_q;
  struct device *dev;
--
2.14.1


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

[SRU] [Xenial] [PATCH 08/14] nvme: Enable autonomous power state transitions

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

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

NVMe devices can advertise multiple power states.  These states can
be either "operational" (the device is fully functional but possibly
slow) or "non-operational" (the device is asleep until woken up).
Some devices can automatically enter a non-operational state when
idle for a specified amount of time and then automatically wake back
up when needed.

The hardware configuration is a table.  For each state, an entry in
the table indicates the next deeper non-operational state, if any,
to autonomously transition to and the idle time required before
transitioning.

This patch teaches the driver to program APST so that each successive
non-operational state will be entered after an idle time equal to 100%
of the total latency (entry plus exit) associated with that state.
The maximum acceptable latency is controlled using dev_pm_qos
(e.g. power/pm_qos_latency_tolerance_us in sysfs); non-operational
states with total latency greater than this value will not be used.
As a special case, setting the latency tolerance to 0 will disable
APST entirely.  On hardware without APST support, the sysfs file will
not be exposed.

The latency tolerance for newly-probed devices is set by the module
parameter nvme_core.default_ps_max_latency_us.

In theory, the device can expose "default" APST table, but this
doesn't seem to function correctly on my device (Samsung 950), nor
does it seem particularly useful.  There is also an optional
mechanism by which a configuration can be "saved" so it will be
automatically loaded on reset.  This can be configured from
userspace, but it doesn't seem useful to support in the driver.

On my laptop, enabling APST seems to save nearly 1W.

The hardware tables can be decoded in userspace with nvme-cli.
'nvme id-ctrl /dev/nvmeN' will show the power state table and
'nvme get-feature -f 0x0c -H /dev/nvme0' will show the current APST
configuration.

This feature is quirked off on a known-buggy Samsung device.

Signed-off-by: Andy Lutomirski <[hidden email]>
Reviewed-by: Christoph Hellwig <[hidden email]>
Signed-off-by: Sagi Grimberg <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(backported from commit c5552fde102fcc3f2cf9e502b8ac90e3500d8fdf)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h |  11 ++++
 include/linux/nvme.h     |   6 ++
 3 files changed, 171 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c0f6b21a9a75..65913b80e84e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -26,6 +26,7 @@
 #include <linux/ptrace.h>
 #include <linux/nvme_ioctl.h>
 #include <linux/t10-pi.h>
+#include <linux/pm_qos.h>
 #include <scsi/sg.h>
 #include <asm/unaligned.h>
 
@@ -39,6 +40,11 @@ module_param(nvme_major, int, 0);
 static int nvme_char_major;
 module_param(nvme_char_major, int, 0);
 
+static unsigned long default_ps_max_latency_us = 25000;
+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 LIST_HEAD(nvme_ctrl_list);
 DEFINE_SPINLOCK(dev_list_lock);
 
@@ -879,6 +885,122 @@ static void nvme_set_queue_limits(struct nvme_ctrl *ctrl,
  blk_queue_virt_boundary(q, ctrl->page_size - 1);
 }
 
+static void nvme_configure_apst(struct nvme_ctrl *ctrl)
+{
+ /*
+ * APST (Autonomous Power State Transition) lets us program a
+ * table of power state transitions that the controller will
+ * perform automatically.  We configure it with a simple
+ * heuristic: we are willing to spend at most 2% of the time
+ * transitioning between power states.  Therefore, when running
+ * in any given state, we will enter the next lower-power
+ * non-operational state after waiting 100 * (enlat + exlat)
+ * microseconds, as long as that state's total latency is under
+ * the requested maximum latency.
+ *
+ * We will not autonomously enter any non-operational state for
+ * which the total latency exceeds ps_max_latency_us.  Users
+ * can set ps_max_latency_us to zero to turn off APST.
+ */
+
+ unsigned apste;
+ struct nvme_feat_auto_pst *table;
+ int ret;
+
+ /*
+ * If APST isn't supported or if we haven't been initialized yet,
+ * then don't do anything.
+ */
+ if (!ctrl->apsta)
+ return;
+
+ if (ctrl->npss > 31) {
+ dev_warn(ctrl->device, "NPSS is invalid; not using APST\n");
+ return;
+ }
+
+ table = kzalloc(sizeof(*table), GFP_KERNEL);
+ if (!table)
+ return;
+
+ if (ctrl->ps_max_latency_us == 0) {
+ /* Turn off APST. */
+ apste = 0;
+ } else {
+ __le64 target = cpu_to_le64(0);
+ int state;
+
+ /*
+ * Walk through all states from lowest- to highest-power.
+ * According to the spec, lower-numbered states use more
+ * power.  NPSS, despite the name, is the index of the
+ * lowest-power state, not the number of states.
+ */
+ for (state = (int)ctrl->npss; state >= 0; state--) {
+ u64 total_latency_us, transition_ms;
+
+ if (target)
+ table->entries[state] = target;
+
+ /*
+ * Is this state a useful non-operational state for
+ * higher-power states to autonomously transition to?
+ */
+ if (!(ctrl->psd[state].flags &
+      NVME_PS_FLAGS_NON_OP_STATE))
+ continue;
+
+ total_latency_us =
+ (u64)le32_to_cpu(ctrl->psd[state].entry_lat) +
+ + le32_to_cpu(ctrl->psd[state].exit_lat);
+ if (total_latency_us > ctrl->ps_max_latency_us)
+ continue;
+
+ /*
+ * This state is good.  Use it as the APST idle
+ * target for higher power states.
+ */
+ transition_ms = total_latency_us + 19;
+ do_div(transition_ms, 20);
+ if (transition_ms > (1 << 24) - 1)
+ transition_ms = (1 << 24) - 1;
+
+ target = cpu_to_le64((state << 3) |
+     (transition_ms << 8));
+ }
+
+ apste = 1;
+ }
+
+ ret = nvme_set_features(ctrl, NVME_FEAT_AUTO_PST, apste,
+ table, sizeof(*table), NULL);
+ if (ret)
+ dev_err(ctrl->device, "failed to set APST feature (%d)\n", ret);
+
+ kfree(table);
+}
+
+static void nvme_set_latency_tolerance(struct device *dev, s32 val)
+{
+ struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+ u64 latency;
+
+ switch (val) {
+ case PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT:
+ case PM_QOS_LATENCY_ANY:
+ latency = U64_MAX;
+ break;
+
+ default:
+ latency = val;
+ }
+
+ if (ctrl->ps_max_latency_us != latency) {
+ ctrl->ps_max_latency_us = latency;
+ nvme_configure_apst(ctrl);
+ }
+}
+
 struct nvme_core_quirk_entry {
  /*
  * NVMe model and firmware strings are padded with spaces.  For
@@ -892,6 +1014,16 @@ struct nvme_core_quirk_entry {
 };
 
 static const struct nvme_core_quirk_entry core_quirks[] = {
+ /*
+ * Seen on a Samsung "SM951 NVMe SAMSUNG 256GB": using APST causes
+ * the controller to go out to lunch.  It dies when the watchdog
+ * timer reads CSTS and gets 0xffffffff.
+ */
+ {
+ .vid = 0x144d,
+ .fr = "BXW75D0Q",
+ .quirks = NVME_QUIRK_NO_APST,
+ },
 };
 
 /* match is null-terminated but idstr is space-padded. */
@@ -933,6 +1065,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
  struct nvme_id_ctrl *id;
  u64 cap;
  int ret, page_shift;
+ u8 prev_apsta;
 
  ret = ctrl->ops->reg_read32(ctrl, NVME_REG_VS, &ctrl->vs);
  if (ret) {
@@ -1000,9 +1133,22 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
  nvme_set_queue_limits(ctrl, ctrl->admin_q);
 
+ ctrl->npss = id->npss;
+ prev_apsta = ctrl->apsta;
+ ctrl->apsta = (ctrl->quirks & NVME_QUIRK_NO_APST) ? 0 : id->apsta;
+ memcpy(ctrl->psd, id->psd, sizeof(ctrl->psd));
+
  kfree(id);
 
+ if (ctrl->apsta && !prev_apsta)
+ dev_pm_qos_expose_latency_tolerance(ctrl->device);
+ else if (!ctrl->apsta && prev_apsta)
+ dev_pm_qos_hide_latency_tolerance(ctrl->device);
+
+ nvme_configure_apst(ctrl);
+
  ctrl->identified = true;
+
  return ret;
 }
 
@@ -1487,6 +1633,14 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
  list_add_tail(&ctrl->node, &nvme_ctrl_list);
  spin_unlock(&dev_list_lock);
 
+ /*
+ * Initialize latency tolerance controls.  The sysfs files won't
+ * be visible to userspace unless the device actually supports APST.
+ */
+ ctrl->device->power.set_latency_tolerance = nvme_set_latency_tolerance;
+ dev_pm_qos_update_user_latency_tolerance(ctrl->device,
+ min(default_ps_max_latency_us, (unsigned long)S32_MAX));
+
  return 0;
 out_release_instance:
  nvme_release_instance(ctrl);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index e8e84aa65c28..eadab0d351d7 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -71,6 +71,11 @@ enum nvme_quirks {
  * readiness, which is done by reading the NVME_CSTS_RDY bit.
  */
  NVME_QUIRK_DELAY_BEFORE_CHK_RDY = (1 << 3),
+
+ /*
+ * APST should not be used.
+ */
+ NVME_QUIRK_NO_APST = (1 << 4),
 };
 
 /* The below value is the specific amount of delay needed before checking
@@ -108,8 +113,14 @@ struct nvme_ctrl {
  u8 event_limit;
  u8 vwc;
  u32 vs;
+ u8 npss;
+ u8 apsta;
  bool subsystem;
  unsigned long quirks;
+ struct nvme_id_power_state psd[32];
+
+ /* Power saving configuration */
+ u64 ps_max_latency_us;
 };
 
 /*
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 7a0e08023d3f..660fc11153e6 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -528,6 +528,12 @@ struct nvme_dsm_range {
  __le64 slba;
 };
 
+/* Features */
+
+struct nvme_feat_auto_pst {
+ __le64 entries[32];
+};
+
 /* Admin commands */
 
 enum nvme_admin_opcode {
--
2.14.1


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

[SRU] [Xenial] [PATCH 09/14] nvme: Adjust the Samsung 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/1664602

I got a couple more reports: the Samsung APST issues appears to
affect multiple 950-series devices in Dell XPS 15 9550 and Precision
5510 laptops.  Change the quirk: rather than blacklisting the
firmware on the first problematic SSD that was reported, disable
APST on all 144d:a802 devices if they're installed in the two
affected Dell models.  While we're at it, disable only the deepest
sleep state instead of all of them -- the reporters say that this is
sufficient to fix the problem.

(I have a device that appears to be entirely identical to one of the
affected devices, but I have a different Dell laptop, so it's not
the case that all Samsung devices with firmware BXW75D0Q are broken
under all circumstances.)

Samsung engineers have an affected system, and hopefully they'll
give us a better workaround some time soon.  In the mean time, this
should minimize regressions.

See https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1678184

Cc: Kai-Heng Feng <[hidden email]>
Signed-off-by: Andy Lutomirski <[hidden email]>
Signed-off-by: Jens Axboe <[hidden email]>
(backported from commit ff5350a86b20de23991e474e006e2ff2732b218e)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 18 ++++++++----------
 drivers/nvme/host/nvme.h |  5 +++++
 drivers/nvme/host/pci.c  | 26 +++++++++++++++++++++++++-
 3 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 65913b80e84e..8fae040e0516 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -942,6 +942,14 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
  if (target)
  table->entries[state] = target;
 
+ /*
+ * Don't allow transitions to the deepest state
+ * if it's quirked off.
+ */
+ if (state == ctrl->npss &&
+    (ctrl->quirks & NVME_QUIRK_NO_DEEPEST_PS))
+ continue;
+
  /*
  * Is this state a useful non-operational state for
  * higher-power states to autonomously transition to?
@@ -1014,16 +1022,6 @@ struct nvme_core_quirk_entry {
 };
 
 static const struct nvme_core_quirk_entry core_quirks[] = {
- /*
- * Seen on a Samsung "SM951 NVMe SAMSUNG 256GB": using APST causes
- * the controller to go out to lunch.  It dies when the watchdog
- * timer reads CSTS and gets 0xffffffff.
- */
- {
- .vid = 0x144d,
- .fr = "BXW75D0Q",
- .quirks = NVME_QUIRK_NO_APST,
- },
 };
 
 /* match is null-terminated but idstr is space-padded. */
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index eadab0d351d7..1b90d7a7386a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -76,6 +76,11 @@ enum nvme_quirks {
  * APST should not be used.
  */
  NVME_QUIRK_NO_APST = (1 << 4),
+
+ /*
+ * The deepest sleep state should not be used.
+ */
+ NVME_QUIRK_NO_DEEPEST_PS = (1 << 5),
 };
 
 /* The below value is the specific amount of delay needed before checking
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 8cbc4c083322..f0c69e58425d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -19,6 +19,7 @@
 #include <linux/cpu.h>
 #include <linux/delay.h>
 #include <linux/dma-attrs.h>
+#include <linux/dmi.h>
 #include <linux/errno.h>
 #include <linux/fs.h>
 #include <linux/genhd.h>
@@ -2198,10 +2199,31 @@ static int nvme_dev_map(struct nvme_dev *dev)
        return -ENODEV;
 }
 
+static unsigned long check_dell_samsung_bug(struct pci_dev *pdev)
+{
+ if (pdev->vendor == 0x144d && pdev->device == 0xa802) {
+ /*
+ * Several Samsung devices seem to drop off the PCIe bus
+ * randomly when APST is on and uses the deepest sleep state.
+ * This has been observed on a Samsung "SM951 NVMe SAMSUNG
+ * 256GB", a "PM951 NVMe SAMSUNG 512GB", and a "Samsung SSD
+ * 950 PRO 256GB", but it seems to be restricted to two Dell
+ * laptops.
+ */
+ if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.") &&
+    (dmi_match(DMI_PRODUCT_NAME, "XPS 15 9550") ||
+     dmi_match(DMI_PRODUCT_NAME, "Precision 5510")))
+ return NVME_QUIRK_NO_DEEPEST_PS;
+ }
+
+ return 0;
+}
+
 static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
  int node, result = -ENOMEM;
  struct nvme_dev *dev;
+ unsigned long quirks = id->driver_data;
 
  node = dev_to_node(&pdev->dev);
  if (node == NUMA_NO_NODE)
@@ -2239,8 +2261,10 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  if (result)
  goto put_pci;
 
+ quirks |= check_dell_samsung_bug(pdev);
+
  result = nvme_init_ctrl(&dev->ctrl, &pdev->dev, &nvme_pci_ctrl_ops,
- id->driver_data);
+ quirks);
  if (result)
  goto release_pools;
 
--
2.14.1


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

[SRU] [Xenial] [PATCH 10/14] nvme: Quirk APST off on "THNSF5256GPUK TOSHIBA"

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

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

There's a report that it malfunctions with APST on.

See https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1678184

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8fae040e0516..1ab61070b133 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1022,6 +1022,15 @@ struct nvme_core_quirk_entry {
 };
 
 static const struct nvme_core_quirk_entry core_quirks[] = {
+ {
+ /*
+ * This Toshiba device seems to die using any APST states.  See:
+ * https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1678184/comments/11
+ */
+ .vid = 0x1179,
+ .mn = "THNSF5256GPUK TOSHIBA",
+ .quirks = NVME_QUIRK_NO_APST,
+ }
 };
 
 /* match is null-terminated but idstr is space-padded. */
--
2.14.1


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

[SRU] [Xenial] [PATCH 11/14] nvme: only consider exit latency when choosing useful non-op power states

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

When a NVMe is in non-op states, the latency is exlat.
The latency will be enlat + exlat only when the NVMe tries to transit
from operational state right atfer it begins to transit to
non-operational state, which should be a rare case.

Therefore, as Andy Lutomirski suggests, use exlat only when deciding power
states to trainsit to.

Signed-off-by: Kai-Heng Feng <[hidden email]>
Signed-off-by: Christoph Hellwig <[hidden email]>
(backported from commit da87591bea92204fcb921bac927666eb7141908e)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1ab61070b133..9c1d675055f9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -894,8 +894,8 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
  * heuristic: we are willing to spend at most 2% of the time
  * transitioning between power states.  Therefore, when running
  * in any given state, we will enter the next lower-power
- * non-operational state after waiting 100 * (enlat + exlat)
- * microseconds, as long as that state's total latency is under
+ * non-operational state after waiting 50 * (enlat + exlat)
+ * microseconds, as long as that state's exit latency is under
  * the requested maximum latency.
  *
  * We will not autonomously enter any non-operational state for
@@ -937,7 +937,7 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
  * lowest-power state, not the number of states.
  */
  for (state = (int)ctrl->npss; state >= 0; state--) {
- u64 total_latency_us, transition_ms;
+ u64 total_latency_us, exit_latency_us, transition_ms;
 
  if (target)
  table->entries[state] = target;
@@ -958,12 +958,15 @@ static void nvme_configure_apst(struct nvme_ctrl *ctrl)
       NVME_PS_FLAGS_NON_OP_STATE))
  continue;
 
- total_latency_us =
- (u64)le32_to_cpu(ctrl->psd[state].entry_lat) +
- + le32_to_cpu(ctrl->psd[state].exit_lat);
- if (total_latency_us > ctrl->ps_max_latency_us)
+ exit_latency_us =
+ (u64)le32_to_cpu(ctrl->psd[state].exit_lat);
+ if (exit_latency_us > ctrl->ps_max_latency_us)
  continue;
 
+ total_latency_us =
+ exit_latency_us +
+ le32_to_cpu(ctrl->psd[state].entry_lat);
+
  /*
  * This state is good.  Use it as the APST idle
  * target for higher power states.
--
2.14.1


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

[SRU] [Xenial] [PATCH 12/14] nvme: relax APST default max latency to 100ms

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

Christoph Hellwig suggests we should to make APST work out of the box.
Hence relax the the default max latency to make them able to enter
deepest power state on default.

Here are id-ctrl excerpts from two high latency NVMes:

vid     : 0x14a4
ssvid   : 0x1b4b
mn      : CX2-GB1024-Q11 NVMe LITEON 1024GB
ps    3 : mp:0.1000W non-operational enlat:5000 exlat:5000 rrt:3 rrl:3
          rwt:3 rwl:3 idle_power:- active_power:-
ps    4 : mp:0.0100W non-operational enlat:50000 exlat:100000 rrt:4 rrl:4
          rwt:4 rwl:4 idle_power:- active_power:-

vid     : 0x15b7
ssvid   : 0x1b4b
mn      : A400 NVMe SanDisk 512GB
ps    3 : mp:0.0500W non-operational enlat:51000 exlat:10000 rrt:0 rrl:0
          rwt:0 rwl:0 idle_power:- active_power:-
ps    4 : mp:0.0055W non-operational enlat:1000000 exlat:100000 rrt:0 rrl:0
          rwt:0 rwl:0 idle_power:- active_power:-

Signed-off-by: Kai-Heng Feng <[hidden email]>
Signed-off-by: Christoph Hellwig <[hidden email]>
(cherry picked from commit 9947d6a09cd71937dade2fc14640e4843ae19802)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 9c1d675055f9..680f57ada425 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -40,7 +40,7 @@ module_param(nvme_major, int, 0);
 static int nvme_char_major;
 module_param(nvme_char_major, int, 0);
 
-static unsigned long default_ps_max_latency_us = 25000;
+static unsigned long default_ps_max_latency_us = 100000;
 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");
--
2.14.1


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

[SRU] [Xenial] [PATCH 13/14] nvme: Quirk APST on Intel 600P/P3100 devices

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

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

They have known firmware bugs.  A fix is apparently in the works --
once fixed firmware is available, someone from Intel (Hi, Keith!)
can adjust the quirk accordingly.

Cc: [hidden email] # v4.11
Cc: Kai-Heng Feng <[hidden email]>
Cc: Mario Limonciello <[hidden email]>
Signed-off-by: Andy Lutomirski <[hidden email]>
Signed-off-by: Christoph Hellwig <[hidden email]>
(backported from commit 50af47d04ca530544b27affffb0722f158e2bb9c)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index f0c69e58425d..689a541c663a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2393,6 +2393,8 @@ static const struct pci_device_id nvme_id_table[] = {
  { PCI_VDEVICE(INTEL, 0x0953),
  .driver_data = NVME_QUIRK_STRIPE_SIZE |
  NVME_QUIRK_DISCARD_ZEROES, },
+ { PCI_VDEVICE(INTEL, 0xf1a5), /* Intel 600P/P3100 */
+ .driver_data = NVME_QUIRK_NO_DEEPEST_PS },
  { PCI_VDEVICE(INTEL, 0x5845), /* Qemu emulated controller */
  .driver_data = NVME_QUIRK_IDENTIFY_CNS, },
  { PCI_DEVICE(0x1c58, 0x0003), /* HGST adapter */
--
2.14.1


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

[SRU] [Xenial] [PATCH 14/14] nvme-pci: disable APST on Samsung SSD 960 EVO + ASUS PRIME B350M-A

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

The NVMe device in question drops off the PCIe bus after system suspend.
I've tried several approaches to workaround this issue, but none of them
works:
- NVME_QUIRK_DELAY_BEFORE_CHK_RDY
- NVME_QUIRK_NO_DEEPEST_PS
- Disable APST before controller shutdown
- Delay between controller shutdown and system suspend
- Explicitly set power state to 0 before controller shutdown

Fortunately it's a desktop, so disable APST won't hurt the battery.

Also, change the quirk function name to reflect it's for vendor
combination quirks.

BugLink: https://bugs.launchpad.net/bugs/1705748
Signed-off-by: Kai-Heng Feng <[hidden email]>
Signed-off-by: Christoph Hellwig <[hidden email]>
(cherry picked from commit 8427bbc224863e14d905c87920d4005cb3e88ac3)
Signed-off-by: Kai-Heng Feng <[hidden email]>
---
 drivers/nvme/host/pci.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 689a541c663a..32349f74ddc6 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -2199,7 +2199,7 @@ static int nvme_dev_map(struct nvme_dev *dev)
        return -ENODEV;
 }
 
-static unsigned long check_dell_samsung_bug(struct pci_dev *pdev)
+static unsigned long check_vendor_combination_bug(struct pci_dev *pdev)
 {
  if (pdev->vendor == 0x144d && pdev->device == 0xa802) {
  /*
@@ -2214,6 +2214,14 @@ static unsigned long check_dell_samsung_bug(struct pci_dev *pdev)
     (dmi_match(DMI_PRODUCT_NAME, "XPS 15 9550") ||
      dmi_match(DMI_PRODUCT_NAME, "Precision 5510")))
  return NVME_QUIRK_NO_DEEPEST_PS;
+ } else if (pdev->vendor == 0x144d && pdev->device == 0xa804) {
+ /*
+ * Samsung SSD 960 EVO drops off the PCIe bus after system
+ * suspend on a Ryzen board, ASUS PRIME B350M-A.
+ */
+ if (dmi_match(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC.") &&
+    dmi_match(DMI_BOARD_NAME, "PRIME B350M-A"))
+ return NVME_QUIRK_NO_APST;
  }
 
  return 0;
@@ -2261,7 +2269,7 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
  if (result)
  goto put_pci;
 
- quirks |= check_dell_samsung_bug(pdev);
+ quirks |= check_vendor_combination_bug(pdev);
 
  result = nvme_init_ctrl(&dev->ctrl, &pdev->dev, &nvme_pci_ctrl_ops,
  quirks);
--
2.14.1


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

ACK/cmnt: [SRU] [Xenial] [PATCH 00/14] Enable NVMe APST for Xenial, take two

AceLan Kao
In reply to this post by Kai-Heng Feng
Clear applied to xenial master-next and the kernel can be built without any issues.
This series patchset is significantly helpful for the nvme power consumption, and
would be required to pass the energy star.

Acked-By: AceLan Kao <[hidden email]>

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

ACK: [SRU] [Xenial] [PATCH 00/14] Enable NVMe APST for Xenial, take two

Shrirang Bagul
In reply to this post by Kai-Heng Feng
On Fri, 2018-01-05 at 14:06 +0800, Kai-Heng Feng wrote:

> BugLink: https://bugs.launchpad.net/bugs/1664602
>
> [Impact]
> NVME APST feature is not enabled, so the power consumption is higher.
>
> [Fix]
> Enable NVME APST to conserve energy.
>
> [Test]
> Use 'nvme get-feature -f 0x0c -H /dev/nvme0' from nvme-cli to query the feature.
>
> Before APST enabled:
> get-feature:0xc (Autonomous Power State Transition), Current value:00000000
>        Autonomous Power State Transition Enable (APSTE): Disabled
>
> After APST enabled:
> get-feature:0xc (Autonomous Power State Transition), Current value:0x000001
>        Autonomous Power State Transition Enable (APSTE): Enabled
>
> [Regression Potential]
> Low. Now Artful is release for some time now, we only have one APST
> issue, which is fixed by a new workaround.
>
> Andy Lutomirski (8):
>   nvme/scsi: Remove power management support
>   nvme: Fix nvme_get/set_features() with a NULL result pointer
>   nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
>   nvme: Add a quirk mechanism that uses identify_ctrl
>   nvme: Enable autonomous power state transitions
>   nvme: Adjust the Samsung APST quirk
>   nvme: Quirk APST off on "THNSF5256GPUK TOSHIBA"
>   nvme: Quirk APST on Intel 600P/P3100 devices
>
> Christoph Hellwig (3):
>   nvme: return the whole CQE through the request passthrough interface
>   nvme: factor out a add nvme_is_write helper
>   nvme: Modify and export sync command submission for fabrics
>
> Kai-Heng Feng (3):
>   nvme: only consider exit latency when choosing useful non-op power
>     states
>   nvme: relax APST default max latency to 100ms
>   nvme-pci: disable APST on Samsung SSD 960 EVO + ASUS PRIME B350M-A
>
>  drivers/nvme/host/core.c | 287 +++++++++++++++++++++++++++++++++++++++++++----
>  drivers/nvme/host/nvme.h |  27 ++++-
>  drivers/nvme/host/pci.c  |  50 +++++++--
>  drivers/nvme/host/scsi.c |  80 +------------
>  include/linux/nvme.h     |   6 +
>  5 files changed, 341 insertions(+), 109 deletions(-)
>
Acked-By: Shrirang Bagul <[hidden email]>
> --
> 2.14.1
>
>
--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (817 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

APPLIED: [SRU] [Xenial] [PATCH 00/14] Enable NVMe APST for Xenial, take two

Khalid Elmously
In reply to this post by Kai-Heng Feng
Applied to Xenial - I'm so glad these 14 patches applied cleanly!


On 2018-01-05 14:06:03 , Kai-Heng Feng wrote:

> BugLink: https://bugs.launchpad.net/bugs/1664602
>
> [Impact]
> NVME APST feature is not enabled, so the power consumption is higher.
>
> [Fix]
> Enable NVME APST to conserve energy.
>
> [Test]
> Use 'nvme get-feature -f 0x0c -H /dev/nvme0' from nvme-cli to query the feature.
>
> Before APST enabled:
> get-feature:0xc (Autonomous Power State Transition), Current value:00000000
>        Autonomous Power State Transition Enable (APSTE): Disabled
>
> After APST enabled:
> get-feature:0xc (Autonomous Power State Transition), Current value:0x000001
>        Autonomous Power State Transition Enable (APSTE): Enabled
>
> [Regression Potential]
> Low. Now Artful is release for some time now, we only have one APST
> issue, which is fixed by a new workaround.
>
> Andy Lutomirski (8):
>   nvme/scsi: Remove power management support
>   nvme: Fix nvme_get/set_features() with a NULL result pointer
>   nvme: Pass pointers, not dma addresses, to nvme_get/set_features()
>   nvme: Add a quirk mechanism that uses identify_ctrl
>   nvme: Enable autonomous power state transitions
>   nvme: Adjust the Samsung APST quirk
>   nvme: Quirk APST off on "THNSF5256GPUK TOSHIBA"
>   nvme: Quirk APST on Intel 600P/P3100 devices
>
> Christoph Hellwig (3):
>   nvme: return the whole CQE through the request passthrough interface
>   nvme: factor out a add nvme_is_write helper
>   nvme: Modify and export sync command submission for fabrics
>
> Kai-Heng Feng (3):
>   nvme: only consider exit latency when choosing useful non-op power
>     states
>   nvme: relax APST default max latency to 100ms
>   nvme-pci: disable APST on Samsung SSD 960 EVO + ASUS PRIME B350M-A
>
>  drivers/nvme/host/core.c | 287 +++++++++++++++++++++++++++++++++++++++++++----
>  drivers/nvme/host/nvme.h |  27 ++++-
>  drivers/nvme/host/pci.c  |  50 +++++++--
>  drivers/nvme/host/scsi.c |  80 +------------
>  include/linux/nvme.h     |   6 +
>  5 files changed, 341 insertions(+), 109 deletions(-)
>
> --
> 2.14.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