Hi all-
These are APST improvements for 4.12 or so. The first one fixes a
buggy comment. The second makes debugging easier. The third makes
it possible to force APST on despite quirks.
Andy Lutomirski (3):
nvme: Fix APST comment
nvme: Display raw APST configuration via DYNAMIC_DEBUG
nvme: Add nvme_core.force_apst to ignore the NO_APST quirk
drivers/nvme/host/core.c | 38 ++++++++++++++++++++++++++++++++++++--
1 file changed, 36 insertions(+), 2 deletions(-)
--
2.9.3
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 <[email protected]>
---
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 b0c692a14e9b..05ba4f8bb73b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1278,6 +1278,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;
/*
@@ -1299,6 +1301,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;
@@ -1348,9 +1351,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.9.3
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 <[email protected]>
---
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 05ba4f8bb73b..5249027a76ca 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.9.3
There was a typo in the description of the timeout heuristic.
Signed-off-by: Andy Lutomirski <[email protected]>
---
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 eeb409c287b8..b0c692a14e9b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1267,7 +1267,7 @@ 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)
+ * non-operational state after waiting 50 * (enlat + exlat)
* microseconds, as long as that state's total latency is under
* the requested maximum latency.
*
--
2.9.3
On Fri, Apr 21, 2017 at 04:19:22PM -0700, Andy Lutomirski wrote:
> There was a typo in the description of the timeout heuristic.
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>
Looks fine,
Reviewed-by: Christoph Hellwig <[email protected]>
On Fri, Apr 21 2017, Andy Lutomirski wrote:
> Hi all-
>
> These are APST improvements for 4.12 or so. The first one fixes a
> buggy comment. The second makes debugging easier. The third makes
> it possible to force APST on despite quirks.
>
> Andy Lutomirski (3):
> nvme: Fix APST comment
> nvme: Display raw APST configuration via DYNAMIC_DEBUG
> nvme: Add nvme_core.force_apst to ignore the NO_APST quirk
>
> drivers/nvme/host/core.c | 38 ++++++++++++++++++++++++++++++++++++--
> 1 file changed, 36 insertions(+), 2 deletions(-)
I have added these for 4.12, thanks Andy.
--
Jens Axboe
On Mon, Apr 24, 2017 at 10:04:02PM -0600, Jens Axboe wrote:
> I have added these for 4.12, thanks Andy.
This shouldn't conflict with what we've queued up in the nvme tree so
we should be ok..